[config] migrate to koanf #7
Conversation
WalkthroughReplaces the prior envconfig/godotenv loader with a Koanf-based multi-source loader. Load now accepts variadic options to enable optional S3 and local YAML sources, then loads dotenv and environment variables, lowercases keys, parses JSON/CSV values, skips not-exist sources, wraps step errors, and adds ErrMissingAWSCreds. (38 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Loader as config.Load
participant Koanf as Koanf
participant S3 as AWS S3
participant FS as Local FS
participant Env as Environment
Caller->>Loader: Load(cfg, opts...)
Loader->>Loader: apply options
alt WithS3YAML
Loader->>S3: GetObject(bucket, key) (reads AWS creds from env)
S3-->>Loader: bytes / 404 / error
alt bytes
Loader->>Koanf: Load(bytes, yaml.Parser)
else 404
note right of Loader #f9f9c8: skip S3 source
else error
note right of Loader #f9d0d0: return wrapped "load s3" or ErrMissingAWSCreds
end
end
alt WithLocalYAML
Loader->>FS: Read file (path)
FS-->>Loader: bytes / os.ErrNotExist / error
alt bytes
Loader->>Koanf: Load(bytes, yaml.Parser)
else os.ErrNotExist
note right of Loader #f9f9c8: skip local YAML
else error
note right of Loader #f9d0d0: return wrapped "load yaml"
end
end
Loader->>FS: Read .env (file.Provider + dotenv.Parser, lowercased keys)
FS-->>Loader: kv / os.ErrNotExist / error
alt kv
Loader->>Koanf: Merge dotenv
else os.ErrNotExist
note right of Loader #f9f9c8: skip dotenv
else error
note right of Loader #f9d0d0: return wrapped "load dotenv"
end
Loader->>Env: Read environment (env.Provider, lowercased keys, JSON/CSV transform)
Env-->>Loader: kv
Loader->>Koanf: Merge env (env vars override)
Loader->>Koanf: Unmarshal("", cfg)
Koanf-->>Loader: populated cfg / unmarshal error
alt success
Loader-->>Caller: return nil
else error
note right of Loader #f9d0d0: return wrapped "unmarshal"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
3-3: Pin CI workflows to Go 1.24.x
Your GitHub Actions workflows currently usego-version: stable(lines 34 & 55), which may install a newer Go release than thego 1.24.1specified in go.mod. Update both instances to:uses: actions/setup-go@v5 with: go-version: '1.24.x'
🧹 Nitpick comments (4)
go.mod (1)
11-11: Optional: align to env provider v2 API.You’re using
providers/env v1.x. Koanf v2 also offersproviders/env/v2with anOpt-based API; consider standardizing on the v2 provider for consistency with other v2 modules. This is optional; v1 works.Reference: Koanf docs list env/v2 and its API. (pkg.go.dev, github.com)
config/options.go (1)
3-8: Nit: prefer initialism-casing (YAML).Consider
withYAMLinstead ofwithYamlfor Go initialism consistency. This is cosmetic; change only if you touch the field names elsewhere.config/config.go (2)
17-30: Clarify precedence in the doc comment.Given Koanf merges sequentially, later sources override earlier ones. Make this explicit to avoid confusion.
-// It looks for configuration in the following order: +// It looks for configuration in the following order (later overrides earlier): // 1. S3, if `withS3` is provided. // 2. Local file, if `withYaml` is provided. // 3. `.env` file in the current working directory. // 4. Environment variables.Reference: Koanf merges subsequent loads over prior ones. (github.com)
73-75: Optional: adopt env/v2 provider API.If you move to
providers/env/v2, switch to theenv.Optstyle withTransformFuncto future-proof the API.Example (if you choose to change imports):
k.Load(env.Provider(".", env.Opt{ Prefix: "", TransformFunc: func(k, v string) (string, any) { return strings.ReplaceAll(strings.ToLower(k), "_", "."), v }, }), nil)Docs: env/v2 API. (pkg.go.dev)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
config/config.go(1 hunks)config/options.go(1 hunks)go.mod(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config/config.go (3)
config/options.go (1)
Option(10-10)http/config.go (1)
Config(5-9)redis/config.go (1)
Config(3-5)
🪛 GitHub Check: Lint
config/config.go
[failure] 31-31:
config.options is missing fields withYaml, withS3Bucket, withS3ObjectKey (exhaustruct)
[failure] 43-43:
do not define dynamic errors, use wrapped static errors instead: "errors.New("AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_REGION must be set")" (err113)
🪛 GitHub Actions: Go
config/config.go
[error] 43-43: GolangCI-Lint: do not define dynamic errors; use wrapped static errors instead. Found: errors.New("AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_REGION must be set"). Command: golangci-lint run --timeout=5m.
🔇 Additional comments (1)
config/config.go (1)
55-59: Verify missing-object handling for S3.
errors.Is(err, os.ErrNotExist)may not match the provider’s “not found” error. Ensure the S3 provider returnsos.ErrNotExiston missing objects, or extend the check to handle provider-specific not-found conditions.Run a quick check against a non-existent key and confirm the error type; adjust the skip condition if needed.
References: Koanf S3 provider usage; behavior is provider-defined. (github.com)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
config/options.go (1)
26-33: Doc fix for WithS3YAML looks correct.
Accurately describes S3 source and skip-on-missing behavior.go.mod (1)
46-46: Fix invalid module path: use gopkg.in/yaml.v3.
Currentgo.yaml.in/yaml/v3breaks resolution.Apply:
- go.yaml.in/yaml/v3 v3.0.3 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirectThen run: go mod tidy
config/config.go (2)
33-35: Nice: avoids exhaustruct by using new(options).
Resolves the linter finding.
17-18: Good switch to a sentinel error for AWS creds.
Complies with err113 and clarifies failure mode.Also applies to: 44-46
🧹 Nitpick comments (4)
config/options.go (1)
3-8: Consider supporting multiple local YAML overlays.
Single string limits layering (e.g., base + env override). Future-proof by making withYaml a []string and allowing multiple WithLocalYAML calls to append.go.mod (1)
9-14: Align deps with code changes (.env handling).
After switching to godotenv (see config/config.go suggestion), drop koanf dotenv parser and make godotenv a direct dep.- github.com/knadh/koanf/parsers/dotenv v1.1.0 + // (removed) using github.com/joho/godotenv for .envMove:
- github.com/joho/godotenv v1.5.1 // indirect + github.com/joho/godotenv v1.5.1Run: go mod tidy
Also applies to: 30-30
config/config.go (2)
32-36: Add a nil target guard to prevent panics.
Return a clear error if caller passes nil.@@ func Load[T any](c *T, opts ...Option) error { + if c == nil { + return errNilConfigTarget + } options := new(options) @@ if err := k.Unmarshal("", c); err != nil { return fmt.Errorf("unmarshal: %w", err) }Add near the existing sentinel:
var errMissingAWSCreds = errors.New("AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_REGION must be set") +var errNilConfigTarget = errors.New("nil config target")Also applies to: 85-87
63-68: Optional: support multiple local YAML layers and explicit parse order.
Allow repeated WithLocalYAML calls (append) and load them in order for base->env->local overrides.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
config/config.go(1 hunks)config/options.go(1 hunks)go.mod(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config/config.go (1)
config/options.go (1)
Option(10-10)
🔇 Additional comments (4)
config/options.go (1)
18-24: Docs and behavior for WithLocalYAML are clear and consistent with loader.
Matches the skip-on-ENOENT behavior in config/config.go.go.mod (1)
3-3: Confirm toolchain availability for go 1.24.1.
Ensure CI/build images have Go 1.24.1; otherwise set to the project’s supported version.config/config.go (2)
57-61: Confirm NotExist semantics for S3 provider.
Ensure missing object maps to os.ErrNotExist; otherwise the skip-on-missing doc/behavior diverges.
75-83: Double-check koanf/env/v2 Options usage. Ensure that theenv.Provider("__", env.Opt{…})call and theTransformFuncsignature align exactly with thekoanf/env/v2version specified in yourgo.modto avoid subtle API drift.
capcom6
force-pushed
the
config/migrate-to-koanf
branch
from
fe349d1 to
83b67e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
3-3: Fixgodirective: patch versions are invalid in go.mod.Use major.minor only; toolchain patches belong in a separate
toolchaindirective.-go 1.24.1 +go 1.24
♻️ Duplicate comments (1)
go.mod (1)
46-46: Fix invalid module path for YAML v3.Typo:
go.yaml.in/yaml/v3→gopkg.in/yaml.v3. This breaks resolution.Option A (let tidy add correct entry):
- go.yaml.in/yaml/v3 v3.0.3 // indirect + // removed; run `go mod tidy` to add gopkg.in/yaml.v3Option B (pin explicitly):
- go.yaml.in/yaml/v3 v3.0.3 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect
🧹 Nitpick comments (1)
config/config.go (1)
75-83: Parse env values into types (bool/int/float/slices) to improve DX.Currently all envs are strings; unmarshalling may not coerce slices like
HTTP__PROXIES. Add lightweight parsing inTransformFunc.- if err := k.Load(env.Provider("__", env.Opt{ - Prefix: "", - TransformFunc: func(k, v string) (string, any) { - return strings.ToLower(k), v - }, - EnvironFunc: nil, - }), nil); err != nil { + if err := k.Load(env.Provider("__", env.Opt{ + Prefix: "", + TransformFunc: func(k, v string) (string, any) { + k = strings.ToLower(k) + // CSV -> []string + if strings.Contains(v, ",") { + parts := strings.Split(v, ",") + for i := range parts { + parts[i] = strings.TrimSpace(parts[i]) + } + return k, parts + } + // bool/int/float + if b, err := strconv.ParseBool(v); err == nil { + return k, b + } + if i, err := strconv.ParseInt(v, 10, 64); err == nil { + return k, i + } + if f, err := strconv.ParseFloat(v, 64); err == nil { + return k, f + } + return k, v + }, + }), nil); err != nil {Add import:
import "strconv"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
config/config.go(1 hunks)config/options.go(1 hunks)go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/options.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T00:53:47.758Z
Learnt from: capcom6
PR: android-sms-gateway/core#7
File: config/config.go:7-15
Timestamp: 2025-09-10T00:53:47.758Z
Learning: The knanf/parsers/dotenv package provides ParserEnv function with signature ParserEnv(prefix, delim string, cb func(s string) string) that allows the DOTENV parser to behave like the env.Provider.
Applied to files:
config/config.go
🧬 Code graph analysis (1)
config/config.go (4)
logger/logger.go (1)
New(10-24)config/options.go (1)
Option(10-10)http/config.go (1)
Config(5-9)redis/config.go (1)
Config(3-5)
🔇 Additional comments (9)
go.mod (2)
9-14: Koanf deps look good.The selected koanf modules and versions are consistent with v2 usage.
30-30: Ignore removal ofgithub.com/joho/godotenv
It’s transitively required by thegithub.com/knadh/koanf/parsers/dotenvmodule, sogo mod tidycorrectly retains it.Likely an incorrect or invalid review comment.
config/config.go (7)
19-31: Docstring ordering and behavior are clear.Order and skip-on-not-exist policy are well documented and match the intended flow.
32-35: Good: exhaustruct-friendly options init.
new(options)avoids the exhaustruct lint while keeping zero-values.
17-17: Good: sentinel error for missing AWS creds.Addresses
err113and keeps error stable.
63-69: Local YAML optional load looks correct.Skips on
os.ErrNotExist; good.
70-73: dotenv load via koanf parser: LGTM.Using
ParserEnv("", "__", strings.ToLower)keeps nesting consistent with env.
85-87: Unmarshal step is fine.Defaults (
""root) into the target struct is standard.
36-37: End-to-end sanity check passed Verified imports and koanf API usage by runninggo vet ./...,golangci-lint run --timeout=5m, andgo build ./...with zero issues.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
3-3: Fix invalidgodirective: patch version not allowed.
go.modonly accepts major.minor (e.g.,go 1.24).go 1.24.1will fail module validation.Apply:
-go 1.24.1 +go 1.24
♻️ Duplicate comments (2)
go.mod (1)
49-49: Remove invalid/duplicate YAML module path.
go.yaml.in/yaml/v3is a typo and breaks resolution; you already have the correctgopkg.in/yaml.v3. Drop the bad line and tidy.Apply:
- go.yaml.in/yaml/v3 v3.0.3 // indirectThen run:
go mod tidy.Also applies to: 54-54
config/config.go (1)
58-62: Harden S3 “not found” handling; avoid brittle 404 substring match.S3 providers may return NoSuchKey/NotFound without “404” text. Treat common not-found shapes as skip; return other errors.
Apply:
- err := k.Load(s3.Provider(s3Config), yaml.Parser()) - if err != nil && !strings.Contains(err.Error(), "404") { - return fmt.Errorf("load s3: %w", err) - } + err := k.Load(s3.Provider(s3Config), yaml.Parser()) + if err != nil { + msg := strings.ToLower(err.Error()) + if errors.Is(err, os.ErrNotExist) || + strings.Contains(msg, "nosuchkey") || + strings.Contains(msg, "not found") || + strings.Contains(msg, "status code: 404") { + // skip + } else { + return fmt.Errorf("load s3: %w", err) + } + }Optionally extract an
isS3NotFound(err error) boolhelper for reuse.
🧹 Nitpick comments (4)
config/config.go (1)
71-88: ExtractenvTransformand extend JSON handling (optional).Pull into a package-level var or func to reduce
Loadcomplexity and enable reuse. If desired, support JSON arrays ([...]) in addition to objects.Example:
- envTransform := func(k, v string) (string, any) { + envTransform := func(k, v string) (string, any) { k = strings.ToLower(k) - // CSV -> []string - if strings.HasPrefix(v, "{") && strings.HasSuffix(v, "}") { + // JSON object -> map + if strings.HasPrefix(v, "{") && strings.HasSuffix(v, "}") { var m map[string]any if err := json.Unmarshal([]byte(v), &m); err == nil { return k, m } } + // JSON array -> []any + if strings.HasPrefix(v, "[") && strings.HasSuffix(v, "]") { + var a []any + if err := json.Unmarshal([]byte(v), &a); err == nil { + return k, a + } + } - if strings.Contains(v, ",") { + // CSV -> []string + if strings.Contains(v, ",") { parts := strings.Split(v, ",") for i := range parts { parts[i] = strings.TrimSpace(parts[i]) } return k, parts } return k, v }config/config_test.go (3)
110-112: Adopt testify best practices per linter (require for error presence; use ErrorIs/Contains).Switch to
require.Errorto short-circuit andassert.ErrorIs/assert.ErrorContainsfor clarity.Apply:
@@ - assert.Error(t, err) - assert.True(t, errors.Is(err, errMissingAWSCreds)) + require.Error(t, err) + assert.ErrorIs(t, err, errMissingAWSCreds) @@ - assert.Error(t, err) - assert.True(t, strings.Contains(err.Error(), "load yaml")) + require.Error(t, err) + assert.ErrorContains(t, err, "load yaml") @@ - assert.Error(t, err) - assert.True(t, strings.Contains(err.Error(), "unmarshal")) + require.Error(t, err) + assert.ErrorContains(t, err, "unmarshal") @@ - assert.Error(t, err) - assert.True(t, errors.Is(err, errMissingAWSCreds)) + require.Error(t, err) + assert.ErrorIs(t, err, errMissingAWSCreds) @@ - assert.Error(t, err) - assert.True(t, errors.Is(err, errMissingAWSCreds)) + require.Error(t, err) + assert.ErrorIs(t, err, errMissingAWSCreds)Also applies to: 127-129, 204-206, 374-377, 392-396
344-360: Avoid external network in unit tests.Set
AWS_ENDPOINTto a non-routable local endpoint so this test fails fast without dialing S3.Apply:
os.Setenv("AWS_REGION", "us-east-1") +os.Setenv("AWS_ENDPOINT", "http://127.0.0.1:9") // force connection error locally defer func() { os.Unsetenv("AWS_ACCESS_KEY_ID") os.Unsetenv("AWS_SECRET_ACCESS_KEY") os.Unsetenv("AWS_REGION") + os.Unsetenv("AWS_ENDPOINT") }()
32-41: Uset.Setenvfor cleanup-free env management (optional).Reduces boilerplate and eliminates risk of leaked env between tests. Requires Go ≥1.17 (you target 1.24).
Example:
- os.Setenv("DATABASE__HOST", "localhost") - os.Setenv("DATABASE__PORT", "5432") - os.Setenv("SERVER__PORT", "8080") - defer func() { os.Unsetenv(...); ... }() + t.Setenv("DATABASE__HOST", "localhost") + t.Setenv("DATABASE__PORT", "5432") + t.Setenv("SERVER__PORT", "8080")Also applies to: 143-151, 245-253
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
config/config.go(1 hunks)config/config_test.go(1 hunks)go.mod(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T00:53:47.758Z
Learnt from: capcom6
PR: android-sms-gateway/core#7
File: config/config.go:7-15
Timestamp: 2025-09-10T00:53:47.758Z
Learning: The knanf/parsers/dotenv package provides ParserEnv function with signature ParserEnv(prefix, delim string, cb func(s string) string) that allows the DOTENV parser to behave like the env.Provider.
Applied to files:
config/config.go
🧬 Code graph analysis (2)
config/config_test.go (2)
config/config.go (1)
Load(33-108)config/options.go (2)
WithLocalYAML(20-24)WithS3YAML(28-33)
config/config.go (1)
config/options.go (1)
Option(10-10)
🪛 GitHub Check: Lint
config/config_test.go
[failure] 395-395:
require-error: for error assertions use require (testifylint)
[failure] 377-377:
error-is-as: use assert.ErrorIs (testifylint)
[failure] 376-376:
require-error: for error assertions use require (testifylint)
[failure] 206-206:
contains: use assert.Contains (testifylint)
[failure] 205-205:
require-error: for error assertions use require (testifylint)
[failure] 128-128:
contains: use assert.Contains (testifylint)
[failure] 127-127:
require-error: for error assertions use require (testifylint)
[failure] 111-111:
error-is-as: use assert.ErrorIs (testifylint)
[failure] 110-110:
require-error: for error assertions use require (testifylint)
config/config.go
[failure] 33-33:
cognitive complexity 27 of func Load is high (> 20) (gocognit)
🪛 GitHub Actions: Go
config/config.go
[error] 33-33: golangci-lint: cognitive complexity 27 of func Load is high (> 20) (gocognit)
🔇 Additional comments (4)
go.mod (1)
32-32: Keepgithub.com/joho/godotenv: required by Koanf’s dotenv parser. go mod why showskoanf/parsers/dotenvimports it, sogo mod tidycorrectly preserves it as an indirect dependency.Likely an incorrect or invalid review comment.
config/config.go (3)
33-36: Good: fixed exhaustruct withnew(options).This addresses the linter warning cleanly.
39-47: Credential validation looks correct.Sentinel
errMissingAWSCreds+ explicit env checks read well.
90-93: Upgrade dotenv parser or fallback to ParserEnvParserEnvWithValue isn’t available in knadh/koanf/parsers/dotenv v1.1.0—either bump the dependency to v2.1.1+ (where ParserEnvWithValue was introduced) or revert to ParserEnv to prevent build failures:
--- config/config.go @@ -90,7 +90,7 @@ func loadEnv() error { - err := k.Load(file.Provider(".env"), dotenv.ParserEnvWithValue("", "__", envTransform)) + err := k.Load(file.Provider(".env"), dotenv.ParserEnv("", "__", strings.ToLower))⛔ Skipped due to learnings
Learnt from: capcom6 PR: android-sms-gateway/core#7 File: config/config.go:7-15 Timestamp: 2025-09-10T00:53:47.758Z Learning: The knanf/parsers/dotenv package provides ParserEnv function with signature ParserEnv(prefix, delim string, cb func(s string) string) that allows the DOTENV parser to behave like the env.Provider.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
config/config_test.go (2)
1-1: About test package name (optional).Lint suggests config_test, but these tests reference unexported errMissingAWSCreds. Either keep package config and ignore testpackage, or export the sentinel (e.g., ErrMissingAWSCreds) and switch to config_test.
If you want to export and switch, I can generate the follow-up diff across config and tests.
319-330: Avoid potential live S3 calls in unit tests (optional).If loadFromS3 performs real network I/O, this test may be flaky/slow. Consider injecting an S3 client interface or adding a test hook/option to bypass network, or guard with testing.Short().
I can provide a minimal S3 client interface and constructor option to inject a no-op/mock for tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/config.go(1 hunks)config/config_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/config.go
🧰 Additional context used
🧬 Code graph analysis (1)
config/config_test.go (2)
config/config.go (1)
Load(33-60)config/options.go (2)
WithLocalYAML(20-24)WithS3YAML(28-33)
🪛 GitHub Check: Lint
config/config_test.go
[failure] 155-155:
os.Chdir() could be replaced by t.Chdir() in TestConfigurationPrecedence (usetesting)
[failure] 154-154:
os.Chdir() could be replaced by t.Chdir() in TestConfigurationPrecedence (usetesting)
[failure] 48-48:
os.Chdir() could be replaced by t.Chdir() in TestLoadWithNoOptions (usetesting)
[failure] 47-47:
os.Chdir() could be replaced by t.Chdir() in TestLoadWithNoOptions (usetesting)
[failure] 1-1:
package should be config_test instead of config (testpackage)
[failure] 356-356:
require-error: for error assertions use require (testifylint)
[failure] 342-342:
require-error: for error assertions use require (testifylint)
[failure] 188-188:
require-error: for error assertions use require (testifylint)
[failure] 120-120:
require-error: for error assertions use require (testifylint)
[failure] 103-103:
require-error: for error assertions use require (testifylint)
🪛 GitHub Actions: Go
config/config_test.go
[error] 103-103: GolangCI-Lint error in config/config_test.go:103:2: require-error: for error assertions use require (testifylint)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
config/config_test.go (5)
102-104: Use require.ErrorIs to satisfy testifylint and fail fast.- require.Error(t, err) - assert.ErrorIs(t, err, config.ErrMissingAWSCreds) + require.Error(t, err) + require.ErrorIs(t, err, config.ErrMissingAWSCreds)
119-121: Use require.ErrorContains for error content checks.- require.Error(t, err) - assert.ErrorContains(t, err, "load yaml") + require.Error(t, err) + require.ErrorContains(t, err, "load yaml")
184-187: Use require.ErrorContains here as well.- // Should get an error due to invalid port conversion - require.Error(t, err) - assert.ErrorContains(t, err, "unmarshal") + // Should get an error due to invalid port conversion + require.Error(t, err) + require.ErrorContains(t, err, "unmarshal")
327-329: Switch to require.ErrorIs for consistency with lint rule.- require.Error(t, err) - assert.ErrorIs(t, err, config.ErrMissingAWSCreds) + require.Error(t, err) + require.ErrorIs(t, err, config.ErrMissingAWSCreds)
341-343: Same here: require.ErrorIs.- require.Error(t, err) - assert.ErrorIs(t, err, config.ErrMissingAWSCreds) + require.Error(t, err) + require.ErrorIs(t, err, config.ErrMissingAWSCreds)
🧹 Nitpick comments (5)
config/config.go (4)
20-33: Docs nit: align wording with option names.Bullet 1 says “S3, if
withS3is provided.” Options arewithS3Bucket/withS3ObjectKey. Update for accuracy.-// 1. S3, if `withS3` is provided. +// 1. S3, if `withS3Bucket` and `withS3ObjectKey` are provided.
37-37: Consider opt-in StrictMerge to catch type conflicts.If you want to fail fast when sources disagree on types, expose an option to use
koanf.NewWithConf(koanf.Conf{Delim: ".", StrictMerge: true}); default can remain permissive.- k := koanf.New(".") + k := koanf.New(".") // optionally: koanf.NewWithConf(koanf.Conf{Delim: ".", StrictMerge: true})
62-91: Treat common S3 “not found” conditions as skip, not only literal 404.S3 returns “NoSuchKey” (404) for missing objects, and can return 403 when you lack ListBucket permission even if the object doesn’t exist. Only skipping on a string “404” is brittle. Skip on well-known not-found signals; still surface 403/AccessDenied. (repost.aws)
- err := k.Load(s3.Provider(s3Config), yaml.Parser()) - if err != nil && !strings.Contains(err.Error(), "404") { - return fmt.Errorf("load s3: %w", err) - } + err := k.Load(s3.Provider(s3Config), yaml.Parser()) + if err != nil { + // Skip on common not-found signals; surface everything else. + if errors.Is(err, os.ErrNotExist) { + return nil + } + msg := strings.ToLower(err.Error()) + if strings.Contains(msg, "nosuchkey") || + strings.Contains(msg, "no such key") || + strings.Contains(msg, "no such bucket") || + strings.Contains(msg, "status code: 404") { + return nil + } + return fmt.Errorf("load s3: %w", err) + }
127-152: Optional: widen value typing (bools and numbers) for env/.env.Helps unmarshalling without relying on weak string-to-number coercion.
func envTransform(k, v string) (string, any) { k = strings.ToLower(k) + // booleans + switch strings.ToLower(v) { + case "true": + return k, true + case "false": + return k, false + } + // integers / floats + if i, err := strconv.ParseInt(v, 10, 64); err == nil { + return k, i + } + if f, err := strconv.ParseFloat(v, 64); err == nil { + return k, f + } // JSON object -> map if strings.HasPrefix(v, "{") && strings.HasSuffix(v, "}") { var m map[string]any if err := json.Unmarshal([]byte(v), &m); err == nil { return k, m } } // JSON array -> []any if strings.HasPrefix(v, "[") && strings.HasSuffix(v, "]") { var a []any if err := json.Unmarshal([]byte(v), &a); err == nil { return k, a } } // CSV -> []string if strings.Contains(v, ",") { parts := strings.Split(v, ",") for i := range parts { parts[i] = strings.TrimSpace(parts[i]) } return k, parts } return k, v }Additional import required:
import ( "encoding/json" + "strconv" "errors" "fmt" "os" "strings"config/config_test.go (1)
94-101: Prefer t.Setenv over os.Unsetenv for isolation.
t.Setenvties cleanup to the test and avoids cross-test leakage.- // Ensure AWS credentials are not set - os.Unsetenv("AWS_ACCESS_KEY_ID") - os.Unsetenv("AWS_SECRET_ACCESS_KEY") - os.Unsetenv("AWS_REGION") + // Ensure AWS credentials are not set + t.Setenv("AWS_ACCESS_KEY_ID", "") + t.Setenv("AWS_SECRET_ACCESS_KEY", "") + t.Setenv("AWS_REGION", "")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/config.go(1 hunks)config/config_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T00:53:47.758Z
Learnt from: capcom6
PR: android-sms-gateway/core#7
File: config/config.go:7-15
Timestamp: 2025-09-10T00:53:47.758Z
Learning: The knanf/parsers/dotenv package provides ParserEnv function with signature ParserEnv(prefix, delim string, cb func(s string) string) that allows the DOTENV parser to behave like the env.Provider.
Applied to files:
config/config.go
🧬 Code graph analysis (2)
config/config.go (1)
config/options.go (1)
Option(10-10)
config/config_test.go (2)
config/config.go (2)
Load(33-60)ErrMissingAWSCreds(18-18)config/options.go (2)
WithLocalYAML(20-24)WithS3YAML(28-33)
🔇 Additional comments (1)
config/config.go (1)
10-16: LGTM: correct Koanf usage for dotenv and env/v2 with typed value mapping.
dotenv.ParserEnvWithValue("", "__", envTransform)andenv.Provider("__", env.Opt{ TransformFunc: envTransform })correctly leverage the v2 API whereTransformFunchas signaturefunc(k, v string) (string, any). YourenvTransformmatches this and enables lowercasing plus typed values. I used the retrieved learning about ParserEnv while reviewing this. (pkg.go.dev)Also applies to: 106-113, 115-125, 127-152
capcom6
force-pushed
the
config/migrate-to-koanf
branch
from
2cc0558 to
9482bc4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
config/options.go (1)
26-33: WithS3YAML doc fix looks good.This addresses the earlier copy/paste issue.
config/config_test.go (4)
47-48: Good: switched to t.Chdir.Resolves prior linter warning.
151-153: Good: t.Chdir usage here too.
221-223: Good: t.Chdir adoption.
250-252: Good: t.Chdir adoption.
🧹 Nitpick comments (7)
config/options.go (1)
12-16: Guard against nil options in apply.A nil Option would panic; add a small guard.
func (o *options) apply(opts ...Option) { - for _, opt := range opts { - opt(o) - } + for _, opt := range opts { + if opt == nil { + continue + } + opt(o) + } }config/config_test.go (6)
102-104: Use require.ErrorIs to short‑circuit on failure (testifylint).- require.Error(t, err) - assert.ErrorIs(t, err, config.ErrMissingAWSCreds) + require.Error(t, err) + require.ErrorIs(t, err, config.ErrMissingAWSCreds)
119-121: Prefer require.ErrorContains for fatal error checks.- require.Error(t, err) - assert.ErrorContains(t, err, "load yaml") + require.Error(t, err) + require.ErrorContains(t, err, "load yaml")
185-187: Use require.ErrorContains to fail fast.- require.Error(t, err) - assert.ErrorContains(t, err, "unmarshal") + require.Error(t, err) + require.ErrorContains(t, err, "unmarshal")
327-329: Switch to require.ErrorIs.- require.Error(t, err) - assert.ErrorIs(t, err, config.ErrMissingAWSCreds) + require.Error(t, err) + require.ErrorIs(t, err, config.ErrMissingAWSCreds)
341-343: Switch to require.ErrorIs.- require.Error(t, err) - assert.ErrorIs(t, err, config.ErrMissingAWSCreds) + require.Error(t, err) + require.ErrorIs(t, err, config.ErrMissingAWSCreds)
1-344: Optional: de‑dup test helpers.Consider tiny helpers for writing files and chdir to reduce repetition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
config/config.go(1 hunks)config/config_test.go(1 hunks)config/options.go(1 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- config/config.go
🧰 Additional context used
🧬 Code graph analysis (1)
config/config_test.go (2)
config/config.go (2)
Load(33-60)ErrMissingAWSCreds(18-18)config/options.go (2)
WithLocalYAML(20-24)WithS3YAML(28-33)
🔇 Additional comments (2)
config/options.go (1)
18-24: WithLocalYAML: docs + behavior LGTM.config/config_test.go (1)
313-315: NotErrorIs usage is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
config/config_test.go (1)
47-47: Nice: switched to t.Chdir (lint resolved).This addresses the earlier lint about os.Chdir usage.
🧹 Nitpick comments (4)
config/config_test.go (4)
189-204: Reduce boilerplate: merge trivial option-behavior tests.Combine these into a small table test to cut noise.
-// TestWithLocalYAMLOptionBehavior tests WithLocalYAML option behavior -func TestWithLocalYAMLOptionBehavior(t *testing.T) { - // Test that WithLocalYAML returns a valid Option - option := config.WithLocalYAML("/path/to/config.yaml") - - require.NotNil(t, option) -} - -// TestWithS3YAMLOptionBehavior tests WithS3YAML option behavior -func TestWithS3YAMLOptionBehavior(t *testing.T) { - // Test that WithS3YAML returns a valid Option - option := config.WithS3YAML("test-bucket", "config.yaml") - - require.NotNil(t, option) -} +func TestOptionConstructors(t *testing.T) { + tests := []struct { + name string + fn func() config.Option + }{ + {"WithLocalYAML", func() config.Option { return config.WithLocalYAML("/path/to/config.yaml") }}, + {"WithS3YAML", func() config.Option { return config.WithS3YAML("test-bucket", "config.yaml") }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.NotNil(t, tt.fn()) + }) + } +}
205-232: Covers env mapping; consider one extra check.Optional: add a username var to prove nested mapping beyond ports/host.
t.Setenv("DATABASE__HOST", "test-host") t.Setenv("DATABASE__PORT", "5432") +t.Setenv("DATABASE__USERNAME", "env-user") ... assert.Equal(t, "test-host", cfg.Database.Host) assert.Equal(t, 5432, cfg.Database.Port) +assert.Equal(t, "env-user", cfg.Database.Username)
234-263: .env JSON-to-map relies on custom parsing—add a negative test.If the parser attempts JSON decoding, add a test with malformed JSON to assert a clear error (or skip of the key). Keeps behavior explicit.
Would you like me to draft a small TestDotEnvInvalidJSON that expects require.ErrorContains(err, "dotenv") or similar?
302-316: Avoid real network in “valid AWS creds” test.This test may attempt S3 calls. Force a fast, local failure to prevent egress/flake while still asserting “not ErrMissingAWSCreds”.
t.Setenv("AWS_ACCESS_KEY_ID", "test-key") t.Setenv("AWS_SECRET_ACCESS_KEY", "test-secret") t.Setenv("AWS_REGION", "us-east-1") +// Avoid hitting real AWS: disable IMDS and point S3 endpoint to an unroutable target. +t.Setenv("AWS_EC2_METADATA_DISABLED", "true") +t.Setenv("AWS_ENDPOINT_URL_S3", "http://127.0.0.1:0") ... -// We expect this to fail for other reasons (like S3 connection), but not due to missing credentials -require.NotErrorIs(t, err, config.ErrMissingAWSCreds) +// Expect either nil or a non-credential error, but never ErrMissingAWSCreds. +require.NotErrorIs(t, err, config.ErrMissingAWSCreds)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/config_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config/config_test.go (2)
config/config.go (2)
Load(33-60)ErrMissingAWSCreds(18-18)config/options.go (2)
WithLocalYAML(20-24)WithS3YAML(28-33)
🔇 Additional comments (12)
config/config_test.go (12)
1-11: Solid test package setup and imports.Using config_test avoids import cycles; mixing require for error checks and assert for value comparisons aligns with testifylint.
13-27: Schema mirrors koanf tags cleanly.Field tags match expected keys; good basis for unmarshalling.
53-58: Precedence assertion is correct.Env vars overriding .env matches loader order.
60-90: Local YAML load path covered well.Validates nested fields and feature flags; looks good.
92-104: S3 option: missing creds sentinel verified.Asserts ErrMissingAWSCreds; good.
106-121: Error wrapping contract enforced.Checks “load yaml” propagation; good guard.
123-164: End‑to‑end precedence (S3 < YAML < .env < env) validated.Matches Load() source order.
165-187: Unmarshal failure path covered.Asserts “unmarshal” wrapper; good.
264-273: Non-existent YAML gracefully skipped.Confirms os.ErrNotExist pass-through; good.
276-300: Multi-source merge covered.Env wins over YAML as expected.
317-329: Partial creds rejection verified.Sentinel usage correct.
331-343: Empty creds rejection verified.Good negative coverage.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
config/config_test.go (2)
46-48: Good fix: t.Chdir removes cwd leak and silences the linter.Also applies to: 151-153, 221-223, 250-252
102-104: Good switch to require. for error assertions.*
This aligns with testifylint and short‑circuits on failures.Also applies to: 119-121, 185-187, 327-329, 341-343
🧹 Nitpick comments (3)
config/config_test.go (3)
266-273: Use OS‑agnostic non‑existent path to avoid platform assumptions.func TestLoadWithNonExistentFile(t *testing.T) { - var cfg TestConfig - err := config.Load(&cfg, config.WithLocalYAML("/non/existent/file.yaml")) + tmpDir := t.TempDir() + var cfg TestConfig + err := config.Load(&cfg, config.WithLocalYAML(filepath.Join(tmpDir, "does-not-exist.yaml"))) require.NoError(t, err)
275-300: Test name mismatch: only one option is used.
Consider renaming to TestYAMLPlusEnvPrecedence (or add another option) for clarity.
41-44: Reduce repetition with tiny test helpers (writeTempFile/withDotEnv).
Keeps tests compact and consistent; optional.Example helpers:
func writeTempFile(t *testing.T, dir, name, content string) string { t.Helper() p := filepath.Join(dir, name) require.NoError(t, os.WriteFile(p, []byte(content), 0o644)) return p }Also applies to: 77-78, 113-114, 137-139, 148-149, 178-179, 218-220, 247-249, 289-291, 339-340
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/config_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config/config_test.go (2)
config/config.go (2)
Load(33-60)ErrMissingAWSCreds(18-18)config/options.go (3)
WithLocalYAML(20-24)WithS3YAML(28-33)Option(10-10)
🔇 Additional comments (1)
config/config_test.go (1)
29-58: Overall: scenario coverage looks solid and matches intended precedence/behavior.
LGTM across these tests.Also applies to: 60-90, 106-121, 123-162, 164-187, 205-232, 234-263, 317-329, 331-343
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
config/config_test.go (2)
25-26: Nice: t.Chdir usage.Good switch from os.Chdir; avoids CWD leaks and fixes the linter.
289-300: Make TestLoadWithValidAWSCredentials deterministic and avoid require.NotErrorIs.Prevent real S3/IMDS calls (flaky/slow CI) and prefer errors.Is for wider testify compatibility. Also previously requested.
func TestLoadWithValidAWSCredentials(t *testing.T) { // Set up valid AWS credentials t.Setenv("AWS_ACCESS_KEY_ID", "test-key") t.Setenv("AWS_SECRET_ACCESS_KEY", "test-secret") t.Setenv("AWS_REGION", "us-east-1") + // Avoid network calls; fail fast + t.Setenv("AWS_EC2_METADATA_DISABLED", "true") + t.Setenv("AWS_ENDPOINT_URL_S3", "http://127.0.0.1:9") + t.Setenv("AWS_MAX_ATTEMPTS", "1") @@ - // We expect this to fail for other reasons (like S3 connection), but not due to missing credentials - require.NotErrorIs(t, err, config.ErrMissingAWSCreds) + // We expect failure for other reasons (like S3 connection), but not due to missing credentials + require.False(t, errors.Is(err, config.ErrMissingAWSCreds), "unexpected ErrMissingAWSCreds: %v", err) }
🧹 Nitpick comments (2)
config/config_test.go (2)
3-11: Add errors import for the errors.Is check.import ( + "errors" "os" "path/filepath" "testing"
210-211: Use consistent file mode literal (0o644).Matches writeTempFile and improves readability in octal.
- err := os.WriteFile(envFile, []byte(envContent), 0644) + err := os.WriteFile(envFile, []byte(envContent), 0o644)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/config_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config/config_test.go (2)
config/config.go (2)
Load(33-60)ErrMissingAWSCreds(18-18)config/options.go (3)
WithLocalYAML(20-24)WithS3YAML(28-33)Option(10-10)
🪛 GitHub Check: Lint
config/config_test.go
[failure] 22-22:
withDotEnv - result 0 (string) is never used (unparam)
🪛 GitHub Actions: Go
config/config_test.go
[error] 22-22: golangci-lint run --timeout=5m: config/config_test.go:22:52: withDotEnv - result 0 (string) is never used (unparam)
🔇 Additional comments (1)
config/config_test.go (1)
127-127: Comment/test mismatch: S3 precedence not exercised here.Either adjust the comment to “YAML < .env < env vars” or add an S3 precedence case in a separate test.
-// TestConfigurationPrecedence tests configuration precedence (S3 < YAML < .env < env vars) +// TestConfigurationPrecedence tests configuration precedence (YAML < .env < env vars)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
config/config_test.go (1)
286-299: Make TestLoadWithValidAWSCredentials deterministic (no real S3) and avoid testify version coupling.Without guarding, the AWS SDK may hit IMDS/network and be slow/flaky. Also NotErrorIs ties you to newer testify.
func TestLoadWithValidAWSCredentials(t *testing.T) { // Set up valid AWS credentials t.Setenv("AWS_ACCESS_KEY_ID", "test-key") t.Setenv("AWS_SECRET_ACCESS_KEY", "test-secret") t.Setenv("AWS_REGION", "us-east-1") + // Avoid IMDS & real S3 calls; fail fast offline + t.Setenv("AWS_EC2_METADATA_DISABLED", "true") + t.Setenv("AWS_ENDPOINT_URL_S3", "http://127.0.0.1:9") + t.Setenv("AWS_MAX_ATTEMPTS", "1") @@ - // We expect this to fail for other reasons (like S3 connection), but not due to missing credentials - require.NotErrorIs(t, err, config.ErrMissingAWSCreds) + // We expect failure for other reasons (e.g., connect), but not ErrMissingAWSCreds + require.False(t, errors.Is(err, config.ErrMissingAWSCreds), "unexpected ErrMissingAWSCreds: %v", err)Also consider adding the same IMDS-disabling env in the missing/partial/empty creds tests to reduce credential chain fallbacks.
🧹 Nitpick comments (5)
config/config_test.go (5)
126-157: Strengthen precedence test to actually validate .env > YAML.As written, env overrides everything; it doesn’t prove .env beats YAML. Drop the SERVER__PORT env var and assert 8080 (from .env).
- t.Setenv("SERVER__PORT", "8888") @@ - // Environment variables should have highest precedence + // Env should win for DB values; for server, .env should beat YAML assert.Equal(t, "env-host", cfg.Database.Host) assert.Equal(t, 9999, cfg.Database.Port) - assert.Equal(t, 8888, cfg.Server.Port) + assert.Equal(t, 8080, cfg.Server.Port)
203-214: Deduplicate: reuse withDotEnv helper here.Keeps setup consistent and removes boilerplate.
tmpDir := t.TempDir() - envFile := filepath.Join(tmpDir, ".env") - - err := os.WriteFile(envFile, []byte(envContent), 0644) - require.NoError(t, err) - - // Change to temp directory - t.Chdir(tmpDir) + withDotEnv(t, tmpDir, envContent)
3-11: Import errors for the assertion change above.Required for errors.Is.
import ( + "errors" "os" "path/filepath" "testing"
98-110: Disable IMDS in the missing-creds test to avoid slow fallbacks.Minor determinism/CI speedup.
// Ensure AWS credentials are not set t.Setenv("AWS_ACCESS_KEY_ID", "") t.Setenv("AWS_SECRET_ACCESS_KEY", "") t.Setenv("AWS_REGION", "") + t.Setenv("AWS_EC2_METADATA_DISABLED", "true")Apply similarly to partial/empty creds tests if you see flakiness.
1-1: Quick repo verification — lint invariantsNo os.Chdir or assert.* error assertions found. One require.NotErrorIs usage: config/config_test.go:298 —
require.NotErrorIs(t, err, config.ErrMissingAWSCreds). Replace with an errors.Is-based check, e.g.require.False(t, errors.Is(err, config.ErrMissingAWSCreds)).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/config_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config/config_test.go (2)
config/config.go (2)
Load(33-60)ErrMissingAWSCreds(18-18)config/options.go (3)
WithLocalYAML(20-24)WithS3YAML(28-33)Option(10-10)
🔇 Additional comments (2)
config/config_test.go (2)
13-19: Helper is clean and safe.Simple, deterministic temp writer with proper perms and require on error. LGTM.
21-26: Good use of t.Chdir.Prevents CWD leaks and addresses common linter complaints. Nice.
capcom6
force-pushed
the
config/migrate-to-koanf
branch
from
d0a6358 to
47691c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
config/config.go (4)
34-36: LGTM: exhaustruct fix.Using new(options) resolves the exhaustruct lint cleanly.
18-18: LGTM: sentinel error for AWS creds.ErrMissingAWSCreds addresses err113 and clarifies intent.
37-55: LGTM: helper split + precedence.The decomposition drops cognitive complexity and preserves S3 < YAML < .env < env precedence.
85-88: Skip common S3 “not found” variants, not just literal 404.Broaden not-found handling to align with docs and avoid noisy failures from NoSuchKey/NotFound.
- err := k.Load(s3.Provider(s3Config), yaml.Parser()) - if err != nil && !strings.Contains(err.Error(), "404") { - return fmt.Errorf("load s3: %w", err) - } + err := k.Load(s3.Provider(s3Config), yaml.Parser()) + if err != nil { + msg := strings.ToLower(err.Error()) + if errors.Is(err, os.ErrNotExist) || + strings.Contains(msg, "nosuchkey") || + strings.Contains(msg, "not found") || + strings.Contains(msg, "status code: 404") || + strings.Contains(msg, "404") { + // skip missing object/bucket + } else { + return fmt.Errorf("load s3: %w", err) + } + }config/config_test.go (2)
21-26: LGTM: use t.Chdir in helpers.Prevents CWD leaks across tests and satisfies linter.
282-293: Make S3 test deterministic and avoid testify version coupling.Disable IMDS, force an unreachable endpoint, cap retries, and avoid NotErrorIs dependency.
Add import:
@@ -import ( +import ( + "errors"Refactor the test:
func TestLoadWithValidAWSCredentials(t *testing.T) { // Set up valid AWS credentials t.Setenv("AWS_ACCESS_KEY_ID", "test-key") t.Setenv("AWS_SECRET_ACCESS_KEY", "test-secret") t.Setenv("AWS_REGION", "us-east-1") + // Avoid real network/IMDS and fail fast + t.Setenv("AWS_EC2_METADATA_DISABLED", "true") + t.Setenv("AWS_ENDPOINT_URL_S3", "http://127.0.0.1:9") + t.Setenv("AWS_MAX_ATTEMPTS", "1") // Test that the function doesn't return ErrMissingAWSCreds when credentials are present var cfg TestConfig err := config.Load(&cfg, config.WithS3YAML("test-bucket", "test-key")) // We expect this to fail for other reasons (like S3 connection), but not due to missing credentials - require.NotErrorIs(t, err, config.ErrMissingAWSCreds) + require.False(t, errors.Is(err, config.ErrMissingAWSCreds), "unexpected ErrMissingAWSCreds: %v", err) }
🧹 Nitpick comments (4)
config/config.go (3)
67-71: Support AWS_ENDPOINT_URL_S3 as an alias.Improves DX and avoids relying on a non-standard env name.
- endpoint := os.Getenv("AWS_ENDPOINT") + endpoint := os.Getenv("AWS_ENDPOINT") + if endpoint == "" { + endpoint = os.Getenv("AWS_ENDPOINT_URL_S3") + }
20-33: Doc tweak: call out S3 not-found skip explicitly.Clarify behavior for 404/NoSuchKey to match implementation.
-// If any of the above sources result in an error (other than `os.ErrNotExist`), it will be returned. +// If any of the above sources result in an error (other than `os.ErrNotExist`), it will be returned. +// For S3, common "not found" conditions (e.g., 404/NoSuchKey) are treated as non-fatal and skipped.
143-151: CSV heuristic is over-broad; consider opt-in.Blindly splitting on commas can mangle URLs, DSNs, etc. Prefer an opt‑in prefix.
- // CSV -> []string - if strings.Contains(v, ",") { + // CSV (opt-in via "csv:" prefix) -> []string + if strings.HasPrefix(v, "csv:") { + v = strings.TrimPrefix(v, "csv:") parts := strings.Split(v, ",") for i := range parts { parts[i] = strings.TrimSpace(parts[i]) } return k, parts }If you rely on the current implicit CSV behavior anywhere, keep it and we can scope a safer heuristic instead.
config/config_test.go (1)
196-217: Optional: add coverage for JSON arrays and CSV opt-in.Validates envTransform for []any and csv: behavior.
func TestEnvTransform_JSONArray_And_CSV(t *testing.T) { t.Setenv("FEATURE_FLAGS", `["debug","beta"]`) t.Setenv("SERVER__ALLOWED_HOSTS", "csv:api.local, admin.local") var cfg struct { FeatureFlags []string `koanf:"feature_flags"` Server struct { AllowedHosts []string `koanf:"allowed_hosts"` } `koanf:"server"` } require.NoError(t, config.Load(&cfg)) require.ElementsMatch(t, []string{"debug", "beta"}, cfg.FeatureFlags) require.ElementsMatch(t, []string{"api.local", "admin.local"}, cfg.Server.AllowedHosts) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
config/config.go(1 hunks)config/config_test.go(1 hunks)config/options.go(1 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- config/options.go
- go.mod
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T00:53:47.798Z
Learnt from: capcom6
PR: android-sms-gateway/core#7
File: config/config.go:7-15
Timestamp: 2025-09-10T00:53:47.798Z
Learning: The knanf/parsers/dotenv package provides ParserEnv function with signature ParserEnv(prefix, delim string, cb func(s string) string) that allows the DOTENV parser to behave like the env.Provider.
Applied to files:
config/config.go
🧬 Code graph analysis (2)
config/config_test.go (2)
config/config.go (2)
Load(33-60)ErrMissingAWSCreds(18-18)config/options.go (3)
WithLocalYAML(20-24)WithS3YAML(28-33)Option(10-10)
config/config.go (1)
config/options.go (1)
Option(10-10)
🔇 Additional comments (1)
config/config.go (1)
106-113: LGTM: dotenv via ParserEnvWithValue + transform.Good choice to mirror env.Provider semantics and enable typed values.
Summary by CodeRabbit