Repository navigation
fix: Profile and identity flags loading and propagation - #1805
Conversation
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
|
CodeRabbit (@coderabbitai) full review please |
|
Andriy Knysh (@aknysh): I'll conduct a full review of the PR. 🧠 Learnings used✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR implements CLI flag synchronization for profiles and identities to Viper before config loading, adds profile parsing for commands with disabled flag parsing, enables identity inheritance from parent auth managers in nested components, and merges global authentication configuration into component-level auth sections with extensive test coverage for isolation and edge cases. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant Root as cmd/root.go
participant Viper as Viper Config
participant Load as pkg/config/load.go
participant Schema as ConfigSchema
User ->> Root: Execute command with --profile/--identity flags
Root ->> Root: Parse flags via Cobra
Root ->> Root: syncGlobalFlagsToViper()
activate Root
Root ->> Viper: Set "profile" & "identity" into Viper
deactivate Root
Root ->> Load: LoadConfig()
activate Load
Load ->> Load: getProfilesFromFlagsOrEnv()
Load ->> Viper: Read profile from Viper (flag or env)
Viper -->> Load: Return profiles with source
alt Profiles found
Load ->> Load: Load and apply profile configs
end
Load ->> Schema: Unmarshal final config
deactivate Load
Load -->> Root: Return ConfigAndStacksInfo
sequenceDiagram
participant Parent as Parent Component
participant Helper as terraform_nested_auth_helper.go
participant AuthMgr as AuthManager (Parent)
participant ChildAuthMgr as AuthManager (Child)
participant Child as Child Component
Parent ->> Helper: createComponentAuthManager(parentAuthManager, ...)
activate Helper
Helper ->> AuthMgr: Get identity chain
AuthMgr -->> Helper: Return chain[last] as identityName
Helper ->> Helper: Inherit identityName from parent
Helper ->> ChildAuthMgr: Create with inherited identityName
deactivate Helper
ChildAuthMgr -->> Child: Provide inherited identity for child
sequenceDiagram
participant Exec as ProcessComponentConfig()
participant Utils as utils_auth.go
participant Merge as mergeGlobalAuthConfig()
participant Component as Component Auth Section
Exec ->> Utils: Check if component auth is empty
alt Auth section empty & atmosConfig provided
Utils ->> Merge: Deep merge global auth into component
activate Merge
Merge ->> Merge: buildGlobalAuthSection(atmosConfig)
Merge ->> Merge: getComponentAuthSection(component)
Merge ->> Merge: Deep merge: global as base, component overrides
deactivate Merge
Merge -->> Component: Updated auth section with merged config
else Auth section present
Utils -->> Component: Use existing component auth
end
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1805 +/- ##
==========================================
+ Coverage 71.45% 71.50% +0.05%
==========================================
Files 461 462 +1
Lines 43631 43752 +121
==========================================
+ Hits 31175 31286 +111
- Misses 9911 9915 +4
- Partials 2545 2551 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Added comprehensive unit tests to increase code coverage for the profile and identity flag fixes: - pkg/config/load_flags_test.go: Tests getProfilesFromFlagsOrEnv() function covering environment variables, CLI flags (both syntaxes), and edge cases - internal/exec/terraform_nested_auth_helper_test.go: Added TestIdentityInheritanceLogic to verify identity extraction from AuthManager chain for nested component operations These tests cover the new code paths added for fixing: 1. Profile loading from --profile CLI flag 2. Identity propagation to nested components via parent AuthManager chain 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
Bot
added
size/xl
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
CodeRabbit (@coderabbitai) full review please |
|
Andriy Knysh (@aknysh): I'll conduct a full review of the PR. 🧠 Learnings used✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
pkg/config/load.go (2)
37-60: Profile parsing fallback looks solid; consider respecting--passthrough semantics.The manual
parseProfilesFromArgs(os.Args)approach is a reasonable workaround for the Viper timing issue and the implementation covers the expected--profile value/--profile=value/ comma-separated forms.One subtle edge case: because it scans the full argument list without stopping at
--, any--profilethat is intended to be passed through to a downstream tool after--would still be interpreted as an Atmos profile. If you expect passthrough usage with--profilefor nested tools, it may be worth short‑circuiting the scan once a bare--is encountered so behavior matches Cobra’s flag parsing more closely.Also applies to: 62-84
143-152: LoadConfig integration is good; log message could better distinguish env vs flag.The backfill into
configAndStacksInfo.ProfilesFromArgviagetProfilesFromFlagsOrEnvnicely centralizes profile resolution and keeps precedence (ProfilesFromArgwins) clear.Minor nit: when
source == "env", the log message"Profiles loaded from CLI "+sourcewill read as"Profiles loaded from CLI env", which is a bit confusing. If you care about log clarity, consider either adjusting the prefix or loggingsourceas a separate key/value field instead of concatenating it into the message.internal/exec/terraform_nested_auth_helper_test.go (1)
292-339: Identity-chain test is clear; consider extracting a shared helper to avoid duplication.The table‑driven cases nicely pin down the “last element or empty string” behavior for the identity chain and match the production logic.
Right now the test re‑implements that logic inline; if this pattern grows, it might be worth extracting a small helper (e.g.
identityFromChain([]string) string) and testing that directly, so you don’t have to keep test and implementation in lockstep manually.pkg/config/load_flags_test.go (1)
1-89: Flag/env resolution tests match the intended precedence and fallback.The subtests clearly exercise the env‑first behavior and the os.Args fallback for both
--profile valueand--profile=value, plus the “no profiles” and empty‑slice cases. Given eachsetupViperoverwrites the"profile"key andos.Argsare restored viadefer, the tests look robust for the current usage.If you later expand this to touch more viper keys, you might consider resetting or scoping viper state per test, but for this focused helper it’s not strictly necessary.
internal/exec/terraform_nested_auth_helper.go (1)
153-169: Identity inheritance from parent AuthManager looks correct; consider centralizing the chain-to-identity helper.Using the last element of
parentAuthManager.GetChain()as the inheritedidentityNameis a sensible way to propagate an explicitly selected identity into nested component auth, while still falling back to auto‑detection when the chain is empty or the parent manager is nil.Since the same “last element or empty string” logic is now mirrored in tests, you might consider extracting a tiny helper (either here or in the auth package) to turn a chain into an identity name and have both production code and tests call it. That would reduce duplication and make future changes to the inheritance rule safer.
Also, good call on passing
cfg.IdentityFlagSelectValuethrough toCreateAndAuthenticateManagerso selection behavior remains centralized.Also applies to: 173-176
docs/fixes/profile-and-identity-flags-loading.md (1)
13-13: Fix markdown formatting issues: hard tabs and missing language specifications.Static analysis identified several markdown linting issues:
Hard tabs in code blocks (MD010): Lines 284–303, 326–344, 544–548, 563–563, 631–633, 661–671, 689–691 use tabs instead of spaces. Replace with spaces for consistency.
Missing language specifications (MD040): Code fences at lines 13, 221, 240, 250, 608, 614 should specify language (e.g.,
```yaml,```go). This enables proper syntax highlighting.List indentation (MD007): Lines 771–772 have incorrect indentation (2 spaces instead of 0); adjust to align with the list format.
These are minor but worth fixing for documentation consistency and readability.
Also applies to: 221-221, 240-240, 250-250, 284-303, 326-344, 544-548, 563-563, 631-633, 661-671, 689-691, 771-772
📜 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)
docs/fixes/profile-and-identity-flags-loading.md(1 hunks)internal/exec/terraform_nested_auth_helper.go(1 hunks)internal/exec/terraform_nested_auth_helper_test.go(1 hunks)pkg/config/load.go(2 hunks)pkg/config/load_flags_test.go(1 hunks)pkg/config/load_profile_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (26)
📓 Common learnings
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.
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.
📚 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:
pkg/config/load_flags_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:
pkg/config/load_flags_test.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/config/load_flags_test.gopkg/config/load.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/config/load_flags_test.gopkg/config/load.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
pkg/config/load_flags_test.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/config/load_flags_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:
internal/exec/terraform_nested_auth_helper.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:
docs/fixes/profile-and-identity-flags-loading.md
📚 Learning: 2025-06-23T02:14:30.937Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1327
File: cmd/terraform.go:111-117
Timestamp: 2025-06-23T02:14:30.937Z
Learning: In cmd/terraform.go, flags for the DescribeAffected function are added dynamically at runtime when info.Affected is true. This is intentional to avoid exposing internal flags like "file", "format", "verbose", "include-spacelift-admin-stacks", "include-settings", and "upload" in the terraform command interface, while still providing them for the shared DescribeAffected function used by both `atmos describe affected` and `atmos terraform apply --affected`.
Applied to files:
docs/fixes/profile-and-identity-flags-loading.md
📚 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:
docs/fixes/profile-and-identity-flags-loading.mdpkg/config/load.go
📚 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:
docs/fixes/profile-and-identity-flags-loading.md
📚 Learning: 2025-10-03T18:02:08.535Z
Learnt from: Benbentwo
Repo: cloudposse/atmos PR: 1475
File: internal/exec/terraform.go:269-272
Timestamp: 2025-10-03T18:02:08.535Z
Learning: In internal/exec/terraform.go, when auth.TerraformPreHook fails, the error is logged but execution continues. This is a deliberate design choice to allow Terraform commands to proceed even if authentication setup fails, rather than failing fast.
Applied to files:
docs/fixes/profile-and-identity-flags-loading.md
📚 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:
docs/fixes/profile-and-identity-flags-loading.md
📚 Learning: 2024-11-24T19:13:10.287Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2024-11-24T19:13:10.287Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.
Applied to files:
docs/fixes/profile-and-identity-flags-loading.md
📚 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:
docs/fixes/profile-and-identity-flags-loading.md
📚 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:
docs/fixes/profile-and-identity-flags-loading.md
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2024-10-27T04:41:49.199Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
Applied to files:
docs/fixes/profile-and-identity-flags-loading.md
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 775
File: internal/exec/template_funcs_component.go:157-159
Timestamp: 2024-11-12T03:16:02.910Z
Learning: In the Go code for `componentFunc` in `internal/exec/template_funcs_component.go`, the function `cleanTerraformWorkspace` does not return errors, and it's acceptable if the file does not exist. Therefore, error handling for `cleanTerraformWorkspace` is not needed.
Applied to files:
docs/fixes/profile-and-identity-flags-loading.md
📚 Learning: 2024-10-27T04:28:40.966Z
Learnt from: haitham911
Repo: cloudposse/atmos PR: 727
File: internal/exec/terraform_clean.go:155-175
Timestamp: 2024-10-27T04:28:40.966Z
Learning: In the `CollectDirectoryObjects` function in `internal/exec/terraform_clean.go`, recursive search through all subdirectories is not needed.
Applied to files:
docs/fixes/profile-and-identity-flags-loading.md
📚 Learning: 2024-12-17T07:08:41.288Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 863
File: internal/exec/yaml_func_terraform_output.go:34-38
Timestamp: 2024-12-17T07:08:41.288Z
Learning: In the `processTagTerraformOutput` function within `internal/exec/yaml_func_terraform_output.go`, parameters are separated by spaces and do not contain spaces. Therefore, using `strings.Fields()` for parsing is acceptable, and there's no need to handle parameters with spaces.
Applied to files:
docs/fixes/profile-and-identity-flags-loading.md
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
pkg/config/load.go
📚 Learning: 2025-05-22T15:42:10.906Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1261
File: internal/exec/utils.go:639-640
Timestamp: 2025-05-22T15:42:10.906Z
Learning: In the Atmos codebase, when appending slices with `args := append(configAndStacksInfo.CliArgs, configAndStacksInfo.AdditionalArgsAndFlags...)`, it's intentional that the result is not stored back in the original slice. This pattern is used when the merged result serves a different purpose than the original slices, such as when creating a filtered version for component section assignments.
Applied to files:
pkg/config/load.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/config/load.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:
pkg/config/load.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
pkg/config/load.go
🧬 Code graph analysis (2)
internal/exec/terraform_nested_auth_helper.go (1)
pkg/auth/manager_helpers.go (1)
CreateAndAuthenticateManager(180-222)
pkg/config/load.go (1)
pkg/logger/log.go (1)
Debug(24-26)
🪛 LanguageTool
docs/fixes/profile-and-identity-flags-loading.md
[typographical] ~55-~55: Consider using a typographic opening quote here.
Context: ...ers": null- Authentication fails with "No valid credential sources found" -AT...
(EN_QUOTES)
[typographical] ~55-~55: Consider using a typographic close quote here.
Context: ... with "No valid credential sources found" - `ATMOS_PROFILE=managers atmos describ...
(EN_QUOTES)
[grammar] ~353-~353: Please add a punctuation mark at the end of paragraph.
Context: ... flags 5. Log which method was used for debugging Why This Works: - **Environment v...
(PUNCTUATION_PARAGRAPH_END)
[grammar] ~359-~359: Please add a punctuation mark at the end of paragraph.
Context: ...t pass ProfilesFromArg explicitly still works ### Testing Strategy #### Integration...
(PUNCTUATION_PARAGRAPH_END)
[typographical] ~491-~491: Consider using typographic quotation marks here.
Context: ...er()) ``` - Creates binding between "profile" key and --profile flag - Binding exists...
(EN_QUOTES)
[grammar] ~523-~523: Please add a punctuation mark at the end of paragraph.
Context: ...o InitCliConfig, changing many function signatures #### Option 4: Parse os.Args Manually ...
(PUNCTUATION_PARAGRAPH_END)
[grammar] ~576-~576: Please add a punctuation mark at the end of paragraph.
Context: ...e function - ✅ Easy to replace later if needed ## Related Issue: Identity Flag Not Pr...
(PUNCTUATION_PARAGRAPH_END)
[typographical] ~601-~601: Consider using a typographic opening quote here.
Context: ...sted component (vpc) - Error message: "Multiple default identities found. Pleas...
(EN_QUOTES)
🪛 markdownlint-cli2 (0.18.1)
docs/fixes/profile-and-identity-flags-loading.md
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
221-221: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
240-240: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
250-250: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
284-284: Hard tabs
Column: 1
(MD010, no-hard-tabs)
285-285: Hard tabs
Column: 1
(MD010, no-hard-tabs)
286-286: Hard tabs
Column: 1
(MD010, no-hard-tabs)
287-287: Hard tabs
Column: 1
(MD010, no-hard-tabs)
288-288: Hard tabs
Column: 1
(MD010, no-hard-tabs)
289-289: Hard tabs
Column: 1
(MD010, no-hard-tabs)
290-290: Hard tabs
Column: 1
(MD010, no-hard-tabs)
291-291: Hard tabs
Column: 1
(MD010, no-hard-tabs)
292-292: Hard tabs
Column: 1
(MD010, no-hard-tabs)
293-293: Hard tabs
Column: 1
(MD010, no-hard-tabs)
294-294: Hard tabs
Column: 1
(MD010, no-hard-tabs)
295-295: Hard tabs
Column: 1
(MD010, no-hard-tabs)
296-296: Hard tabs
Column: 1
(MD010, no-hard-tabs)
297-297: Hard tabs
Column: 1
(MD010, no-hard-tabs)
298-298: Hard tabs
Column: 1
(MD010, no-hard-tabs)
299-299: Hard tabs
Column: 1
(MD010, no-hard-tabs)
300-300: Hard tabs
Column: 1
(MD010, no-hard-tabs)
301-301: Hard tabs
Column: 1
(MD010, no-hard-tabs)
302-302: Hard tabs
Column: 1
(MD010, no-hard-tabs)
326-326: Hard tabs
Column: 1
(MD010, no-hard-tabs)
328-328: Hard tabs
Column: 1
(MD010, no-hard-tabs)
329-329: Hard tabs
Column: 1
(MD010, no-hard-tabs)
330-330: Hard tabs
Column: 1
(MD010, no-hard-tabs)
331-331: Hard tabs
Column: 1
(MD010, no-hard-tabs)
332-332: Hard tabs
Column: 1
(MD010, no-hard-tabs)
333-333: Hard tabs
Column: 1
(MD010, no-hard-tabs)
334-334: Hard tabs
Column: 1
(MD010, no-hard-tabs)
335-335: Hard tabs
Column: 1
(MD010, no-hard-tabs)
336-336: Hard tabs
Column: 1
(MD010, no-hard-tabs)
337-337: Hard tabs
Column: 1
(MD010, no-hard-tabs)
338-338: Hard tabs
Column: 1
(MD010, no-hard-tabs)
339-339: Hard tabs
Column: 1
(MD010, no-hard-tabs)
340-340: Hard tabs
Column: 1
(MD010, no-hard-tabs)
341-341: Hard tabs
Column: 1
(MD010, no-hard-tabs)
342-342: Hard tabs
Column: 1
(MD010, no-hard-tabs)
343-343: Hard tabs
Column: 1
(MD010, no-hard-tabs)
544-544: Hard tabs
Column: 4
(MD010, no-hard-tabs)
545-545: Hard tabs
Column: 4
(MD010, no-hard-tabs)
546-546: Hard tabs
Column: 4
(MD010, no-hard-tabs)
547-547: Hard tabs
Column: 4
(MD010, no-hard-tabs)
548-548: Hard tabs
Column: 4
(MD010, no-hard-tabs)
563-563: Hard tabs
Column: 4
(MD010, no-hard-tabs)
608-608: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
614-614: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
631-631: Hard tabs
Column: 1
(MD010, no-hard-tabs)
632-632: Hard tabs
Column: 1
(MD010, no-hard-tabs)
633-633: Hard tabs
Column: 1
(MD010, no-hard-tabs)
661-661: Hard tabs
Column: 1
(MD010, no-hard-tabs)
662-662: Hard tabs
Column: 1
(MD010, no-hard-tabs)
663-663: Hard tabs
Column: 1
(MD010, no-hard-tabs)
664-664: Hard tabs
Column: 1
(MD010, no-hard-tabs)
665-665: Hard tabs
Column: 1
(MD010, no-hard-tabs)
666-666: Hard tabs
Column: 1
(MD010, no-hard-tabs)
667-667: Hard tabs
Column: 1
(MD010, no-hard-tabs)
668-668: Hard tabs
Column: 1
(MD010, no-hard-tabs)
669-669: Hard tabs
Column: 1
(MD010, no-hard-tabs)
670-670: Hard tabs
Column: 1
(MD010, no-hard-tabs)
671-671: Hard tabs
Column: 1
(MD010, no-hard-tabs)
689-689: Hard tabs
Column: 1
(MD010, no-hard-tabs)
690-690: Hard tabs
Column: 1
(MD010, no-hard-tabs)
691-691: Hard tabs
Column: 1
(MD010, no-hard-tabs)
771-771: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
772-772: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
⏰ 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)
pkg/config/load_profile_test.go (1)
1-68: Profile-arg parsing tests give good coverage of real-world inputs.The table‑driven tests exercise the key
--profilesyntaxes and edge cases (comma lists, spaces, missing values, multiple flags) and align well with the intended behavior ofparseProfilesFromArgs. No issues spotted here.docs/fixes/profile-and-identity-flags-loading.md (1)
1-790: Documentation is comprehensive and technically sound.The documentation clearly explains both issues (profile flag loading and identity propagation), provides thorough root cause analysis with concrete execution flows, describes the workaround rationale, and includes before/after verification output. The narrative is well-structured and accessible for maintainers and future contributors. No technical inaccuracies identified. Learnings from prior PRs (identity flag persistence, Cobra subcommand inheritance) are relevant context.
Added tests to increase coverage for profile and identity flag handling: - TestGetProfilesFromFlagsOrEnv: Added test for environment variable precedence - TestResolveAuthManagerForNestedComponent_WithoutAuthSection: Tests early return paths - TestHasDefaultIdentity_EdgeCases: Tests additional edge cases including: - Multiple defaults - Missing default field - Mixed valid/invalid configs These tests target the uncovered code paths in: - pkg/config/load.go: getProfilesFromFlagsOrEnv() and edge cases - internal/exec/terraform_nested_auth_helper.go: hasDefaultIdentity() and early returns 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
9f054ec
|
Andriy Knysh (@aknysh): I'll conduct a full review of the PR. 🧠 Learnings used✅ Actions performedFull review triggered. |
Fixed the precedence order in getProfilesFromFlagsOrEnv() to ensure CLI flags take precedence over environment variables, which is the expected behavior for CLI applications. Before: Environment variables took precedence over CLI flags After: CLI flags override environment variables (correct behavior) Precedence order (highest to lowest): 1. CLI flags (--profile) 2. Environment variables (ATMOS_PROFILE) 3. Config file 4. Defaults Updated test case to verify correct precedence behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
NOTICE (1)
68-71: Re-confirm NOTICE is generator output after all dep bumpsAll these NOTICE deltas (new/updated AWS/GCP/OpenTelemetry/etc. license URLs and the new
service/signinentry) should be the exact output of./scripts/generate-notice.shfor the finalgo.modstate, not hand-tuned text. Given earlier bot feedback on this same PR/file, I’d just re-run the generator one more time after your last dependency changes, replace NOTICE with that output, and push — if this commit already reflects that, feel free to mark this thread resolved.Also applies to: 100-103, 108-111, 112-115, 116-119, 120-123, 124-127, 128-131, 136-139, 144-147, 148-151, 152-155, 156-159, 164-167, 168-171, 172-175, 176-179, 180-183, 192-195, 272-275, 416-419, 444-447, 448-451, 500-503, 508-511, 561-564, 613-616
🧹 Nitpick comments (1)
internal/exec/utils_auth_merge_test.go (1)
12-331: Consider adding nil safety test cases.The table-driven tests provide good coverage of auth config variations. However, consider adding test cases for:
atmosConfigbeing nilcomponentSectionbeing nil- Both nil
These edge cases would verify defensive programming in
mergeGlobalAuthConfig.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
NOTICE(10 hunks)internal/exec/utils.go(4 hunks)internal/exec/utils_auth_merge_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/utils.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
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.
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.
📚 Learning: 2025-09-29T02:20:11.636Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/validate_component.go:117-118
Timestamp: 2025-09-29T02:20:11.636Z
Learning: The ValidateComponent function in internal/exec/validate_component.go had its componentSection parameter type refined from `any` to `map[string]any` without adding new parameters. This is a type safety improvement, not a signature change requiring call site updates.
Applied to files:
internal/exec/utils_auth_merge_test.go
📚 Learning: 2025-11-01T20:24:29.557Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1714
File: NOTICE:0-0
Timestamp: 2025-11-01T20:24:29.557Z
Learning: In the cloudposse/atmos repository, the NOTICE file is programmatically generated and should not be manually edited. Issues with dependency license URLs in NOTICE will be resolved when upstream package metadata is corrected.
Applied to files:
NOTICE
📚 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:
NOTICE
📚 Learning: 2025-02-21T20:56:05.539Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Applied to files:
NOTICE
🧬 Code graph analysis (1)
internal/exec/utils_auth_merge_test.go (3)
pkg/schema/schema.go (3)
AtmosConfiguration(53-94)Logs(451-454)ConfigAndStacksInfo(620-714)pkg/schema/schema_auth.go (3)
AuthConfig(4-12)IdentityVia(60-63)KeyringConfig(15-18)pkg/config/const.go (1)
AuthSectionName(84-84)
⏰ 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)
internal/exec/utils_auth_merge_test.go (3)
1-11: Imports look good.Clean setup with appropriate test dependencies and internal package references.
427-459: Test logic is sound for verifying auth section preservation.This test correctly validates that
postProcessTemplatesAndYamlFunctionscopies auth configuration fromComponentSectiontoComponentAuthSection. The setup and assertions are appropriate.
378-392: Type assertions could panic before assertions run.Lines 378, 384, 390 perform type assertions and then check
ok, but if the assertion fails the panic happens beforeassert.True. Use two-value form:- providers, ok := configAndStacksInfo.ComponentAuthSection["providers"].(map[string]schema.Provider) - assert.True(t, ok, "providers should be map[string]schema.Provider") + providers, ok := configAndStacksInfo.ComponentAuthSection["providers"].(map[string]schema.Provider) + if !assert.True(t, ok, "providers should be map[string]schema.Provider") { + return + }Same pattern for identities (line 384) and logs (line 390).
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/utils.go (1)
31-61: Global auth merge helper correctly handles logs/keyring-only configsThe helper now builds the auth section from providers, identities, logs, and keyring first and only skips the merge when the entire section is empty, so logs-only and keyring-only global auth configs flow into components while still avoiding a no-op when global auth is truly empty. The conditional call in
ProcessComponentConfig(len(componentAuthSection) == 0 && atmosConfig != nil) also safely avoids overwriting existing per-component auth. This is a solid fix that matches the intended precedence semantics.If you ever reuse
mergeGlobalAuthConfigoutsideProcessComponentConfig, a defensiveif atmosConfig == nil { return map[string]any{} }at the top could guard against accidental nil callers, but it's not necessary given the current, single call site. Based on learnings.Also applies to: 153-161
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
NOTICE(10 hunks)internal/exec/utils.go(4 hunks)internal/exec/utils_auth_merge_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
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.
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.
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.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
📚 Learning: 2025-11-01T20:24:29.557Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1714
File: NOTICE:0-0
Timestamp: 2025-11-01T20:24:29.557Z
Learning: In the cloudposse/atmos repository, the NOTICE file is programmatically generated and should not be manually edited. Issues with dependency license URLs in NOTICE will be resolved when upstream package metadata is corrected.
Applied to files:
NOTICE
📚 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:
NOTICE
📚 Learning: 2025-02-21T20:56:05.539Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Applied to files:
NOTICE
📚 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:
internal/exec/utils.go
📚 Learning: 2024-12-07T16:16:13.038Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
Applied to files:
internal/exec/utils.go
📚 Learning: 2025-09-29T02:20:11.636Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/validate_component.go:117-118
Timestamp: 2025-09-29T02:20:11.636Z
Learning: The ValidateComponent function in internal/exec/validate_component.go had its componentSection parameter type refined from `any` to `map[string]any` without adding new parameters. This is a type safety improvement, not a signature change requiring call site updates.
Applied to files:
internal/exec/utils.gointernal/exec/utils_auth_merge_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/utils.go
📚 Learning: 2024-11-13T21:37:07.852Z
Learnt from: Cerebrovinny
Repo: cloudposse/atmos PR: 764
File: internal/exec/describe_stacks.go:289-295
Timestamp: 2024-11-13T21:37:07.852Z
Learning: In the `internal/exec/describe_stacks.go` file of the `atmos` project written in Go, avoid extracting the stack name handling logic into a helper function within the `ExecuteDescribeStacks` method, even if the logic appears duplicated.
Applied to files:
internal/exec/utils.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:
internal/exec/utils.go
📚 Learning: 2025-05-22T15:42:10.906Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1261
File: internal/exec/utils.go:639-640
Timestamp: 2025-05-22T15:42:10.906Z
Learning: In the Atmos codebase, when appending slices with `args := append(configAndStacksInfo.CliArgs, configAndStacksInfo.AdditionalArgsAndFlags...)`, it's intentional that the result is not stored back in the original slice. This pattern is used when the merged result serves a different purpose than the original slices, such as when creating a filtered version for component section assignments.
Applied to files:
internal/exec/utils.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:
internal/exec/utils.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:
internal/exec/utils.go
📚 Learning: 2025-04-04T02:03:23.676Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.
Applied to files:
internal/exec/utils.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:
internal/exec/utils.go
📚 Learning: 2024-11-19T23:00:45.899Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 795
File: internal/exec/stack_processor_utils.go:378-386
Timestamp: 2024-11-19T23:00:45.899Z
Learning: In the `ProcessYAMLConfigFile` function within `internal/exec/stack_processor_utils.go`, directory traversal in stack imports is acceptable and should not be restricted.
Applied to files:
internal/exec/utils.go
📚 Learning: 2025-08-16T23:33:07.477Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:651-652
Timestamp: 2025-08-16T23:33:07.477Z
Learning: In the cloudposse/atmos Go codebase, ExecuteDescribeDependents expects a pointer to AtmosConfiguration (*schema.AtmosConfiguration), so when calling it with a value returned by cfg.InitCliConfig (which returns schema.AtmosConfiguration), the address-of operator (&) is necessary: ExecuteDescribeDependents(&atmosConfig, ...).
Applied to files:
internal/exec/utils.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
internal/exec/utils.go
📚 Learning: 2025-08-16T23:32:40.412Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1405
File: internal/exec/describe_dependents_test.go:455-456
Timestamp: 2025-08-16T23:32:40.412Z
Learning: In the cloudposse/atmos Go codebase, `InitCliConfig` returns a `schema.AtmosConfiguration` value (not a pointer), while `ExecuteDescribeDependents` expects a `*schema.AtmosConfiguration` pointer parameter. Therefore, when passing the result of `InitCliConfig` to `ExecuteDescribeDependents`, use `&atmosConfig` to pass the address of the value.
Applied to files:
internal/exec/utils.go
🧬 Code graph analysis (2)
internal/exec/utils.go (2)
pkg/schema/schema.go (2)
AtmosConfiguration(53-94)Logs(451-454)pkg/config/const.go (1)
AuthSectionName(84-84)
internal/exec/utils_auth_merge_test.go (3)
pkg/schema/schema.go (3)
AtmosConfiguration(53-94)Logs(451-454)ConfigAndStacksInfo(620-714)pkg/schema/schema_auth.go (3)
AuthConfig(4-12)IdentityVia(60-63)KeyringConfig(15-18)pkg/config/const.go (1)
AuthSectionName(84-84)
⏰ 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)
internal/exec/utils.go (1)
391-401: PassingatmosConfigintoProcessComponentConfigkeeps global auth/profile context consistentThreading
atmosConfigthrough bothProcessStackscall sites intoProcessComponentConfigensures the global auth block from CLI config is always available when resolving component auth (including profile-driven auth without explicitauth.yamlimports), without changing the rest of the processing flow. Wiring looks consistent and localized.Also applies to: 426-436
internal/exec/utils_auth_merge_test.go (1)
12-459: Auth-merge tests give strong coverage across edge cases and post-processingThe table-driven cases and the additional post-processing tests collectively exercise empty, partial, and full auth configurations (including logs/keyring-only), as well as the “do not overwrite component auth” guard and
postProcessTemplatesAndYamlFunctionsbehavior. This should catch regressions in both the merge helper and its integration points. Nicely done.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
NOTICE (1)
68-71: Run the NOTICE generator before merge.Pipeline confirms the NOTICE file is still out of sync. Run
./scripts/generate-notice.shafter finalizing dependency changes and commit the result.Also applies to: 102-102, 110-110, 114-114, 118-118, 122-122, 126-126, 130-130, 138-138, 146-146, 150-150, 154-154, 158-158, 164-167, 170-170, 174-174, 178-178, 182-182, 194-194, 254-254, 274-274, 418-418, 446-446, 450-450, 502-502, 510-510, 563-563, 615-615
🧹 Nitpick comments (4)
internal/exec/utils_auth.go (1)
42-46: Consider only adding non-empty log fields to the map.Lines 44-45 always set both
levelandfilekeys, even when they're empty strings. If only one field is set (e.g., just level), the other will be an empty string in the merged config.if atmosConfig.Auth.Logs.Level != "" || atmosConfig.Auth.Logs.File != "" { - globalAuthSection["logs"] = map[string]any{ - "level": atmosConfig.Auth.Logs.Level, - "file": atmosConfig.Auth.Logs.File, - } + logs := map[string]any{} + if atmosConfig.Auth.Logs.Level != "" { + logs["level"] = atmosConfig.Auth.Logs.Level + } + if atmosConfig.Auth.Logs.File != "" { + logs["file"] = atmosConfig.Auth.Logs.File + } + if len(logs) > 0 { + globalAuthSection["logs"] = logs + } }internal/exec/utils_auth_merge_deepmerge_test.go (3)
86-88: Add defensive assertion before accessing nested map.Line 87 casts
providers["shared"]tomap[string]interface{}without checking if the cast succeeded. If the merge produces a different structure, this will panic.// Verify component override wins - sharedMap := providers["shared"].(map[string]interface{}) + sharedMap, ok := providers["shared"].(map[string]interface{}) + assert.True(t, ok, "shared should be a map") assert.Equal(t, "eu-west-1", sharedMap["region"], "Component should override region")
111-113: Add defensive assertion before accessing nested map.Line 112 casts without checking if the cast succeeded.
// Verify component override wins - sharedMap := identities["shared"].(map[string]interface{}) + sharedMap, ok := identities["shared"].(map[string]interface{}) + assert.True(t, ok, "shared should be a map") assert.Equal(t, true, sharedMap["default"], "Component should override default")
230-235: Add defensive assertions for type casts.Lines 230 and 234 perform type assertions without checking if they succeeded.
- logs := result["logs"].(map[string]any) + logs, ok := result["logs"].(map[string]any) + assert.True(t, ok, "logs should be map[string]any") assert.Equal(t, "Info", logs["level"]) assert.Equal(t, "/tmp/auth.log", logs["file"]) - keyring := result["keyring"].(map[string]interface{}) + keyring, ok := result["keyring"].(map[string]interface{}) + assert.True(t, ok, "keyring should be map[string]interface{}") assert.Equal(t, "system", keyring["type"])
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
NOTICE(10 hunks)internal/exec/utils.go(4 hunks)internal/exec/utils_auth.go(1 hunks)internal/exec/utils_auth_merge_deepmerge_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/utils.go
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
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.
Learnt from: osterman
Repo: cloudposse/atmos PR: 1498
File: website/src/components/Screengrabs/atmos-terraform-metadata--help.html:25-55
Timestamp: 2025-10-07T00:25:16.333Z
Learning: In Atmos CLI, subcommands inherit flags from their parent commands via Cobra's command inheritance. For example, `atmos terraform metadata --help` shows `--affected` and related flags inherited from the parent `terraform` command (defined in cmd/terraform.go), even though the metadata subcommand doesn't explicitly define these flags. This is expected Cobra behavior and auto-generated help screengrabs accurately reflect this inheritance.
📚 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:
internal/exec/utils_auth.gointernal/exec/utils_auth_merge_deepmerge_test.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:
internal/exec/utils_auth.go
📚 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:
internal/exec/utils_auth.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/utils_auth.go
📚 Learning: 2024-12-11T18:40:12.808Z
Learnt from: Listener430
Repo: cloudposse/atmos PR: 844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
Applied to files:
internal/exec/utils_auth.go
📚 Learning: 2025-11-01T20:24:29.557Z
Learnt from: osterman
Repo: cloudposse/atmos PR: 1714
File: NOTICE:0-0
Timestamp: 2025-11-01T20:24:29.557Z
Learning: In the cloudposse/atmos repository, the NOTICE file is programmatically generated and should not be manually edited. Issues with dependency license URLs in NOTICE will be resolved when upstream package metadata is corrected.
Applied to files:
NOTICE
📚 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:
NOTICE
📚 Learning: 2025-02-21T20:56:05.539Z
Learnt from: samtholiya
Repo: cloudposse/atmos PR: 1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-02-21T20:56:05.539Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Applied to files:
NOTICE
📚 Learning: 2025-09-29T02:20:11.636Z
Learnt from: aknysh
Repo: cloudposse/atmos PR: 1540
File: internal/exec/validate_component.go:117-118
Timestamp: 2025-09-29T02:20:11.636Z
Learning: The ValidateComponent function in internal/exec/validate_component.go had its componentSection parameter type refined from `any` to `map[string]any` without adding new parameters. This is a type safety improvement, not a signature change requiring call site updates.
Applied to files:
internal/exec/utils_auth_merge_deepmerge_test.go
🧬 Code graph analysis (2)
internal/exec/utils_auth.go (3)
pkg/schema/schema.go (2)
AtmosConfiguration(53-94)Logs(451-454)pkg/merge/merge.go (1)
Merge(389-427)pkg/config/const.go (1)
AuthSectionName(84-84)
internal/exec/utils_auth_merge_deepmerge_test.go (3)
pkg/schema/schema.go (3)
AtmosConfiguration(53-94)Logs(451-454)ConfigAndStacksInfo(620-714)pkg/schema/schema_auth.go (2)
AuthConfig(4-12)KeyringConfig(15-18)pkg/config/const.go (1)
AuthSectionName(84-84)
🪛 GitHub Actions: Dependency Review
NOTICE
[error] 1-1: NOTICE file is out of date. Run './scripts/generate-notice.sh' locally and commit the changes.
⏰ 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 (5)
internal/exec/utils_auth.go (3)
12-30: The dual behavior (return + mutation) is correct for this use case.The function both returns the merged auth map and updates
componentSection["auth"]in-place (line 28). The comment explains this preventspostProcessTemplatesAndYamlFunctionsfrom overwriting with empty auth, which aligns with the PR's goal of proper auth propagation.
56-61: Clean extraction with safe fallback.The type assertion handles missing or mistyped auth sections gracefully.
64-76: Verify that silent merge failure handling is acceptable.When
m.Merge()fails (line 22-24), this function returns a fallback config without logging or surfacing the error. Merge failures might indicate configuration issues that operators should know about.Consider whether merge failures should be logged (at DEBUG or WARN level) to help diagnose configuration problems during troubleshooting.
internal/exec/utils_auth_merge_deepmerge_test.go (2)
14-153: Comprehensive test coverage for deep-merge scenarios.The table-driven approach covers empty, global-only, component-only, and override cases effectively. The test verifies both the merge result and the side effect on
componentSection.
156-201: Integration test validates the fix for auth propagation.This test verifies that merged auth survives
postProcessTemplatesAndYamlFunctions, which directly addresses the PR's goal of preventing auth section overwrites.
2 participants
what
--profileCLI flag not loading profile configuration--identityCLI flag not propagating to nested component operations when using--identitywith--profile--profileflag load and merge correctly with global configuration--identitychoice propagates to all nested operations (YAML functions like!terraform.state) when using--identitywith--profilewhy
Issue 1: Profile Loading from CLI Flag
When using the
--profileCLI flag, profile configuration was not loaded or merged with global configuration, causing authentication failures even when valid profiles were defined. TheATMOS_PROFILEenvironment variable worked correctly, but the--profileflag did not.Root Cause: Viper's
BindPFlag()creates a binding between Viper key and Cobra flag, but flag values aren't synchronized into Viper immediately when commands execute. Environment variables work because they're read directly into Viper without synchronization delay.Solution: Implemented dual approach with correct precedence order:
os.Argsparsing - highest priority)os.Getenv()read - lower priority)This ensures both
ATMOS_PROFILEenv var and--profileCLI flag work correctly, with CLI flags taking precedence as expected.Precedence Order (highest to lowest):
--profile)ATMOS_PROFILE)Files Modified:
pkg/config/load.go- AddedparseProfilesFromArgs()andgetProfilesFromFlagsOrEnv()with correct precedencepkg/config/load_profile_test.go(NEW) - Comprehensive test suite with 9 test casespkg/config/load_flags_test.go(NEW) - Tests for precedence and environment variable handlingIssue 2: Identity Flag Not Propagating to Nested Components
After fixing profile loading, a related issue was discovered: when using
--profileand--identityflags together, the identity selector still appeared during nested component operations (such as!terraform.stateYAML functions).Root Cause: When YAML template functions need to fetch state from other components, they create component-specific AuthManagers. The original implementation did not inherit the user's explicitly specified identity, always passing empty string which triggered auto-detection. With profiles containing multiple default identities, auto-detection showed the selector prompt.
Solution: Extract the authenticated identity from the parent AuthManager's chain using
GetChain()and pass it to nested component AuthManager creation. This ensures the user's--identitychoice propagates to all nested operations.Files Modified:
internal/exec/terraform_nested_auth_helper.go- UpdatedcreateComponentAuthManager()to inherit identity from parent AuthManagerinternal/exec/terraform_nested_auth_helper_test.go- Added comprehensive tests for identity inheritanceIssue 3: Test Isolation (CI Failures)
Initial implementation caused test failures in CI due to Viper caching environment variables.
Root Cause: Viper caches environment variable values on first read. In CI, if
ATMOS_PROFILEwas set by the environment or previous tests, Viper retained the cached value even after tests cleaned up, causing "profile not found" errors.Solution: Changed to read
ATMOS_PROFILEdirectly usingos.Getenv()instead of Viper. This provides:t.Setenv()cleanup works correctly)Testing:
All issues were thoroughly tested:
Profile Flag:
--profile managers✅ATMOS_PROFILE=managers✅--profile=managers,staging✅atmos terraform plan --profile managers✅Identity Flag:
--identityflag: No selector, uses specified identity ✅--identityflag: Shows selector once, nested operations inherit selected identity ✅Test Isolation:
t.Setenv()✅Success Criteria:
All success criteria met:
--profileCLI flag loads profile configuration and merges with global configATMOS_PROFILEenvironment variable continues to work--identityflag propagates to nested component operationsreferences
docs/fixes/profile-and-identity-flags-loading.md- Complete technical documentation of all issues and fixes!terraform.statefunctions #1786 - Initial work on auth context propagation through nested operationsSummary by CodeRabbit
Release Notes
New Features
--profileand--identityflags with better environment variable supportBug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.