Add global --chdir flag for changing working directory #1644
Conversation
📝 WalkthroughWalkthroughAdds a global CLI flag Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Atmos CLI
participant Root as RootCmd
participant Config as Config Loader
participant Cmd as Subcommand
User->>CLI: atmos [--chdir /path] [subcommand]
CLI->>Root: RootCmd.Execute()
Root->>Root: processChdirFlag()
note right of Root #EAF2F8: resolve flag or ATMOS_CHDIR\n(filepath.Abs -> os.Stat -> os.Chdir)
alt resolved and accessible directory
Root->>Config: Load configuration (from new working dir)
Config-->>Root: config loaded
Root->>Cmd: execute subcommand
Cmd-->>User: result
else invalid path / access error
Root-->>User: exit with wrapped error (not found / access / not a dir)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
cmd/root.go(3 hunks)cmd/root_chdir_test.go(1 hunks)cmd/root_test.go(2 hunks)website/blog/2025-01-15-chdir-flag.md(1 hunks)website/docs/cli/global-flags.mdx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands
Implement each CLI command in a separate file under cmd/
Use Viper for managing configuration, environment variables, and flags in the CLI
Keep separation of concerns between CLI interface (cmd) and business logic
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in Cobra command help
Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Follow single responsibility; separate command interface from business logic
Provide meaningful user feedback and include progress indicators for long-running operations
Provide clear error messages to users and troubleshooting hints where appropriate
cmd/**/*.go: Define one Cobra command per file under cmd/ and follow example pattern with embedded markdown examples.
New commands should rely on automatic telemetry capture; add explicit CaptureCmd/CaptureCmdString only for non-standard paths.
Files:
cmd/root_test.gocmd/root.gocmd/root_chdir_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Write unit tests as table-driven tests focused on behavior; prefer high coverage for pkg/ and internal/exec/; comments must end with periods.
Use t.Skipf with a reason instead of t.Skip; never skip without a reason.
Test files must mirror implementation file names (e.g., aws_ssm_store_test.go pairs with aws_ssm_store.go).
Files:
cmd/root_test.gocmd/root_chdir_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All comments must end with periods across all Go code (godot linter enforced).
Organize imports into three groups (stdlib, 3rd-party, Atmos) separated by blank lines and sorted alphabetically within groups; preserve existing aliases.
Add defer perf.Track(<atmosConfig|nil>, ".")() at the start of all public and critical private functions, followed by a blank line.
All errors must be wrapped using static errors from errors/errors.go; prefer errors.Join for multiple errors, fmt.Errorf with %w for context, and check with errors.Is; never use dynamic errors directly.
Use utils.PrintfMarkdown() to render embedded markdown examples for CLI help output.
Co-locate unit tests with implementation files; integration tests reside under tests/.
Distinguish UI output from logging: UI prompts/status/errors to stderr; data/results to stdout; logging for system/debug info only with structured fields.
Most text UI must go to stderr; only data/results to stdout. Prefer utils.PrintfMessageToTUI for UI messages.
Bind all environment variables with viper.BindEnv(); every env var must have an ATMOS_ alternative binding.
Favor cross-platform code: prefer SDKs over external binaries, use filepath/os/runtime, and handle OS-specific differences or use build tags.
For non-standard execution paths, capture telemetry using telemetry.CaptureCmd or telemetry.CaptureCmdString without collecting user data.
Search for existing methods and utilities (internal/exec, pkg/) before implementing new functionality; prefer reuse/refactoring over duplication.
Files:
cmd/root_test.gocmd/root.gocmd/root_chdir_test.go
cmd/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Place each CLI command in its own file (one command per file) under cmd/.
Files:
cmd/root_test.gocmd/root.gocmd/root_chdir_test.go
cmd/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Place command tests under cmd/ as *_test.go files.
Files:
cmd/root_test.gocmd/root_chdir_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Files:
cmd/root.go
cmd/root.go
📄 CodeRabbit inference engine (CLAUDE.md)
Standard commands gain telemetry via RootCmd.ExecuteC() in cmd/root.go; do not add extra telemetry for standard Cobra commands.
Files:
cmd/root.go
website/**
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
website/**: Update website documentation in website/ when adding features
Ensure consistency between CLI help text and website documentation
Follow the website's documentation structure and style
Keep website code in website/ and follow its architecture/style; test changes locally
Keep CLI and website documentation in sync; document new features with examples and use cases
Files:
website/docs/cli/global-flags.mdxwebsite/blog/2025-01-15-chdir-flag.md
website/docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always build the website after any docs changes to website/docs (*.mdx or *.md).
Files:
website/docs/cli/global-flags.mdx
🧠 Learnings (10)
📓 Common learnings
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
cmd/root_test.gocmd/root_chdir_test.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
PR: cloudposse/atmos#1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
cmd/root_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
cmd/root_test.gocmd/root.gocmd/root_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
cmd/root_test.gocmd/root.gocmd/root_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
cmd/root_test.gocmd/root_chdir_test.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
PR: cloudposse/atmos#844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
cmd/root.go
📚 Learning: 2025-10-07T00:25:16.333Z
Learnt from: osterman
PR: cloudposse/atmos#1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
Applied to files:
website/docs/cli/global-flags.mdx
📚 Learning: 2025-09-10T17:34:52.568Z
Learnt from: Benbentwo
PR: cloudposse/atmos#1475
File: pkg/auth/providers/github/oidc.go:96-100
Timestamp: 2025-09-10T17:34:52.568Z
Learning: The ATMOS_ environment variable binding guideline applies to Atmos configuration variables, not external service-required environment variables like GitHub Actions OIDC variables (GITHUB_ACTIONS, ACTIONS_ID_TOKEN_*) which must use their standard names.
Applied to files:
website/docs/cli/global-flags.mdx
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
website/docs/cli/global-flags.mdx
🧬 Code graph analysis (3)
cmd/root_test.go (1)
pkg/config/homedir/homedir.go (1)
Reset(94-98)
cmd/root.go (2)
errors/error_funcs.go (1)
CheckErrorPrintAndExit(55-72)errors/errors.go (3)
ErrPathResolution(36-36)ErrWorkdirNotExist(35-35)ErrStatFile(277-277)
cmd/root_chdir_test.go (1)
cmd/root.go (2)
RootCmd(55-216)Execute(490-537)
🪛 LanguageTool
website/blog/2025-01-15-chdir-flag.md
[grammar] ~20-~20: There might be a mistake here.
Context: ...maintain Previously, you had to either: - Manually cd to each directory before r...
(QB_NEW_EN)
[grammar] ~163-~163: There might be a mistake here.
Context: ...--chdir` flag works consistently across: - Linux - All distributions - macOS ...
(QB_NEW_EN)
[grammar] ~164-~164: There might be a mistake here.
Context: ... across: - Linux - All distributions - macOS - All versions - Windows - N...
(QB_NEW_EN)
[grammar] ~165-~165: There might be a mistake here.
Context: ...distributions - macOS - All versions - Windows - Native support Both absolut...
(QB_NEW_EN)
[grammar] ~174-~174: There might be a mistake here.
Context: ...ase-path` resolution → command execution - Path resolution: Relative paths are re...
(QB_NEW_EN)
[grammar] ~206-~206: There might be a mistake here.
Context: ...Resources - Global Flags Documentation - Atmos CLI Reference - [G...
(QB_NEW_EN)
[grammar] ~207-~207: There might be a mistake here.
Context: ...cli/global-flags) - Atmos CLI Reference - [GitHub Repository](https://github.com/cl...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
github-actions
bot
added
size/xl
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1644 +/- ##
==========================================
+ Coverage 66.04% 66.12% +0.07%
==========================================
Files 343 343
Lines 38736 38778 +42
==========================================
+ Hits 25584 25642 +58
+ Misses 11163 11149 -14
+ Partials 1989 1987 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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)
website/docs/cli/global-flags.mdx (1)
135-141: Align docs’ default--logs-levelwith CLI default
Docs list the default asWarning(website/docs/cli/global-flags.mdx lines 138–141) but the CLI flag default isInfo(cmd/root.go). Update the docs to showInfo.
🧹 Nitpick comments (5)
cmd/root_chdir_test.go (5)
81-88: Remove unused fieldenvVarfrom test table.It’s unused after fixing the fallback and adds noise.
- envVar string expectError bool expectWd string // Expected working directory (relative to test directory) setup func(t *testing.T) string cleanup func(t *testing.T, dir string)
316-323: Prefer testing the real chdir logic.Instead of manual os.Chdir here, call the same helper used in production (e.g., processChdirFlag) to reduce drift and increase fidelity.
270-276: Use t.Chdir for test directory changes.The codebase supports t.Chdir(); it simplifies setup/cleanup and avoids manual os.Chdir bookkeeping.
Based on learnings
336-355: Integration test doesn’t assert order.“chdir processes before base-path” only creates files. Add an assertion that cwd equals chdir target before base‑path is evaluated.
61-64: Helper expects error on valid directory.The final branch assumes chdir should fail for a directory; this won’t happen and is currently unreachable. Consider removing or inverting the assertion for clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
cmd/reset_rootcmd_test.go(1 hunks)cmd/root_chdir_test.go(1 hunks)cmd/root_test.go(2 hunks)cmd/testing_helpers_test.go(1 hunks)website/docs/cli/global-flags.mdx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
website/**
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
website/**: Update website documentation in website/ when adding features
Ensure consistency between CLI help text and website documentation
Follow the website's documentation structure and style
Keep website code in website/ and follow its architecture/style; test changes locally
Keep CLI and website documentation in sync; document new features with examples and use cases
Files:
website/docs/cli/global-flags.mdx
{website/docs/**/*.{md,mdx},website/static/img/**,website/sidebars.js,website/src/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Always build the website (npm run build from website/) after modifying docs, images, sidebars, or website components; ensure no broken links or missing assets.
Files:
website/docs/cli/global-flags.mdx
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands
Implement each CLI command in a separate file under cmd/
Use Viper for managing configuration, environment variables, and flags in the CLI
Keep separation of concerns between CLI interface (cmd) and business logic
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in Cobra command help
Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Follow single responsibility; separate command interface from business logic
Provide meaningful user feedback and include progress indicators for long-running operations
Provide clear error messages to users and troubleshooting hints where appropriateNew commands automatically get telemetry via RootCmd.ExecuteC(); for non-standard paths, call telemetry.CaptureCmd/CaptureCmdString.
Files:
cmd/reset_rootcmd_test.gocmd/root_chdir_test.gocmd/root_test.gocmd/testing_helpers_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenariosAlways use t.Skipf with a reason (never t.Skip or Skipf without a reason) for skipped tests.
Files:
cmd/reset_rootcmd_test.gocmd/root_chdir_test.gocmd/root_test.gocmd/testing_helpers_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All comments must end with periods. Applies to single-line, multi-line, inline, and documentation comments (enforced by golangci-lint godot).
Organize imports into three groups separated by blank lines: standard library, third-party, and github.com/cloudposse/atmos packages; sort alphabetically within each group; keep existing aliases.
Add defer perf.Track(...) to all public and critical private functions with a blank line after the call; use "packagename.FunctionName" label; pass atmosConfig if available, else nil.
Error handling must use static errors from errors/errors.go; prefer errors.Join to combine; use fmt.Errorf with %w for context; use errors.Is for checks; never use dynamic errors directly.
Distinguish structured logging from UI output: user prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results go to stdout. Prefer utils.PrintfMessageToTUI for UI messages.
All new configs must support Go templating using FuncMap() from internal/exec/template_funcs.go; test template rendering with various contexts.
Always use viper.BindEnv for environment variables and provide ATMOS_ alternatives for every env var.
Files:
cmd/reset_rootcmd_test.gocmd/root_chdir_test.gocmd/root_test.gocmd/testing_helpers_test.go
cmd/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
One Cobra command per file in cmd/.
Files:
cmd/reset_rootcmd_test.gocmd/root_chdir_test.gocmd/root_test.gocmd/testing_helpers_test.go
cmd/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Command tests should live under cmd/.
Files:
cmd/reset_rootcmd_test.gocmd/root_chdir_test.gocmd/root_test.gocmd/testing_helpers_test.go
🧠 Learnings (9)
📓 Common learnings
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
📚 Learning: 2025-10-07T00:25:16.333Z
Learnt from: osterman
PR: cloudposse/atmos#1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
Applied to files:
website/docs/cli/global-flags.mdx
📚 Learning: 2025-09-05T14:57:37.360Z
Learnt from: RoseSecurity
PR: cloudposse/atmos#1448
File: cmd/ansible.go:26-28
Timestamp: 2025-09-05T14:57:37.360Z
Learning: The Atmos codebase uses a consistent pattern for commands that delegate to external tools: `PersistentFlags().Bool("", false, doubleDashHint)` where doubleDashHint provides help text about using double dashes to separate Atmos options from native command arguments. This pattern is used across terraform, packer, helmfile, atlantis, aws, and ansible commands.
Applied to files:
website/docs/cli/global-flags.mdx
📚 Learning: 2025-09-10T17:34:52.568Z
Learnt from: Benbentwo
PR: cloudposse/atmos#1475
File: pkg/auth/providers/github/oidc.go:96-100
Timestamp: 2025-09-10T17:34:52.568Z
Learning: The ATMOS_ environment variable binding guideline applies to Atmos configuration variables, not external service-required environment variables like GitHub Actions OIDC variables (GITHUB_ACTIONS, ACTIONS_ID_TOKEN_*) which must use their standard names.
Applied to files:
website/docs/cli/global-flags.mdx
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
website/docs/cli/global-flags.mdx
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
cmd/root_chdir_test.gocmd/root_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
cmd/root_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
cmd/root_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
cmd/root_chdir_test.go
🧬 Code graph analysis (4)
cmd/reset_rootcmd_test.go (1)
cmd/root.go (1)
RootCmd(97-212)
cmd/root_chdir_test.go (1)
cmd/root.go (2)
RootCmd(97-212)Execute(486-533)
cmd/root_test.go (1)
cmd/root.go (1)
RootCmd(97-212)
cmd/testing_helpers_test.go (1)
cmd/root.go (1)
RootCmd(97-212)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: website-deploy-preview
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (4)
cmd/root_test.go (2)
19-25: Good env hygiene before Execute().Unsetting ATMOS_CHDIR up front (and deferring cleanup) avoids cross‑test pollution. Solid.
47-54: Resetting logger and RootCmd state is correct.Buffer reset + explicit RootCmd reset/setArgs ensures deterministic output. LGTM.
cmd/reset_rootcmd_test.go (1)
59-63: Confirm logs-level default matches CLI.Test expects default "Info". Docs mention default "Warning". Please confirm actual flag default to avoid brittle tests.
website/docs/cli/global-flags.mdx (1)
22-82: Excellent clarity on --chdir vs --base-path.Examples and precedence are clear and match Cobra semantics. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/root_chdir_test.go (1)
238-241: Env var fallback logic prevents ATMOS_CHDIR from being read.The condition
envVar != ""prevents readingATMOS_CHDIRfrom the actual environment when the test struct fieldenvVaris empty. This breaks the "ATMOS_CHDIR environment variable" test case.This issue was already flagged in a previous review. Apply the suggested fix:
-// Check environment variable if flag is empty. -if chdir == "" && envVar != "" { - chdir = os.Getenv("ATMOS_CHDIR") -} +// If flag empty, fall back to environment variable. +if chdir == "" { + chdir = os.Getenv("ATMOS_CHDIR") +}
🧹 Nitpick comments (8)
cmd/root_process_chdir_test.go (4)
122-125: Refactor to avoid brittle string-based test name check.The logic uses string comparison on
tt.nameto determine behavior, which breaks if test names change. Consider adding an explicit field to the test struct to control this behavior instead.Add a field to the test struct:
tests := []struct { name string flagValue string envValue string + useSetupResult bool // If true, use setup() result as flagValue when flagValue is empty setup func(t *testing.T) string expectError bool errorMsg string }{Then replace lines 122-125:
-// Set flag value if provided, otherwise use setup result. -flagValue := tt.flagValue -if flagValue == "" && expectedDir != "" && tt.name != "no chdir flag or env var" { - flagValue = expectedDir -} +// Set flag value if provided, otherwise use setup result if configured. +flagValue := tt.flagValue +if flagValue == "" && tt.useSetupResult && expectedDir != "" { + flagValue = expectedDir +}
150-155: Handle EvalSymlinks errors to avoid false positives.Ignoring errors from
filepath.EvalSymlinkscould mask failures in symlink resolution, leading to incorrect test results.Apply this diff:
if expectedDir != "" && expectedDir != originalWd { currentWd, _ := os.Getwd() // Use EvalSymlinks for macOS compatibility. - expectedResolved, _ := filepath.EvalSymlinks(expectedDir) - currentResolved, _ := filepath.EvalSymlinks(currentWd) - assert.Equal(t, expectedResolved, currentResolved) + expectedResolved, err1 := filepath.EvalSymlinks(expectedDir) + currentResolved, err2 := filepath.EvalSymlinks(currentWd) + if err1 == nil && err2 == nil { + assert.Equal(t, expectedResolved, currentResolved) + } else { + // Fall back to direct comparison if symlink resolution fails. + assert.Equal(t, expectedDir, currentWd) + } }
186-188: Handle EvalSymlinks errors for robust comparison.Same issue as in TestProcessChdirFlag - ignoring symlink resolution errors could cause false positives.
Apply this diff:
currentWd, _ := os.Getwd() -expectedResolved, _ := filepath.EvalSymlinks(tmpDir) -currentResolved, _ := filepath.EvalSymlinks(currentWd) -assert.Equal(t, expectedResolved, currentResolved) +expectedResolved, err1 := filepath.EvalSymlinks(tmpDir) +currentResolved, err2 := filepath.EvalSymlinks(currentWd) +if err1 == nil && err2 == nil { + assert.Equal(t, expectedResolved, currentResolved) +} else { + assert.Equal(t, tmpDir, currentWd) +}
221-223: Handle EvalSymlinks errors for robust comparison.Ignoring errors from
filepath.EvalSymlinkscould mask symlink resolution failures.Apply this diff:
currentWd, _ := os.Getwd() -expectedResolved, _ := filepath.EvalSymlinks(flagDir) -currentResolved, _ := filepath.EvalSymlinks(currentWd) -assert.Equal(t, expectedResolved, currentResolved) +expectedResolved, err1 := filepath.EvalSymlinks(flagDir) +currentResolved, err2 := filepath.EvalSymlinks(currentWd) +if err1 == nil && err2 == nil { + assert.Equal(t, expectedResolved, currentResolved) +} else { + assert.Equal(t, flagDir, currentWd) +}cmd/root_chdir_test.go (4)
198-212: String-based test case selection is fragile.Using
switch tt.namewith string literals makes the tests brittle - renaming a test case breaks the logic. Consider adding a test type field or using the setup function's behavior to determine the path.Add a field to the test struct to indicate how args should be constructed:
tests := []struct { name string args []string envVar string + argsConstructor func(testDir string) []string // Optional: builds args from testDir expectError bool expectWd string setup func(t *testing.T) string cleanup func(t *testing.T, dir string) }{Then use this field instead of the switch statement.
261-263: Handle EvalSymlinks errors to prevent false positives.Ignoring errors from
filepath.EvalSymlinkscould mask symlink resolution failures and lead to incorrect test assertions.Apply this diff:
// Use EvalSymlinks to resolve paths on macOS where /var -> /private/var. currentWd, err := os.Getwd() require.NoError(t, err) -expectedResolved, _ := filepath.EvalSymlinks(absPath) -currentResolved, _ := filepath.EvalSymlinks(currentWd) -assert.Equal(t, expectedResolved, currentResolved, "Should be in the expected directory") +expectedResolved, err1 := filepath.EvalSymlinks(absPath) +currentResolved, err2 := filepath.EvalSymlinks(currentWd) +if err1 == nil && err2 == nil { + assert.Equal(t, expectedResolved, currentResolved, "Should be in the expected directory") +} else { + // Fall back to direct comparison if symlink resolution fails. + assert.Equal(t, absPath, currentWd, "Should be in the expected directory") +}
336-339: Handle EvalSymlinks errors consistently.Multiple locations in this file ignore symlink resolution errors. Apply the same fix pattern as suggested for lines 261-263.
Lines 336-339:
wd, _ := os.Getwd() expectedAbs, _ := filepath.Abs(tmpDir) -expectedResolved, _ := filepath.EvalSymlinks(expectedAbs) -wdResolved, _ := filepath.EvalSymlinks(wd) -assert.Equal(t, expectedResolved, wdResolved, "Working directory should match chdir target") +expectedResolved, err1 := filepath.EvalSymlinks(expectedAbs) +wdResolved, err2 := filepath.EvalSymlinks(wd) +if err1 == nil && err2 == nil { + assert.Equal(t, expectedResolved, wdResolved, "Working directory should match chdir target") +} else { + assert.Equal(t, expectedAbs, wd, "Working directory should match chdir target") +}
386-389: Symlink test skip reason should be more specific.The skip message mentions Windows but doesn't clarify that this is specifically about symlink creation privileges, not a general Windows limitation.
Apply this diff:
err := os.Symlink(targetDir, symlinkPath) if err != nil { - t.Skipf("Skipping symlink test on Windows: symlinks require special privileges") + t.Skipf("Cannot create symlink (may require admin privileges on Windows): %v", err) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cmd/root_chdir_test.go(1 hunks)cmd/root_process_chdir_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands
Implement each CLI command in a separate file under cmd/
Use Viper for managing configuration, environment variables, and flags in the CLI
Keep separation of concerns between CLI interface (cmd) and business logic
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in Cobra command help
Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Follow single responsibility; separate command interface from business logic
Provide meaningful user feedback and include progress indicators for long-running operations
Provide clear error messages to users and troubleshooting hints where appropriate
cmd/**/*.go: Use utils.PrintfMarkdown() to render embedded markdown content for CLI command help/examples.
One Cobra command per file in cmd/; keep files focused and small.
Files:
cmd/root_process_chdir_test.gocmd/root_chdir_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Files:
cmd/root_process_chdir_test.gocmd/root_chdir_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout; prefer utils.PrintfMessageToTUI for UI messages.
All new configurations must support Go templating using existing utilities and available template functions.
Prefer SDKs over external binaries for cross-platform support; use filepath/os/runtime for portability.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Use structured logging with levels (Fatal>Error>Warn>Debug>Trace); avoid string interpolation and ensure logging does not affect execution.
Prefer re...
Files:
cmd/root_process_chdir_test.gocmd/root_chdir_test.go
cmd/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Command tests live under cmd/ alongside command implementations.
Files:
cmd/root_process_chdir_test.gocmd/root_chdir_test.go
🧠 Learnings (6)
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to cmd/*/*_test.go : Add tests for new commands under cmd/<command>/<command>_test.go.
Applied to files:
cmd/root_process_chdir_test.gocmd/root_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
cmd/root_process_chdir_test.gocmd/root_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
cmd/root_process_chdir_test.gocmd/root_chdir_test.go
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to internal/exec/*_test.go : Add comprehensive tests for template functions under internal/exec/ with *_test.go files.
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
cmd/root_process_chdir_test.gocmd/root_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
cmd/root_process_chdir_test.gocmd/root_chdir_test.go
🧬 Code graph analysis (1)
cmd/root_chdir_test.go (1)
cmd/root.go (2)
RootCmd(97-212)Execute(486-533)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: website-deploy-preview
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/root_process_chdir_test.go (1)
134-144: Add test coverage for short flag form-C.The flag is registered with both
--chdirand-C, but only the long form is tested. Add a test case using-Cto ensure both forms work correctly.Add this test case to the table:
{ name: "valid path with short flag form", setup: func(t *testing.T) string { return t.TempDir() }, expectError: false, },Then update the flag parsing logic to handle the short form:
if flagValue != "" { - testCmd.SetArgs([]string{"--chdir", flagValue}) - require.NoError(t, testCmd.ParseFlags([]string{"--chdir", flagValue})) + flagName := "--chdir" + if tt.name == "valid path with short flag form" { + flagName = "-C" + } + testCmd.SetArgs([]string{flagName, flagValue}) + require.NoError(t, testCmd.ParseFlags([]string{flagName, flagValue})) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
cmd/root_chdir_test.go(1 hunks)cmd/root_process_chdir_test.go(1 hunks)cmd/testing_helpers_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/testing_helpers_test.go
- cmd/root_chdir_test.go
🧰 Additional context used
📓 Path-based instructions (4)
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands
Implement each CLI command in a separate file under cmd/
Use Viper for managing configuration, environment variables, and flags in the CLI
Keep separation of concerns between CLI interface (cmd) and business logic
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in Cobra command help
Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Follow single responsibility; separate command interface from business logic
Provide meaningful user feedback and include progress indicators for long-running operations
Provide clear error messages to users and troubleshooting hints where appropriate
cmd/**/*.go: Use utils.PrintfMarkdown() to render embedded markdown content for CLI command help/examples.
One Cobra command per file in cmd/; keep files focused and small.
Files:
cmd/root_process_chdir_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Files:
cmd/root_process_chdir_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout; prefer utils.PrintfMessageToTUI for UI messages.
All new configurations must support Go templating using existing utilities and available template functions.
Prefer SDKs over external binaries for cross-platform support; use filepath/os/runtime for portability.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Use structured logging with levels (Fatal>Error>Warn>Debug>Trace); avoid string interpolation and ensure logging does not affect execution.
Prefer re...
Files:
cmd/root_process_chdir_test.go
cmd/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Command tests live under cmd/ alongside command implementations.
Files:
cmd/root_process_chdir_test.go
🧠 Learnings (5)
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to cmd/*/*_test.go : Add tests for new commands under cmd/<command>/<command>_test.go.
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
cmd/root_process_chdir_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: website-deploy-preview
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cmd/root_process_chdir_test.go (3)
173-179: Check errors in directory verification.Ignoring errors from
os.Getwd()andfilepath.EvalSymlinks()can mask test failures if symlink resolution fails.if expectedDir != "" && expectedDir != originalWd { - currentWd, _ := os.Getwd() + currentWd, err := os.Getwd() + require.NoError(t, err, "failed to get current directory") // Use EvalSymlinks for macOS compatibility. - expectedResolved, _ := filepath.EvalSymlinks(expectedDir) - currentResolved, _ := filepath.EvalSymlinks(currentWd) + expectedResolved, err := filepath.EvalSymlinks(expectedDir) + require.NoError(t, err, "failed to resolve expected directory") + currentResolved, err := filepath.EvalSymlinks(currentWd) + require.NoError(t, err, "failed to resolve current directory") assert.Equal(t, expectedResolved, currentResolved) }
209-211: Check errors in directory verification.Same issue as in the main test function: errors from
os.Getwd()andfilepath.EvalSymlinks()are ignored, which can hide test failures.-currentWd, _ := os.Getwd() -expectedResolved, _ := filepath.EvalSymlinks(tmpDir) -currentResolved, _ := filepath.EvalSymlinks(currentWd) +currentWd, err := os.Getwd() +require.NoError(t, err, "failed to get current directory") +expectedResolved, err := filepath.EvalSymlinks(tmpDir) +require.NoError(t, err, "failed to resolve expected directory") +currentResolved, err := filepath.EvalSymlinks(currentWd) +require.NoError(t, err, "failed to resolve current directory") assert.Equal(t, expectedResolved, currentResolved)
244-246: Check errors in directory verification.Third occurrence of the same issue: errors from
os.Getwd()andfilepath.EvalSymlinks()are ignored.-currentWd, _ := os.Getwd() -expectedResolved, _ := filepath.EvalSymlinks(flagDir) -currentResolved, _ := filepath.EvalSymlinks(currentWd) +currentWd, err := os.Getwd() +require.NoError(t, err, "failed to get current directory") +expectedResolved, err := filepath.EvalSymlinks(flagDir) +require.NoError(t, err, "failed to resolve expected directory") +currentResolved, err := filepath.EvalSymlinks(currentWd) +require.NoError(t, err, "failed to resolve current directory") assert.Equal(t, expectedResolved, currentResolved)
🧹 Nitpick comments (2)
cmd/testing_helpers_test.go (1)
26-52: Well-implemented flag reset logic.The implementation correctly addresses the past review feedback:
- Checks both local and persistent flag sets
- Resets both value and
Changedflag- Intentionally excludes config flags with clear explanation
The ignored error on line 49 is acceptable for cleanup code with standard flag types.
Optionally, you could log unexpected Set failures for debugging:
if f != nil { // Reset to default and clear Changed. - _ = f.Value.Set(f.DefValue) + if err := f.Value.Set(f.DefValue); err != nil { + t.Logf("Warning: failed to reset flag %q: %v", flag, err) + } f.Changed = false }cmd/root_process_chdir_test.go (1)
144-149: Consider simplifying flag value assignment logic.The conditional on lines 144-149 has multiple conditions that make the intent less clear. Consider extracting to a helper or adding clarifying comments.
For example, you could restructure as:
// Determine flag value: use explicit flagValue if set, otherwise use expectedDir // (except for tests that explicitly test env var without flag). flagValue := tt.flagValue shouldUseFlagValue := flagValue == "" && expectedDir != "" && tt.name != "no chdir flag or env var" && tt.name != "valid absolute path via env var" if shouldUseFlagValue { flagValue = expectedDir }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.gitignore(1 hunks)cmd/root_process_chdir_test.go(1 hunks)cmd/root_test.go(2 hunks)cmd/testing_helpers_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/root_test.go
🧰 Additional context used
📓 Path-based instructions (4)
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands
Implement each CLI command in a separate file under cmd/
Use Viper for managing configuration, environment variables, and flags in the CLI
Keep separation of concerns between CLI interface (cmd) and business logic
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in Cobra command help
Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Follow single responsibility; separate command interface from business logic
Provide meaningful user feedback and include progress indicators for long-running operations
Provide clear error messages to users and troubleshooting hints where appropriate
cmd/**/*.go: Use utils.PrintfMarkdown() to render embedded markdown content for CLI command help/examples.
One Cobra command per file in cmd/; keep files focused and small.
Files:
cmd/testing_helpers_test.gocmd/root_process_chdir_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Files:
cmd/testing_helpers_test.gocmd/root_process_chdir_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout; prefer utils.PrintfMessageToTUI for UI messages.
All new configurations must support Go templating using existing utilities and available template functions.
Prefer SDKs over external binaries for cross-platform support; use filepath/os/runtime for portability.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Use structured logging with levels (Fatal>Error>Warn>Debug>Trace); avoid string interpolation and ensure logging does not affect execution.
Prefer re...
Files:
cmd/testing_helpers_test.gocmd/root_process_chdir_test.go
cmd/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Command tests live under cmd/ alongside command implementations.
Files:
cmd/testing_helpers_test.gocmd/root_process_chdir_test.go
🧠 Learnings (8)
📓 Common learnings
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to cmd/*/*_test.go : Add tests for new commands under cmd/<command>/<command>_test.go.
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
PR: cloudposse/atmos#1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
cmd/root_process_chdir_test.go
🧬 Code graph analysis (1)
cmd/testing_helpers_test.go (1)
cmd/root.go (1)
RootCmd(97-212)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: website-deploy-preview
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (7)
cmd/testing_helpers_test.go (4)
1-3: Clean package setup.Package declaration and imports are appropriate for a test helper file.
5-19: Excellent documentation.The documentation clearly explains the problem (global RootCmd state pollution), provides usage examples, and helps developers understand when and how to use this helper.
20-21: Proper test helper implementation.The function signature and
t.Helper()call follow Go testing best practices.
23-24: Args cleared correctly.This clears the command arguments as intended.
cmd/root_process_chdir_test.go (3)
1-12: Imports follow guidelines.Package declaration and imports are properly structured with standard library and third-party dependencies correctly grouped and sorted.
16-21: Setup and cleanup handling is appropriate.The test correctly saves and restores the working directory. Ignoring errors in cleanup is acceptable for controlled test scenarios.
103-105: Missing period in Skip message.The Skip message doesn't end with a period, violating the godot linter requirement that all Go comments must end with periods.
Apply this diff:
- t.Skip("Skipping permission-based test on Windows") + t.Skip("Skipping permission-based test on Windows.")Likely an incorrect or invalid review comment.
|
@coderabbitai full review on all files and code changes in this PR |
aknysh
added
the
no-release
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/config/load_config_test.go (1)
355-395: Error path testing is valuable.The test validates YAML parsing error handling. While the table-driven structure with a single case could be simplified to a direct test, the current approach is fine and allows easy expansion in the future.
If you prefer, this could be simplified since there's only one test case:
func TestLoadConfigFromCLIArgs_InvalidYAML(t *testing.T) { tmpDir := t.TempDir() // Create invalid YAML file. invalidYaml := filepath.Join(tmpDir, "invalid.yaml") require.NoError(t, os.WriteFile(invalidYaml, []byte("invalid: [unclosed"), 0o644)) configInfo := schema.ConfigAndStacksInfo{ AtmosConfigFilesFromArg: []string{invalidYaml}, } _, err := InitCliConfig(configInfo, false) assert.Error(t, err, "Should fail for invalid YAML syntax") }However, the table-driven approach makes future additions easier.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
cmd/root_chdir_test.go(1 hunks)cmd/root_test.go(2 hunks)pkg/config/load_config_test.go(2 hunks)tools/lintroller/rule_os_chdir.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout; prefer utils.PrintfMessageToTUI for UI messages.
All new configurations must support Go templating using existing utilities and available template functions.
Prefer SDKs over external binaries for cross-platform support; use filepath/os/runtime for portability.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Use structured logging with levels (Fatal>Error>Warn>Debug>Trace); avoid string interpolation and ensure logging does not affect execution.
Prefer re...
Files:
tools/lintroller/rule_os_chdir.gopkg/config/load_config_test.gocmd/root_test.gocmd/root_chdir_test.go
**/!(*_test).go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Document all exported functions, types, and methods with Go doc comments
Add
defer perf.Track()to all public functions and critical private functions; include a blank line after the call; use package-prefixed names; pass atmosConfig when present, else nil.
Files:
tools/lintroller/rule_os_chdir.go
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/config/load_config_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Files:
pkg/config/load_config_test.gocmd/root_test.gocmd/root_chdir_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for packages live under pkg/ alongside implementation files.
Files:
pkg/config/load_config_test.go
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands
Implement each CLI command in a separate file under cmd/
Use Viper for managing configuration, environment variables, and flags in the CLI
Keep separation of concerns between CLI interface (cmd) and business logic
Use kebab-case for command-line flags
Provide comprehensive help text for all commands and flags
Include examples in Cobra command help
Use Viper for configuration management; support files, env vars, and flags with precedence flags > env > config > defaults
Follow single responsibility; separate command interface from business logic
Provide meaningful user feedback and include progress indicators for long-running operations
Provide clear error messages to users and troubleshooting hints where appropriate
cmd/**/*.go: Use utils.PrintfMarkdown() to render embedded markdown content for CLI command help/examples.
One Cobra command per file in cmd/; keep files focused and small.
Files:
cmd/root_test.gocmd/root_chdir_test.go
cmd/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Command tests live under cmd/ alongside command implementations.
Files:
cmd/root_test.gocmd/root_chdir_test.go
🧠 Learnings (5)
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
tools/lintroller/rule_os_chdir.gocmd/root_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
tools/lintroller/rule_os_chdir.gocmd/root_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
tools/lintroller/rule_os_chdir.gocmd/root_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
tools/lintroller/rule_os_chdir.gocmd/root_chdir_test.go
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to cmd/*/*_test.go : Add tests for new commands under cmd/<command>/<command>_test.go.
Applied to files:
cmd/root_test.gocmd/root_chdir_test.go
🧬 Code graph analysis (3)
pkg/config/load_config_test.go (3)
errors/errors.go (4)
ErrEmptyConfigFile(103-103)ErrExpectedFile(100-100)ErrEmptyConfigPath(102-102)ErrAtmosDirConfigNotFound(92-92)pkg/schema/schema.go (1)
ConfigAndStacksInfo(460-539)pkg/config/config.go (1)
InitCliConfig(25-62)
cmd/root_test.go (1)
cmd/testkit_test.go (1)
NewTestKit(55-65)
cmd/root_chdir_test.go (2)
cmd/testkit_test.go (1)
NewTestKit(55-65)cmd/root.go (3)
RootCmd(103-218)Cleanup(310-312)Execute(492-539)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (34)
- GitHub Check: [mock-macos] examples/demo-vendoring
- GitHub Check: [mock-macos] tests/fixtures/scenarios/complete
- GitHub Check: [mock-macos] examples/demo-component-versions
- GitHub Check: [mock-macos] examples/demo-atlantis
- GitHub Check: [mock-macos] examples/demo-context
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [mock-linux] examples/demo-context
- GitHub Check: [mock-windows] examples/demo-vendoring
- GitHub Check: [mock-windows] examples/demo-component-versions
- GitHub Check: [validate] quick-start-advanced
- GitHub Check: [validate] quick-start-simple
- GitHub Check: [mock-windows] examples/demo-context
- GitHub Check: [mock-linux] tests/fixtures/scenarios/complete
- GitHub Check: [mock-linux] examples/demo-vendoring
- GitHub Check: [mock-windows] examples/demo-atlantis
- GitHub Check: [mock-linux] examples/demo-component-versions
- GitHub Check: [mock-linux] examples/demo-atlantis
- GitHub Check: [validate] demo-localstack
- GitHub Check: [validate] demo-context
- GitHub Check: [validate] demo-stacks
- GitHub Check: [validate] demo-helmfile
- GitHub Check: [lint] demo-context
- GitHub Check: [localstack] demo-localstack
- GitHub Check: [lint] quick-start-advanced
- GitHub Check: [k3s] demo-helmfile
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [lint] Dockerfile
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint (golangci)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (16)
tools/lintroller/rule_os_chdir.go (1)
27-30: LGTM! Targeted exception for chdir testing.The new conditional properly exempts chdir test files from the os.Chdir restriction, allowing tests to exercise actual directory change behavior while maintaining the rule for other test files.
cmd/root_test.go (4)
19-22: LGTM! Appropriate CI environment handling.The TTY availability check with descriptive skip message correctly handles CI environments where
/dev/ttyisn't accessible.
24-34: LGTM! Test isolation properly established.NewTestKit ensures RootCmd state restoration, and the working directory save/restore pattern with
t.Chdirprovides automatic cleanup.
36-41: LGTM! Dual cleanup pattern correctly implemented.The immediate unset (line 40) clears state from previous tests, while the deferred unset (line 41) cleans up after this test. The comment accurately explains why
t.Setenvcan't be used here.
64-65: LGTM! Buffer reset prevents cross-test contamination.Resetting the logger buffer immediately before
Execute()ensures clean state and eliminates output from previous tests.cmd/root_chdir_test.go (7)
13-32: LGTM! Helper properly tests production code.
testChdirErrorcorrectly exercises the actualprocessChdirFlag(RootCmd)function rather than duplicating validation logic, providing accurate production code coverage for error cases.
34-234: LGTM! Comprehensive table-driven test coverage.The test structure properly covers absolute/relative paths, short/long flag forms, environment variable fallback, flag precedence, error conditions, and edge cases. The cleanup pattern with
os.Chdir(originalWd)before test end (line 231) is critical for Windows temp directory removal.
200-203: LGTM! Correct fallback to ATMOS_CHDIR.The logic properly falls back to the environment variable when the flag is empty, matching the production code's precedence behavior.
220-225: LGTM! EvalSymlinks handles macOS path resolution.Using
filepath.EvalSymlinkscorrectly handles macOS where/varsymlinks to/private/var, ensuring path comparisons work across platforms.
236-324: LGTM! Integration test validates chdir-before-config ordering.The test properly verifies that directory changes occur before configuration loading and base-path processing, which is critical to the --chdir flag's design.
326-438: LGTM! Edge cases thoroughly tested.Symlink handling, paths with spaces, and relative parent directory references are all covered. The symlink test appropriately skips on Windows when privileges are insufficient (line 350).
440-492: LGTM! Precedence validation is explicit and clear.The test explicitly verifies that CLI flag takes precedence over
ATMOS_CHDIRenvironment variable, and that the environment variable is used when no flag is provided.pkg/config/load_config_test.go (4)
11-11: Import alias improves clarity.The
errUtilsalias effectively distinguishes Atmos-specific errors from the standard library'serrorspackage, making the test assertions more readable.
180-224: Solid edge case coverage.This test effectively validates file path handling with proper use of
assert.ErrorIsto check for specific error types. The table-driven approach covers empty paths, type mismatches, and mixed valid/invalid inputs.
226-279: Directory validation well tested.The test mirrors the file validation approach and covers directory-specific edge cases including the distinction between files and directories. Error assertions are thorough.
281-353: Config file variant detection well covered.The test validates discovery of both
atmos.yamland.atmos.yamlvariants across multiple directories, plus the error case when no config exists. This provides good coverage for the config file resolution logic.
|
@aknysh, I'll perform a comprehensive review of all files and code changes in this PR. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pkg/config/load_config_test.go (1)
285-285: Add periods to inline comments.Per the coding guidelines, all comments must end with periods (enforced by golangci-lint godot).
Apply this diff:
- // Directory with atmos.yaml + // Directory with atmos.yaml.- // Directory with .atmos.yaml + // Directory with .atmos.yaml.- // Directory with no config file + // Directory with no config file.- // Create invalid YAML file + // Create invalid YAML file.As per coding guidelines
Also applies to: 294-294, 303-303, 359-359
CLAUDE.md (1)
268-276: Fix example: avoid shadowing testing.T; use tk.Use a new variable for TestKit and pass it to require/assert to keep examples compilable and idiomatic.
func TestMyCommand(t *testing.T) { - t := cmd.NewTestKit(t) // Wraps testing.TB with automatic cleanup + tk := cmd.NewTestKit(t) // Wraps testing.TB with automatic cleanup // Rest of test - all testing.TB methods available RootCmd.SetArgs([]string{"terraform", "plan"}) - require.NoError(t, RootCmd.PersistentFlags().Set("chdir", "/tmp")) + require.NoError(tk, RootCmd.PersistentFlags().Set("chdir", "/tmp")) // State automatically restored when test completes. }cmd/testing_helpers_test.go (1)
3-7: String slice flags restored inaccurately; use pflag.SliceValue + Replace.Current logic records String() (with brackets) and feeds it to Set(), which pflag parses as literal items (“[a”, “b]”). Clear the slice and restore via SliceValue.Replace.
@@ -import ( - "reflect" - - "github.com/spf13/pflag" -) +import ( + "github.com/spf13/pflag" +) @@ type flagSnapshot struct { - value string - changed bool + value string + slice []string + isSlice bool + changed bool } @@ snapshotFlags := func(flagSet *pflag.FlagSet) { flagSet.VisitAll(func(f *pflag.Flag) { - snapshot.flags[f.Name] = flagSnapshot{ - value: f.Value.String(), - changed: f.Changed, - } + snap := flagSnapshot{ + value: f.Value.String(), + changed: f.Changed, + } + if sv, ok := f.Value.(pflag.SliceValue); ok { + snap.slice = append([]string(nil), sv.GetSlice()...) + snap.isSlice = true + } + snapshot.flags[f.Name] = snap }) } @@ -func restoreStringSliceFlag(f *pflag.Flag, snap flagSnapshot) { - // Use reflection to access the underlying slice and clear it. - v := reflect.ValueOf(f.Value) - if v.Kind() == reflect.Ptr { - v = v.Elem() - } - // Look for a field that holds the slice (usually "value"). - if v.Kind() == reflect.Struct { - valueField := v.FieldByName("value") - if valueField.IsValid() && valueField.CanSet() { - // Reset to empty slice to prevent append behavior. - valueField.Set(reflect.MakeSlice(valueField.Type(), 0, 0)) - } - } - // Reset Changed state before setting value. - f.Changed = false - // Set the snapshot value if not default. - if snap.value != "[]" && snap.value != "" { - _ = f.Value.Set(snap.value) - } - // Restore Changed state. - f.Changed = snap.changed +func restoreStringSliceFlag(f *pflag.Flag, snap flagSnapshot) { + if !snap.isSlice { + return + } + if sv, ok := f.Value.(pflag.SliceValue); ok { + _ = sv.Replace(append([]string(nil), snap.slice...)) + } else { + // Fallback (should not happen for stringSlice/stringArray). + _ = f.Value.Set("") + } + f.Changed = snap.changed }Also applies to: 9-13, 33-41, 49-75
🧹 Nitpick comments (3)
cmd/root_test.go (1)
63-63: Minor: Extra blank line.Line 63 appears to be an accidental blank line that doesn't match the surrounding formatting.
Consider removing this extra blank line for consistency:
os.Args = []string{"atmos", "about"} - // Reset buffer to ensure clean state (previous tests may have written to logger). buf.Reset()cmd/testkit_test.go (1)
16-21: Docs: prefer tk := NewTestKit(t) in examples to avoid shadowing confusion.Purely a readability tweak in comments; tests are correct.
-// t := NewTestKit(t) +// tk := NewTestKit(t) @@ -// t := NewTestKit(t) +// tk := NewTestKit(t) @@ -// t := NewTestKit(t) // Each subtest gets cleanup +// tk := NewTestKit(t) // Each subtest gets cleanupAlso applies to: 37-41, 45-54
cmd/root_process_chdir_test.go (1)
69-76: Improve portability and reduce cleanup redundancy.
- Use a per-test temp root for the “non-existent directory” case so it’s OS-agnostic.
- Rely on t.Cleanup to restore CWD; the extra Chdir at the end is redundant.
@@ - { - name: "non-existent directory", - flagValue: "/this/path/does/not/exist", - setup: func(t *testing.T) string { - return "" - }, - expectError: true, - errorMsg: "directory does not exist", - }, + { + name: "non-existent directory", + setup: func(t *testing.T) string { + base := t.TempDir() + return filepath.Join(base, "does-not-exist") + }, + expectError: true, + errorMsg: "directory does not exist", + }, @@ - // IMPORTANT: Change back to original directory BEFORE test ends. - // On Windows, temp dir cleanup fails if we're still inside it. - _ = os.Chdir(originalWd) + // Cleanup to originalWd is already registered via t.Cleanup above.Also applies to: 182-185
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (59)
.github/workflows/codeql.yml(1 hunks)CLAUDE.md(1 hunks)cmd/auth_env_test.go(5 hunks)cmd/auth_login_test.go(4 hunks)cmd/auth_user_test.go(8 hunks)cmd/auth_validate_test.go(3 hunks)cmd/auth_whoami_test.go(4 hunks)cmd/describe_affected_test.go(4 hunks)cmd/describe_dependents_test.go(4 hunks)cmd/describe_stacks_test.go(5 hunks)cmd/describe_workflows_test.go(4 hunks)cmd/packer_init_test.go(1 hunks)cmd/packer_inspect_test.go(1 hunks)cmd/packer_output_test.go(1 hunks)cmd/packer_validate_test.go(1 hunks)cmd/packer_version_test.go(1 hunks)cmd/root.go(4 hunks)cmd/root_chdir_test.go(1 hunks)cmd/root_heatmap_test.go(5 hunks)cmd/root_process_chdir_test.go(1 hunks)cmd/root_test.go(2 hunks)cmd/terraform_generate_backend_test.go(1 hunks)cmd/terraform_generate_varfile_test.go(1 hunks)cmd/testing_helpers_snapshot_test.go(1 hunks)cmd/testing_helpers_test.go(1 hunks)cmd/testing_main_test.go(1 hunks)cmd/testkit_test.go(1 hunks)cmd/workflow_test.go(1 hunks)errors/errors.go(1 hunks)pkg/config/load_config_args.go(2 hunks)pkg/config/load_config_test.go(2 hunks)tests/snapshots/TestCLICommands_atmos_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_about_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_generate_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_generate_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_env_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_exec_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_login_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_user_configure_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_validate_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_auth_whoami_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_helmfile_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_helmfile_apply_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_helmfile_apply_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_helmfile_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_--help_alias_subcommand_check.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden(1 hunks)tools/lintroller/rule_os_chdir.go(1 hunks)website/blog/2025-01-15-chdir-flag.md(1 hunks)website/docs/cli/global-flags.mdx(3 hunks)
🧰 Additional context used
🧠 Learnings (27)
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Use table-driven tests for multiple scenarios
Applied to files:
cmd/describe_workflows_test.gocmd/describe_dependents_test.go
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to cmd/*/*_test.go : Add tests for new commands under cmd/<command>/<command>_test.go.
Applied to files:
cmd/testing_main_test.gocmd/describe_dependents_test.gocmd/auth_env_test.goCLAUDE.mdcmd/root_test.gocmd/testing_helpers_snapshot_test.gocmd/auth_validate_test.gocmd/auth_whoami_test.gocmd/root_process_chdir_test.gocmd/root_chdir_test.gocmd/auth_user_test.go
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to cmd/**/*_test.go : Command tests live under cmd/ alongside command implementations.
Applied to files:
cmd/testing_main_test.gocmd/auth_env_test.goCLAUDE.mdcmd/auth_validate_test.gocmd/auth_whoami_test.gocmd/auth_user_test.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
cmd/root.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.
Applied to files:
cmd/root.go
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to **/*.go : All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Applied to files:
cmd/testkit_test.gocmd/testing_helpers_snapshot_test.gopkg/config/load_config_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*.go : Document complex logic with inline comments
Applied to files:
cmd/testkit_test.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
PR: cloudposse/atmos#825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
tests/snapshots/TestCLICommands_atmos_atlantis_generate_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_auth_exec_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_helmfile_help.stdout.goldentests/snapshots/TestCLICommands_atmos_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_helmfile_apply_help.stdout.goldentests/snapshots/TestCLICommands_atmos_auth_env_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_helmfile_apply_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_terraform_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_about_--help.stdout.golden
📚 Learning: 2025-10-07T00:25:16.333Z
Learnt from: osterman
PR: cloudposse/atmos#1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
Applied to files:
tests/snapshots/TestCLICommands_atmos_terraform_--help_alias_subcommand_check.stdout.goldentests/snapshots/TestCLICommands_atmos_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_terraform_help.stdout.goldentests/snapshots/TestCLICommands_atmos_terraform_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_atlantis_--help.stdout.goldenwebsite/docs/cli/global-flags.mdxtests/snapshots/TestCLICommands_atmos_about_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to **/*_test.go : Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Applied to files:
CLAUDE.md
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to **/*.go : 80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
CLAUDE.md
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to pkg/**/*_test.go : Unit tests for packages live under pkg/ alongside implementation files.
Applied to files:
CLAUDE.md
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to tests/**/*_test.go : Integration tests live under tests/ using fixtures in tests/test-cases/.
Applied to files:
CLAUDE.md
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
tests/snapshots/TestCLICommands_atmos_--help.stdout.goldencmd/root_process_chdir_test.gocmd/root_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
cmd/root_process_chdir_test.gocmd/root_chdir_test.gotools/lintroller/rule_os_chdir.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
cmd/root_process_chdir_test.gocmd/root_chdir_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
cmd/root_process_chdir_test.gocmd/root_chdir_test.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
PR: cloudposse/atmos#1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-09-23T02:30:42.362Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-09-23T02:30:42.362Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
cmd/root_process_chdir_test.go
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
PR: cloudposse/atmos#1327
File: cmd/terraform.go:111-117
Timestamp: 2025-06-23T02:14:30.937Z
Learning: In cmd/terraform.go, flags for the DescribeAffected function are added dynamically at runtime when info.Affected is true. This is intentional to avoid exposing internal flags like "file", "format", "verbose", "include-spacelift-admin-stacks", "include-settings", and "upload" in the terraform command interface, while still providing them for the shared DescribeAffected function used by both `atmos describe affected` and `atmos terraform apply --affected`.
Applied to files:
cmd/describe_affected_test.go
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to website/** : Before committing documentation/site changes, run npm run build in website/ and fix errors, broken links, and missing images.
Applied to files:
website/docs/cli/global-flags.mdx
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to website/src/** : After modifying any component under website/src/, build the website and fix any MDX/component errors.
Applied to files:
website/docs/cli/global-flags.mdx
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to website/docs/** : After modifying any docs under website/docs/, build the website (npm run build) and ensure no errors.
Applied to files:
website/docs/cli/global-flags.mdx
📚 Learning: 2025-09-05T14:57:37.360Z
Learnt from: RoseSecurity
PR: cloudposse/atmos#1448
File: cmd/ansible.go:26-28
Timestamp: 2025-09-05T14:57:37.360Z
Learning: The Atmos codebase uses a consistent pattern for commands that delegate to external tools: `PersistentFlags().Bool("", false, doubleDashHint)` where doubleDashHint provides help text about using double dashes to separate Atmos options from native command arguments. This pattern is used across terraform, packer, helmfile, atlantis, aws, and ansible commands.
Applied to files:
website/docs/cli/global-flags.mdx
📚 Learning: 2025-09-10T17:34:52.568Z
Learnt from: Benbentwo
PR: cloudposse/atmos#1475
File: pkg/auth/providers/github/oidc.go:96-100
Timestamp: 2025-09-10T17:34:52.568Z
Learning: The ATMOS_ environment variable binding guideline applies to Atmos configuration variables, not external service-required environment variables like GitHub Actions OIDC variables (GITHUB_ACTIONS, ACTIONS_ID_TOKEN_*) which must use their standard names.
Applied to files:
website/docs/cli/global-flags.mdx
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
PR: cloudposse/atmos#1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain has been updated to follow XDG Base Directory Specification with helper functions GetXDGCacheDir() and GetXDGTempCacheDir() in toolchain/xdg_cache.go, using XDG_CACHE_HOME when set and falling back to ~/.cache/atmos-toolchain, making it consistent with atmos core's XDG compliance.
Applied to files:
website/docs/cli/global-flags.mdx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Lint (golangci)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Summary
🔇 Additional comments (87)
errors/errors.go (1)
99-99: LGTM!The new error sentinels are well-named and correctly positioned.
ErrFileAccessDeniedproperly distinguishes permission/access failures from "not found" scenarios, whileErrEmptyConfigPathandErrEmptyConfigFileprovide clear signals for empty input validation.Also applies to: 102-103
pkg/config/load_config_args.go (2)
114-124: Solid validation and error distinction.The empty-path check upfront is clean. Using
os.IsNotExist(err)to distinguish "not found" from permission errors (EPERM, EACCES) properly addresses the issue flagged in past reviews. Callers can now useerrors.Is()to distinguish scenarios correctly.
136-146: Error handling is correct.The empty-path validation and the use of
ErrFileAccessDeniedfor non-NotExist stat errors aligns with the fix confirmed in commit d432cdd. Error wrapping with%wpreserves the underlying cause for inspection.pkg/config/load_config_test.go (1)
180-224: Comprehensive test coverage.The new test suites thoroughly exercise edge cases for file and directory validation, including empty paths, type mismatches, and mixed valid/invalid entries. The use of
errors.Is()confirms the correct error sentinels are returned. Well done.Also applies to: 226-279, 281-353, 355-395
tests/snapshots/TestCLICommands_atmos_auth_user_configure_--help.stdout.golden (1)
19-21: LGTM!The new
--chdirflag documentation is clear and properly formatted in the help output. The explanation effectively communicates the flag's purpose.tools/lintroller/rule_os_chdir.go (1)
27-30: LGTM!The bypass logic is appropriate for chdir-specific test files that need to exercise
os.Chdirdirectly. The comment clearly explains the intent, and the string matching approach is reasonable for Go file naming conventions..github/workflows/codeql.yml (1)
93-97: LGTM!Pre-populating the module cache is a solid approach to prevent goanalysis_metalinter failures. The step placement is appropriate (after Go setup, before linting), and the comment provides helpful context with the upstream issue reference.
tests/snapshots/TestCLICommands_atmos_auth_validate_--help.stdout.golden (1)
21-23: LGTM!The
--chdirflag documentation is consistent with other help outputs and clearly explains the flag's behavior.cmd/root.go (5)
59-73: LGTM!The function signature and initial logic are solid. The precedence handling (flag > env var) is clear, and the
nolintcomment appropriately explains whyos.Getenvis necessary before config loading.
75-92: LGTM!Path validation is thorough and handles all edge cases appropriately. The error differentiation (non-existent vs. stat error vs. not-a-directory) provides clear feedback, and including the actual path in error messages aids debugging.
94-99: LGTM!The directory change and error handling are straightforward and correct. Using the validated absolute path ensures consistent behavior.
124-127: LGTM!The
processChdirFlagcall is correctly placed before config loading and runs unconditionally (even for help commands), ensuring the flag is always honored and validated. This addresses the previous review concern.
652-652: LGTM!Flag registration is correct. The
-Cshorthand follows Unix conventions (git, make), and the description clearly communicates the flag's purpose. Registering as a persistent flag ensures it's available globally.cmd/terraform_generate_backend_test.go (1)
17-18: LGTM!Adding
NewTestKit(t)initialization follows the test isolation pattern introduced in this PR. Placement at the test start ensures proper state management for RootCmd.cmd/packer_inspect_test.go (1)
14-15: LGTM!Consistent test initialization pattern. Placing it before the skip check ensures proper setup even if the test is skipped.
cmd/workflow_test.go (1)
13-14: LGTM!Test initialization follows the established pattern. Placement at test entry ensures proper state management throughout the test lifecycle.
cmd/packer_output_test.go (1)
14-15: LGTM! Test isolation applied correctly.The TestKit initialization is properly placed at the start of the test, ensuring RootCmd state is snapshotted and restored after test completion. This prevents flag pollution between tests.
cmd/packer_version_test.go (1)
14-15: LGTM! Consistent test isolation pattern.TestKit properly initialized before test execution. This ensures deterministic RootCmd state management.
cmd/describe_stacks_test.go (3)
17-18: LGTM! Test isolation initialized correctly.TestKit setup at the test function start ensures proper RootCmd state management.
50-51: Excellent! Subtest isolation applied.TestKit initialization at both the parent test (line 50) and within each subtest (line 106) ensures proper isolation for table-driven tests. This prevents state pollution between test cases.
Also applies to: 106-107
149-150: LGTM! Consistent pattern across all test functions.TestKit properly initialized in both test functions, maintaining isolation consistency across the test suite.
Also applies to: 166-167
cmd/packer_init_test.go (1)
11-12: LGTM! Test isolation applied correctly.TestKit initialization ensures deterministic test behavior and proper RootCmd state cleanup.
cmd/auth_whoami_test.go (2)
18-19: LGTM! Proper test and subtest isolation.TestKit initialization at both parent test (line 18) and within each subtest (line 124) ensures complete isolation for table-driven test scenarios.
Also applies to: 124-125
208-209: LGTM! Consistent test isolation across all test functions.TestKit properly applied to all test functions in the suite, maintaining consistent RootCmd state management.
Also applies to: 224-225
cmd/terraform_generate_varfile_test.go (1)
17-18: LGTM! Test isolation correctly initialized.TestKit setup ensures proper RootCmd state management throughout the test execution.
tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_help.stdout.golden (1)
67-69: LGTM! Snapshot correctly reflects new global flag.The
--chdir/-Cflag documentation is properly formatted and positioned in the Global Flags section, accurately reflecting the new feature added in this PR.tests/snapshots/TestCLICommands_atmos_atlantis_generate_help.stdout.golden (1)
24-26: LGTM! Consistent snapshot update.The
--chdirflag documentation matches the format in other help snapshots, ensuring consistent user experience across all commands.tests/snapshots/TestCLICommands_atmos_helmfile_--help.stdout.golden (1)
39-41: LGTM! Snapshot correctly reflects the new global flag.The
-C, --chdirflag is properly documented in the help output with a clear description. The positioning and formatting are consistent with other global flags.tests/snapshots/TestCLICommands_atmos_terraform_--help_alias_subcommand_check.stdout.golden (1)
164-166: LGTM! Help output correctly updated.The global flag is properly reflected in the Terraform command help output.
tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden (1)
55-57: LGTM! Snapshot accurately captures the flag addition.tests/snapshots/TestCLICommands_atmos_helmfile_help.stdout.golden (1)
39-41: LGTM! Consistent flag documentation.tests/snapshots/TestCLICommands_atmos_atlantis_--help.stdout.golden (1)
27-29: LGTM! Atlantis command help correctly updated.cmd/auth_login_test.go (3)
17-17: LGTM! TestKit initialization properly added.The TestKit setup at the beginning of the test ensures proper test isolation and RootCmd state management, consistent with the PR's test infrastructure improvements.
128-128: LGTM! Subtest isolation correctly added.TestKit initialization within the subtest ensures each test case runs with a clean RootCmd state.
197-197: LGTM! Test isolation consistently applied.Both test functions properly initialize TestKit for deterministic test execution.
Also applies to: 213-213
tests/snapshots/TestCLICommands_atmos_helmfile_apply_--help.stdout.golden (1)
21-23: LGTM! Help output correctly reflects the new flag.tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden (1)
37-40: LGTM! Snapshot correctly captures the flag in help output.The flag description is properly formatted across multiple lines as needed.
tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden (1)
55-57: Global flag addition looks correct.The new
-C, --chdirflag appears properly in the help output with clear description and consistent formatting.tests/snapshots/TestCLICommands_atmos_about_--help.stdout.golden (1)
19-21: Help output correctly updated.The
--chdirflag appears consistently in the about command's global flags.cmd/describe_affected_test.go (4)
16-17: Test isolation properly initialized.TestKit ensures deterministic test state and automatic cleanup.
38-39: TestKit initialization added.Proper test harness setup for isolation.
105-106: Subtest isolation established.Adding TestKit at both parent and subtest levels ensures proper state isolation for each test case.
158-159: Test setup looks good.Consistent pattern with other tests in the file.
cmd/packer_validate_test.go (1)
14-15: Test harness initialized correctly.Clean addition at the start of the test function.
cmd/auth_validate_test.go (3)
14-15: Test initialization looks good.Proper setup for test isolation at the parent level.
163-164: Subtest isolation added.Each test case gets its own clean state.
245-246: Integration test properly initialized.TestKit ensures clean state for integration testing.
tests/snapshots/TestCLICommands_atmos_atlantis_help.stdout.golden (1)
27-29: Atlantis help output updated correctly.The global flag appears as expected in the atlantis command help.
cmd/root_heatmap_test.go (5)
17-18: Test isolation initialized.Clean setup for the heatmap display test.
47-48: Subtest isolation added.Each heatmap mode test gets independent state.
85-86: TTY test properly isolated.Both parent and subtest levels have TestKit initialization for complete isolation.
Also applies to: 99-100
116-117: Nested subtests isolated correctly.The flag tests demonstrate proper isolation at multiple nesting levels.
Also applies to: 120-121, 128-129
137-138: Non-TTY test initialized.Consistent pattern applied throughout the test file.
tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_--help.stdout.golden (1)
67-69: Repo-config help output updated.The global
--chdirflag appears consistently across all command help outputs.tests/snapshots/TestCLICommands_atmos_--help.stdout.golden (1)
43-45: LGTM! The new chdir flag is correctly documented.The -C, --chdir flag appears in the expected location with clear, concise help text. The two-line description format matches the existing style for other flags.
tests/snapshots/TestCLICommands_atmos_auth_login_--help.stdout.golden (1)
19-21: LGTM! Global flag correctly inherited.The chdir flag appears correctly in the auth login subcommand's global flags, maintaining consistent formatting and placement.
tests/snapshots/TestCLICommands_atmos_auth_env_--help.stdout.golden (1)
21-23: LGTM! Consistent flag documentation.The chdir flag is properly displayed in the auth env command help with consistent formatting.
tests/snapshots/TestCLICommands_atmos_auth_whoami_--help.stdout.golden (1)
21-23: LGTM! Flag documentation remains consistent.The chdir flag is correctly shown in the auth whoami command help.
cmd/describe_workflows_test.go (4)
16-16: LGTM! Test isolation properly initialized.The TestKit initialization ensures each test runs with a clean RootCmd state, preventing test pollution from global flags like --chdir.
64-64: LGTM! Subtest isolation added.Each subtest now gets its own isolated environment, which is essential for table-driven tests.
84-84: LGTM! Test harness properly initialized.The TestKit setup ensures this test runs with predictable RootCmd state.
113-113: LGTM! Error test properly isolated.Error condition tests benefit from isolation to ensure they start from a known state.
cmd/testing_main_test.go (1)
10-23: LGTM! Proper package-level test lifecycle management.This TestMain implementation correctly snapshots RootCmd state before tests and restores it after, ensuring the cmd package doesn't pollute state for other test packages. The pattern of snapshot → run → restore → exit is the right approach.
cmd/describe_dependents_test.go (4)
16-16: LGTM! Test isolation initialized.The TestKit setup provides clean state for this test function.
43-43: LGTM! Test harness properly added.TestKit initialization ensures deterministic test behavior.
85-85: LGTM! Subtest isolation in place.Each iteration of the table-driven test gets isolated state.
105-105: LGTM! Error test isolation added.The TestKit ensures this error test starts from a clean slate.
tests/snapshots/TestCLICommands_atmos_atlantis_generate_--help.stdout.golden (1)
24-26: LGTM! Flag documentation consistent across all commands.The chdir flag is properly documented in the atlantis generate command help, maintaining the same format and placement as all other commands.
tests/snapshots/TestCLICommands_atmos_helmfile_apply_help.stdout.golden (1)
21-22: LGTM! Consistent snapshot update across all help outputs.The addition of the
-C, --chdirflag to the Global Flags section is consistent across all updated help snapshots (helmfile apply, auth exec, terraform, terraform apply). The flag description and placement are correct.cmd/auth_env_test.go (1)
19-19: LGTM! TestKit integration for test isolation.The TestKit initialization pattern is applied consistently across test functions and subtests, ensuring proper RootCmd state isolation and cleanup. This aligns with the PR's systematic approach to prevent test pollution.
cmd/auth_user_test.go (1)
16-16: LGTM! Consistent TestKit pattern applied.TestKit initialization is properly placed at the start of each test function and nested subtest, matching the pattern used throughout the codebase for test isolation.
cmd/root_test.go (4)
19-22: Good defensive check for CI environments.Skipping when TTY is unavailable ensures the test doesn't fail in CI environments where
/dev/ttyisn't accessible. This improves test reliability.
27-34: Clean working directory management.Using
t.Chdir()for directory restoration is idiomatic and ensures cleanup even if the test panics. Note that this requires Go 1.17+, which should be fine given the project's dependencies.
36-41: Correct handling of ATMOS_CHDIR cleanup.The explicit
os.Unsetenvbefore the defer is necessary becauset.Setenvonly restores to the original value (not unset state). The comment clearly explains this nuance.
64-65: Good isolation practice with buffer reset.Resetting the buffer before command execution ensures clean state and prevents cross-test pollution from previous tests that may have written to the logger.
website/blog/2025-01-15-chdir-flag.md (1)
202-207: Fix processing order conflict; version placeholder requires manual resolution.The processing order conflict is real. Lines 99–115 state chdir → base-path → config, while lines 202–207 incorrectly state chdir → config → base-path. The diff in the review is correct.
Version placeholder confirmed at line 210 ("Atmos vX.X.X"). The actual Atmos CLI version wasn't found in the repository snapshot. You'll need to confirm the current release version before merge.
Doc links are valid:
/cli/global-flagsand/cli/commands/commands.mdxboth exist and resolve correctly.website/docs/cli/global-flags.mdx (3)
23-82: Excellent documentation for the new --chdir flag.The documentation clearly explains the flag's purpose, use cases, and differences from
--base-path. The collapsible comparison section is particularly helpful for users who might confuse these two flags.
341-347: ATMOS_CHDIR environment variable properly documented.Clear documentation of the environment variable with proper precedence information (CLI flag takes precedence).
425-450: Comprehensive examples cover real-world scenarios.The directory change examples effectively demonstrate development workflows, CI/CD pipelines, and multi-repo operations. Well done.
cmd/testing_helpers_snapshot_test.go (3)
10-126: Comprehensive snapshot state capture tests.The test cases properly verify capturing of args, flag values, changed states, and both local and persistent flags. The args capture/restoration test (lines 75-104) addresses the previous review feedback effectively.
128-209: Thorough restoration tests.These tests validate that flag values and changed states are correctly restored to their snapshot values, including empty/default states. Well structured.
211-263: Immutability and cycle tests provide good coverage.The immutability test ensures snapshots are independent of live state changes, and the cycle test validates the complete workflow. Solid test design.
cmd/root_chdir_test.go (5)
13-32: Test helper properly exercises production code.Good implementation that calls the actual
processChdirFlag()function to validate error handling. This addresses the previous review concern about code duplication.
34-234: Comprehensive chdir flag testing.Excellent test coverage including absolute/relative paths, short/long flag forms, environment variable handling, precedence, and error conditions. The use of
filepath.EvalSymlinksfor macOS symlink compatibility shows attention to cross-platform details.
236-324: Integration tests validate real-world usage.These tests properly verify that chdir works with actual Atmos configuration files and directory structures. The verification that chdir processes before base-path is particularly important for understanding the flag's behavior.
326-438: Edge case tests handle tricky scenarios.The symlink test appropriately skips on Windows when privileges are insufficient, paths with spaces are tested, and parent directory navigation is verified. Thorough coverage of edge cases.
440-492: Precedence test clearly validates flag priority.The test properly verifies that the CLI flag takes precedence over the environment variable, and also confirms that the env var is used when the flag is not specified. Clear and effective test.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/config/load_config_test.go (1)
355-395: Error path test looks good.The test properly validates that invalid YAML triggers an error. The table-driven structure allows easy expansion for additional error scenarios.
Optional: Consider checking for a specific YAML parsing error type rather than just any error, to ensure the failure happens for the expected reason. This is nice-to-have rather than required.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/config/load_config_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/config/load_config_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Files:
pkg/config/load_config_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout; prefer utils.PrintfMessageToTUI for UI messages.
All new configurations must support Go templating using existing utilities and available template functions.
Prefer SDKs over external binaries for cross-platform support; use filepath/os/runtime for portability.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Use structured logging with levels (Fatal>Error>Warn>Debug>Trace); avoid string interpolation and ensure logging does not affect execution.
Prefer re...
Files:
pkg/config/load_config_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for packages live under pkg/ alongside implementation files.
Files:
pkg/config/load_config_test.go
🧠 Learnings (1)
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to **/*.go : All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Applied to files:
pkg/config/load_config_test.go
🧬 Code graph analysis (1)
pkg/config/load_config_test.go (3)
errors/errors.go (4)
ErrEmptyConfigFile(103-103)ErrExpectedFile(100-100)ErrEmptyConfigPath(102-102)ErrAtmosDirConfigNotFound(92-92)pkg/schema/schema.go (1)
ConfigAndStacksInfo(460-539)pkg/config/config.go (1)
InitCliConfig(25-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Run pre-commit hooks
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
🔇 Additional comments (3)
pkg/config/load_config_test.go (3)
11-11: Clean import alias.The
errUtilsalias properly distinguishes the Atmos errors package from stdlib errors and improves readability in the error assertions below.
180-224: Solid edge-case coverage.The table-driven test properly covers empty paths, directory-vs-file mismatches, and mixed scenarios. Error checking via
errors.Isaligns with the guidelines.Minor note: Line 201 has an empty
wantErrorbut still validates the error type viacheckErr. Consider adding a brief message for consistency, though the current approach works fine.
226-279: Good directory validation coverage.Comprehensive edge-case testing for directory validation with proper error-type checking. The test mirrors the file validation structure nicely.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/load_config_test.go (1)
65-67: Flip assert.Equal args (expected, actual).Current order is actual, expected; swap for clearer failures.
- assert.Equal(t, atmosConfig.Logs.Level, "Debug", "Logs level should be Debug") - assert.Equal(t, atmosConfig.Logs.File, "/dev/stderr", "Logs file should be /dev/stderr") - assert.Equal(t, atmosConfig.CliConfigPath, connectPaths([]string{filepath.Dir(atmosConfigFilePath), tmpDir2}), "CliConfigPath should be the concatenation of config files and directories") + assert.Equal(t, "Debug", atmosConfig.Logs.Level, "Logs level should be Debug") + assert.Equal(t, "/dev/stderr", atmosConfig.Logs.File, "Logs file should be /dev/stderr") + assert.Equal(t, connectPaths([]string{filepath.Dir(atmosConfigFilePath), tmpDir2}), atmosConfig.CliConfigPath, "CliConfigPath should be the concatenation of config files and directories")
♻️ Duplicate comments (1)
pkg/config/load_config_test.go (1)
158-158: Add period to inline comment (godot).- expected: "C:\\Program Files;D:\\Games;E:\\Projects;", // trailing semicolon present + expected: "C:\\Program Files;D:\\Games;E:\\Projects;", // Trailing semicolon present.As per coding guidelines
🧹 Nitpick comments (5)
pkg/config/load_config_test.go (5)
19-24: Make comment/message match the file actually created.We create test-config.yaml, not atmos.yaml. Tweak the comment and error for accuracy.
- // Create atmos.yaml file. + // Create primary config file. @@ - t.Fatalf("Failed to create atmos.yaml file: %v", err) + t.Fatalf("Failed to create config file: %v", err)
21-36: Defer file closes to avoid leaks on early failures.Safer and cleaner; then drop explicit Close() calls.
f, err := os.Create(atmosConfigFilePath) if err != nil { t.Fatalf("Failed to create config file: %v", err) } + defer f.Close() @@ - f.Close() + // close via defer @@ f2, err := os.Create(atmosConfigFilePath2) if err != nil { t.Fatalf("Failed to create atmos.yaml file: %v", err) } + defer f2.Close() @@ - f2.Close() + // close via deferAlso applies to: 40-54
3-13: Import runtime for GOOS-gated Windows test.Needed by the Windows-only guard below.
import ( "os" "path/filepath" + "runtime" "testing"As per coding guidelines.
144-178: Gate Windows-only expectations and fix godot on inline comment.This test hardcodes ';' and a trailing separator, which is Windows-specific. Skip on non-Windows; also add terminal period to the comment.
func TestConnectPaths_WindowsPaths(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skipf("Windows-only semantics: semicolon-separated PATH and trailing delimiter.") + } @@ - expected: "C:\\Program Files;D:\\Games;E:\\Projects;", // trailing semicolon present + expected: "C:\\Program Files;D:\\Games;E:\\Projects;", // Trailing semicolon present.As per coding guidelines.
123-141: Strengthen negative-path assertions.When tc.expectError is true, also assert specific error kinds (errors.Is) to lock behavior:
- invalid config file path → ErrFileNotFound (if defined).
- invalid config directory path → ErrAtmosDirConfigNotFound.
- both invalid → errors.Is(err, ErrFileNotFound) or errors.Is(err, ErrAtmosDirConfigNotFound).
If ErrFileNotFound exists in errors/errors.go, consider:
// inside tc.expectError branch switch tc.name { case "invalid config file path": assert.ErrorIs(t, err, errUtils.ErrFileNotFound) case "invalid config directory path": assert.ErrorIs(t, err, errUtils.ErrAtmosDirConfigNotFound) case "both invalid file and directory": assert.True(t, errors.Is(err, errUtils.ErrFileNotFound) || errors.Is(err, errUtils.ErrAtmosDirConfigNotFound), "should contain at least one of the expected sentinel errors", ) }If ErrFileNotFound is not present, skip that assertion and keep the existing check.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/config/load_config_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
pkg/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Place business logic in pkg rather than in cmd
Files:
pkg/config/load_config_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for multiple scenarios
**/*_test.go: Use table-driven unit tests for pure functions and focus on behavior; co-locate tests; target >80% coverage for pkg/ and internal/exec/.
Always use t.Skipf() with a clear reason; never use t.Skip() or t.Skipf without a reason.
Files:
pkg/config/load_config_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: All code must pass golangci-lint checks
Follow Go error handling idioms and use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider custom error types for domain-specific errors
Follow standard Go coding style; run gofmt and goimports
Use snake_case for environment variables
Document complex logic with inline comments
**/*.go: All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Group imports into three sections (stdlib, 3rd-party, Atmos), separated by blank lines; sort alphabetically within each group; preserve existing aliases.
Configuration loading must use Viper with precedence CLI → ENV → files → defaults; bind config name atmos and add path, AutomaticEnv, and ATMOS prefix.
All errors must be wrapped using static errors (defined in errors/errors.go); use errors.Join for multiple errors; fmt.Errorf with %w for context; use errors.Is for checks; never compare error strings.
Distinguish structured logging from UI output: UI prompts/status/errors to stderr; data/results to stdout; never use logging for UI.
Most text UI must go to stderr; only data/results to stdout; prefer utils.PrintfMessageToTUI for UI messages.
All new configurations must support Go templating using existing utilities and available template functions.
Prefer SDKs over external binaries for cross-platform support; use filepath/os/runtime for portability.
For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.
80% minimum coverage on new/changed lines and include unit tests for new features; add integration tests for CLI using tests/ fixtures.
Always bind environment variables with viper.BindEnv and provide ATMOS_ alternatives for every env var.
Use structured logging with levels (Fatal>Error>Warn>Debug>Trace); avoid string interpolation and ensure logging does not affect execution.
Prefer re...
Files:
pkg/config/load_config_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for packages live under pkg/ alongside implementation files.
Files:
pkg/config/load_config_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
📚 Learning: 2025-10-16T15:18:00.319Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-16T15:18:00.319Z
Learning: Applies to **/*.go : All Go comments must end with periods; applies to single-line, multi-line, inline, and documentation comments (golangci-lint godot).
Applied to files:
pkg/config/load_config_test.go
🧬 Code graph analysis (1)
pkg/config/load_config_test.go (3)
errors/errors.go (4)
ErrEmptyConfigFile(103-103)ErrExpectedFile(100-100)ErrEmptyConfigPath(102-102)ErrAtmosDirConfigNotFound(92-92)pkg/schema/schema.go (1)
ConfigAndStacksInfo(460-539)pkg/config/config.go (1)
InitCliConfig(25-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: website-deploy-preview
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
…gha-cache * 'fix-gha-cache' of github.com:cloudposse/atmos: Isolate AWS env vars during authentication (#1654) Changes auto-committed by Conductor (#1670) Add global --chdir flag for changing working directory (#1644) Fix: Preserve exit codes from shell commands and workflows (#1660) fix: isolate golangci-lint custom build to prevent git corruption (#1666)
|
These changes were released in v1.195.0-rc.1. |
what
--chdir/-Cflag for changing working directory before command executionATMOS_CHDIRenvironment variable support as alternative to flagcmd/root_chdir_test.gofilewhy
references
t.Setenv()andt.Chdir()testing
Chdir Flag Tests
cmd/root_chdir_test.gofile (separated to comply with file length lint rules)-C) and long (--chdir) flag formsTest Isolation & TestKit
cmd.NewTestKit(t)wrapper following Go 1.15+ testing.TB interface patternTest Results
documentation
website/docs/cli/global-flags.mdxwith flag description, usage, and exampleswebsite/blog/2025-01-15-chdir-flag.mdannouncing the featuretechnical details
TestKit Implementation
testing.TBinterface for composable test helperst.Cleanup()testing.TBmethods pass through:Helper(),Log(),Setenv(),Cleanup(), etc.Error Message Improvements
Code Quality
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Validation
Tests