refactor: change .kortex-cli directory to .kdn#205
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated default storage directory name from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 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. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
I'm wondering why the configuration folder should be .kdn and not .kaiden
usually most of cli tools have short names but the config folder is keeping the name of the project no ?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/cmd/root.go (1)
31-33: Normalize fallback storage path to absolute.When
os.UserHomeDir()fails, fallback remains relative (.kdn). Prefer resolving it withfilepath.Abs()to keep storage behavior deterministic.♻️ Proposed refactor
homeDir, err := os.UserHomeDir() defaultStoragePath := ".kdn" // fallback to current directory if err == nil { defaultStoragePath = filepath.Join(homeDir, ".kdn") +} else if absFallback, absErr := filepath.Abs(defaultStoragePath); absErr == nil { + defaultStoragePath = absFallback }As per coding guidelines: "
**/*.go: Convert relative paths to absolute withfilepath.Abs()".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/root.go` around lines 31 - 33, The fallback storage path defaultStoragePath is left as a relative ".kdn" when os.UserHomeDir() fails; call filepath.Abs(".kdn") and assign the resolved absolute path to defaultStoragePath (handle any error from filepath.Abs by retaining the original value or returning/logging as appropriate) so storage path is deterministic—update the logic around defaultStoragePath/homeDir in root.go to use filepath.Abs for the fallback.pkg/cmd/root_test.go (1)
87-90: Make this test environment-independent.
TestRootCmd_StorageFlagcan fail ifKORTEX_CLI_STORAGEis set in the process environment. Consider isolating env state in this test to avoid flakes.♻️ Proposed refactor
func TestRootCmd_StorageFlag(t *testing.T) { - t.Parallel() + t.Setenv("KORTEX_CLI_STORAGE", "") rootCmd := NewRootCmd()As per coding guidelines: "
**/*_test.go: Tests usingt.Setenv()cannot uset.Parallel()on the parent test function."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/root_test.go` around lines 87 - 90, TestRootCmd_StorageFlag is flaky because it depends on the KORTEX_CLI_STORAGE env var; unset it at test start with t.Setenv("KORTEX_CLI_STORAGE", "") so flag.DefValue is evaluated in a clean environment, and ensure the test (or its parent) does not call t.Parallel() per the guideline about t.Setenv; update the TestRootCmd_StorageFlag test accordingly to call t.Setenv before any flag evaluation and remove any t.Parallel() usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cmd/root_test.go`:
- Around line 87-90: TestRootCmd_StorageFlag is flaky because it depends on the
KORTEX_CLI_STORAGE env var; unset it at test start with
t.Setenv("KORTEX_CLI_STORAGE", "") so flag.DefValue is evaluated in a clean
environment, and ensure the test (or its parent) does not call t.Parallel() per
the guideline about t.Setenv; update the TestRootCmd_StorageFlag test
accordingly to call t.Setenv before any flag evaluation and remove any
t.Parallel() usage.
In `@pkg/cmd/root.go`:
- Around line 31-33: The fallback storage path defaultStoragePath is left as a
relative ".kdn" when os.UserHomeDir() fails; call filepath.Abs(".kdn") and
assign the resolved absolute path to defaultStoragePath (handle any error from
filepath.Abs by retaining the original value or returning/logging as
appropriate) so storage path is deterministic—update the logic around
defaultStoragePath/homeDir in root.go to use filepath.Abs for the fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5378050-d6f2-4f9c-888b-32d1cf9642d3
📒 Files selected for processing (6)
AGENTS.mdREADME.mdpkg/cmd/root.gopkg/cmd/root_test.goskills/cross-platform-development/SKILL.mdskills/working-with-config-system/SKILL.md
I tried to differentiate the cli vs the project: kdn vs kaiden. Before it was In #208 is was But we can have another approach if you prefer |
|
ok let's keep so we'll have because I think it might be confusing then to have in the home folder a |
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
might need to move it to ~/.config subfolder (at least on macOS/Linux)
|
config files --> ~/.config/kdn |
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
4 participants
fixes #201