fix: thread auth identity through describe/list affected for S3 state reads#2250
Merged
fix: thread auth identity through describe/list affected for S3 state reads#2250
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Contributor
📝 WalkthroughWalkthroughThis PR threads an AuthManager (derived from a CLI Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant AuthMgr as AuthManager
participant List as ListAffected
participant Describe as DescribeAffected
participant Proc as ComponentProcessor
participant TF as TerraformState
CLI->>CLI: read & normalize --identity
CLI->>AuthMgr: create & authenticate (identity)
CLI->>List: ExecuteListAffected(opts + AuthManager)
List->>Describe: ExecuteDescribeAffected(..., AuthManager)
Describe->>Proc: processComponentEntry(component, AuthManager)
Proc->>Proc: resolve per-component AuthManager (when YAML funcs enabled)
Proc->>TF: GetTerraformState(..., resolved AuthContext)
TF-->>Describe: return state
Describe-->>List: affected components
List-->>CLI: output results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/terraform_affected_graph.go (1)
6-6:⚠️ Potential issue | 🟡 MinorInconsistent logger import.
Line 6 uses
log "github.com/charmbracelet/log"while the codebase standard islog "github.com/cloudposse/atmos/pkg/logger". This could lead to inconsistent logging behavior and missing structured log features.As per coding guidelines: "Organize imports in three groups separated by blank lines, sorted alphabetically: 1) Go stdlib, 2) 3rd-party (NOT cloudposse/atmos), 3) Atmos packages. Maintain standard aliases:
cfg,log,u,errUtils."Proposed fix
- log "github.com/charmbracelet/log" + log "github.com/cloudposse/atmos/pkg/logger"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/terraform_affected_graph.go` at line 6, Replace the incorrect third-party logger import alias `log "github.com/charmbracelet/log"` with the project-standard `log "github.com/cloudposse/atmos/pkg/logger"` and reorganize the import block into three groups (stdlib, third-party, Atmos packages) sorted alphabetically with blank lines between groups; ensure standard alias usage for Atmos imports (e.g., `cfg`, `log`, `u`, `errUtils`) so any references to the `log` alias in this file (e.g., calls to log.*) use the correct structured logger.
🧹 Nitpick comments (3)
docs/fixes/2026-03-25-describe-affected-auth-identity-not-used.md (2)
86-87: Add punctuation at end of list item.Static analysis flagged the missing punctuation.
-- `terraform_affected.go` passes `args.AuthManager` +- `terraform_affected.go` passes `args.AuthManager`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/fixes/2026-03-25-describe-affected-auth-identity-not-used.md` around lines 86 - 87, The list item "- `terraform_affected.go` passes `args.AuthManager`" is missing terminal punctuation; update that list entry to end with a period (or the project's preferred punctuation) so it reads, for example, "- `terraform_affected.go` passes `args.AuthManager`." to satisfy static analysis and maintain consistency.
44-49: Add language specifier to fenced code block.The code block showing the call chain should have a language specifier for consistency.
-``` +```text DescribeAffectedCmdArgs.AuthManager (has value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/fixes/2026-03-25-describe-affected-auth-identity-not-used.md` around lines 44 - 49, Update the fenced code block that shows the call chain so it includes a language specifier (e.g., ```text) before the block; specifically modify the block containing "DescribeAffectedCmdArgs.AuthManager (has value)" and the subsequent lines referencing Execute(), executeDescribeAffected(), and ExecuteDescribeStacks(...) to start with a language-tagged fence (for example ```text) to match repository formatting conventions.internal/exec/describe_affected_helpers.go (1)
29-43: Consider consolidating these long signatures with an options struct.These APIs are getting parameter-heavy, and adding auth expanded that further. Wrapping the mutable toggles/deps into an options struct (or functional options) would reduce call-site fragility and make future identity-related additions safer.
♻️ Suggested direction (minimal shape)
+type DescribeAffectedOptions struct { + IncludeSpaceliftAdminStacks bool + IncludeSettings bool + Stack string + ProcessTemplates bool + ProcessYAMLFunctions bool + Skip []string + ExcludeLocked bool + AuthManager auth.AuthManager +} - -func ExecuteDescribeAffectedWithTargetRefClone( - atmosConfig *schema.AtmosConfiguration, - ref string, - sha string, - sshKeyPath string, - sshKeyPassword string, - includeSpaceliftAdminStacks bool, - includeSettings bool, - stack string, - processTemplates bool, - processYamlFunctions bool, - skip []string, - excludeLocked bool, - authManager auth.AuthManager, -) ([]schema.Affected, *plumbing.Reference, *plumbing.Reference, string, error) { +func ExecuteDescribeAffectedWithTargetRefClone( + atmosConfig *schema.AtmosConfiguration, + ref string, + sha string, + sshKeyPath string, + sshKeyPassword string, + opts DescribeAffectedOptions, +) ([]schema.Affected, *plumbing.Reference, *plumbing.Reference, string, error) {As per coding guidelines, "Use functional options pattern for configuration instead of functions with many parameters.".
Also applies to: 190-202, 280-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/describe_affected_helpers.go` around lines 29 - 43, The ExecuteDescribeAffectedWithTargetRefClone signature is too long and should be replaced with an options struct (or functional options) to group toggles, paths, and dependencies: create a DescribeAffectedOptions struct (fields: Ref, SHA, SSHKeyPath, SSHKeyPassword, IncludeSpaceliftAdminStacks, IncludeSettings, Stack, ProcessTemplates, ProcessYamlFunctions, Skip []string, ExcludeLocked, AuthManager auth.AuthManager, etc.), change the function signature to ExecuteDescribeAffectedWithTargetRefClone(atmosConfig *schema.AtmosConfiguration, opts DescribeAffectedOptions) ([]schema.Affected, *plumbing.Reference, *plumbing.Reference, string, error), update callers to construct and pass the options struct (or provide helpers to build it), and apply the same refactor to the other affected entry points mentioned (consolidate their parameter lists into the same or similar options type) so future additions become non-breaking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/exec/terraform_affected_graph.go`:
- Line 6: Replace the incorrect third-party logger import alias `log
"github.com/charmbracelet/log"` with the project-standard `log
"github.com/cloudposse/atmos/pkg/logger"` and reorganize the import block into
three groups (stdlib, third-party, Atmos packages) sorted alphabetically with
blank lines between groups; ensure standard alias usage for Atmos imports (e.g.,
`cfg`, `log`, `u`, `errUtils`) so any references to the `log` alias in this file
(e.g., calls to log.*) use the correct structured logger.
---
Nitpick comments:
In `@docs/fixes/2026-03-25-describe-affected-auth-identity-not-used.md`:
- Around line 86-87: The list item "- `terraform_affected.go` passes
`args.AuthManager`" is missing terminal punctuation; update that list entry to
end with a period (or the project's preferred punctuation) so it reads, for
example, "- `terraform_affected.go` passes `args.AuthManager`." to satisfy
static analysis and maintain consistency.
- Around line 44-49: Update the fenced code block that shows the call chain so
it includes a language specifier (e.g., ```text) before the block; specifically
modify the block containing "DescribeAffectedCmdArgs.AuthManager (has value)"
and the subsequent lines referencing Execute(), executeDescribeAffected(), and
ExecuteDescribeStacks(...) to start with a language-tagged fence (for example
```text) to match repository formatting conventions.
In `@internal/exec/describe_affected_helpers.go`:
- Around line 29-43: The ExecuteDescribeAffectedWithTargetRefClone signature is
too long and should be replaced with an options struct (or functional options)
to group toggles, paths, and dependencies: create a DescribeAffectedOptions
struct (fields: Ref, SHA, SSHKeyPath, SSHKeyPassword,
IncludeSpaceliftAdminStacks, IncludeSettings, Stack, ProcessTemplates,
ProcessYamlFunctions, Skip []string, ExcludeLocked, AuthManager
auth.AuthManager, etc.), change the function signature to
ExecuteDescribeAffectedWithTargetRefClone(atmosConfig
*schema.AtmosConfiguration, opts DescribeAffectedOptions) ([]schema.Affected,
*plumbing.Reference, *plumbing.Reference, string, error), update callers to
construct and pass the options struct (or provide helpers to build it), and
apply the same refactor to the other affected entry points mentioned
(consolidate their parameter lists into the same or similar options type) so
future additions become non-breaking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ed6bcbd-6c2b-49c0-ad13-538796566a30
📒 Files selected for processing (17)
cmd/list/affected.godocs/fixes/2026-03-25-describe-affected-auth-identity-not-used.mdinternal/exec/atlantis_generate_repo_config.gointernal/exec/describe_affected.gointernal/exec/describe_affected_helpers.gointernal/exec/describe_affected_test.gointernal/exec/describe_affected_utils.gointernal/exec/describe_affected_utils_2.gointernal/exec/describe_affected_utils_test.gointernal/exec/describe_stacks_component_processor.gointernal/exec/terraform_affected.gointernal/exec/terraform_affected_graph.gointernal/exec/terraform_state_utils.gopkg/ai/tools/atmos/describe_affected.gopkg/describe/describe_affected_test.gopkg/list/list_affected.gotests/describe_affected_include_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2250 +/- ##
==========================================
+ Coverage 77.18% 77.22% +0.03%
==========================================
Files 1015 1015
Lines 96021 96065 +44
==========================================
+ Hits 74117 74182 +65
+ Misses 17713 17695 -18
+ Partials 4191 4188 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…affected - Add TestAffectedIdentityFlagParsing: 3 cases covering flag > viper > empty precedence for --identity flag in list affected command - Add IdentityName to AffectedOptions struct test and defaults test - Add TestAffectedCommandOptions_IdentityName: 3 cases for field propagation - Add TestDescribeStacksAuthManager_NoPerComponentAuthWhenYamlFunctionsDisabled: covers the processYamlFunctions=false branch in processComponentEntry Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/list/affected_test.go (1)
152-158: Test doesn't replicate full production logic—missing normalization step.The comment says "Replicate the logic from affectedCmd.RunE" but the production code at
cmd/list/affected.go:71appliescfg.NormalizeIdentityValue(identityName)after reading from flag/viper. This test omits that step.For accuracy, either:
- Add the normalization call to truly replicate production behavior, or
- Update the comment to clarify it only tests precedence, not the full identity resolution path.
Option 1: Include normalization
// Replicate the logic from affectedCmd.RunE. var identityName string if cmd.Flags().Changed("identity") { identityName, _ = cmd.Flags().GetString("identity") } else { identityName = v.GetString("identity") } + identityName = cfg.NormalizeIdentityValue(identityName)This would require adding the
cfgimport.Option 2: Clarify the comment
- // Replicate the logic from affectedCmd.RunE. + // Replicate the flag/viper precedence logic (normalization tested separately).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/list/affected_test.go` around lines 152 - 158, The test's identity resolution block (reading identityName via cmd.Flags().GetString and v.GetString) omits the production normalization step used in affectedCmd.RunE; call cfg.NormalizeIdentityValue(identityName) after determining identityName (and add the cfg import) so the test mirrors production behavior, or alternatively update the test comment to state it only verifies precedence and does not perform normalization—prefer adding the normalization call to fully replicate affectedCmd.RunE's logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/list/affected_test.go`:
- Around line 152-158: The test's identity resolution block (reading
identityName via cmd.Flags().GetString and v.GetString) omits the production
normalization step used in affectedCmd.RunE; call
cfg.NormalizeIdentityValue(identityName) after determining identityName (and add
the cfg import) so the test mirrors production behavior, or alternatively update
the test comment to state it only verifies precedence and does not perform
normalization—prefer adding the normalization call to fully replicate
affectedCmd.RunE's logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e67d6af0-a8ea-4199-8ee5-b003fc4c4145
📒 Files selected for processing (3)
cmd/list/affected_test.gointernal/exec/describe_stacks_authmanager_propagation_test.gopkg/list/list_affected_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/list/list_affected_test.go
aknysh
approved these changes
Mar 25, 2026
|
These changes were released in v1.212.1-rc.0. |
|
These changes were released in v1.213.0-test.4. |
|
These changes were released in v1.212.1-rc.1. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
AuthManagerthrough the entire describe affected call chain soExecuteDescribeStacksreceives the identity credentials instead ofnilGetTerraformStateto use the resolved component-specificAuthContextfor S3 backend reads instead of the (potentially nil) passed-inauthContextExecuteDescribeStacksgated behindprocessYamlFunctions, so each component can use its own identity for!terraform.statereads--identity/-iflag through thelist affectedcommand, which had the flag registered (inherited fromlistCmd) but never read it or created anAuthManagerwhy
atmos list affected --ref refs/heads/mainfailing with S3 auth errors despite validatmos authidentityresolveAuthManagerForNestedComponentcorrectly created per-component AuthManagers, but the credentials were never used for the actual S3GetObjectcallGetTerraformStateignored resolved AuthContext for backend reads, (3) no per-component identity resolution inExecuteDescribeStacks, (4)list affectednever read the--identityflagatmos auth shellworked because it setsATMOS_IDENTITYenv var (viper fallback), but explicit-i admin-accountwas silently ignored bylist affectedreferences
docs/fixes/2026-03-25-describe-affected-auth-identity-not-used.md— detailed fix documentationdocs/fixes/nested-terraform-state-auth-context-propagation.md— original nested auth fixdocs/fixes/2026-03-03-yaml-functions-auth-multi-component.md— multi-component auth fixSummary by CodeRabbit
New Features
--identityflag tolist affectedfor explicit identity selection.Bug Fixes
Documentation
Tests