caib: change config paths to follow XDG practices#146
Merged
Conversation
📝 WalkthroughWalkthroughReplaces hard-coded ~/.caib paths with XDG-style helpers: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/caib/config/config.go (1)
62-67: Replace hardcoded.config/.cachepaths withos.UserConfigDir()/os.UserCacheDir().The code currently uses
os.UserHomeDir()with hardcoded.configand.cachesubdirectories, which bypassesXDG_CONFIG_HOMEandXDG_CACHE_HOMEon Linux and ignores platform-specific conventions on macOS (~/Library/Application Support,~/Library/Caches) and Windows (%AppData%,%LocalAppData%). Go 1.22'sos.UserConfigDir()andos.UserCacheDir()handle all three platforms correctly and respect XDG environment variables on Linux.Update
JumpstarterEndpoint()(line 62),ConfigDirPath()(line 199), andCacheDirPath()(line 208) to use the appropriate stdlib helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/config/config.go` around lines 62 - 67, Replace the hardcoded use of os.UserHomeDir() + ".config"/".cache" in JumpstarterEndpoint, ConfigDirPath, and CacheDirPath with the platform-aware helpers: call os.UserConfigDir() when building config paths (including the "jumpstarter" subfolder) and os.UserCacheDir() for cache paths; handle and return errors consistently if those calls fail and update any filepath.Join usages to join the returned config/cache dir with "jumpstarter" (or other subpaths) instead of using ".config" or ".cache".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/config/config.go`:
- Around line 161-167: Read() currently only reads from configFilePath(),
causing upgrades to lose legacy configs; update Read() to try the new path first
and if reading fails with os.IsNotExist (or when file missing) attempt a legacy
path returned by a new helper legacyConfigFilePath() and read that file instead;
implement legacyConfigFilePath() (use os.UserHomeDir and filepath.Join(home,
".caib", configFile)) and return the data loaded from legacy if present so
CLIConfig and callers of Read() receive the legacy configuration transparently.
---
Nitpick comments:
In `@cmd/caib/config/config.go`:
- Around line 62-67: Replace the hardcoded use of os.UserHomeDir() +
".config"/".cache" in JumpstarterEndpoint, ConfigDirPath, and CacheDirPath with
the platform-aware helpers: call os.UserConfigDir() when building config paths
(including the "jumpstarter" subfolder) and os.UserCacheDir() for cache paths;
handle and return errors consistently if those calls fail and update any
filepath.Join usages to join the returned config/cache dir with "jumpstarter"
(or other subpaths) instead of using ".config" or ".cache".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c74234f8-7b4d-4862-8f33-3d943c66e89e
📒 Files selected for processing (8)
cmd/caib/auth/config.gocmd/caib/auth/config_test.gocmd/caib/auth/oidc.gocmd/caib/auth/oidc_test.gocmd/caib/auth/wrapper_test.gocmd/caib/config/config.gocmd/caib/config/config_test.gocmd/caib/root.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/config/config.go`:
- Around line 62-67: DeriveServerFromJumpstarter currently hardcodes
~/.config/jumpstarter by using os.UserHomeDir and filepath.Join, which ignores
XDG_CONFIG_HOME; update the discovery to use os.UserConfigDir() and join that
directory with "jumpstarter" (replace the jmpDir construction in
DeriveServerFromJumpstarter / config.go), keeping the explicit "~/.config/caib"
and "~/.cache/caib" usage only for caib-owned files as intended so Jumpstarter
discovery respects user XDG config overrides.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87ca4b1c-f4b3-4340-969a-19667a71f158
📒 Files selected for processing (6)
cmd/caib/auth/config.gocmd/caib/auth/config_test.gocmd/caib/auth/oidc_test.gocmd/caib/auth/wrapper_test.gocmd/caib/config/config.gocmd/caib/config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/caib/auth/oidc_test.go
- cmd/caib/auth/wrapper_test.go
- cmd/caib/auth/config_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cmd/caib/auth/config_test.go (1)
153-153: ReuseoidcConfigPath()instead of rebuilding the auth config path.These cases hardcode
.config/caib/config.jsoneven thoughcmd/caib/auth/config.goalready resolves that path fromHOME. Calling the helper here would remove another copy of the layout and make future path changes cheaper.♻️ Minimal cleanup
- configDir := filepath.Join(tempDir, ".config", "caib") + configPath, err := oidcConfigPath() + Expect(err).NotTo(HaveOccurred()) + configDir := filepath.Dir(configPath)- configPath := filepath.Join(tempDir, ".config", "caib", "config.json") + configPath, err := oidcConfigPath() + Expect(err).NotTo(HaveOccurred())Also applies to: 182-182, 194-194, 243-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/auth/config_test.go` at line 153, The test hardcodes the auth config path using filepath.Join(tempDir, ".config", "caib") (and similar occurrences at the other lines) instead of reusing the project's resolver; replace those hardcoded constructions in cmd/caib/auth/config_test.go with calls to oidcConfigPath() (or a test helper that calls oidcConfigPath() after setting HOME to tempDir) so the tests derive the same .config/caib/config.json location as config.go; update all occurrences mentioned (lines ~153, 182, 194, 243) to use oidcConfigPath() to avoid duplicate layout logic.cmd/caib/auth/wrapper_test.go (1)
109-109: Build the cache dir fromtokenCachePath()here as well.
RefreshCachedToken()ultimately relies ontokenCachePath(), so these tests can derive the directory from the same helper instead of embedding another copy of.cache/caib.♻️ Minimal cleanup
- cacheDir := filepath.Join(tempDir, ".cache", "caib") + cachePath, err := tokenCachePath() + Expect(err).NotTo(HaveOccurred()) + cacheDir := filepath.Dir(cachePath)Also applies to: 174-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/auth/wrapper_test.go` at line 109, Tests are constructing cacheDir manually; instead derive the directory using the existing tokenCachePath helper so RefreshCachedToken() and the tests use the same logic—replace the manual filepath.Join(tempDir, ".cache", "caib") with a call to tokenCachePath(tempDir) (or the directory part of its result) where cacheDir is set in wrapper_test.go (occurrences around the cacheDir assignment and the similar occurrence later near line 174) to ensure consistency with RefreshCachedToken()'s behavior.cmd/caib/auth/oidc_test.go (1)
114-114: UsetokenCachePath()in these tests.These literals duplicate the cache-path logic that
cmd/caib/auth/oidc.gonow centralizes. SinceHOMEis already isolated inBeforeEach, reusing the helper here would keep the tests aligned with the production path format.♻️ Minimal cleanup
- cachePath := filepath.Join(tempDir, ".cache", "caib", tokenCacheFile) + cachePath, err := tokenCachePath() + Expect(err).NotTo(HaveOccurred())- cacheDir := filepath.Join(tempDir, ".cache", "caib") + cachePath, err := tokenCachePath() + Expect(err).NotTo(HaveOccurred()) + cacheDir := filepath.Dir(cachePath)Also applies to: 227-227, 249-249, 267-267, 422-422, 467-467, 519-519, 560-560, 610-610
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/auth/oidc_test.go` at line 114, Tests in oidc_test.go currently construct cachePath using filepath.Join(tempDir, ".cache", "caib", tokenCacheFile) which duplicates the production logic; replace those literals with calls to the centralized helper tokenCachePath() so tests use the same path computation as cmd/caib/auth/oidc.go. Locate each occurrence (e.g., the assignment to cachePath at the shown line and the other occurrences flagged) and replace the manual join with tokenCachePath() (ensuring any required tempDir/HOME setup from BeforeEach still makes tokenCachePath() return the intended path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/caib/auth/config_test.go`:
- Line 153: The test hardcodes the auth config path using filepath.Join(tempDir,
".config", "caib") (and similar occurrences at the other lines) instead of
reusing the project's resolver; replace those hardcoded constructions in
cmd/caib/auth/config_test.go with calls to oidcConfigPath() (or a test helper
that calls oidcConfigPath() after setting HOME to tempDir) so the tests derive
the same .config/caib/config.json location as config.go; update all occurrences
mentioned (lines ~153, 182, 194, 243) to use oidcConfigPath() to avoid duplicate
layout logic.
In `@cmd/caib/auth/oidc_test.go`:
- Line 114: Tests in oidc_test.go currently construct cachePath using
filepath.Join(tempDir, ".cache", "caib", tokenCacheFile) which duplicates the
production logic; replace those literals with calls to the centralized helper
tokenCachePath() so tests use the same path computation as
cmd/caib/auth/oidc.go. Locate each occurrence (e.g., the assignment to cachePath
at the shown line and the other occurrences flagged) and replace the manual join
with tokenCachePath() (ensuring any required tempDir/HOME setup from BeforeEach
still makes tokenCachePath() return the intended path).
In `@cmd/caib/auth/wrapper_test.go`:
- Line 109: Tests are constructing cacheDir manually; instead derive the
directory using the existing tokenCachePath helper so RefreshCachedToken() and
the tests use the same logic—replace the manual filepath.Join(tempDir, ".cache",
"caib") with a call to tokenCachePath(tempDir) (or the directory part of its
result) where cacheDir is set in wrapper_test.go (occurrences around the
cacheDir assignment and the similar occurrence later near line 174) to ensure
consistency with RefreshCachedToken()'s behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3dcd87d1-cab6-4736-ad95-89cfb6311d38
📒 Files selected for processing (6)
cmd/caib/auth/config.gocmd/caib/auth/config_test.gocmd/caib/auth/oidc_test.gocmd/caib/auth/wrapper_test.gocmd/caib/config/config.gocmd/caib/config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/caib/config/config_test.go
- cmd/caib/auth/config.go
bennyz
reviewed
Mar 9, 2026
bennyz
reviewed
Mar 9, 2026
bennyz
reviewed
Mar 9, 2026
Move caib-owned config and OIDC cache files to fixed ~/.config/caib and ~/.cache/caib locations, and update login/help text to match the new paths. Assisted-by: gpt-5.3 Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Adjust caib auth and config tests to use ~/.config/caib and ~/.cache/caib locations so coverage matches the new fixed path behavior. Assisted-by: gpt-5.3 Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Use XDG_CONFIG_HOME and XDG_CACHE_HOME for caib-owned paths with home-directory fallbacks, and update tests to cover custom XDG locations and avoid CI env leakage. Assisted-by: gpt-5.3 Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
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 (2)
cmd/caib/auth/oidc_test.go (1)
127-136:⚠️ Potential issue | 🟡 MinorUnset
HOMEon cleanup when it was absent originally.These blocks restore
XDG_CACHE_HOMEcorrectly, butHOMEis only restored when non-empty. If the test process starts withHOMEunset, subsequent specs keep a removed temp directory as their home.Minimal fix
- if originalHome != "" { - _ = os.Setenv("HOME", originalHome) - } + if originalHome != "" { + _ = os.Setenv("HOME", originalHome) + } else { + _ = os.Unsetenv("HOME") + }Also applies to: 224-233, 433-442, 557-566
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/auth/oidc_test.go` around lines 127 - 136, The AfterEach cleanup currently restores HOME only when originalHome is non-empty, leaving HOME set to the test tempDir if HOME was originally unset; update the cleanup logic in the AfterEach blocks (referencing originalHome, tempDir and AfterEach in oidc_test.go) so that if originalHome is non-empty you call os.Setenv("HOME", originalHome) and otherwise you call os.Unsetenv("HOME"); apply the same change to the other matching AfterEach blocks (the ones at the other locations noted) to ensure HOME is properly unset when it was absent originally.cmd/caib/config/config.go (1)
61-70:⚠️ Potential issue | 🟠 MajorUse
os.UserConfigDir()andos.UserCacheDir()instead of manually resolving XDG paths.
JumpstarterEndpoint,DirPath, andCacheDirPathaccept relative paths inXDG_CONFIG_HOMEandXDG_CACHE_HOMEwithout validation. The XDG Base Directory specification requires these to be absolute paths. When set to relative values (e.g.,XDG_CONFIG_HOME=.), the current code treats them as valid and builds config/cache paths relative to the current directory, leaking state into arbitrary locations.Go's standard library
os.UserConfigDir()andos.UserCacheDir()properly validate these paths and error on relative values, while also eliminating duplicated resolution logic. Replace the manual implementation with these functions.Minimal fix
func JumpstarterEndpoint() string { - xdgBase := os.Getenv("XDG_CONFIG_HOME") - if xdgBase == "" { - home, err := os.UserHomeDir() - if err != nil { - return "" - } - xdgBase = filepath.Join(home, ".config") - } + xdgBase, err := os.UserConfigDir() + if err != nil { + return "" + } jmpDir := filepath.Join(xdgBase, "jumpstarter") @@ func DirPath() (string, error) { - if base := strings.TrimSpace(os.Getenv("XDG_CONFIG_HOME")); base != "" { - return filepath.Join(base, appDirName), nil - } - - home, err := os.UserHomeDir() + base, err := os.UserConfigDir() if err != nil { return "", err } - return filepath.Join(home, ".config", appDirName), nil + return filepath.Join(base, appDirName), nil } @@ func CacheDirPath() (string, error) { - if base := strings.TrimSpace(os.Getenv("XDG_CACHE_HOME")); base != "" { - return filepath.Join(base, appDirName), nil - } - - home, err := os.UserHomeDir() + base, err := os.UserCacheDir() if err != nil { return "", err } - return filepath.Join(home, ".cache", appDirName), nil + return filepath.Join(base, appDirName), nil }Also applies to lines 203–226.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/config/config.go` around lines 61 - 70, The current implementations of JumpstarterEndpoint, DirPath, and CacheDirPath manually read XDG env vars and accept relative values, which can leak state; replace that manual logic with os.UserConfigDir() (for config paths) and os.UserCacheDir() (for cache paths), check for errors and return "" on error, and then filepath.Join the returned directory with "jumpstarter" (and any relative subpath argument used by DirPath/CacheDirPath). Update JumpstarterEndpoint to call os.UserConfigDir(), join with "jumpstarter" to produce the endpoint directory; update DirPath to get cfg := os.UserConfigDir() and return filepath.Join(cfg, "jumpstarter", rel) (or "" on error); update CacheDirPath to use os.UserCacheDir() similarly. Remove the manual XDG_RESOLVE logic and rely on the stdlib functions for absolute-path validation.
🧹 Nitpick comments (1)
cmd/caib/auth/config_test.go (1)
143-145: Add a spec that actually setsXDG_CONFIG_HOME.Every setup here unsets
XDG_CONFIG_HOME, so the new override branch in the auth config path logic still is not exercised. A single read/save case with a custom XDG base would keep this change pinned down.Also applies to: 232-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/auth/config_test.go` around lines 143 - 145, Add a test case that explicitly sets XDG_CONFIG_HOME to a custom temp directory and exercises the auth config read/save logic instead of unsetting it: instead of calling os.Unsetenv("XDG_CONFIG_HOME") in the setup (where originalXDGConfig and os.Setenv("HOME", tempDir) are used), create a new spec that calls os.Setenv("XDG_CONFIG_HOME", customXDGDir) and then invokes the same save/read assertions (the code paths that currently reference originalXDGConfig and the existing read/save helpers) so the XDG override branch is executed; also add teardown to restore originalXDGConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/auth/config_test.go`:
- Around line 148-156: The AfterEach cleanup only restores HOME when
originalHome != "" which leaves HOME set to the temp dir if the test runner
started with HOME unset; change the AfterEach that references originalHome to
restore the original state by calling os.Unsetenv("HOME") when originalHome ==
"" and os.Setenv("HOME", originalHome) otherwise (mirror the existing
XDG_CONFIG_HOME handling). Update the other AfterEach block that uses
originalHome the same way so both restore HOME correctly.
---
Outside diff comments:
In `@cmd/caib/auth/oidc_test.go`:
- Around line 127-136: The AfterEach cleanup currently restores HOME only when
originalHome is non-empty, leaving HOME set to the test tempDir if HOME was
originally unset; update the cleanup logic in the AfterEach blocks (referencing
originalHome, tempDir and AfterEach in oidc_test.go) so that if originalHome is
non-empty you call os.Setenv("HOME", originalHome) and otherwise you call
os.Unsetenv("HOME"); apply the same change to the other matching AfterEach
blocks (the ones at the other locations noted) to ensure HOME is properly unset
when it was absent originally.
In `@cmd/caib/config/config.go`:
- Around line 61-70: The current implementations of JumpstarterEndpoint,
DirPath, and CacheDirPath manually read XDG env vars and accept relative values,
which can leak state; replace that manual logic with os.UserConfigDir() (for
config paths) and os.UserCacheDir() (for cache paths), check for errors and
return "" on error, and then filepath.Join the returned directory with
"jumpstarter" (and any relative subpath argument used by DirPath/CacheDirPath).
Update JumpstarterEndpoint to call os.UserConfigDir(), join with "jumpstarter"
to produce the endpoint directory; update DirPath to get cfg :=
os.UserConfigDir() and return filepath.Join(cfg, "jumpstarter", rel) (or "" on
error); update CacheDirPath to use os.UserCacheDir() similarly. Remove the
manual XDG_RESOLVE logic and rely on the stdlib functions for absolute-path
validation.
---
Nitpick comments:
In `@cmd/caib/auth/config_test.go`:
- Around line 143-145: Add a test case that explicitly sets XDG_CONFIG_HOME to a
custom temp directory and exercises the auth config read/save logic instead of
unsetting it: instead of calling os.Unsetenv("XDG_CONFIG_HOME") in the setup
(where originalXDGConfig and os.Setenv("HOME", tempDir) are used), create a new
spec that calls os.Setenv("XDG_CONFIG_HOME", customXDGDir) and then invokes the
same save/read assertions (the code paths that currently reference
originalXDGConfig and the existing read/save helpers) so the XDG override branch
is executed; also add teardown to restore originalXDGConfig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77ef3997-a53c-4dab-b65f-874f3d1ea8f1
📒 Files selected for processing (8)
cmd/caib/auth/config.gocmd/caib/auth/config_test.gocmd/caib/auth/oidc.gocmd/caib/auth/oidc_test.gocmd/caib/auth/wrapper_test.gocmd/caib/config/config.gocmd/caib/config/config_test.gocmd/caib/root.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/caib/auth/wrapper_test.go
- cmd/caib/auth/oidc.go
- cmd/caib/root.go
- cmd/caib/auth/config.go
bennyz
approved these changes
Mar 10, 2026
10 tasks
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
2 participants
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.
releted to #143
Summary by CodeRabbit