feat: supporting docker secrets#47
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Docker Secrets support: config.Get() reflectively reads secret files for fields tagged secret:"true", then parses env vars and validates config. Integrates changes into main and client initialization, updates tests for secret loading and precedence, and documents the new Docker Secrets usage. ChangesDocker Secrets Configuration Implementation
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/config/config_test.go (1)
214-229: ⚡ Quick winAdd one positive assertion for host loading from secrets.
This test skips every
*_HOSTfield by overriding them from env, so the suite never proves that host URLs actually load from Docker secrets even though that is part of the new contract.🤖 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/config/config_test.go` around lines 214 - 229, The test currently skips host fields (envName "NGINX_PROXY_MANAGER_HOST" and "PIHOLE_HOST") so it never verifies that host URL fields are loaded from secrets; update the loop in Test (where cfgVal := reflect.ValueOf(config).Elem(), typ and field are used) to include at least one positive assertion for a host field: when envName matches one of the host keys, do not continue—compute expected := "secret-val-for-"+envName and assert.Equal(t, expected, cfgVal.Field(i).String(), "Field %s was not loaded correctly from secret %s") so the test proves host-loading from secrets (reference: cfgVal, typ, field.Tag, envName, NGINX_PROXY_MANAGER_HOST, PIHOLE_HOST).
🤖 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.
Inline comments:
In `@main.go`:
- Around line 26-29: The startup error message in the main function is too
specific for what config.Get() now does. Update the log.Error call in the
config.Get() error path to use a broader message that reflects loading and
validating configuration from all supported sources, while still keeping the
existing "error", err structured field.
In `@pkg/config/config_test.go`:
- Around line 15-18: The test TestGetConfig_EnvVars (and other tests calling
Get()) is not isolated from ambient .env files because Get() calls
godotenv.Load(); before calling Get() change the current working directory to an
empty temp dir (e.g., use t.TempDir() and os.Chdir) so godotenv.Load() can't
read a checked-in .env and then restore the cwd in a defer; update tests that
call unsetAllConfigEnvVars(), dockerSecretRootPath, or Get() to perform this cwd
switch to ensure missing-variable cases are deterministic.
In `@pkg/config/config.go`:
- Line 54: The code currently calls strings.TrimSpace on the secret file content
(variable content) which strips all leading/trailing whitespace; change it to
only strip line endings so significant spaces in secrets are preserved—for
example replace the strings.TrimSpace(string(content)) usage on the return line
with a call that only removes "\n" and "\r" (e.g. using strings.TrimRight with
"\r\n" or equivalent) so leading/trailing spaces remain intact while Docker
newline characters are removed.
---
Nitpick comments:
In `@pkg/config/config_test.go`:
- Around line 214-229: The test currently skips host fields (envName
"NGINX_PROXY_MANAGER_HOST" and "PIHOLE_HOST") so it never verifies that host URL
fields are loaded from secrets; update the loop in Test (where cfgVal :=
reflect.ValueOf(config).Elem(), typ and field are used) to include at least one
positive assertion for a host field: when envName matches one of the host keys,
do not continue—compute expected := "secret-val-for-"+envName and
assert.Equal(t, expected, cfgVal.Field(i).String(), "Field %s was not loaded
correctly from secret %s") so the test proves host-loading from secrets
(reference: cfgVal, typ, field.Tag, envName, NGINX_PROXY_MANAGER_HOST,
PIHOLE_HOST).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d4582aa-6af4-448a-bb02-a45f4fe99997
📒 Files selected for processing (5)
docs/configuration.mdmain.gopkg/clients/clients.gopkg/config/config.gopkg/config/config_test.go
Summary by CodeRabbit
New Features
Documentation
Validation
Tests