Conversation
Dependency ReviewThe following issues were found:
License Issuesgo.mod
Scanned Files
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1859 +/- ##
==========================================
- Coverage 73.69% 73.59% -0.11%
==========================================
Files 736 744 +8
Lines 67269 67665 +396
==========================================
+ Hits 49577 49795 +218
- Misses 14321 14484 +163
- Partials 3371 3386 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughAdds ECR authentication: new schema types and integrations, an integration registry, AWS ECR helpers, a Docker config manager with file locking, CLI command Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as atmos CLI
participant AuthMgr as AuthManager
participant Registry as Integrations Registry
participant AWS as AWS ECR API
participant Docker as Docker Config Manager
CLI->>AuthMgr: auth ecr-login (explicit | named | identity)
alt named integration or identity-linked
AuthMgr->>Registry: Lookup/Create integration (kind "aws/ecr")
Registry-->>AuthMgr: ECRIntegration
AuthMgr->>AuthMgr: Authenticate identity -> obtain creds
else explicit registries
CLI->>AuthMgr: use env-provided AWS creds
end
AuthMgr->>AWS: GetAuthorizationToken(creds, account, region)
AWS-->>AuthMgr: token + proxyEndpoint + expiresAt
AuthMgr->>Docker: WriteAuth(registryURL, username, password)
Docker-->>AuthMgr: success / error
AuthMgr-->>CLI: report results, export DOCKER_CONFIG on success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.go📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Files:
**/*_test.go📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Files:
pkg/**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (13)📓 Common learnings📚 Learning: 2026-01-01T18:25:25.942ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2025-09-25T01:02:48.697ZApplied to files:
📚 Learning: 2026-01-01T18:25:25.942ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2026-01-01T18:25:25.942ZApplied to files:
📚 Learning: 2025-11-24T17:35:37.209ZApplied to files:
📚 Learning: 2026-01-01T18:25:25.942ZApplied to files:
📚 Learning: 2025-03-25T12:23:42.649ZApplied to files:
📚 Learning: 2026-01-01T18:25:25.942ZApplied to files:
📚 Learning: 2025-12-21T04:10:29.030ZApplied to files:
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (5)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/prd/ecr-authentication.md (2)
109-109: Add language specifications to fenced code blocks.Lines 109 and 161 contain fenced code blocks without language specifications. Specify
textorplaintextfor ASCII diagrams and directory structures to comply with markdownlint rules.-``` +```text ┌─────────────────────────────────────────────────────────────────┐ │ 1. User Executes Command │-``` +```text pkg/auth/ ├── identities/Also applies to: 161-161
337-341: Optional: Use typographical (curly) quotes in error message table.Lines 337, 338, and 341 use straight quotes (
") in user-facing message strings. Consider switching to typographical quotes ("and") for consistency with typographic conventions, though this is a minor style preference.
📜 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)
docs/prd/ecr-authentication.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/prd/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use kebab-case for PRD filenames:
command-registry-pattern.md,error-handling-strategy.md,testing-strategy.md. All PRDs indocs/prd/
Files:
docs/prd/ecr-authentication.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
🪛 Gitleaks (8.30.0)
docs/prd/ecr-authentication.md
[high] 234-234: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
docs/prd/ecr-authentication.md
[typographical] ~337-~337: Consider using a typographic opening quote here.
Context: ...enticated | ErrAuthenticationFailed | "Source identity 'aws-dev' is not authent...
(EN_QUOTES)
[typographical] ~337-~337: Consider using a typographic close quote here.
Context: ...enticated. Run: atmos auth login aws-dev" | | Invalid AWS credentials | `ErrECRAu...
(EN_QUOTES)
[typographical] ~338-~338: Consider using a typographic opening quote here.
Context: ...ntials | ErrECRAuthenticationFailed | "Failed to retrieve ECR authorization tok...
(EN_QUOTES)
[typographical] ~341-~341: Consider using a typographic close quote here.
Context: ... expired. Run: atmos auth refresh my-ecr" | ### 7. File Locking The Docker conf...
(EN_QUOTES)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/ecr-authentication.md
109-109: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
🔇 Additional comments (1)
docs/prd/ecr-authentication.md (1)
1-516: Comprehensive and well-structured PRD.The document provides a solid foundation for ECR authentication: clear problem statement, well-articulated design goals, detailed technical specifications with code examples, implementation phases, and security considerations. The user experience examples effectively illustrate the desired workflow, and the error handling matrix is helpful for developers. Filename follows kebab-case convention per guidelines.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/prd/ecr-authentication.md (1)
537-537: Add punctuation at end of paragraph.Line 537 is missing a period at the end:
...Atmos secret masking patterns→...Atmos secret masking patterns.- 6. **Secret Masking**: ECR tokens follow Atmos secret masking patterns + 6. **Secret Masking**: ECR tokens follow Atmos secret masking patterns.
📜 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)
docs/prd/ecr-authentication.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/prd/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use kebab-case for PRD filenames:
command-registry-pattern.md,error-handling-strategy.md,testing-strategy.md. All PRDs indocs/prd/
Files:
docs/prd/ecr-authentication.md
🧠 Learnings (11)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2024-12-13T15:33:34.159Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: pkg/config/cache.go:17-31
Timestamp: 2024-12-13T15:33:34.159Z
Learning: In `pkg/config/cache.go`, when `XDG_CACHE_HOME` is not set, falling back to `.` (current directory) is acceptable and aligns with the requirement to primarily use `XDG_CACHE_HOME` for the cache directory.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 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:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-03-12T21:38:42.699Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1139
File: pkg/config/go-homedir/homedir.go:183-196
Timestamp: 2025-03-12T21:38:42.699Z
Learning: The code in pkg/config/go-homedir is a direct fork of the mitchellh/go-homedir package and was intentionally imported as-is without modifications to maintain consistency with the original source. Security concerns or other improvements may be addressed in future PRs.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-12-06T19:24:49.343Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T19:24:49.343Z
Learning: Applies to **/*.go : Use `viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")` for environment variables - ATMOS_ prefix required. Configuration loading precedence: CLI flags → ENV vars → config files → defaults
Applied to files:
docs/prd/ecr-authentication.md
🪛 Gitleaks (8.30.0)
docs/prd/ecr-authentication.md
[high] 250-250: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
docs/prd/ecr-authentication.md
[typographical] ~364-~364: Consider using a typographic opening quote here.
Context: ...enticated | ErrAuthenticationFailed | "Source identity 'aws-dev' is not authent...
(EN_QUOTES)
[typographical] ~364-~364: Consider using a typographic close quote here.
Context: ...enticated. Run: atmos auth login aws-dev" | | Invalid AWS credentials | `ErrECRAu...
(EN_QUOTES)
[typographical] ~365-~365: Consider using a typographic opening quote here.
Context: ...ntials | ErrECRAuthenticationFailed | "Failed to retrieve ECR authorization tok...
(EN_QUOTES)
[typographical] ~368-~368: Consider using a typographic close quote here.
Context: ... expired. Run: atmos auth refresh my-ecr" | ### 7. File Locking The Docker conf...
(EN_QUOTES)
[typographical] ~532-~532: Consider using typographic quotation marks here.
Context: ...ored in Atmos XDG config directory (via pkg/xdg.GetXDGConfigDir("docker", 0700)), separate from user's default ...
(EN_QUOTES)
[grammar] ~537-~537: Please add a punctuation mark at the end of paragraph.
Context: ... ECR tokens follow Atmos secret masking patterns ## References - [AWS ECR GetAuthoriza...
(PUNCTUATION_PARAGRAPH_END)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/ecr-authentication.md
109-109: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
docs/prd/ecr-authentication.md (2)
7-7: Confirm XDG directory path precision.The document uses both
~/.config/atmos/docker/config.jsonand references to the XDG helper. Ensure the documented path aligns with whatpkg/xdg.GetXDGConfigDir("docker", 0700)actually returns. The phrase "viapkg/xdg.GetXDGConfigDir("docker", 0700)" at line 532 suggests the implementation detail is already resolved, but the path representation should be consistent.Also applies to: 60-60
220-230: GetXDGConfigDir function signature and behavior are correct.The code example accurately reflects the actual function signature in
pkg/xdg/xdg.go:GetXDGConfigDir(subpath string, perm os.FileMode) (string, error). The second parameter correctly accepts file permission mode (e.g.,0700as octal). The implementation follows proper XDG precedence:ATMOS_XDG_CONFIG_HOME(Atmos-specific override) →XDG_CONFIG_HOME(standard XDG) → platform defaults from the adrg/xdg library. The example usageGetXDGConfigDir("docker", 0700)is sound.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/prd/ecr-authentication.md (1)
346-368: Use typographic (curly) quotes for consistency in error message examples.Lines 364-368 use straight quotes in the error message examples. While functional, using typographic quotes (
"and") would improve markdown presentation. Minor style issue.Example for line 364:
-| Source identity not authenticated | `ErrAuthenticationFailed` | "Source identity 'aws-dev' is not authenticated. Run: atmos auth login aws-dev" | +| Source identity not authenticated | `ErrAuthenticationFailed` | "Source identity 'aws-dev' is not authenticated. Run: atmos auth login aws-dev" |(Note: This is a low-priority formatting preference, not a functional issue.)
📜 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)
docs/prd/ecr-authentication.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/prd/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
PRDs (Product Requirements Documents) must be in
docs/prd/using kebab-case filenames:command-registry-pattern.md,error-handling-strategy.md,testing-strategy.md
Files:
docs/prd/ecr-authentication.md
🧠 Learnings (11)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2024-12-13T15:33:34.159Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: pkg/config/cache.go:17-31
Timestamp: 2024-12-13T15:33:34.159Z
Learning: In `pkg/config/cache.go`, when `XDG_CACHE_HOME` is not set, falling back to `.` (current directory) is acceptable and aligns with the requirement to primarily use `XDG_CACHE_HOME` for the cache directory.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 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:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-03-12T21:38:42.699Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1139
File: pkg/config/go-homedir/homedir.go:183-196
Timestamp: 2025-03-12T21:38:42.699Z
Learning: The code in pkg/config/go-homedir is a direct fork of the mitchellh/go-homedir package and was intentionally imported as-is without modifications to maintain consistency with the original source. Security concerns or other improvements may be addressed in future PRs.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to cmd/**/*.go : Use `viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")` - ATMOS_ prefix required for environment variables.
Applied to files:
docs/prd/ecr-authentication.md
🪛 Gitleaks (8.30.0)
docs/prd/ecr-authentication.md
[high] 250-250: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
docs/prd/ecr-authentication.md
[typographical] ~364-~364: Consider using a typographic opening quote here.
Context: ...enticated | ErrAuthenticationFailed | "Source identity 'aws-dev' is not authent...
(EN_QUOTES)
[typographical] ~364-~364: Consider using a typographic close quote here.
Context: ...enticated. Run: atmos auth login aws-dev" | | Invalid AWS credentials | `ErrECRAu...
(EN_QUOTES)
[typographical] ~365-~365: Consider using a typographic opening quote here.
Context: ...ntials | ErrECRAuthenticationFailed | "Failed to retrieve ECR authorization tok...
(EN_QUOTES)
[typographical] ~368-~368: Consider using a typographic close quote here.
Context: ... expired. Run: atmos auth refresh my-ecr" | ### 7. File Locking The Docker conf...
(EN_QUOTES)
[typographical] ~532-~532: Consider using typographic quotation marks here.
Context: ...ored in Atmos XDG config directory (via pkg/xdg.GetXDGConfigDir("docker", 0700)), separate from user's default ...
(EN_QUOTES)
[grammar] ~537-~537: Please add a punctuation mark at the end of paragraph.
Context: ... ECR tokens follow Atmos secret masking patterns ## References - [AWS ECR GetAuthoriza...
(PUNCTUATION_PARAGRAPH_END)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/ecr-authentication.md
109-109: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
docs/prd/ecr-authentication.md (3)
245-256: Gitleaks alert is a false positive; example base64 string is safe.Line 250 triggered a gitleaks alert for "Generic API Key," but this is example documentation data (
QVdTOmV5SjBlWEJsLi4u), not a real credential. No action needed.
1-544: Solid PRD with clear architecture and addressing of previous feedback.This is a comprehensive and well-structured ECR authentication PRD. Key strengths:
- Addresses prior feedback: Uses
pkg/xdg.GetXDGConfigDir()(line 223) and defines identity kind constants (lines 316-323) as requested- Clear design flow: Problem → goals → technical specification → examples → checklist → success criteria
- Practical examples: Basic auth, multi-registry, whoami, logout, devcontainers, GitHub Actions
- Strong security posture: Credential isolation, file permissions, token lifetime, cleanup, no secrets in logs
- Well-organized phases: Implementation broken into 4 phases for clarity
- Proper XDG compliance: Respects
ATMOS_XDG_CONFIG_HOMEandXDG_CONFIG_HOMEper learningsThe document correctly:
- Uses kebab-case filename as per coding guidelines ✓
- Applies learnings on XDG Base Directory Specification ✓
- Defines constants instead of magic strings ✓
Minor formatting issues noted above; substance is solid.
374-385: Add language specification to file locking code block.Line 374 opens a fenced code block without a language identifier. Markdownlint requires this.
-import "github.com/gofrs/flock" +```go +import "github.com/gofrs/flock"⛔ Skipped due to learnings
Learnt from: CR Repo: cloudposse/atmos PR: 0 File: .cursor/rules/atmos-rules.mdc:0-0 Timestamp: 2025-11-24T17:35:37.209Z Learning: Applies to **/*.go : All code must pass golangci-lint checksLearnt from: CR Repo: cloudposse/atmos PR: 0 File: .cursor/rules/atmos-rules.mdc:0-0 Timestamp: 2025-11-24T17:35:37.209Z Learning: Applies to **/*.go : Document complex logic with inline comments in Go code
103565f to
ec2ae66
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/prd/ecr-authentication.md (1)
651-659: Minor: Polish typographical style (optional).The LanguageTool suggestions are stylistic only. If you'd like to polish further:
- Consider em dashes (—) instead of hyphens in descriptive phrases (e.g., lines 7, 45)
- Curly quotation marks around
"docker"on line 653 (though backticks are appropriate for code references)These are optional and the document reads well as-is.
📜 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)
docs/prd/ecr-authentication.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/prd/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
PRDs (Product Requirements Documents) must be in
docs/prd/using kebab-case filenames:command-registry-pattern.md,error-handling-strategy.md,testing-strategy.md
Files:
docs/prd/ecr-authentication.md
🧠 Learnings (17)
📚 Learning: 2025-01-25T03:51:57.689Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2024-12-13T15:33:34.159Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: pkg/config/cache.go:17-31
Timestamp: 2024-12-13T15:33:34.159Z
Learning: In `pkg/config/cache.go`, when `XDG_CACHE_HOME` is not set, falling back to `.` (current directory) is acceptable and aligns with the requirement to primarily use `XDG_CACHE_HOME` for the cache directory.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 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:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-03-12T21:38:42.699Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1139
File: pkg/config/go-homedir/homedir.go:183-196
Timestamp: 2025-03-12T21:38:42.699Z
Learning: The code in pkg/config/go-homedir is a direct fork of the mitchellh/go-homedir package and was intentionally imported as-is without modifications to maintain consistency with the original source. Security concerns or other improvements may be addressed in future PRs.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to cmd/**/*.go : Use `viper.BindEnv("ATMOS_VAR", "ATMOS_VAR", "FALLBACK")` - ATMOS_ prefix required for environment variables.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Document complex logic with inline comments in Go code
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow standard Go coding style: use `gofmt` and `goimports` to format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to **/*.go : NEVER delete existing comments without a very strong reason. Comments document why/how/what/where. Preserve helpful comments, update to match code, refactor for clarity, add context when modifying.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-12-10T23:12:15.768Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-10T23:12:15.768Z
Learning: Applies to **/*.go : ALWAYS ask for user approval before adding nolint comments. Prefer refactoring over nolint. Exception for bubbletea models: `//nolint:gocritic // bubbletea models must be passed by value`. Exception for intentional subprocess calls: `//nolint:gosec // intentional subprocess call`. NEVER add nolint for gocognit, cyclomatic complexity, magic numbers, or nestif.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
docs/prd/ecr-authentication.md
🪛 Gitleaks (8.30.0)
docs/prd/ecr-authentication.md
[high] 283-283: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
docs/prd/ecr-authentication.md
[typographical] ~7-~7: To join two clauses or introduce examples, consider using an em dash.
Context: ...AWS identities 2. Standalone command - atmos auth ecr-login for ad-hoc use wi...
(DASH_RULE)
[typographical] ~45-~45: To join two clauses or introduce examples, consider using an em dash.
Context: ...tion **Desired Experience (Option A - Automatic):**bash # Configure ecr_lo...
(DASH_RULE)
[typographical] ~653-~653: Consider using typographic quotation marks here.
Context: ...ored in Atmos XDG config directory (via pkg/xdg.GetXDGConfigDir("docker", 0700)), separate from user's default ...
(EN_QUOTES)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
docs/prd/ecr-authentication.md (2)
1-86: Excellent PRD structure and scope.This is a well-crafted product requirements document with clear problem statement, concrete user impact examples, and thoughtful design goals. The hybrid approach (PostAuthenticate hook + standalone command) is pragmatic and aligns well with existing Atmos workflows.
277-287: Gitleaks may flag the example auth token (false positive).The example base64 auth token on line 283 (
QVdTOmV5SjBlWEJsLi4u) might trigger security scanning in CI/CD, though this is clearly documentation/example content. Consider adding a comment in the code block clarifying it's a dummy example, or note in surrounding text that this is a sample format.Optional clarification:
**Docker Config Format:** -```json +```json { "auths": { "123456789012.dkr.ecr.us-east-2.amazonaws.com": { - "auth": "QVdTOmV5SjBlWEJsLi4u" + "auth": "QVdTOmV5SjBlWEJsLi4u" // Example: base64(AWS:token) } } }Alternatively, document in the text that the
authfield contains a base64-encodedusername:password(already mentioned on line 289, so this is fine as-is).
ae517e3 to
86a4033
Compare
|
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. |
0f03436 to
3d5ab73
Compare
|
@aknysh I'll trigger a full review of the PR now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
NOTICE (1)
140-143: Pipeline still reports NOTICE out of date.The past review indicated this was fixed in commit 1aa171a, but the pipeline failure persists. Run
./scripts/generate-notice.shagain and commit the result.
🧹 Nitpick comments (8)
website/blog/2025-12-15-ecr-authentication-integration.mdx (1)
190-190: Add missing period for consistency.The line ends without punctuation. While minor, keeping punctuation consistent improves readability.
🔎 Suggested fix
-- File locking via `gofrs/flock` prevents concurrent modification of Docker config +- File locking via `gofrs/flock` prevents concurrent modification of Docker config.pkg/auth/hooks_test.go (1)
110-120: Consider using mockgen for test mocks.The manual stub methods work but violate the coding guideline to generate mocks with
go.uber.org/mock/mockgen. The//go:generatedirective approach ensures the mock stays in sync with interface changes and reduces manual maintenance.As per coding guidelines: "Define interfaces for all major functionality; use dependency injection for testability; generate mocks with
go.uber.org/mock/mockgen(with//go:generatedirectives), never manual mocks"pkg/auth/identities/aws/assume_root_test.go (1)
1414-1424: Follow the suggestion in line 1327-1328 to use mockgen.The comment on lines 1327-1328 already recommends using mockgen to generate this mock. With the interface growing (now adding three more methods), this is a good time to act on that suggestion and replace the manual mock with a generated one.
As per coding guidelines: "generate mocks with
go.uber.org/mock/mockgen(with//go:generatedirectives), never manual mocks"pkg/auth/integrations/registry.go (2)
16-22: Consider adding validation in Register.The function doesn't validate that
kindis non-empty or thatfactoryis non-nil. Registering an empty kind or nil factory could cause runtime panics later in Create.🔎 Suggested validation
func Register(kind string, factory IntegrationFactory) { + if kind == "" { + panic("integration kind cannot be empty") + } + if factory == nil { + panic("integration factory cannot be nil") + } registryMu.Lock() defer registryMu.Unlock() registry[kind] = factory }
43-53: Consider sorting the returned kinds for deterministic output.Go map iteration order is undefined, so ListKinds returns kinds in non-deterministic order. This could cause flaky tests or inconsistent CLI output.
🔎 Suggested fix
+import ( + "sort" +) + func ListKinds() []string { registryMu.RLock() defer registryMu.RUnlock() kinds := make([]string, 0, len(registry)) for kind := range registry { kinds = append(kinds, kind) } + sort.Strings(kinds) return kinds }cmd/auth_ecr_login.go (1)
22-45: Consider embedding usage markdown per command guidelines.Other commands in the codebase use
//go:embedto load usage fromcmd/markdown/*_usage.mdfiles. This provides consistent formatting and follows the project pattern.Based on coding guidelines: "Embed examples from
cmd/markdown/*_usage.mdusing//go:embedand render withutils.PrintfMarkdown()"pkg/auth/manager_integrations_test.go (1)
452-475: Consider using generated mock instead of manual mock.Per coding guidelines, mocks should be generated with
go.uber.org/mock/mockgen. However, this is a simple test-local mock, so it's acceptable as a minor trade-off for simplicity.pkg/auth/cloud/docker/config.go (1)
78-80: Consider more robust fallback for Docker config directory.The fallback to
".docker"(line 80) is relative to the current working directory, which could lead to inconsistent behavior depending on where Atmos is invoked. While this is an edge case (homedir should rarely fail), consider using a more predictable fallback likeos.TempDir()or failing explicitly with a clear error message.🔎 Alternative: explicit error on fallback
homeDir, err := homedir.Dir() if err != nil { - // Fall back to current directory if home cannot be determined. - return ".docker" + // Return empty and let caller handle the error when mkdir fails. + // This provides clearer error messaging than silently using cwd. + return "" }
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (12)
internal/exec/terraform_output_utils_test.go (1)
60-71: Prefererrors.Is()for error checking.Line 70 checks the error message string directly. Per guidelines, use
errors.Is()with a sentinel error fromerrors/errors.goif one exists for "not implemented" cases.🔎 Example refactor using errors.Is()
If a sentinel error like
ErrNotImplementedexists inerrors/errors.go:+ errUtils "github.com/cloudposse/atmos/pkg/errors" + ctx := context.Background() _, err := wrapper.AuthenticateProvider(ctx, "test-provider") - // Should return an error, not panic. require.Error(t, err) - assert.Contains(t, err.Error(), "not implemented") + assert.True(t, errors.Is(err, errUtils.ErrNotImplemented))As per coding guidelines, all error checking should use
errors.Is()for consistency and type safety.pkg/auth/integrations/registry_test.go (2)
30-270: Consider test helpers for registry isolation.The registry snapshot/restore pattern (lines 32-45, 63-76, 98-111, 127-140, 187-200, 227-240) is repeated across multiple tests. Extracting this to a helper function would reduce duplication and make tests more maintainable.
🔎 Proposed helper function
Add to the test file:
// setupRegistryTest clears the registry for testing and returns a cleanup function. func setupRegistryTest(t *testing.T) { t.Helper() registryMu.Lock() originalRegistry := make(map[string]IntegrationFactory) for k, v := range registry { originalRegistry[k] = v } registry = make(map[string]IntegrationFactory) registryMu.Unlock() t.Cleanup(func() { registryMu.Lock() registry = originalRegistry registryMu.Unlock() }) }Then simplify each test:
func TestRegister(t *testing.T) { - // Clear the registry for this test. - registryMu.Lock() - originalRegistry := make(map[string]IntegrationFactory) - for k, v := range registry { - originalRegistry[k] = v - } - registry = make(map[string]IntegrationFactory) - registryMu.Unlock() - - // Restore the original registry after the test. - t.Cleanup(func() { - registryMu.Lock() - registry = originalRegistry - registryMu.Unlock() - }) + setupRegistryTest(t) // Register a test factory.
96-157: Consider table-driven tests for Create error scenarios.Tests for Create with various error conditions (unknown kind, factory error, nil config, nil inner config) follow a similar pattern. A table-driven test would improve maintainability and make it easier to add new error scenarios.
🔎 Proposed table-driven structure
func TestCreate_Errors(t *testing.T) { tests := []struct { name string setup func() // Register factories if needed config *IntegrationConfig expectedError error errorContains string }{ { name: "unknown kind", setup: func() {}, config: &IntegrationConfig{Name: "test", Config: &schema.Integration{Kind: "unknown/kind"}}, expectedError: errUtils.ErrUnknownIntegrationKind, }, { name: "factory error", setup: func() { Register("error/kind", func(config *IntegrationConfig) (Integration, error) { return nil, errors.New("factory error") }) }, config: &IntegrationConfig{Name: "test", Config: &schema.Integration{Kind: "error/kind"}}, errorContains: "factory error", }, { name: "nil config", setup: func() {}, config: nil, expectedError: errUtils.ErrIntegrationNotFound, }, { name: "nil inner config", setup: func() {}, config: &IntegrationConfig{Name: "test", Config: nil}, expectedError: errUtils.ErrIntegrationNotFound, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { setupRegistryTest(t) if tt.setup != nil { tt.setup() } _, err := Create(tt.config) require.Error(t, err) if tt.expectedError != nil { assert.ErrorIs(t, err, tt.expectedError) } if tt.errorContains != "" { assert.Contains(t, err.Error(), tt.errorContains) } }) } }pkg/auth/integrations/aws/ecr_test.go (1)
201-216: Consider testing Execute with mocked credentials.The test validates nil credential handling but doesn't verify the actual ECR authentication flow. Consider adding a test with mocked credentials and AWS SDK calls to ensure Execute properly handles the authentication token flow.
Per the coding guidelines: "Define interfaces for all major functionality; use dependency injection for testability; generate mocks with
go.uber.org/mock/mockgen." The Execute method likely calls AWS SDK methods that should be mocked to test the full authentication flow without hitting real AWS services.Based on learnings, tests should not hit real external services and should use interfaces + dependency injection for testability.
pkg/auth/manager_environment.go (1)
60-62: Consider wrapping with sentinel error.The error at line 61 doesn't use a sentinel from
errors/errors.go. Per coding guidelines, errors should be wrapped with static sentinels.Suggested fix
- return nil, fmt.Errorf("failed to prepare shell environment for identity %q: %w", identityName, err) + return nil, fmt.Errorf("%w: identity %q: %w", errUtils.ErrAuthManager, identityName, err)pkg/auth/manager_chain.go (1)
46-46: Consider using ctx parameter instead of context.Background().Line 46 uses
context.Background()but this method could accept a context from callers for cancellation propagation. Same pattern at line 291.docs/prd/ecr-authentication.md (1)
949-949: Minor: Missing period at end of line 949.Add a period after "cross-platform support" for consistency with surrounding sections.
cmd/auth_ecr_login_test.go (2)
12-22: Initialize all AuthConfig map fields for completeness.The test constructs an AuthConfig with Providers and Identities but omits the Integrations field. Initialize it to an empty map for consistency and to prevent nil-pointer issues if the manager logic iterates over integrations.
🔎 Proposed fix
func TestCreateECRAuthManager_Success(t *testing.T) { // Test that createECRAuthManager can be created with valid config. authConfig := &schema.AuthConfig{ Providers: map[string]schema.Provider{}, Identities: map[string]schema.Identity{}, + Integrations: map[string]schema.Integration{}, } manager, err := createECRAuthManager(authConfig) require.NoError(t, err) assert.NotNil(t, manager) }
52-57: Consider consolidating with TestAuthECRLoginCmd_TooManyArgs.This test duplicates the 2-argument error case already covered in TestAuthECRLoginCmd_TooManyArgs (lines 44-50). Consider either removing this test or merging it into a single table-driven test that covers all arg count scenarios (0, 1, 2+ args) to eliminate duplication.
As per coding guidelines: use table-driven tests for testing multiple scenarios.
pkg/auth/manager_integrations_test.go (3)
333-362: Consider consolidating with TestManager_ExecuteIntegration_NoIdentity.These two tests (NoIdentity and EmptyIdentity) validate nearly identical error paths and could be combined into a single table-driven test with cases for nil Via and empty Identity string. This would reduce duplication and make the test suite more maintainable.
As per coding guidelines: use table-driven tests for testing multiple scenarios.
409-429: Consider adding explicit verification.This test currently only verifies that triggerIntegrations doesn't panic when skipIntegrationsKey is set. Consider adding an assertion to verify the skip actually occurred, such as using a mock integration or checking that no integration execution was attempted.
As per coding guidelines: test behavior, not implementation; avoid tautological tests.
431-450: Consider adding explicit verification.Similar to TestManager_triggerIntegrations_SkippedWithContextKey, this test only verifies no panic occurs. Add an assertion to verify the expected no-op behavior.
As per coding guidelines: test behavior, not implementation; avoid tautological tests.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
NOTICE (1)
140-143: NOTICE file now includes ECR SDK dependency.The regenerated file properly includes the new
github.com/aws/aws-sdk-go-v2/service/ecrentry with Apache-2.0 license. Based on learnings, this file is programmatically generated - looks good.pkg/auth/cloud/aws/ecr.go (1)
102-107: BuildRegistryURL now has perf.Track per previous feedback.Addressed from prior review.
🧹 Nitpick comments (9)
website/blog/2025-12-15-ecr-authentication-integration.mdx (1)
15-15: Consider using standard truncate marker.Per coding guidelines, blog posts should use
<!--truncate-->after intro. The JSX-style{/* truncate */}works in MDX but the standard HTML comment is more conventional.Suggested change
-{/* truncate */} +<!--truncate-->pkg/auth/identities/aws/assume_root_test.go (1)
1414-1424: Mock methods correctly implement new interface requirements.These no-op implementations satisfy the expanded AuthManager interface without affecting existing test behavior. The pattern is consistent with the existing mock methods in this file.
Optional: Consider mockgen for interface changes
The comment at line 1327 suggests using mockgen if the AuthManager interface changes frequently. Since this PR adds three new methods across multiple test files, this might be a good time to consider generated mocks. However, this is purely optional—manual mocks are working fine for the current test needs.
pkg/auth/cloud/aws/ecr_test.go (1)
1-210: Consider adding tests for AWS SDK functions.The URL utility functions have excellent coverage. However,
GetAuthorizationTokenandLoadDefaultAWSCredentials(likely in ecr.go) appear untested. Consider adding unit tests with AWS SDK mocks to reach >80% coverage.Based on coding guidelines: Use interfaces and dependency injection for testability, generate mocks with mockgen.
pkg/schema/schema_auth.go (1)
118-141: LGTM!The new types follow schema conventions with proper tags and documentation. The structure aligns with the one-registry-per-integration design mentioned in PR objectives.
Optional consideration for future extensibility:
IntegrationSpec.Registry is ECR-specific (type *ECRRegistry). If many integration kinds are added with their own configs, consider using a generic Spec map[string]interface{} pattern (like Provider.Spec line 36) to avoid field proliferation. For now, with ECR and planned EKS integrations, the current approach is clean and type-safe.
pkg/auth/manager_environment.go (1)
61-61: Wrap error with a sentinel error from errors/errors.go.Line 61 uses a direct
fmt.Errorfwithout wrapping with a sentinel error. Per coding guidelines, all errors must be wrapped using static errors defined inerrors/errors.go.🔎 Suggested fix
- return nil, fmt.Errorf("failed to prepare shell environment for identity %q: %w", identityName, err) + return nil, fmt.Errorf("%w: failed to prepare shell environment for identity %q: %w", errUtils.ErrAuthManager, identityName, err)docs/prd/ecr-authentication.md (1)
949-949: Add period at end of paragraph.The paragraph ends without punctuation. Add a period for consistency with other sections.
-`pkg/config/homedir` for cross-platform support +`pkg/config/homedir` for cross-platform support.Based on learnings, markdownlint issues from past reviews were addressed in commit 103565f.
pkg/auth/manager_integrations_test.go (1)
452-475: Unused mock type.The
mockCredentialstype is defined and verified againsttypes.ICredentials, but it doesn't appear to be used in any of the tests in this file. Consider removing it or adding tests that actually exercise it.pkg/auth/manager_integrations.go (1)
190-195: Consider continuing on integration failure.Currently, the first failing integration stops execution. For consistency with
triggerIntegrations(which logs and continues), you might want to collect errors and return them all, or at least continue attempting remaining integrations.Option to continue on failure
// Execute each linked integration. + var errs []error for _, integrationName := range linkedIntegrations { if err := m.executeIntegration(ctx, integrationName, whoami.Credentials); err != nil { - return fmt.Errorf("integration '%s' failed: %w", integrationName, err) + errs = append(errs, fmt.Errorf("integration '%s' failed: %w", integrationName, err)) } } + if len(errs) > 0 { + return errors.Join(errs...) + } return nilpkg/auth/cloud/docker/config.go (1)
77-81: Homedir fallback to ".docker" may be unexpected.If
homedir.Dir()fails, falling back to.docker(current directory) could write config in an unexpected location. Consider returning an error instead.Return error on homedir failure
homeDir, err := homedir.Dir() if err != nil { - // Fall back to current directory if home cannot be determined. - return ".docker" + // Log warning but still attempt current directory as last resort + log.Warn("Failed to determine home directory, using current directory", "error", err) + return ".docker" }At minimum, consider logging a warning so users know something unusual happened.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
website/blog/2025-12-15-ecr-authentication-integration.mdx (1)
186-190: Cross-platform section looks solid.Good coverage of the implementation details. One small note: the file locking bullet on line 190 is missing terminal punctuation. This was flagged in prior review.
Proposed fix
-- File locking via `gofrs/flock` prevents concurrent modification of Docker config +- File locking via `gofrs/flock` prevents concurrent modification of Docker config.
🧹 Nitpick comments (2)
cmd/auth_ecr_login.go (2)
114-117: Potential double-wrapping of error.
ParseRegistryURLalready wraps errors withErrECRInvalidRegistry. Re-wrapping here adds redundancy. Consider simplifying.Proposed fix
accountID, region, err := awsCloud.ParseRegistryURL(registry) if err != nil { - return fmt.Errorf("%w: invalid registry URL %s: %w", errUtils.ErrECRInvalidRegistry, registry, err) + return err // ParseRegistryURL already wraps with ErrECRInvalidRegistry }
119-122: Same pattern - consider simplifying.
GetAuthorizationTokenwraps withErrECRAuthFailed. Re-wrapping may be intentional for context, but adds nesting.Proposed fix
result, err := awsCloud.GetAuthorizationToken(ctx, creds, accountID, region) if err != nil { - return fmt.Errorf("%w: ECR login failed for %s: %w", errUtils.ErrECRAuthFailed, registry, err) + return fmt.Errorf("ECR login failed for %s: %w", registry, 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 (15)
cmd/auth_ecr_login.gocmd/auth_ecr_login_test.godocs/prd/ecr-authentication.mdinternal/exec/terraform_output_authcontext_wrapper_test.gointernal/exec/terraform_output_utils_test.gopkg/auth/cloud/docker/config.gopkg/auth/integrations/registry.gopkg/auth/integrations/registry_test.gopkg/auth/manager_environment.gopkg/auth/manager_integrations.gopkg/auth/manager_integrations_test.gotests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsonwebsite/blog/2025-12-15-ecr-authentication-integration.mdxwebsite/src/data/roadmap.jswebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
💤 Files with no reviewable changes (1)
- internal/exec/terraform_output_authcontext_wrapper_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/auth/integrations/registry.go
- pkg/auth/manager_integrations_test.go
- website/src/data/roadmap.js
- pkg/auth/manager_environment.go
🧰 Additional context used
📓 Path-based instructions (7)
cmd/**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
cmd/**/*.go: Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file undercmd/directory
Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Provide meaningful feedback to users and include progress indicators for long-running operations in CLI commands
cmd/**/*.go: Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions, except trivial getters/setters, command constructors, simple factory functions, and functions that only delegate to another tracked function
Commands MUST use the command registry pattern viaCommandProviderinterface; seedocs/prd/command-registry-pattern.mdandcmd/internal/registry.go
Embed examples fromcmd/markdown/*_usage.mdusing//go:embedand render withutils.PrintfMarkdown()
Files:
cmd/auth_ecr_login.gocmd/auth_ecr_login_test.go
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
Never delete existing comments without a very strong reason; preserve helpful comments explaining why/how/what/where, and update comments to match code when refactoring
Organize imports in three groups separated by blank lines, sorted alphabetically: (1) Go stdlib, (2) 3rd-party (NOT cloudposse/atmos), (3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Useflags.NewStandardParser()for command-specific flags; NEVER callviper.BindEnv()orviper.BindPFlag()directly (enforced by Forbidigo)
All errors MUST be wrapped using static errors defined inerrors/errors.go; useerrors.Joinfor combining multiple errors,fmt.Errorfwith%wfor adding string context, error builder for complex errors, anderrors.Is()for error checking. Never use dynamic errors directly
Define interfaces for all major funct...
Files:
cmd/auth_ecr_login.gocmd/auth_ecr_login_test.gopkg/auth/integrations/registry_test.gointernal/exec/terraform_output_utils_test.gopkg/auth/manager_integrations.gopkg/auth/cloud/docker/config.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Usecmd.NewTestKit(t)for cmd tests to auto-clean RootCmd state (flags, args)
Test behavior, not implementation; never test stub functions; avoid tautological tests; useerrors.Is()for error checking; remove always-skipped tests
Prefer unit tests with mocks over integration tests; use interfaces and dependency injection for testability; use table-driven tests for comprehensive coverage; target >80% coverage; skip tests gracefully with helpers fromtests/test_preconditions.go
Never manually edit golden snapshot files undertests/test-cases/ortests/testdata/; always use-regenerate-snapshotsflag. Never use pipe redirection when running tests as it breaks TTY detection
Files:
cmd/auth_ecr_login_test.gopkg/auth/integrations/registry_test.gointernal/exec/terraform_output_utils_test.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/**/*.go: Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions, except trivial getters/setters, command constructors, simple factory functions, and functions that only delegate to another tracked function. Usenilif no atmosConfig param
Avoid adding new functions topkg/utils/; create purpose-built packages for new functionality (e.g.,pkg/store/,pkg/git/,pkg/pro/,pkg/filesystem/)
Files:
pkg/auth/integrations/registry_test.gopkg/auth/manager_integrations.gopkg/auth/cloud/docker/config.go
website/**
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
website/**: Update website documentation in thewebsite/directory when adding new features, ensure consistency between CLI help text and website documentation, and follow the website's documentation structure and style
Keep website code in thewebsite/directory, follow the existing website architecture and style, and test website changes locally before committing
Keep CLI documentation and website documentation in sync and document new features on the website with examples and use cases
Files:
website/blog/2025-12-15-ecr-authentication-integration.mdxwebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
website/blog/**/*.mdx
📄 CodeRabbit inference engine (CLAUDE.md)
Blog posts MUST use
.mdxwith YAML front matter (slug, title, authors, tags),<!--truncate-->after intro. Only use tags defined inwebsite/blog/tags.yml(never invent new tags). Only use authors fromwebsite/blog/authors.ymlor add new entry. Valid tags: feature, enhancement, bugfix, dx, breaking-change, security, documentation, deprecation, core
Files:
website/blog/2025-12-15-ecr-authentication-integration.mdx
internal/exec/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
New templates should support Go templating with
FuncMap()frominternal/exec/template_funcs.go
Files:
internal/exec/terraform_output_utils_test.go
🧠 Learnings (70)
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: When adding new CLI command: (1) Create `cmd/[command]/` with CommandProvider interface, (2) Add blank import to `cmd/root.go`, (3) Implement in `internal/exec/mycommand.go`, (4) Add tests and Docusaurus docs in `website/docs/cli/commands/`, (5) Build website: `cd website && npm run build`. See `docs/developing-atmos-commands.md` and `docs/prd/command-registry-pattern.md`
Applied to files:
cmd/auth_ecr_login.gocmd/auth_ecr_login_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to cmd/**/*.go : Commands MUST use the command registry pattern via `CommandProvider` interface; see `docs/prd/command-registry-pattern.md` and `cmd/internal/registry.go`
Applied to files:
cmd/auth_ecr_login.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Use Cobra's recommended command structure with a root command and subcommands, implementing each command in a separate file under `cmd/` directory
Applied to files:
cmd/auth_ecr_login.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to cmd/**/*.go : Provide comprehensive help text for all commands and flags, include examples in command help, and follow Go's documentation conventions in Cobra command definitions
Applied to files:
cmd/auth_ecr_login.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
cmd/auth_ecr_login.gopkg/auth/cloud/docker/config.godocs/prd/ecr-authentication.md
📚 Learning: 2025-12-13T04:37:25.223Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/root.go:0-0
Timestamp: 2025-12-13T04:37:25.223Z
Learning: In Atmos cmd/root.go Execute(), after cfg.InitCliConfig, we must call both toolchainCmd.SetAtmosConfig(&atmosConfig) and toolchain.SetAtmosConfig(&atmosConfig) so the CLI wrapper and the toolchain package receive configuration; missing either can cause nil-pointer panics in toolchain path resolution.
Applied to files:
cmd/auth_ecr_login.go
📚 Learning: 2025-10-10T23:51:36.597Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1599
File: internal/exec/terraform.go:394-402
Timestamp: 2025-10-10T23:51:36.597Z
Learning: In Atmos (internal/exec/terraform.go), when adding OpenTofu-specific flags like `--var-file` for `init`, do not gate them based on command name (e.g., checking if `info.Command == "tofu"` or `info.Command == "opentofu"`) because command names don't reliably indicate the actual binary being executed (symlinks, aliases). Instead, document the OpenTofu requirement in code comments and documentation, trusting users who enable the feature (e.g., `PassVars`) to ensure their terraform command points to an OpenTofu binary.
Applied to files:
cmd/auth_ecr_login.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
cmd/auth_ecr_login.gointernal/exec/terraform_output_utils_test.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
cmd/auth_ecr_login.gointernal/exec/terraform_output_utils_test.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
cmd/auth_ecr_login.gocmd/auth_ecr_login_test.gointernal/exec/terraform_output_utils_test.go
📚 Learning: 2025-12-13T06:19:17.739Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: pkg/http/client.go:0-0
Timestamp: 2025-12-13T06:19:17.739Z
Learning: In pkg/http/client.go, GetGitHubTokenFromEnv intentionally reads viper.GetString("github-token") to honor precedence of the persistent --github-token flag over ATMOS_GITHUB_TOKEN and GITHUB_TOKEN. Do not replace this with os.Getenv in future reviews; using Viper here is by design.
Applied to files:
cmd/auth_ecr_login.gopkg/auth/cloud/docker/config.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
cmd/auth_ecr_login.gopkg/auth/cloud/docker/config.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 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/auth_ecr_login.gointernal/exec/terraform_output_utils_test.gopkg/auth/manager_integrations.go
📚 Learning: 2024-10-31T19:25:41.298Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.
Applied to files:
cmd/auth_ecr_login.godocs/prd/ecr-authentication.md
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Use Registry Pattern for extensibility; existing implementations include Command Registry (`cmd/internal/registry.go` with `CommandProvider` interface) and Store Registry (`pkg/store/registry.go` with multi-provider implementations)
Applied to files:
cmd/auth_ecr_login.gopkg/auth/integrations/registry_test.go
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
cmd/auth_ecr_login.gocmd/auth_ecr_login_test.gopkg/auth/manager_integrations.gopkg/auth/cloud/docker/config.godocs/prd/ecr-authentication.md
📚 Learning: 2025-01-17T00:18:57.769Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
Applied to files:
cmd/auth_ecr_login.gopkg/auth/integrations/registry_test.go
📚 Learning: 2025-01-25T03:51:57.689Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Applied to files:
cmd/auth_ecr_login.go
📚 Learning: 2025-12-13T06:07:37.766Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:37.766Z
Learning: cloudposse/atmos: For PRD docs (docs/prd/*.md), markdownlint issues like MD040/MD010/MD034 can be handled in a separate documentation cleanup commit and should not block the current PR.
Applied to files:
cmd/auth_ecr_login.gopkg/auth/integrations/registry_test.go
📚 Learning: 2025-01-09T22:37:01.004Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.
Applied to files:
cmd/auth_ecr_login.go
📚 Learning: 2024-11-18T13:59:10.824Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 768
File: internal/exec/vendor_model_component.go:3-20
Timestamp: 2024-11-18T13:59:10.824Z
Learning: When replacing significant dependencies like `go-getter` that require extensive changes, prefer to address them in separate PRs.
Applied to files:
cmd/auth_ecr_login.go
📚 Learning: 2025-12-13T06:10:13.688Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: errors/errors.go:184-203
Timestamp: 2025-12-13T06:10:13.688Z
Learning: cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs; canonical toolchain sentinels live under toolchain/registry with re-exports in toolchain/errors.go.
Applied to files:
cmd/auth_ecr_login.gopkg/auth/integrations/registry_test.gopkg/auth/manager_integrations.go
📚 Learning: 2025-09-13T16:39:20.007Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: cmd/markdown/atmos_toolchain_aliases.md:2-4
Timestamp: 2025-09-13T16:39:20.007Z
Learning: In the cloudposse/atmos repository, CLI documentation files in cmd/markdown/ follow a specific format that uses " $ atmos command" (with leading space and dollar sign prompt) in code blocks. This is the established project convention and should not be changed to comply with standard markdownlint rules MD040 and MD014.
Applied to files:
cmd/auth_ecr_login.godocs/prd/ecr-authentication.md
📚 Learning: 2025-09-07T18:07:00.549Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: cmd/auth_login.go:43-44
Timestamp: 2025-09-07T18:07:00.549Z
Learning: In the atmos project, the identity flag is defined as a persistent flag on the auth root command (cmd/auth.go), making it available to all auth subcommands without needing to be redefined in each individual subcommand.
Applied to files:
cmd/auth_ecr_login.go
📚 Learning: 2025-12-13T04:37:40.435Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/toolchain/get.go:23-40
Timestamp: 2025-12-13T04:37:40.435Z
Learning: In Go CLI command files using Cobra, constrain the subcommand to accept at most one positional argument (MaximumNArgs(1)) so it supports both listing all items (zero args) and fetching a specific item (one arg). Define and parse flags with a standard parser (e.g., flags.NewStandardParser()) and avoid binding flags to Viper (no viper.BindEnv/BindPFlag). This promotes explicit argument handling and predictable flag behavior across command files.
Applied to files:
cmd/auth_ecr_login.gocmd/auth_ecr_login_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
cmd/auth_ecr_login.gocmd/auth_ecr_login_test.gopkg/auth/integrations/registry_test.gointernal/exec/terraform_output_utils_test.gopkg/auth/manager_integrations.gopkg/auth/cloud/docker/config.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
cmd/auth_ecr_login_test.gopkg/auth/integrations/registry_test.gointernal/exec/terraform_output_utils_test.gopkg/auth/manager_integrations.go
📚 Learning: 2025-12-10T18:32:43.260Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:43.260Z
Learning: In cmd subpackages, tests should avoid using NewTestKit(t) unless tests actually interact with RootCmd (e.g., execute commands via RootCmd or modify RootCmd state). For structural tests that only verify command structure/flags without touching RootCmd, TestKit cleanup is unnecessary.
Applied to files:
cmd/auth_ecr_login_test.go
📚 Learning: 2025-04-11T22:06:46.999Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.
Applied to files:
cmd/auth_ecr_login_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces and dependency injection for testability; use table-driven tests for comprehensive coverage; target >80% coverage; skip tests gracefully with helpers from `tests/test_preconditions.go`
Applied to files:
pkg/auth/integrations/registry_test.gointernal/exec/terraform_output_utils_test.gopkg/auth/manager_integrations.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*.go : Define interfaces for all major functionality; use dependency injection for testability; generate mocks with `go.uber.org/mock/mockgen` (with `//go:generate` directives), never manual mocks
Applied to files:
pkg/auth/integrations/registry_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/auth/integrations/registry_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
Applied to files:
pkg/auth/integrations/registry_test.gointernal/exec/terraform_output_utils_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/auth/integrations/registry_test.gointernal/exec/terraform_output_utils_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; use `errors.Is()` for error checking; remove always-skipped tests
Applied to files:
pkg/auth/integrations/registry_test.gointernal/exec/terraform_output_utils_test.go
📚 Learning: 2025-11-10T23:23:39.771Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/registry/aqua/aqua_test.go:417-442
Timestamp: 2025-11-10T23:23:39.771Z
Learning: In Atmos toolchain AquaRegistry, tests should not hit real GitHub. Use the options pattern via WithGitHubBaseURL to inject an httptest server URL and make GetLatestVersion/GetAvailableVersions deterministic.
Applied to files:
pkg/auth/integrations/registry_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*.go : All comments must end with periods (enforced by `godot` linter)
Applied to files:
pkg/auth/integrations/registry_test.gowebsite/blog/2025-12-15-ecr-authentication-integration.mdx
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*.go : Never delete existing comments without a very strong reason; preserve helpful comments explaining why/how/what/where, and update comments to match code when refactoring
Applied to files:
pkg/auth/integrations/registry_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Document complex logic with inline comments in Go code
Applied to files:
pkg/auth/integrations/registry_test.godocs/prd/ecr-authentication.md
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*.go : Keep files focused and small (<600 lines); use one cmd/impl per file; co-locate tests; never use `//revive:disable:file-length-limit`
Applied to files:
pkg/auth/integrations/registry_test.go
📚 Learning: 2024-12-02T21:26:32.337Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 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:
pkg/auth/integrations/registry_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Ensure all tests pass, verify code coverage meets targets, run golangci-lint and fix any issues, and update documentation before submitting pull requests
Applied to files:
pkg/auth/integrations/registry_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : All code must pass golangci-lint checks
Applied to files:
pkg/auth/integrations/registry_test.go
📚 Learning: 2025-09-10T21:17:55.273Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1466
File: toolchain/http_client_test.go:3-10
Timestamp: 2025-09-10T21:17:55.273Z
Learning: In the cloudposse/atmos repository, imports should never be changed as per samtholiya's coding guidelines.
Applied to files:
tests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Never manually edit golden snapshot files under `tests/test-cases/` or `tests/testdata/`; always use `-regenerate-snapshots` flag. Never use pipe redirection when running tests as it breaks TTY detection
Applied to files:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: ErrWrappingFormat is correctly defined as "%w: %w" in the errors package and is used throughout the codebase to wrap two error types together. The usage fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) is the correct pattern when both arguments are error types.
Applied to files:
pkg/auth/manager_integrations.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow Go's error handling idioms: use meaningful error messages, wrap errors with context using `fmt.Errorf("context: %w", err)`, and consider using custom error types for domain-specific errors
Applied to files:
pkg/auth/manager_integrations.go
📚 Learning: 2025-09-10T22:38:42.212Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/identities/aws/user.go:141-145
Timestamp: 2025-09-10T22:38:42.212Z
Learning: The user confirmed that the errors package has an error string wrapping format, contradicting the previous learning about ErrWrappingFormat being invalid. The current usage of fmt.Errorf(errUtils.ErrWrappingFormat, errUtils.ErrAuthAwsFileManagerFailed, err) appears to be the correct pattern.
Applied to files:
pkg/auth/manager_integrations.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*.go : All errors MUST be wrapped using static errors defined in `errors/errors.go`; use `errors.Join` for combining multiple errors, `fmt.Errorf` with `%w` for adding string context, error builder for complex errors, and `errors.Is()` for error checking. Never use dynamic errors directly
Applied to files:
pkg/auth/manager_integrations.go
📚 Learning: 2025-09-29T15:47:10.908Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/terraform_cli_args_utils.go:64-73
Timestamp: 2025-09-29T15:47:10.908Z
Learning: In the Atmos codebase, viper.BindEnv is required for CLI commands in the cmd/ package, but internal utilities can use os.Getenv directly when parsing environment variables for business logic purposes. The requirement to use viper is specific to the CLI interface layer, not all environment variable access throughout the codebase.
Applied to files:
pkg/auth/cloud/docker/config.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
pkg/auth/cloud/docker/config.godocs/prd/ecr-authentication.md
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*.go : Use `flags.NewStandardParser()` for command-specific flags; NEVER call `viper.BindEnv()` or `viper.BindPFlag()` directly (enforced by Forbidigo)
Applied to files:
pkg/auth/cloud/docker/config.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use Viper for managing configuration, environment variables, and flags in CLI commands
Applied to files:
pkg/auth/cloud/docker/config.go
📚 Learning: 2025-09-23T04:43:31.857Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1430
File: pkg/config/config.go:98-107
Timestamp: 2025-09-23T04:43:31.857Z
Learning: In the Atmos codebase, NO_PAGER environment variable handling is intentionally kept as direct os.Getenv() access in pkg/config/config.go rather than using Viper binding, because adding no_pager to the config file would be confusing for users. This is an acknowledged exception to the normal Viper binding pattern for environment variables.
Applied to files:
pkg/auth/cloud/docker/config.go
📚 Learning: 2025-04-10T20:48:22.687Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1147
File: pkg/config/load.go:0-0
Timestamp: 2025-04-10T20:48:22.687Z
Learning: In the `bindEnv` function in `pkg/config/load.go`, panic is used deliberately instead of returning errors because errors from `BindEnv` would only occur due to developer mistakes. Using panic helps with early detection of these developer errors during initialization.
Applied to files:
pkg/auth/cloud/docker/config.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Configuration loading precedence: CLI flags → ENV vars → config files → defaults (use Viper). Environment variables require ATMOS_ prefix via `viper.BindEnv("ATMOS_VAR", ...)`
Applied to files:
pkg/auth/cloud/docker/config.godocs/prd/ecr-authentication.md
📚 Learning: 2025-11-10T03:03:31.505Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 0
File: :0-0
Timestamp: 2025-11-10T03:03:31.505Z
Learning: In the Atmos codebase, commands using the `StandardParser` flag pattern (from pkg/flags) do NOT need explicit `viper.BindPFlag()` calls in their code. The StandardParser encapsulates flag binding internally: flags are registered via `parser.RegisterFlags(cmd)` in init(), and bound via `parser.BindFlagsToViper(cmd, v)` in RunE, which internally calls viper.BindPFlag for each flag. This pattern is used throughout Atmos (e.g., cmd/toolchain/get.go, cmd/toolchain/info.go, cmd/toolchain/install.go, cmd/toolchain/path.go). Do not flag missing viper.BindPFlag calls when StandardParser is used.
Applied to files:
pkg/auth/cloud/docker/config.go
📚 Learning: 2025-12-13T04:37:45.831Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: cmd/toolchain/get.go:23-40
Timestamp: 2025-12-13T04:37:45.831Z
Learning: In cmd/toolchain/get.go, the get subcommand intentionally uses cobra.MaximumNArgs(1) so it works with zero args (list all tools) or one arg (a specific tool). Flags are defined via flags.NewStandardParser() with --all (bool) and --limit (int); no direct viper.BindEnv/BindPFlag calls are used.
Applied to files:
pkg/auth/cloud/docker/config.go
📚 Learning: 2025-12-13T03:21:35.786Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1813
File: cmd/terraform/shell.go:28-73
Timestamp: 2025-12-13T03:21:35.786Z
Learning: In Atmos, when calling cfg.InitCliConfig, you must first populate the schema.ConfigAndStacksInfo struct with global flag values using flags.ParseGlobalFlags(cmd, v) rather than passing an empty struct. The LoadConfig function (pkg/config/load.go) reads config selection fields (AtmosConfigFilesFromArg, AtmosConfigDirsFromArg, BasePath, ProfilesFromArg) directly from the ConfigAndStacksInfo struct, NOT from Viper. Passing an empty struct causes config selection flags (--base-path, --config, --config-path, --profile) to be silently ignored. Correct pattern: parse flags → populate struct → call InitCliConfig. See cmd/terraform/plan_diff.go for reference implementation.
Applied to files:
pkg/auth/cloud/docker/config.go
📚 Learning: 2025-12-13T06:07:34.794Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: docs/prd/tool-dependencies-integration.md:58-64
Timestamp: 2025-12-13T06:07:34.794Z
Learning: For docs in the cloudposse/atmos repository under docs/prd/, markdownlint issues MD040, MD010, and MD034 should be deferred to a separate documentation cleanup commit and must not block the current PR. If needed, address these issues in a follow-up PR dedicated to documentation improvements.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain should follow XDG Base Directory Specification like the rest of atmos core, using XDG_CACHE_HOME environment variable when available and falling back to ~/.cache when not set, instead of hardcoding ~/.cache/tools-cache paths.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2024-12-13T15:33:34.159Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: pkg/config/cache.go:17-31
Timestamp: 2024-12-13T15:33:34.159Z
Learning: In `pkg/config/cache.go`, when `XDG_CACHE_HOME` is not set, falling back to `.` (current directory) is acceptable and aligns with the requirement to primarily use `XDG_CACHE_HOME` for the cache directory.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: XDG Base Directory Specification compliance implementation for atmos toolchain is complete: created toolchain/xdg_cache.go with GetXDGCacheDir() and GetXDGTempCacheDir() functions, updated toolchain/installer.go and cmd/toolchain_clean.go to use these XDG helpers, and changed all cache paths from hardcoded ~/.cache/tools-cache to XDG-compliant ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback).
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 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:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: The atmos toolchain XDG compliance implementation is complete with GetXDGCacheDir() and GetXDGTempCacheDir() functions in toolchain/xdg_cache.go, updated installer.go and toolchain_clean.go to use these helpers, and changed cache paths from ~/.cache/tools-cache to ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain when XDG_CACHE_HOME is not set).
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-09-08T01:25:44.958Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1466
File: website/docs/cli/commands/toolchain/usage.mdx:117-121
Timestamp: 2025-09-08T01:25:44.958Z
Learning: Final XDG Base Directory Specification implementation for atmos toolchain is complete and verified: toolchain/xdg_cache.go provides GetXDGCacheDir() and GetXDGTempCacheDir() functions, all hardcoded ~/.cache/tools-cache paths have been replaced with XDG-compliant paths using ${XDG_CACHE_HOME}/atmos-toolchain (or ~/.cache/atmos-toolchain fallback), and tests have been updated to expect the new path structure.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-03-12T21:38:42.699Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1139
File: pkg/config/go-homedir/homedir.go:183-196
Timestamp: 2025-03-12T21:38:42.699Z
Learning: The code in pkg/config/go-homedir is a direct fork of the mitchellh/go-homedir package and was intentionally imported as-is without modifications to maintain consistency with the original source. Security concerns or other improvements may be addressed in future PRs.
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Follow standard Go coding style: use `gofmt` and `goimports` to format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Applied to files:
docs/prd/ecr-authentication.md
📚 Learning: 2025-11-10T20:03:56.875Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1775
File: pkg/auth/providers/aws/sso_provisioning.go:40-79
Timestamp: 2025-11-10T20:03:56.875Z
Learning: In the Atmos AWS SSO provider (pkg/auth/providers/aws/sso_provisioning.go), the OAuth access token from the AWS SSO device flow is intentionally stored in the `AccessKeyID` field of `AWSCredentials` during authentication. This token is then extracted and used for ListAccounts and ListAccountRoles API calls during identity provisioning. This design reuses the existing `AWSCredentials` type for token transport rather than creating a separate credential type.
Applied to files:
docs/prd/ecr-authentication.md
🧬 Code graph analysis (3)
cmd/auth_ecr_login.go (8)
pkg/config/config.go (1)
InitCliConfig(28-67)errors/errors.go (7)
ErrFailedToInitConfig(468-468)ErrFailedToInitializeAuthManager(603-603)ErrECRLoginNoArgs(747-747)ErrIntegrationFailed(739-739)ErrECRInvalidRegistry(746-746)ErrECRAuthFailed(743-743)ErrDockerConfigWrite(748-748)pkg/auth/cloud/aws/ecr.go (3)
LoadDefaultAWSCredentials(165-186)ParseRegistryURL(111-123)GetAuthorizationToken(40-100)pkg/ui/formatter.go (1)
Success(204-211)pkg/auth/types/interfaces.go (1)
AuthManager(173-295)pkg/auth/credentials/store.go (1)
NewCredentialStore(42-46)pkg/auth/validation/validator.go (1)
NewValidator(19-21)pkg/auth/manager.go (1)
NewAuthManager(102-145)
cmd/auth_ecr_login_test.go (3)
pkg/schema/schema_auth.go (1)
AuthConfig(4-13)pkg/schema/schema.go (1)
Integrations(865-869)cmd/cmd_utils.go (1)
Contains(1303-1310)
pkg/auth/integrations/registry_test.go (4)
pkg/auth/integrations/types.go (2)
IntegrationFactory(35-35)IntegrationConfig(29-32)pkg/schema/schema_auth.go (5)
Integration(120-124)IntegrationVia(127-129)Identity(50-60)IntegrationSpec(132-135)ECRRegistry(138-141)pkg/auth/integrations/registry.go (4)
Register(20-30)Create(33-49)ListKinds(52-62)IsRegistered(65-70)errors/errors.go (2)
ErrUnknownIntegrationKind(738-738)ErrIntegrationNotFound(737-737)
🪛 Gitleaks (8.30.0)
docs/prd/ecr-authentication.md
[high] 472-472: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
docs/prd/ecr-authentication.md
[typographical] ~17-~17: Consider using a typographic opening quote here.
Context: ...isting identity. This cleanly separates "who you are" (identity) from "derived cr...
(EN_QUOTES)
[typographical] ~17-~17: Consider using a typographic close quote here.
Context: ...ity. This cleanly separates "who you are" (identity) from "derived credentials fo...
(EN_QUOTES)
[typographical] ~17-~17: Consider using a typographic opening quote here.
Context: ...separates "who you are" (identity) from "derived credentials for services" (integ...
(EN_QUOTES)
[typographical] ~17-~17: Consider using a typographic close quote here.
Context: ...) from "derived credentials for services" (integrations). ## Problem Statement ...
(EN_QUOTES)
[typographical] ~85-~85: Consider using a typographic opening quote here.
Context: ...eparation of Concerns**: Identities for "who you are", integrations for "derived ...
(EN_QUOTES)
[typographical] ~85-~85: Consider using a typographic close quote here.
Context: ... Concerns**: Identities for "who you are", integrations for "derived service cred...
(EN_QUOTES)
[typographical] ~85-~85: Consider using a typographic opening quote here.
Context: ...ies for "who you are", integrations for "derived service credentials" 2. **Explic...
(EN_QUOTES)
[typographical] ~85-~85: Consider using a typographic close quote here.
Context: ...rations for "derived service credentials" 2. Explicit Configuration: Each int...
(EN_QUOTES)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/exec/terraform_output_utils_test.go (1)
174-284: Remove this test – it violates coding guidelines.This test verifies that stub methods panic, which explicitly contradicts the guideline: "never test stub functions; avoid tautological tests." A past review comment requested removal of
TestAuthContextWrapper_StubMethodsPanic, which was marked as addressed, but this test appears to be the same one renamed toTestAuthContextWrapper_PanicMethods.These stubs are implementation details designed to panic if accidentally called. Testing them doesn't validate behavior—it only confirms the implementation matches itself.
🔎 Recommended action
-func TestAuthContextWrapper_PanicMethods(t *testing.T) { - wrapper := &authContextWrapper{ - stackInfo: &schema.ConfigAndStacksInfo{}, - } - ctx := context.Background() - - // Test that GetCachedCredentials panics. - assert.Panics(t, func() { - _, _ = wrapper.GetCachedCredentials(ctx, "test") - }) - - // Test that Authenticate panics. - assert.Panics(t, func() { - _, _ = wrapper.Authenticate(ctx, "test") - }) - - // Test that Whoami panics. - assert.Panics(t, func() { - _, _ = wrapper.Whoami(ctx, "test") - }) - - // Test that Validate panics. - assert.Panics(t, func() { - _ = wrapper.Validate() - }) - - // Test that GetDefaultIdentity panics. - assert.Panics(t, func() { - _, _ = wrapper.GetDefaultIdentity(false) - }) - - // Test that ListProviders panics. - assert.Panics(t, func() { - _ = wrapper.ListProviders() - }) - - // Test that Logout panics. - assert.Panics(t, func() { - _ = wrapper.Logout(ctx, "test", false) - }) - - // Test that GetChain panics. - assert.Panics(t, func() { - _ = wrapper.GetChain() - }) - - // Test that ListIdentities panics. - assert.Panics(t, func() { - _ = wrapper.ListIdentities() - }) - - // Test that GetProviderForIdentity panics. - assert.Panics(t, func() { - _ = wrapper.GetProviderForIdentity("test") - }) - - // Test that GetFilesDisplayPath panics. - assert.Panics(t, func() { - _ = wrapper.GetFilesDisplayPath("test") - }) - - // Test that GetProviderKindForIdentity panics. - assert.Panics(t, func() { - _, _ = wrapper.GetProviderKindForIdentity("test") - }) - - // Test that GetIdentities panics. - assert.Panics(t, func() { - _ = wrapper.GetIdentities() - }) - - // Test that GetProviders panics. - assert.Panics(t, func() { - _ = wrapper.GetProviders() - }) - - // Test that LogoutProvider panics. - assert.Panics(t, func() { - _ = wrapper.LogoutProvider(ctx, "test", false) - }) - - // Test that LogoutAll panics. - assert.Panics(t, func() { - _ = wrapper.LogoutAll(ctx, false) - }) - - // Test that GetEnvironmentVariables panics. - assert.Panics(t, func() { - _, _ = wrapper.GetEnvironmentVariables("test") - }) - - // Test that PrepareShellEnvironment panics. - assert.Panics(t, func() { - _, _ = wrapper.PrepareShellEnvironment(ctx, "test", nil) - }) - - // Test that ExecuteIntegration panics. - assert.Panics(t, func() { - _ = wrapper.ExecuteIntegration(ctx, "test") - }) - - // Test that ExecuteIdentityIntegrations panics. - assert.Panics(t, func() { - _ = wrapper.ExecuteIdentityIntegrations(ctx, "test") - }) - - // Test that GetIntegration panics. - assert.Panics(t, func() { - _, _ = wrapper.GetIntegration("test") - }) -}Based on coding guidelines.
🧹 Nitpick comments (10)
pkg/http/client.go (1)
145-158: Token resolution logic is solid.The implementation correctly prioritizes viper (for --github-token), then falls back to ATMOS_GITHUB_TOKEN, then GITHUB_TOKEN. This matches the documented precedence and aligns with the established design pattern for this function.
Based on learnings, using viper here is intentional, and the os.Getenv fallback is justified for non-toolchain commands.
Optional: Add periods to nolint comments
Per coding guidelines, all comments should end with periods:
- //nolint:forbidigo // Direct env lookup required for non-toolchain commands + //nolint:forbidigo // Direct env lookup required for non-toolchain commands. if token := os.Getenv("ATMOS_GITHUB_TOKEN"); token != "" { return token } - //nolint:forbidigo // Direct env lookup required for non-toolchain commands + //nolint:forbidigo // Direct env lookup required for non-toolchain commands. return os.Getenv("GITHUB_TOKEN")internal/exec/terraform_output_utils_test.go (1)
86-157: Consider table-driven tests for better structure.The tests correctly validate output variable retrieval behavior. However, per coding guidelines favoring table-driven tests, consider consolidating the inline test scenarios into test tables. This would improve maintainability and align with project patterns.
🔎 Example refactor for getTerraformOutputVariable tests
-func TestGetTerraformOutputVariable_SimpleKey(t *testing.T) { - atmosConfig := &schema.AtmosConfiguration{} - outputs := map[string]any{ - "vpc_id": "vpc-12345", - "subnet_ids": []string{"subnet-1", "subnet-2"}, - "null_output": nil, - } - - // Test simple key that exists. - value, exists, err := getTerraformOutputVariable(atmosConfig, "vpc", "dev-us-east-1", outputs, "vpc_id") - require.NoError(t, err) - assert.True(t, exists) - assert.Equal(t, "vpc-12345", value) - - // Test simple key that doesn't exist. - value, exists, err = getTerraformOutputVariable(atmosConfig, "vpc", "dev-us-east-1", outputs, "nonexistent") - require.NoError(t, err) - assert.False(t, exists) - assert.Nil(t, value) - - // Test key with nil value. - value, exists, err = getTerraformOutputVariable(atmosConfig, "vpc", "dev-us-east-1", outputs, "null_output") - require.NoError(t, err) - assert.True(t, exists) - assert.Nil(t, value) -} - -func TestGetTerraformOutputVariable_WithDotPrefix(t *testing.T) { - atmosConfig := &schema.AtmosConfiguration{} - outputs := map[string]any{ - "vpc_id": "vpc-12345", - } - - // Test with dot prefix. - value, exists, err := getTerraformOutputVariable(atmosConfig, "vpc", "dev-us-east-1", outputs, ".vpc_id") - require.NoError(t, err) - assert.True(t, exists) - assert.Equal(t, "vpc-12345", value) -} +func TestGetTerraformOutputVariable(t *testing.T) { + tests := []struct { + name string + outputs map[string]any + key string + expectedValue any + expectedExists bool + }{ + { + name: "simple key exists", + outputs: map[string]any{"vpc_id": "vpc-12345", "subnet_ids": []string{"subnet-1", "subnet-2"}}, + key: "vpc_id", + expectedValue: "vpc-12345", + expectedExists: true, + }, + { + name: "simple key missing", + outputs: map[string]any{"vpc_id": "vpc-12345"}, + key: "nonexistent", + expectedValue: nil, + expectedExists: false, + }, + { + name: "key with nil value", + outputs: map[string]any{"null_output": nil}, + key: "null_output", + expectedValue: nil, + expectedExists: true, + }, + { + name: "with dot prefix", + outputs: map[string]any{"vpc_id": "vpc-12345"}, + key: ".vpc_id", + expectedValue: "vpc-12345", + expectedExists: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + atmosConfig := &schema.AtmosConfiguration{} + value, exists, err := getTerraformOutputVariable(atmosConfig, "vpc", "dev-us-east-1", tt.outputs, tt.key) + require.NoError(t, err) + assert.Equal(t, tt.expectedExists, exists) + assert.Equal(t, tt.expectedValue, value) + }) + } +}Based on coding guidelines.
pkg/auth/cloud/aws/ecr_extended_test.go (1)
28-74: Consider consolidating into a table-driven test.These region-handling tests follow similar patterns and would benefit from a table-driven approach. This would improve maintainability and make it easier to add new scenarios.
Example table-driven structure
func TestBuildAWSConfigFromCreds_RegionHandling(t *testing.T) { tests := []struct { name string credentialsRegion string providedRegion string expectedRegion string wantErr bool }{ { name: "uses provided region", credentialsRegion: "eu-west-1", providedRegion: "us-east-1", expectedRegion: "us-east-1", }, { name: "falls back to credentials region", credentialsRegion: "ap-southeast-1", providedRegion: "", expectedRegion: "ap-southeast-1", }, { name: "success with matching regions", credentialsRegion: "us-west-2", providedRegion: "us-west-2", expectedRegion: "us-west-2", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() awsCreds := &types.AWSCredentials{ AccessKeyID: "AKIAIOSFODNN7EXAMPLE", SecretAccessKey: "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", Region: tt.credentialsRegion, } cfg, err := buildAWSConfigFromCreds(ctx, awsCreds, tt.providedRegion) if tt.wantErr { require.Error(t, err) return } require.NoError(t, err) assert.Equal(t, tt.expectedRegion, cfg.Region) assert.NotNil(t, cfg.Credentials) }) } }pkg/auth/manager_integrations_test.go (3)
407-448: Consider strengthening these no-op tests.Both
triggerIntegrationstests only verify no panic occurs. For more robust verification, consider injecting a mock or spy to confirm no integration execution happened. That said, these are reasonable defensive tests as-is.
494-508: Remove dead code - the nil assignment is immediately overwritten.Setting
config: nilon line 496 has no effect since it's overwritten on lines 500-502 before any method is called. The comment explains the intent, but the code is misleading.Proposed fix
func TestManager_GetIntegration_NilConfig(t *testing.T) { - m := &manager{ - config: nil, - } - - // This would panic, so we test with non-nil config but nil integrations. - m.config = &schema.AuthConfig{ + // Note: A truly nil config would panic; we test nil integrations instead. + m := &manager{ + config: &schema.AuthConfig{ + Integrations: nil, + }, Integrations: nil, } integration, err := m.GetIntegration("test")
285-405: Coverage gap: no success path tests for integration execution.The
ExecuteIntegrationandExecuteIdentityIntegrationstests cover error scenarios thoroughly. Consider adding success path tests that mock the integration factory/registry to verify an integration is created and executed correctly. This would help reach the >80% coverage target per guidelines.pkg/auth/manager_chain_extended_test.go (2)
136-138: Consider adding test coverage for negative index panic.The comment notes that negative indices cause a panic, describing it as a potential bug. Either add a test case that explicitly documents this behavior (using a panic recovery), or consider addressing the underlying issue to return an error or "unknown" instead of panicking.
Example test case with panic recovery
{ name: "negative index panics", index: -1, expected: "unknown", // or document the panic }If you want to test that it panics:
t.Run("negative index panics", func(t *testing.T) { defer func() { if r := recover(); r == nil { t.Errorf("expected panic for negative index") } }() _ = mgr.getChainStepName(-1) })
218-228: Direct struct creation bypasses constructor.This test creates the manager directly using a struct literal rather than
NewAuthManager. While this is acceptable for testing edge cases where the credential store is nil, it's slightly inconsistent with the constructor-based setup pattern used in other tests. Consider whether this edge case could be tested via the constructor or if direct creation is the clearest approach here.pkg/auth/cloud/docker/config_extended_test.go (2)
46-60: Consider using error type checking instead of string matching.The test verifies error messages with
assert.Contains(err.Error(), "invalid JSON"), which is fragile if error messages change. If there's a specific error type for JSON parsing failures (e.g., fromerrors/errors.go), prefererrors.Is()for more robust error checking.As per coding guidelines: "All errors MUST be wrapped using static errors defined in errors/errors.go... use errors.Is() for error checking."
143-143: Simplify the registry name construction.The rune arithmetic
"registry" + string(rune('0'+idx))works for 0-9 but is unnecessarily complex. Usefmt.Sprintf("registry%d.example.com", idx)for clarity.🔎 Proposed refactor
- registry := "registry" + string(rune('0'+idx)) + ".example.com" + registry := fmt.Sprintf("registry%d.example.com", idx)
📜 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 (7)
cmd/auth_ecr_login_test.gointernal/exec/terraform_output_utils_test.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/cloud/docker/config_extended_test.gopkg/auth/manager_chain_extended_test.gopkg/auth/manager_integrations_test.gopkg/http/client.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/auth_ecr_login_test.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
Never delete existing comments without a very strong reason; preserve helpful comments explaining why/how/what/where, and update comments to match code when refactoring
Organize imports in three groups separated by blank lines, sorted alphabetically: (1) Go stdlib, (2) 3rd-party (NOT cloudposse/atmos), (3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Useflags.NewStandardParser()for command-specific flags; NEVER callviper.BindEnv()orviper.BindPFlag()directly (enforced by Forbidigo)
All errors MUST be wrapped using static errors defined inerrors/errors.go; useerrors.Joinfor combining multiple errors,fmt.Errorfwith%wfor adding string context, error builder for complex errors, anderrors.Is()for error checking. Never use dynamic errors directly
Define interfaces for all major funct...
Files:
pkg/auth/cloud/docker/config_extended_test.gopkg/auth/manager_integrations_test.gointernal/exec/terraform_output_utils_test.gopkg/http/client.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/manager_chain_extended_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Usecmd.NewTestKit(t)for cmd tests to auto-clean RootCmd state (flags, args)
Test behavior, not implementation; never test stub functions; avoid tautological tests; useerrors.Is()for error checking; remove always-skipped tests
Prefer unit tests with mocks over integration tests; use interfaces and dependency injection for testability; use table-driven tests for comprehensive coverage; target >80% coverage; skip tests gracefully with helpers fromtests/test_preconditions.go
Never manually edit golden snapshot files undertests/test-cases/ortests/testdata/; always use-regenerate-snapshotsflag. Never use pipe redirection when running tests as it breaks TTY detection
Files:
pkg/auth/cloud/docker/config_extended_test.gopkg/auth/manager_integrations_test.gointernal/exec/terraform_output_utils_test.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/manager_chain_extended_test.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/**/*.go: Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions, except trivial getters/setters, command constructors, simple factory functions, and functions that only delegate to another tracked function. Usenilif no atmosConfig param
Avoid adding new functions topkg/utils/; create purpose-built packages for new functionality (e.g.,pkg/store/,pkg/git/,pkg/pro/,pkg/filesystem/)
Files:
pkg/auth/cloud/docker/config_extended_test.gopkg/auth/manager_integrations_test.gopkg/http/client.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/manager_chain_extended_test.go
internal/exec/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
New templates should support Go templating with
FuncMap()frominternal/exec/template_funcs.go
Files:
internal/exec/terraform_output_utils_test.go
🧠 Learnings (34)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/auth/cloud/docker/config_extended_test.gopkg/auth/manager_integrations_test.gointernal/exec/terraform_output_utils_test.gopkg/auth/manager_chain_extended_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/auth/cloud/docker/config_extended_test.gopkg/auth/manager_integrations_test.gointernal/exec/terraform_output_utils_test.gopkg/auth/manager_chain_extended_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces and dependency injection for testability; use table-driven tests for comprehensive coverage; target >80% coverage; skip tests gracefully with helpers from `tests/test_preconditions.go`
Applied to files:
pkg/auth/cloud/docker/config_extended_test.gopkg/auth/manager_integrations_test.gointernal/exec/terraform_output_utils_test.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/manager_chain_extended_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in Go
Applied to files:
pkg/auth/cloud/docker/config_extended_test.gointernal/exec/terraform_output_utils_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/auth/cloud/docker/config_extended_test.gopkg/auth/manager_integrations_test.gointernal/exec/terraform_output_utils_test.gopkg/http/client.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/manager_chain_extended_test.go
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
pkg/auth/manager_integrations_test.gopkg/auth/manager_chain_extended_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*.go : Define interfaces for all major functionality; use dependency injection for testability; generate mocks with `go.uber.org/mock/mockgen` (with `//go:generate` directives), never manual mocks
Applied to files:
pkg/auth/manager_integrations_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
Applied to files:
pkg/auth/manager_integrations_test.gointernal/exec/terraform_output_utils_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; use `errors.Is()` for error checking; remove always-skipped tests
Applied to files:
pkg/auth/manager_integrations_test.gointernal/exec/terraform_output_utils_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to internal/exec/**/*.go : New templates should support Go templating with `FuncMap()` from `internal/exec/template_funcs.go`
Applied to files:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2025-12-10T18:32:51.237Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1808
File: cmd/terraform/backend/backend_delete_test.go:9-23
Timestamp: 2025-12-10T18:32:51.237Z
Learning: In cmd subpackages (e.g., cmd/terraform/backend/), tests cannot use cmd.NewTestKit(t) due to Go's test visibility rules (NewTestKit is in a parent package test file). These tests only need TestKit if they execute commands through RootCmd or modify RootCmd state. Structural tests that only verify command structure/flags without touching RootCmd don't require TestKit cleanup.
Applied to files:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2024-11-02T15:35:09.958Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.
Applied to files:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2024-12-07T16:19:01.683Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 825
File: internal/exec/terraform.go:30-30
Timestamp: 2024-12-07T16:19:01.683Z
Learning: In `internal/exec/terraform.go`, skipping stack validation when help flags are present is not necessary.
Applied to files:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2025-04-26T15:54:10.506Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 1195
File: internal/exec/terraform_clean.go:99-99
Timestamp: 2025-04-26T15:54:10.506Z
Learning: The error variable `ErrRelPath` is defined in `internal/exec/terraform_clean_util.go` and is used across files in the `exec` package, including in `terraform_clean.go`. This is part of an approach to standardize error handling in the codebase.
Applied to files:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Never manually edit golden snapshot files under `tests/test-cases/` or `tests/testdata/`; always use `-regenerate-snapshots` flag. Never use pipe redirection when running tests as it breaks TTY detection
Applied to files:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2025-11-11T03:47:45.878Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/add_test.go:67-77
Timestamp: 2025-11-11T03:47:45.878Z
Learning: In the cloudposse/atmos codebase, tests should prefer t.Setenv for environment variable setup/teardown instead of os.Setenv/Unsetenv to ensure test-scoped isolation.
Applied to files:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2025-11-11T03:47:59.576Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/which_test.go:166-223
Timestamp: 2025-11-11T03:47:59.576Z
Learning: In the cloudposse/atmos repo, tests that manipulate environment variables should use testing.T.Setenv for automatic setup/teardown instead of os.Setenv/Unsetenv.
Applied to files:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 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:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2025-12-13T06:10:25.156Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: internal/exec/workflow_utils.go:0-0
Timestamp: 2025-12-13T06:10:25.156Z
Learning: Atmos workflows: In internal/exec/workflow_utils.go ExecuteWorkflow, non-identity steps intentionally use baseWorkflowEnv, which is constructed from the parent environment with PATH modifications for the toolchain. Avoid appending os.Environ() again; prefer documenting this behavior and testing that standard environment variables are preserved.
Applied to files:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 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:
internal/exec/terraform_output_utils_test.go
📚 Learning: 2025-12-13T06:19:17.739Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: pkg/http/client.go:0-0
Timestamp: 2025-12-13T06:19:17.739Z
Learning: In pkg/http/client.go, GetGitHubTokenFromEnv intentionally reads viper.GetString("github-token") to honor precedence of the persistent --github-token flag over ATMOS_GITHUB_TOKEN and GITHUB_TOKEN. Do not replace this with os.Getenv in future reviews; using Viper here is by design.
Applied to files:
pkg/http/client.go
📚 Learning: 2025-11-08T19:56:18.660Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1697
File: internal/exec/oci_utils.go:0-0
Timestamp: 2025-11-08T19:56:18.660Z
Learning: In the Atmos codebase, when a function receives an `*schema.AtmosConfiguration` parameter, it should read configuration values from `atmosConfig.Settings` fields rather than using direct `os.Getenv()` or `viper.GetString()` calls. The Atmos pattern is: viper.BindEnv in cmd/root.go binds environment variables → Viper unmarshals into atmosConfig.Settings via mapstructure → business logic reads from the Settings struct. This provides centralized config management, respects precedence, and enables testability. Example: `atmosConfig.Settings.AtmosGithubToken` instead of `os.Getenv("ATMOS_GITHUB_TOKEN")` in functions like `getGHCRAuth` in internal/exec/oci_utils.go.
Applied to files:
pkg/http/client.go
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
pkg/http/client.go
📚 Learning: 2025-07-05T20:59:02.914Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (`_ "github.com/hairyhenderson/gomplate/v4"`) alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.
Applied to files:
pkg/http/client.go
📚 Learning: 2025-03-12T21:38:42.699Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1139
File: pkg/config/go-homedir/homedir.go:183-196
Timestamp: 2025-03-12T21:38:42.699Z
Learning: The code in pkg/config/go-homedir is a direct fork of the mitchellh/go-homedir package and was intentionally imported as-is without modifications to maintain consistency with the original source. Security concerns or other improvements may be addressed in future PRs.
Applied to files:
pkg/http/client.go
📚 Learning: 2025-11-10T23:23:39.771Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1686
File: toolchain/registry/aqua/aqua_test.go:417-442
Timestamp: 2025-11-10T23:23:39.771Z
Learning: In Atmos toolchain AquaRegistry, tests should not hit real GitHub. Use the options pattern via WithGitHubBaseURL to inject an httptest server URL and make GetLatestVersion/GetAvailableVersions deterministic.
Applied to files:
pkg/http/client.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to pkg/**/*.go : Add `defer perf.Track(atmosConfig, "pkg.FuncName")()` + blank line to all public functions, except trivial getters/setters, command constructors, simple factory functions, and functions that only delegate to another tracked function. Use `nil` if no atmosConfig param
Applied to files:
pkg/http/client.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use Viper for managing configuration, environment variables, and flags in CLI commands
Applied to files:
pkg/http/client.go
📚 Learning: 2025-02-20T18:33:16.567Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1077
File: pkg/downloader/custom_github_detector.go:47-68
Timestamp: 2025-02-20T18:33:16.567Z
Learning: Go doesn't have a built-in secure string type. For sensitive data like tokens, using environment variables (which are protected by the OS) is a common and secure practice.
Applied to files:
pkg/http/client.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Configuration loading precedence: CLI flags → ENV vars → config files → defaults (use Viper). Environment variables require ATMOS_ prefix via `viper.BindEnv("ATMOS_VAR", ...)`
Applied to files:
pkg/http/client.go
📚 Learning: 2025-09-29T15:47:10.908Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/terraform_cli_args_utils.go:64-73
Timestamp: 2025-09-29T15:47:10.908Z
Learning: In the Atmos codebase, viper.BindEnv is required for CLI commands in the cmd/ package, but internal utilities can use os.Getenv directly when parsing environment variables for business logic purposes. The requirement to use viper is specific to the CLI interface layer, not all environment variable access throughout the codebase.
Applied to files:
pkg/http/client.go
📚 Learning: 2024-12-15T10:20:08.436Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/cmd_utils.go:454-464
Timestamp: 2024-12-15T10:20:08.436Z
Learning: Avoid adding timeout handling for GitHub API calls in `CheckForAtmosUpdateAndPrintMessage` function in `cmd/cmd_utils.go`, as it might be disabled by user settings.
Applied to files:
pkg/http/client.go
📚 Learning: 2025-09-09T02:14:36.708Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: internal/auth/types/whoami.go:14-15
Timestamp: 2025-09-09T02:14:36.708Z
Learning: The WhoamiInfo struct in internal/auth/types/whoami.go requires the Credentials field to be JSON-serializable for keystore unmarshaling operations, despite security concerns about credential exposure.
Applied to files:
pkg/auth/manager_chain_extended_test.go
🧬 Code graph analysis (4)
pkg/auth/cloud/docker/config_extended_test.go (1)
pkg/auth/cloud/docker/config.go (1)
NewConfigManager(51-65)
internal/exec/terraform_output_utils_test.go (2)
errors/errors.go (1)
ErrNotImplemented(37-37)internal/exec/terraform_output_utils.go (1)
GetStaticRemoteStateOutput(725-762)
pkg/auth/cloud/aws/ecr_extended_test.go (4)
errors/errors.go (1)
ErrECRAuthFailed(743-743)pkg/auth/types/aws_credentials.go (1)
AWSCredentials(16-24)pkg/auth/types/whoami.go (1)
WhoamiInfo(6-26)pkg/auth/cloud/aws/ecr.go (1)
ECRAuthResult(22-27)
pkg/auth/manager_chain_extended_test.go (6)
pkg/auth/types/aws_credentials.go (1)
AWSCredentials(16-24)pkg/auth/credentials/store.go (1)
NewCredentialStore(42-46)pkg/auth/manager.go (1)
NewAuthManager(102-145)pkg/schema/schema_auth.go (2)
AuthConfig(4-13)IdentityVia(63-66)pkg/schema/schema.go (1)
Validate(241-243)pkg/auth/types/interfaces.go (1)
ValidationInfo(347-362)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Summary
🔇 Additional comments (17)
pkg/http/client.go (2)
11-11: LGTM – os import added for environment fallback.Clean addition; required for the os.Getenv calls in the updated GetGitHubTokenFromEnv function.
134-142: Documentation clearly explains the precedence model.The updated comments accurately describe the token resolution order and justify why both viper and direct environment access are needed. This aligns with the established pattern for this function.
internal/exec/terraform_output_utils_test.go (1)
14-72: LGTM!These tests properly validate behavior: nil handling, wrapper initialization, stack info retrieval, and error returns. The
AuthenticateProvidertest correctly useserrors.Is()to check the error type.pkg/auth/cloud/aws/ecr_extended_test.go (3)
1-13: Imports look good.Proper organization with stdlib, third-party, and Atmos packages in separate groups, and correct alias usage.
15-26: Good negative test coverage.Validates that non-AWS credentials are properly rejected with the expected error type and message.
76-93: Mock implementation is appropriate.Minimal but sufficient for testing the non-AWS credentials error case.
pkg/auth/manager_integrations_test.go (4)
1-14: LGTM!Imports are correctly organized: stdlib, third-party, then Atmos packages with proper alias for
errUtils.
16-125: Solid table-driven tests for GetIntegration.Good coverage of nil, empty, not-found, and found scenarios. The separate data validation test complements the table-driven approach well.
127-283: Excellent coverage of findIntegrationsForIdentity.Thorough table-driven test covering default auto_provision behavior, explicit true/false, mixed scenarios, and edge cases. The
ElementsMatchusage handles map iteration order correctly.
523-547: Good edge case test for nil AutoProvision defaulting to true.This explicitly validates the default behavior documented in comments elsewhere.
pkg/auth/manager_chain_extended_test.go (3)
1-15: Imports organized correctly.The package declaration and import organization follow the coding guidelines precisely: stdlib, 3rd-party, and atmos packages in separate groups.
230-335: Solid test coverage for authentication chain building.The tests comprehensively cover standalone identities, provider-based chains, and multi-identity chains. The mock implementation is clean and follows best practices for testing credential expiration behavior.
136-137: Comment must end with a period.Per coding guidelines enforced by the
godotlinter, all comments must end with periods.🔎 Proposed fix
- // Note: negative indices cause a panic in the current implementation. - // This is a potential bug that should be addressed separately. + // Note: negative indices cause a panic in the current implementation. + // This is a potential bug that should be addressed separately.As per coding guidelines.
⛔ Skipped due to learnings
Learnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-01T18:25:25.942Z Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; use `errors.Is()` for error checking; remove always-skipped testsLearnt from: CR Repo: cloudposse/atmos PR: 0 File: .cursor/rules/atmos-rules.mdc:0-0 Timestamp: 2025-11-24T17:35:37.209Z Learning: Applies to **/*_test.go : Use table-driven tests for testing multiple scenarios in GoLearnt from: CR Repo: cloudposse/atmos PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-01T18:25:25.942Z Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces and dependency injection for testability; use table-driven tests for comprehensive coverage; target >80% coverage; skip tests gracefully with helpers from `tests/test_preconditions.go`Learnt from: CR Repo: cloudposse/atmos PR: 0 File: .cursor/rules/atmos-rules.mdc:0-0 Timestamp: 2025-11-24T17:35:37.209Z Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixturesLearnt from: CR Repo: cloudposse/atmos PR: 0 File: .cursor/rules/atmos-rules.mdc:0-0 Timestamp: 2025-11-24T17:35:37.209Z Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packagespkg/auth/cloud/docker/config_extended_test.go (4)
13-28: LGTM - Clean environment variable tests.Both tests properly exercise the config directory resolution with and without the
DOCKER_CONFIGenvironment variable, using appropriate testing helpers.
79-131: LGTM - Solid functional tests.These tests verify critical behaviors: removing multiple registries in one call and preserving non-auth fields during writes. Good coverage of real-world scenarios.
170-198: Edge case tests document current behavior.These tests verify that empty registries and credentials are accepted without validation. The comments indicate this is intentional ("no validation at this level"), which is fine—these tests serve as documentation of that design choice.
31-31: No action needed—setupTestDockerConfigDiris properly defined inpkg/auth/cloud/docker/config_test.goand accessible toconfig_extended_test.goas both are test files in the same package.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
pkg/auth/cloud/aws/ecr_extended_test.go (1)
72-89: Consider using generated mock for consistency.While this manual mock is simple and functional, the guidelines specify using
go.uber.org/mock/mockgenfor all mocks. Consider adding a//go:generatedirective to generate the mock from theICredentialsinterface instead.This ensures consistency with the project's testing patterns and automatic updates when the interface evolves.
Example mockgen directive
Add to a suitable test file (or this file):
//go:generate mockgen -destination=mocks/mock_credentials.go -package=mocks github.com/cloudposse/atmos/pkg/auth/types ICredentialsThen use the generated mock in tests.
pkg/auth/manager_chain_extended_test.go (1)
323-345: Consider using generated mocks for consistency.This manual mock works fine for testing expiration logic, but the coding guidelines prefer
go.uber.org/mock/mockgenwith//go:generatedirectives. For a mock this simple, the manual approach is pragmatic—just something to consider for consistency across the codebase.pkg/auth/manager_integrations_test.go (2)
494-506: Duplicate test case.This test duplicates the "nil integrations map" case already covered in the
TestManager_GetIntegrationtable at lines 25-30. Consider removing this standalone test to reduce duplication.
547-583: Consider consolidating empty map tests.These tests verify behavior with empty integrations maps. Line 547-562 is similar to the not-found test (lines 285-300), and lines 564-583 are similar to the no-linked-integrations test (lines 379-405). While empty vs nil maps are technically different, consider whether both need standalone tests or if they could be additional table entries in the existing tests.
📜 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 (6)
internal/exec/terraform_output_utils_test.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/cloud/docker/config_extended_test.gopkg/auth/manager_chain_extended_test.gopkg/auth/manager_integrations_test.gopkg/http/client.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/http/client.go
- pkg/auth/cloud/docker/config_extended_test.go
- internal/exec/terraform_output_utils_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags in CLI commands
Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
All code must pass golangci-lint checks
Follow Go's error handling idioms: use meaningful error messages, wrap errors with context usingfmt.Errorf("context: %w", err), and consider using custom error types for domain-specific errors
Follow standard Go coding style: usegofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables
Document all exported functions, types, and methods following Go's documentation conventions
Document complex logic with inline comments in Go code
Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults
Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging
**/*.go: All comments must end with periods (enforced bygodotlinter)
Never delete existing comments without a very strong reason; preserve helpful comments explaining why/how/what/where, and update comments to match code when refactoring
Organize imports in three groups separated by blank lines, sorted alphabetically: (1) Go stdlib, (2) 3rd-party (NOT cloudposse/atmos), (3) Atmos packages. Maintain aliases:cfg,log,u,errUtils
Useflags.NewStandardParser()for command-specific flags; NEVER callviper.BindEnv()orviper.BindPFlag()directly (enforced by Forbidigo)
All errors MUST be wrapped using static errors defined inerrors/errors.go; useerrors.Joinfor combining multiple errors,fmt.Errorfwith%wfor adding string context, error builder for complex errors, anderrors.Is()for error checking. Never use dynamic errors directly
Define interfaces for all major funct...
Files:
pkg/auth/manager_chain_extended_test.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/manager_integrations_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Use table-driven tests for testing multiple scenarios in Go
Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
**/*_test.go: Usecmd.NewTestKit(t)for cmd tests to auto-clean RootCmd state (flags, args)
Test behavior, not implementation; never test stub functions; avoid tautological tests; useerrors.Is()for error checking; remove always-skipped tests
Prefer unit tests with mocks over integration tests; use interfaces and dependency injection for testability; use table-driven tests for comprehensive coverage; target >80% coverage; skip tests gracefully with helpers fromtests/test_preconditions.go
Never manually edit golden snapshot files undertests/test-cases/ortests/testdata/; always use-regenerate-snapshotsflag. Never use pipe redirection when running tests as it breaks TTY detection
Files:
pkg/auth/manager_chain_extended_test.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/manager_integrations_test.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/**/*.go: Adddefer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions, except trivial getters/setters, command constructors, simple factory functions, and functions that only delegate to another tracked function. Usenilif no atmosConfig param
Avoid adding new functions topkg/utils/; create purpose-built packages for new functionality (e.g.,pkg/store/,pkg/git/,pkg/pro/,pkg/filesystem/)
Files:
pkg/auth/manager_chain_extended_test.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/manager_integrations_test.go
🧠 Learnings (10)
📓 Common learnings
Learnt from: Listener430
Repo: cloudposse/atmos PR: 934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.
Learnt from: aknysh
Repo: cloudposse/atmos PR: 944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.
📚 Learning: 2025-09-25T01:02:48.697Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: pkg/auth/manager.go:304-312
Timestamp: 2025-09-25T01:02:48.697Z
Learning: The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible.
Applied to files:
pkg/auth/manager_chain_extended_test.gopkg/auth/manager_integrations_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages
Applied to files:
pkg/auth/manager_chain_extended_test.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/manager_integrations_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*_test.go : Include integration tests for command flows and test CLI end-to-end when possible with test fixtures
Applied to files:
pkg/auth/manager_chain_extended_test.gopkg/auth/manager_integrations_test.go
📚 Learning: 2025-09-09T02:14:36.708Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1452
File: internal/auth/types/whoami.go:14-15
Timestamp: 2025-09-09T02:14:36.708Z
Learning: The WhoamiInfo struct in internal/auth/types/whoami.go requires the Credentials field to be JSON-serializable for keystore unmarshaling operations, despite security concerns about credential exposure.
Applied to files:
pkg/auth/manager_chain_extended_test.go
📚 Learning: 2025-12-21T04:10:29.030Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1891
File: internal/exec/describe_affected.go:468-468
Timestamp: 2025-12-21T04:10:29.030Z
Learning: In Go, package-level declarations (constants, variables, types, and functions) are visible to all files in the same package without imports. During reviews in cloudposse/atmos (and similar Go codebases), before suggesting to declare a new identifier, first check if it already exists in another file of the same package. If it exists, you can avoid adding a new declaration; if not, proceed with a proper package-level declaration.
Applied to files:
pkg/auth/manager_chain_extended_test.gopkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/manager_integrations_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Prefer unit tests with mocks over integration tests; use interfaces and dependency injection for testability; use table-driven tests for comprehensive coverage; target >80% coverage; skip tests gracefully with helpers from `tests/test_preconditions.go`
Applied to files:
pkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/manager_integrations_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*_test.go : Test behavior, not implementation; never test stub functions; avoid tautological tests; use `errors.Is()` for error checking; remove always-skipped tests
Applied to files:
pkg/auth/cloud/aws/ecr_extended_test.gopkg/auth/manager_integrations_test.go
📚 Learning: 2026-01-01T18:25:25.942Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-01T18:25:25.942Z
Learning: Applies to **/*.go : Define interfaces for all major functionality; use dependency injection for testability; generate mocks with `go.uber.org/mock/mockgen` (with `//go:generate` directives), never manual mocks
Applied to files:
pkg/auth/manager_integrations_test.go
📚 Learning: 2025-11-24T17:35:37.209Z
Learnt from: CR
Repo: cloudposse/atmos PR: 0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-11-24T17:35:37.209Z
Learning: Applies to **/*.go : Use interfaces for external dependencies to facilitate mocking and consider using testify/mock for creating mock implementations
Applied to files:
pkg/auth/manager_integrations_test.go
🧬 Code graph analysis (3)
pkg/auth/manager_chain_extended_test.go (5)
pkg/auth/types/aws_credentials.go (1)
AWSCredentials(16-24)pkg/auth/credentials/store.go (1)
NewCredentialStore(42-46)pkg/auth/manager.go (1)
NewAuthManager(102-145)pkg/schema/schema_auth.go (2)
AuthConfig(4-13)IdentityVia(63-66)pkg/auth/types/interfaces.go (1)
ValidationInfo(347-362)
pkg/auth/cloud/aws/ecr_extended_test.go (4)
errors/errors.go (1)
ErrECRAuthFailed(743-743)pkg/auth/types/aws_credentials.go (1)
AWSCredentials(16-24)pkg/auth/types/whoami.go (1)
WhoamiInfo(6-26)pkg/auth/types/interfaces.go (1)
ValidationInfo(347-362)
pkg/auth/manager_integrations_test.go (4)
errors/errors.go (4)
ErrIntegrationNotFound(737-737)ErrIntegrationFailed(739-739)ErrIdentityNotFound(611-611)ErrNoLinkedIntegrations(740-740)pkg/schema/schema_auth.go (4)
IntegrationVia(127-129)IntegrationSpec(132-135)ECRRegistry(138-141)AuthConfig(4-13)pkg/auth/validation/validator.go (1)
NewValidator(19-21)pkg/auth/manager.go (1)
NewAuthManager(102-145)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Acceptance Tests (macos)
- GitHub Check: Acceptance Tests (windows)
- GitHub Check: Acceptance Tests (linux)
- GitHub Check: Summary
🔇 Additional comments (13)
pkg/auth/cloud/aws/ecr_extended_test.go (3)
3-13: Import organization looks solid.Correctly separated into three groups with proper aliases.
15-26: Solid error handling test.Verifies that
buildAWSConfigFromCredsproperly rejects non-AWS credential types usingerrors.Is(). Good use ofrequire.Errorandassert.ErrorIs.
28-70: Well-structured table-driven test.Covers the region selection logic comprehensively: provided region priority, credential fallback, and matching scenarios. Good use of example AWS credentials.
pkg/auth/manager_chain_extended_test.go (7)
3-15: LGTM!Import organization follows the coding guidelines with proper grouping: stdlib, third-party, then Atmos packages.
17-54: LGTM!Solid table-driven test covering the key cases: present token, absent token, and nil credentials. Good edge case coverage.
56-96: LGTM!Good approach testing the internal
determineStartingIndexmethod with varied starting indices. The type assertion to access internals is acceptable for unit testing.
98-156: LGTM!Good work documenting the negative index panic behavior with an explicit test. The comment at lines 136-137 appropriately flags this as a potential bug for future attention.
158-226: LGTM!Comprehensive expiration validation testing. The "expiring soon" case at 30 seconds correctly validates the 5-minute buffer behavior.
228-238: LGTM!Good defensive test for the nil credential store edge case. Direct struct initialization makes sense here since
NewAuthManagervalidates against nil.
240-321: LGTM!These tests cover the essential chain-building scenarios: standalone, provider-backed, and multi-hop identity chains. The assertions on chain ordering confirm correct authentication sequence.
pkg/auth/manager_integrations_test.go (3)
16-125: Excellent test coverage for GetIntegration.These tests properly cover the public API with good edge cases (nil, empty, not found, found) and validate the returned data structure. The table-driven approach and error assertions follow guidelines.
285-360: Good coverage of ExecuteIntegration error paths.These tests properly verify error handling for integration not found, missing identity configuration, and empty identity. The use of the constructor and proper error assertions follow guidelines.
362-405: Solid tests for ExecuteIdentityIntegrations.These correctly test identity not found and no linked integrations scenarios through the public API with proper error checking.
|
These changes were released in v1.203.0. |
…workflow-compatible-syntax Resolved conflicts in website/src/data/roadmap.js by: - Keeping version: 'v1.197.0' from main for auth identities milestone - Keeping shipped status, q4-2025 quarter, and changelog from feature branch for unified task execution milestone Merged changes from main include: - ECR authentication integration (PR #1859) - Auth console and manager improvements - Docker config support - Various test improvements and fixes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
what
Implements ECR authentication integration for Atmos, allowing automatic Docker login to AWS ECR registries using Atmos-managed identities.
New
auth.integrationsSectionIntegrations specify which identity they use via
via.identity, and can auto-trigger on identity login viaspec.auto_provision:Design: One Registry Per Integration
Each integration defines a single registry rather than a list. This approach:
dev/ecr/main)New
atmos auth ecr-loginCommandAuto-Provision on Identity Login
When you login with an identity, all integrations with
auto_provision: truethat reference that identity are triggered:Implementation
pkg/schema/schema_auth.goIntegration,IntegrationVia,IntegrationSpec,ECRRegistrystructspkg/auth/integrations/pkg/auth/integrations/aws/ecr.goaws/ecrkind implementationpkg/auth/cloud/aws/ecr.gopkg/auth/cloud/docker/config.gopkg/auth/manager.gofindIntegrationsForIdentity(),triggerIntegrations(),ExecuteIntegration()cmd/auth_ecr_login.goerrors/errors.gowhy
aws ecr get-login-password | docker loginincantationsatmos.yamlalongside identitiesauto_provision: truetrigger on identity login~/.config/atmos/docker/config.json(XDG)atmos auth logindon't block authenticationDesign Decisions
Why
integrationsnotidentities?ECR login and EKS kubeconfig are client-only credential materializations, not identities:
Integrations use an identity to derive credentials for specific services.
Why integrations reference identities (not vice versa)?
Integrations specify their required identity via
via.identityinstead of identities listing their integrations. This:via.provideron identitiesreferences
docs/prd/ecr-authentication.mdwebsite/blog/2025-12-15-ecr-authentication-integration.mdxaws/ekskind) follows same patternSummary by CodeRabbit
New Features
Integrations
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.