fix(auth): honor --identity=false in describe affected and dependents#2471
Merged
Merged
Conversation
`--identity=false` (and aliases `off`/`0`/`no`) normalized correctly at the parser layer in 1.219 (PR #2412), but the disabled signal was only wired through `list instances`. In `describe affected`, the top-level AuthManager correctly became nil, but a nil AuthManager was indistinguishable from "no identity specified" downstream — so the per-component auth resolver still ran whenever `processTemplates` was true (its default), reintroducing the AssumeRoleWithWebIdentity call the user tried to disable. Thread an `AuthDisabled bool` from the cmd layer through `executeDescribeAffectedWith*`, `executeDescribeAffected`, `addDependentsToAffected`, and `ExecuteDescribeDependents`, routing inner stack resolution through `ExecuteDescribeStacksWithAuthDisabled` so `processor.authDisabled=true` short-circuits the per-component resolver. Also propagated through `terraform_affected.go`, `terraform_affected_graph.go`, `pkg/list/list_affected.go`, `pkg/ai/tools/atmos/describe_affected.go`, and `atlantis_generate_repo_config.go` call sites. Extracted `pkg/list/list_affected.go::executeAffectedLogic` into three per-mode helpers to stay under the 60-line function-length limit. Tests: - cmd/describe_affected_test.go::TestDescribeAffectedSetsAuthDisabled verifies `false`/`off`/`0`/`no` env values set `AuthDisabled=true` and `AuthManager=nil`. - internal/exec/describe_affected_authdisabled_test.go verifies `Execute()` forwards `AuthDisabled` to all three helper paths and to `addDependentsToAffected`. - describe_stacks_component_processor_auth_test.go adds the exact `(processTemplates=true, processYamlFunctions=false, authDisabled=true)` regression case from the infra-live CI failure. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds an explicit ChangesAuthDisabled flag threading through describe affected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/describe_affected_test.go (1)
218-251: ⚡ Quick winAdd a direct
--identityflag case to this test.Current cases validate env/viper aliases, but the explicit CLI flag normalization path (
--identity=false|off|0|no) is still untested here. Add at least one case that sets the command flag directly and confirms it wins over env/viper inputs.As per coding guidelines "Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/describe_affected_test.go` around lines 218 - 251, Add a new test case to cover the explicit CLI flag path so flags take precedence over env/viper: update the tests slice in cmd/describe_affected_test.go to include a case that sets the describeAffectedCmd flag "--identity" to a falsey value (e.g., "false" or "off") when calling run(describeAffectedCmd, []string{"--identity=false"}), then assert that captured.AuthDisabled on the exec.DescribeAffectedCmdArgs produced by getRunnableDescribeAffectedCmd (the runnable created around ParseDescribeAffectedCliArgs and the mock exec) reflects the flag (true for disabled); this ensures the describeAffectedCmd flag normalization path is exercised and wins over env/viper inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/exec/describe_dependents.go`:
- Line 45: DescribeDependentsArgs.AuthDisabled is never propagated because
DescribeDependentsExecProps lacks an AuthDisabled field, so
describeDependentsExec.Execute always sees the default false; add an
AuthDisabled bool to DescribeDependentsExecProps, set it from
DescribeDependentsArgs.AuthDisabled where the props are constructed, and ensure
describeDependentsExec.Execute reads the new props.AuthDisabled when choosing
the auth-disabled execution path (reference DescribeDependentsArgs.AuthDisabled,
DescribeDependentsExecProps, and describeDependentsExec.Execute).
---
Nitpick comments:
In `@cmd/describe_affected_test.go`:
- Around line 218-251: Add a new test case to cover the explicit CLI flag path
so flags take precedence over env/viper: update the tests slice in
cmd/describe_affected_test.go to include a case that sets the
describeAffectedCmd flag "--identity" to a falsey value (e.g., "false" or "off")
when calling run(describeAffectedCmd, []string{"--identity=false"}), then assert
that captured.AuthDisabled on the exec.DescribeAffectedCmdArgs produced by
getRunnableDescribeAffectedCmd (the runnable created around
ParseDescribeAffectedCliArgs and the mock exec) reflects the flag (true for
disabled); this ensures the describeAffectedCmd flag normalization path is
exercised and wins over env/viper inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6698dea9-e7d8-4606-9841-2a3aa341c1ed
📒 Files selected for processing (19)
cmd/describe_affected.gocmd/describe_affected_test.gointernal/exec/atlantis_generate_repo_config.gointernal/exec/describe_affected.gointernal/exec/describe_affected_authdisabled_test.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_dependents.gointernal/exec/describe_stacks_component_processor_auth_test.gointernal/exec/terraform_affected.gointernal/exec/terraform_affected_graph.gopkg/ai/tools/atmos/describe_affected.gopkg/describe/describe_affected_test.gopkg/list/list_affected.gotests/describe_affected_greenfield_test.gotests/describe_affected_include_test.go
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (33.01%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2471 +/- ##
==========================================
+ Coverage 78.22% 78.23% +0.01%
==========================================
Files 1119 1119
Lines 106205 106243 +38
==========================================
+ Hits 83074 83124 +50
+ Misses 18494 18478 -16
- Partials 4637 4641 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Address pre-commit hook failures from PR #2471 CI run. The newer gofumpt in CI flagged formatting in four files I touched in the previous commit: - pkg/list/list_affected.go — split `})` from inline closure into `},` + `)` on separate lines. - internal/exec/describe_affected_utils.go — split the long log.Warn message into its own line in two greenfield-handling branches. - internal/exec/atlantis_generate_repo_config.go — split trailing args in two errors.Errorf calls onto their own lines. - internal/exec/describe_affected_utils_2.go — remove redundant parens from `&((*slice)[i])` → `&(*slice)[i]` in four loop bodies. No behavior change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address CodeRabbit review on PR #2471. The previous commit added `AuthDisabled` to `DescribeDependentsArgs` but missed adding it to `DescribeDependentsExecProps`, which is the type that `describe dependents` constructs from CLI flags and passes to `describeDependentsExec.Execute`. The executor then built a `DescribeDependentsArgs` without the flag, so the inner `ExecuteDescribeStacksWithAuthDisabled` always received `authDisabled=false` — re-introducing the per-component auth attempt for `atmos describe dependents --identity=false`. - internal/exec/describe_dependents.go: add `AuthDisabled` to `DescribeDependentsExecProps` and forward it into `DescribeDependentsArgs` inside `describeDependentsExec.Execute`. - cmd/describe_dependents.go: set `describe.AuthDisabled` from the normalized identity name, mirroring the wiring in `cmd/describe_affected.go`. Tests: - cmd/describe_dependents_test.go::TestDescribeDependentsSetsAuthDisabled — table covers `false`/`off`/`0`/`no` env spellings. - internal/exec/describe_dependents_authdisabled_test.go — TestDescribeDependentsExec_Execute_ForwardsAuthDisabled pins the executor-side prop → arg propagation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
May 22, 2026
Contributor
|
These changes were released in v1.220.0-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
--identity=false(and aliasesoff/0/no) inatmos describe affectedso per-component auth resolution is skipped, not just the top-level AuthManager creation.DescribeAffectedCmdArgs.AuthDisabled/DescribeDependentsArgs.AuthDisabledflag from the cmd layer throughexecuteDescribeAffectedWith{TargetRepoPath,TargetRefClone,TargetRefCheckout},executeDescribeAffected,addDependentsToAffected, andExecuteDescribeDependents, routing inner stack resolution throughExecuteDescribeStacksWithAuthDisabled.terraform_affected.go,terraform_affected_graph.go,pkg/list/list_affected.go,pkg/ai/tools/atmos/describe_affected.go, andatlantis_generate_repo_config.goso every caller of the public helpers passes the signal.pkg/list/list_affected.go::executeAffectedLogicinto three per-mode helpers to stay under the 60-line function-length limit after the extra parameter.why
describe affected --upload --process-functions=false --identity=falserun incloudposse/infra-liveCI (failing run) and still gotSTS AssumeRoleWithWebIdentity 403 AccessDeniedfor componenttfstate-plat.--identity=false→__DISABLED__at the parser layer and madeCreateAuthManagerFromIdentity*short-circuit tonil, but it only wired the disabled signal all the way down throughlist instances. Indescribe affected, the top-level AuthManager correctly becamenil, but anilAuthManager was indistinguishable from "no identity specified" downstream. With--process-templates=true(the default),shouldResolvePerComponentAuth(processTemplates, processYamlFunctions)still returnedtrue, so the per-component resolver calledcreateComponentAuthManager, which built a fresh AuthManager fromatmosConfig.Authand tried the assume-role call the user thought they had disabled.--identity=falseactually mean "no auth, anywhere" indescribe affected, matching the contract that already works forlist instances.Tests:
cmd/describe_affected_test.go::TestDescribeAffectedSetsAuthDisabledcoversfalse/off/0/noenv-var spellings and assertsAuthDisabled=trueandAuthManager=nil.internal/exec/describe_affected_authdisabled_test.goverifiesExecute()forwardsAuthDisabledto all three helper paths and toaddDependentsToAffected.internal/exec/describe_stacks_component_processor_auth_test.goadds the exact(processTemplates=true, processYamlFunctions=false, authDisabled=true)regression case from the infra-live CI failure to the existing table.references
fix(auth): normalize --identity=false to disable authentication) which only wired the disabled signal throughlist instances.Summary by CodeRabbit
Bug Fixes
describe affectedanddescribe dependentsnow explicitly record when authentication is disabled (e.g.,--identity=false,off,0,no), ensuring downstream discovery and dependency resolution skip per-component auth and avoid unintended auth attempts.Tests