Support user config file#203
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds XDG user config/cache helpers, centralizes a package-level logger and applies per-user YAML defaults at startup, converts several deploy booleans to pointer-backed fields with Set/Enabled accessors, updates imagecache.New to return errors, and updates commands and tests to use the new APIs. ChangesUser Config Infrastructure and Configuration Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
mclasmeier
force-pushed
the
mc/user-config
branch
2 times, most recently
from
97ae030 to
a58fe13
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/deployer/deployer.go (1)
230-245:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClean up
tempDirbefore returning on image cache init failure.If
imagecache.New(...)fails,New(...)returns immediately and leaves the temp directory created earlier on disk. Repeated failures will leak directories.Suggested fix
d.dockerAuth = dockerauth.New(log) d.imageCache, err = imagecache.New(log, "", 20) if err != nil { - return nil, err + _ = os.RemoveAll(tempDir) + return nil, fmt.Errorf("failed to initialize image cache: %w", err) }🤖 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 `@internal/deployer/deployer.go` around lines 230 - 245, The tempDir created before initializing the Deployer is not removed if imagecache.New(...) fails, leaking a directory; update the New/constructor path in internal/deployer/deployer.go (around tempDir, Deployer struct init, and the imageCache assignment where imagecache.New is called) to remove the tempDir on error (e.g., call os.RemoveAll(tempDir)) before returning the error so the temporary directory is cleaned up on failure.
🤖 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 `@cmd/deploy_test.go`:
- Around line 271-272: The tests currently only set HOME via t.Setenv("HOME",
t.TempDir()), but XDG_CONFIG_HOME can still redirect xdg.UserConfigPath();
update each test instance that sets HOME (the t.Setenv("HOME", t.TempDir())
calls) to also set XDG_CONFIG_HOME to the same temp dir (e.g., add
t.Setenv("XDG_CONFIG_HOME", t.TempDir()) or reuse the same temp dir variable) so
path resolution is deterministic; apply the same change to the other occurrence
noted in the file.
In `@cmd/main.go`:
- Around line 52-55: Do not silently return nil when xdg.UserConfigPath() fails;
instead propagate the error (wrap it) or at minimum log a warning so caller
knows user-config couldn't be resolved. Replace the current branch that does "if
err != nil { return nil }" with code that returns a wrapped error like
fmt.Errorf("resolve user config path: %w", err) (or calls processLogger.Warn/
log.Printf with the error and continues), and ensure the containing function
signature and callers accept/handle the returned error; update imports (fmt or
log) as needed.
- Around line 23-33: globalLogger is declared but only initialized in main(),
causing nil-pointer panics in command handlers like runDeploy and runTeardown
when those functions are invoked outside main(); fix by ensuring globalLogger is
initialized at package load or via a lazy accessor: either set globalLogger =
logger.New() at declaration time (replace the nil pointer declaration) or
implement a getGlobalLogger() that checks if globalLogger==nil and sets it to
logger.New() before returning; update runDeploy/runTeardown to call
getGlobalLogger() (or rely on the package-level initialized globalLogger) so
they never dereference a nil logger.
---
Outside diff comments:
In `@internal/deployer/deployer.go`:
- Around line 230-245: The tempDir created before initializing the Deployer is
not removed if imagecache.New(...) fails, leaking a directory; update the
New/constructor path in internal/deployer/deployer.go (around tempDir, Deployer
struct init, and the imageCache assignment where imagecache.New is called) to
remove the tempDir on error (e.g., call os.RemoveAll(tempDir)) before returning
the error so the temporary directory is cleaned up on failure.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a4b55c3-3288-4a61-b134-df6f99d4da73
📒 Files selected for processing (10)
.github/workflows/docker-build.ymlcmd/deploy.gocmd/deploy_test.gocmd/env.gocmd/main.gocmd/teardown.gointernal/deployer/deployer.gointernal/imagecache/imagecache.gointernal/imagecache/imagecache_test.gointernal/xdg/xdg.go
💤 Files with no reviewable changes (1)
- .github/workflows/docker-build.yml
mclasmeier
force-pushed
the
mc/user-config
branch
6 times, most recently
from
c996323 to
88cd1f0
Compare
mclasmeier
force-pushed
the
mc/user-config
branch
from
88cd1f0 to
816d46b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/paths/paths.go (1)
30-36: ⚡ Quick winConsider wrapping the error for consistency and better debugging context.
CacheDir()returns the raw error fromos.UserCacheDir()without context, whileUserConfigPath()wraps its error with "retrieving user config path". Adding similar wrapping here would improve error messages when cache directory resolution fails.♻️ Suggested improvement
func CacheDir() (string, error) { dir, err := os.UserCacheDir() if err != nil { - return "", err + return "", fmt.Errorf("retrieving cache directory: %w", err) } return filepath.Join(dir, appName), nil }🤖 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 `@internal/paths/paths.go` around lines 30 - 36, CacheDir() currently returns the raw error from os.UserCacheDir(); change it to wrap the error with contextual text (e.g., "retrieving user cache path") before returning so it matches UserConfigPath()'s behavior and gives better debugging context — update the return in CacheDir() to wrap err (using fmt.Errorf(...%w...) or your project's error-wrap helper) while keeping the final filepath.Join(dir, appName) result.
🤖 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 `@internal/deployer/deployer.go`:
- Around line 242-245: When imagecache.New(log, "", 20) fails, the tempDir
created earlier (variable tempDir) is left behind; update the initialization in
NewDeployer (or the constructor where tempDir is created) to call
os.RemoveAll(tempDir) before returning the error from imagecache.New so the
temporary directory is cleaned up when imagecache.New returns an error; ensure
you reference the same tempDir variable and remove it prior to returning nil,
err so callers don't need to rely on deployer.Cleanup().
---
Nitpick comments:
In `@internal/paths/paths.go`:
- Around line 30-36: CacheDir() currently returns the raw error from
os.UserCacheDir(); change it to wrap the error with contextual text (e.g.,
"retrieving user cache path") before returning so it matches UserConfigPath()'s
behavior and gives better debugging context — update the return in CacheDir() to
wrap err (using fmt.Errorf(...%w...) or your project's error-wrap helper) while
keeping the final filepath.Join(dir, appName) result.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: f3e2d8d0-8ddb-417d-bac7-fc9b5d7f3e12
📒 Files selected for processing (12)
cmd/deploy.gocmd/deploy_test.gocmd/env.gocmd/main.gocmd/teardown.gointernal/deployer/config.gointernal/deployer/deploy_via_operator.gointernal/deployer/deployer.gointernal/deployer/operator.gointernal/imagecache/imagecache.gointernal/imagecache/imagecache_test.gointernal/paths/paths.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/imagecache/imagecache_test.go
- cmd/teardown.go
- cmd/env.go
- internal/imagecache/imagecache.go
- cmd/main.go
|
@coderabbitai, addressed your nitpick |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
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 (2)
internal/deployer/config.go (2)
118-131:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
new(true)is invalid Go syntax—code will not compile.The builtin
new()takes a type argument, not a value.new(true)will fail with:true is not a type. To create a*boolpointing totrue, use a helper function or thek8s.io/utils/ptrpackage.🐛 Proposed fix using a helper function
Add a helper at the top of the file:
func boolPtr(b bool) *bool { return &b }Then apply this diff:
func DefaultCentralConfig() CentralConfig { return CentralConfig{ DeployTimeout: DefaultCentralWaitTimeout, Namespace: "acs-central", - EarlyReadiness: new(true), + EarlyReadiness: boolPtr(true), Spec: map[string]interface{}{Alternatively, if
k8s.io/utils/ptris already a dependency:import "k8s.io/utils/ptr" // ... EarlyReadiness: ptr.To(true),🤖 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 `@internal/deployer/config.go` around lines 118 - 131, The EarlyReadiness field in DefaultCentralConfig is using invalid Go syntax `new(true)`; replace that with a pointer-to-bool value (e.g., add a helper func boolPtr(b bool) *bool { return &b } and set EarlyReadiness: boolPtr(true)) or, if available, use the k8s helper and import "k8s.io/utils/ptr" and set EarlyReadiness: ptr.To(true); update DefaultCentralConfig accordingly so it compiles.
251-259:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSame
new(true)compile error as inDefaultCentralConfig.Apply the same fix here using the helper function.
🐛 Proposed fix
func DefaultSecuredClusterConfig() SecuredClusterConfig { return SecuredClusterConfig{ DeployTimeout: DefaultSecuredClusterWaitTimeout, Namespace: "acs-sensor", - EarlyReadiness: new(true), + EarlyReadiness: boolPtr(true), Spec: make(map[string]interface{}), } }🤖 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 `@internal/deployer/config.go` around lines 251 - 259, DefaultSecuredClusterConfig is using new(true) for the EarlyReadiness field which causes the same compile error as before; replace the new(true) expression in the DefaultSecuredClusterConfig function (the EarlyReadiness field) with the boolean pointer helper used in DefaultCentralConfig (the same helper function you used there) so EarlyReadiness is set via that helper function call instead of new(true).
🤖 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 `@internal/deployer/config.go`:
- Around line 118-131: The EarlyReadiness field in DefaultCentralConfig is using
invalid Go syntax `new(true)`; replace that with a pointer-to-bool value (e.g.,
add a helper func boolPtr(b bool) *bool { return &b } and set EarlyReadiness:
boolPtr(true)) or, if available, use the k8s helper and import
"k8s.io/utils/ptr" and set EarlyReadiness: ptr.To(true); update
DefaultCentralConfig accordingly so it compiles.
- Around line 251-259: DefaultSecuredClusterConfig is using new(true) for the
EarlyReadiness field which causes the same compile error as before; replace the
new(true) expression in the DefaultSecuredClusterConfig function (the
EarlyReadiness field) with the boolean pointer helper used in
DefaultCentralConfig (the same helper function you used there) so EarlyReadiness
is set via that helper function call instead of new(true).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 11c109a6-282c-46ab-91b8-8801a22e4397
📒 Files selected for processing (4)
cmd/deploy.gocmd/deploy_test.gointernal/deployer/config.gointernal/deployer/deploy_via_operator.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/deployer/deploy_via_operator.go
- cmd/deploy_test.go
- cmd/deploy.go
|
@coderabbitai, what you are claiming there wrt to |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query:
Length of output: 8128
So ✏️ Learnings added
|
Introduce function for applying user defaults.
Otherwise overwriting of bool settings with a user-config would only work in one direction: from false to true. By using pointers we can reliably detect "is this field set or not" and overwrite. Adjusted call sites for new bool ptrs.
We use this for temporarily saving just the result from the translation of command line args to YAML patches.
mclasmeier
force-pushed
the
mc/user-config
branch
from
83ae551 to
f77598e
Compare
1 participant
Load user defaults from
~/.config/roxie/config.yaml(or$XDG_CONFIG_HOME/roxie/config.yaml) before command execution. The user config overrides hardcoded defaults and is itself overridden by CLI flags.internal/xdgpackage for resolving config and cache directories~/.roxie.image_cacheto~/.cache/roxie/image_cacheglobalLoggercreated inmain(), shared across all commandsfalsebooleans) reliable, this PR also replaces remaining booleans in the config structs with pointers.Important use-case: Allow colleagues who are used to certain workflows to configure roxie according to their needs. Specifically thinking about:
So they don't need to keep repeating
roxie deploy --pause-reconciliation --single-namespace.Summary by CodeRabbit
New Features
Improvements
Tests