fix: env var decoding for bool config fields and redis sentinel multi-node support and change fs.FS path construction#4496
Open
vsengar-79 wants to merge 5 commits into
Open
fix: env var decoding for bool config fields and redis sentinel multi-node support and change fs.FS path construction#4496vsengar-79 wants to merge 5 commits into
vsengar-79 wants to merge 5 commits into
Conversation
Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThree focused edits: normalize the Kafka consumer group viper key casing, add a mapstructure string-to-basic-type decode hook, support comma-separated Redis Sentinel addresses, and switch migration FS path joins to forward-slash ChangesConfiguration and Client Setup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/redis/client.go (1)
3-11:⚠️ Potential issue | 🔴 CriticalAdd the missing
stringsimport inpkg/redis/client.go
strings.Split/strings.TrimSpaceare referenced, but thestringspackage isn’t imported, causingundefined: stringsbuild failure.🔧 Proposed fix
import ( "crypto/tls" "fmt" + "strings" "github.com/redis/go-redis/extra/redisotel/v9" "github.com/redis/go-redis/v9" "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/trace" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/redis/client.go` around lines 3 - 11, The build fails because strings.Split and strings.TrimSpace are used but the strings package isn't imported; update the import block in pkg/redis/client.go to include "strings" (alongside existing imports like "crypto/tls", "fmt", and github.com/redis/go-redis/v9) so functions that call strings.Split and strings.TrimSpace compile correctly.
🧹 Nitpick comments (2)
pkg/redis/client.go (1)
61-64: ⚡ Quick winConsider filtering out empty addresses.
If the config has consecutive commas (like
"node1:26379,,node3:26379") or trailing/leading commas, you'll end up with empty strings insentinelAddrsafter trimming. While this is an edge case, filtering them out would make the initialization more robust and avoid potential confusing errors from the Redis library.♻️ Optional enhancement
// Address may be a comma-separated list of sentinel nodes. sentinelAddrs := strings.Split(o.Address, ",") - for i, a := range sentinelAddrs { - sentinelAddrs[i] = strings.TrimSpace(a) + var trimmed []string + for _, a := range sentinelAddrs { + if addr := strings.TrimSpace(a); addr != "" { + trimmed = append(trimmed, addr) + } } + sentinelAddrs = trimmed🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/redis/client.go` around lines 61 - 64, The sentinel address parsing currently splits o.Address and trims entries into sentinelAddrs but doesn't remove empty strings; update the logic in the sentinelAddrs construction (the code that splits and trims o.Address in pkg/redis/client.go) to filter out any empty entries after trimming (e.g., build a new slice or use a filter) so sentinelAddrs contains only non-empty addresses before it's used to initialize the Redis sentinel client.app/config/viper.go (1)
14-14:StringToBasicTypeHookFunc()is real in your pinned mapstructure, and the hook order looks safe—worth adding a bool-from-string test
app/config/viper.gousesgithub.com/go-viper/mapstructure/v2@v2.5.0;StringToBasicTypeHookFunc()exists and is a composite of string→basic-type hooks (including bool viastrconv.ParseBool).- The bool conversion only runs when the input is a
stringand the destination kind isbool, so it won’t affect YAML-native non-strings orinterface{}/map targets.strconv.ParseBoolacceptstrue/false(any case) plust/fand1/0, but notyes/no/on/off.- Placing it after
StringToSliceHookFunc(",")is consistent: comma-splitting happens first, and bool parsing only kicks in when elements are decoded as strings.Optional: add a focused unit test for decoding bool fields from env-var-style strings (
"true","false","1","0") to lock this behavior down (and optionally assert that"yes"fails).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/config/viper.go` at line 14, The review notes that StringToBasicTypeHookFunc() in app/config/viper.go is present and safe but recommends adding a focused unit test to lock bool-from-string behavior; add a test that decodes into a bool field using mapstructure.Decoder/Unmarshal (or your existing viper decode path) with env-style string inputs ("true","false","1","0") and assert they decode to the expected booleans, and also include a negative case (e.g., "yes") to assert it fails or does not decode to true; reference the existing StringToBasicTypeHookFunc() and StringToSliceHookFunc(",") usage to ensure the test covers string→slice then string→bool ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/redis/client.go`:
- Around line 3-11: The build fails because strings.Split and strings.TrimSpace
are used but the strings package isn't imported; update the import block in
pkg/redis/client.go to include "strings" (alongside existing imports like
"crypto/tls", "fmt", and github.com/redis/go-redis/v9) so functions that call
strings.Split and strings.TrimSpace compile correctly.
---
Nitpick comments:
In `@app/config/viper.go`:
- Line 14: The review notes that StringToBasicTypeHookFunc() in
app/config/viper.go is present and safe but recommends adding a focused unit
test to lock bool-from-string behavior; add a test that decodes into a bool
field using mapstructure.Decoder/Unmarshal (or your existing viper decode path)
with env-style string inputs ("true","false","1","0") and assert they decode to
the expected booleans, and also include a negative case (e.g., "yes") to assert
it fails or does not decode to true; reference the existing
StringToBasicTypeHookFunc() and StringToSliceHookFunc(",") usage to ensure the
test covers string→slice then string→bool ordering.
In `@pkg/redis/client.go`:
- Around line 61-64: The sentinel address parsing currently splits o.Address and
trims entries into sentinelAddrs but doesn't remove empty strings; update the
logic in the sentinelAddrs construction (the code that splits and trims
o.Address in pkg/redis/client.go) to filter out any empty entries after trimming
(e.g., build a new slice or use a filter) so sentinelAddrs contains only
non-empty addresses before it's used to initialize the Redis sentinel client.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45548d14-973d-4460-a050-d15780ea0e08
📒 Files selected for processing (3)
app/config/sink.goapp/config/viper.gopkg/redis/client.go
Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
filepath.Join uses the OS path separator (\ on Windows), which breaks fs.FS implementations that require forward-slash paths per the io/fs spec. Replace with path.Join which always uses /. Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
vsengar-79
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/migrate/fs.go (1)
42-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDirectory entries are read as files after recursion
Nice move switching to
path.Join, but there’s a control-flow bug here: after handling a directory (Line 42-49), the code still falls through tofs.ReadFile(Line 53) for that same directory path. That can return an error (is a directory) and stop traversal for nested migration trees.Suggested fix
if entry.IsDir() { r, err := s.ReadDir(filePath) if err != nil { return nil, err } results = append(results, r...) + continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/migrate/fs.go` around lines 42 - 55, The code recurses into directories with s.ReadDir(entry) but then still falls through to call fs.ReadFile(filePath) for that directory; add control flow to skip file reading for directories (e.g., after appending results from s.ReadDir(filePath) return/continue the loop or use an else) so that fs.ReadFile is only called for non-directory entries; update the block around entry.IsDir(), s.ReadDir(filePath), results append, and fs.ReadFile(s.fsys, filePath) accordingly to prevent "is a directory" read errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tools/migrate/fs.go`:
- Around line 42-55: The code recurses into directories with s.ReadDir(entry)
but then still falls through to call fs.ReadFile(filePath) for that directory;
add control flow to skip file reading for directories (e.g., after appending
results from s.ReadDir(filePath) return/continue the loop or use an else) so
that fs.ReadFile is only called for non-directory entries; update the block
around entry.IsDir(), s.ReadDir(filePath), results append, and
fs.ReadFile(s.fsys, filePath) accordingly to prevent "is a directory" read
errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de577d74-cf2f-4a6c-bd75-eab5aea5d936
📒 Files selected for processing (1)
tools/migrate/fs.go
Contributor
Author
|
Also changed the fs.FS path construction. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Four small fixes for running OpenMeter in Kubernetes where config values arrive as environment variable strings rather than typed YAML values.
app/config/viper.go— bool env var decodingAdded
StringToBasicTypeHookFunc()to the globalDecodeHook. When config values are set via env vars or ConfigMaps, they are always strings —"true","false","1". Without this hook, anyboolconfig field panics on startup with:This affects fields like
DEDUPE_ENABLED,NOTIFICATION_ENABLED, and any other boolean config sourced from the environment.app/config/sink.go— consumer group ID key name typosink.kafka.consumerGroupId→sink.kafka.consumerGroupID(capitalIDto match the struct field's mapstructure tag). The previous key never matched the actual config field, so the default value"openmeter-sink-worker"was never applied. The sink-worker joined Kafka with an empty consumer group ID, which caused Kafka to reject partition assignment and left events unsynced to ClickHouse.pkg/redis/client.go— Redis Sentinel multi-node supportThe
Addressfield now supports a comma-separated list of sentinel nodes (e.g."sentinel1:26379,sentinel2:26379,sentinel3:26379"). Previously it was hardcoded to[]string{o.Address}, which only ever used a single sentinel endpoint regardless of what was configured.tools/migrate/fs.go— usepath.Joininstead offilepath.Joinforfs.FSpath constructionfilepath.Joinuses the OS path separator (\on Windows), which breaksfs.FSimplementations that require forward-slash paths per theio/fsspec. Replace withpath.Joinwhich always uses/Notes for reviewer
StringToBasicTypeHookFunconly activates when the target struct field is a basic type (bool,int,float64) — it has no effect on string fields orinterface{}maps. Existing YAML-based configs are unaffected since YAML already deserialises native types correctly.strings.Spliton a non-comma string returns a single-element slice, identical to the previous[]string{o.Address}behaviour.pathpackage (always forward-slash) is used instead offilepath(OS-dependent separator) becausefs.FSrequires forward-slash paths on all platforms per theio/fsspec; the function parameter was renamed frompathtodirto avoid shadowing the import.Summary by CodeRabbit