Skip to content

Support user config file#203

Open
mclasmeier wants to merge 8 commits into
mainfrom
mc/user-config
Open

Support user config file#203
mclasmeier wants to merge 8 commits into
mainfrom
mc/user-config

Conversation

@mclasmeier
Copy link
Copy Markdown
Collaborator

@mclasmeier mclasmeier commented Jun 4, 2026

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.

  • New internal/xdg package for resolving config and cache directories
  • Migrate image cache from ~/.roxie.image_cache to ~/.cache/roxie/image_cache
  • Consolidate logger: single globalLogger created in main(), shared across all commands
  • Apply user defaults before command execution

Important use-case: Allow colleagues who are used to certain workflows to configure roxie according to their needs. Specifically thinking about:

central:
  namespace: stackrox
  pauseReconciliation: true
securedCluster:
  namespace: stackrox
  pauseReconciliation: true

So they don't need to keep repeating roxie deploy --pause-reconciliation --single-namespace.

@mclasmeier mclasmeier added the ishur-review AI Hackathon test label label Jun 4, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 4, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

Roxie adopts XDG-compliant user configuration and cache directories, centralizes logger initialization with user config defaults, and updates all command handlers to use a global logger instance. The imagecache module gains error-aware XDG cache directory resolution, deployer handles imagecache initialization errors, and a new tryApplyUserDefaults function merges per-user settings at startup.

Changes

User config and cache integration

Layer / File(s) Summary
XDG directory helpers
internal/xdg/xdg.go
New module exposes UserConfigPath() and CacheDir() functions that return per-user roxie config and cache paths under the system XDG directories, propagating errors from os.UserConfigDir() and os.UserCacheDir().
Imagecache error handling and XDG cache integration
internal/imagecache/imagecache.go, internal/imagecache/imagecache_test.go
imagecache.New now returns (*ImageCache, error) and uses xdg.CacheDir() for the default cache location. All four test functions updated to capture and require no error when creating cache instances.
Deployer error handling for imagecache
internal/deployer/deployer.go
deployer.New() now captures the error from imagecache.New and returns early if cache initialization fails.
Centralized logger and user config defaults in main
cmd/main.go
Introduces package-level globalLogger, moves deploySettings into a shared var block, refactors main() to initialize the logger and delegate to a new run() function. Adds tryApplyUserDefaults to locate, read, and merge per-user roxie config from the XDG config path using mergo.Merge with override semantics.
Command layer updates to use globalLogger
cmd/deploy.go, cmd/env.go, cmd/teardown.go
All three command handlers switch from logger.New() to using globalLogger; cmd/env.go removes the now-unused logger import; cmd/deploy.go converts sharedNamespace from var to const.
Tests for user config defaults
cmd/deploy_test.go
Adds imports for yaml.v3, mergo, logger, and xdg; introduces TestApplyUserDefaults table-driven test covering no user config (unchanged), empty field merging, override behavior, and invalid YAML error cases.

CI workflow adjustment

Layer / File(s) Summary
Docker build workflow tag trigger removal
.github/workflows/docker-build.yml
Removes tags: - 'v*' from the push trigger; workflow now runs only on push to main and pull requests targeting main, not on version tag pushes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • vladbologa
  • davdhacs
  • AlexVulaj

Poem

A rabbit hops through config files with glee,
Centralizing logs for harmony! 🐰
XDG paths now guide the cache,
User defaults applied in a flash—
Configuration, cleaner than ever to see! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main objective of the pull request: adding support for loading user configuration from a config file.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mc/user-config

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mclasmeier mclasmeier force-pushed the mc/user-config branch 2 times, most recently from 97ae030 to a58fe13 Compare June 4, 2026 16:05
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Clean up tempDir before 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

📥 Commits

Reviewing files that changed from the base of the PR and between 76f4de3 and 4e9865f.

📒 Files selected for processing (10)
  • .github/workflows/docker-build.yml
  • cmd/deploy.go
  • cmd/deploy_test.go
  • cmd/env.go
  • cmd/main.go
  • cmd/teardown.go
  • internal/deployer/deployer.go
  • internal/imagecache/imagecache.go
  • internal/imagecache/imagecache_test.go
  • internal/xdg/xdg.go
💤 Files with no reviewable changes (1)
  • .github/workflows/docker-build.yml

Comment thread cmd/deploy_test.go Outdated
Comment thread cmd/main.go Outdated
Comment thread cmd/main.go
@mclasmeier mclasmeier force-pushed the mc/user-config branch 3 times, most recently from 574ab05 to 8500c92 Compare June 4, 2026 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ishur-review AI Hackathon test label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant