fix: improve describe affected detection for YAML functions and source/provision sections#2061
fix: improve describe affected detection for YAML functions and source/provision sections#2061
Conversation
…se issue Add test fixture and tests to reproduce the issue where `atmos describe affected` fails when a new component exists in BASE (main) but not in HEAD (PR branch). The issue occurs when: 1. PR1 introduces a new component and merges to main 2. PR2 (based on old main) runs describe affected against current main 3. Atmos fails because YAML functions like !terraform.state try to resolve components in HEAD that only exist in BASE Added: - Test fixture: tests/fixtures/scenarios/atmos-describe-affected-new-component-in-base/ - Tests: TestDescribeAffectedNewComponentInBase, TestDescribeAffectedNewComponentInBaseWithYamlFunctions - Documentation: docs/fixes/describe-affected-new-component-in-base.md Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When running `atmos describe affected`, YAML functions like `!terraform.state` and `!terraform.output` were failing to find components that exist in BASE but not in HEAD. This happened because ExecuteDescribeComponent was called with nil AtmosConfig, causing it to create a new config from CWD (HEAD) instead of using the modified config pointing to BASE. Changes: - Add AtmosConfig field to ExecuteDescribeComponentParams - Pass atmosConfig through GetTerraformState to ExecuteDescribeComponent - Add AtmosConfig field to DescribeComponentParams in pkg/terraform/output - Update componentDescriberAdapter to pass AtmosConfig - Update tests to verify fix works Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The `describe affected` command now properly detects changes to: - `source` section (e.g., source.version changes for vendoring) - `provision` section (e.g., workdir configuration changes) Previously, only vars, env, settings, and metadata sections were checked, causing components with source or provision changes to be incorrectly excluded from affected results. Changes: - Add source section check with "stack.source" affected reason - Add provision section check with "stack.provision" affected reason - Apply to all component types: terraform, helmfile, and packer - Add test fixtures for source vendoring and workdir scenarios - Add comprehensive tests for source.version and workdir changes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update documentation to reflect the completed fix: - Change title to indicate fix is complete - Add status badge and commit reference - Update test description - Rename "Proposed Fix" to "Applied Fix" Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Rename fix documentation files with date prefix for better organization: - describe-affected-yaml-functions-ignore-base-paths.md - describe-affected-source-provision-sections.md Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove commit hash from source-provision doc - Add FIXED status to yaml-functions doc Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add detailed analysis section explaining: - Why workdir files are runtime artifacts, not source-controlled - What changes are detected vs not detected - The detection flow from config change to vendoring Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The issue applies to any PR with source/provision changes, not specifically to a two-PR scenario. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
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. |
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
📝 WalkthroughWalkthroughUpdated dependency versions and Atmos CLI to 1.206.0. Added AtmosConfig field to ExecuteDescribeComponentParams for proper path resolution in YAML functions. Introduced source and provision section change detection in describe-affected logic. Added comprehensive test fixtures and test cases for edge cases involving new components in BASE and source vendoring. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 #2061 +/- ##
==========================================
+ Coverage 76.04% 76.06% +0.01%
==========================================
Files 797 797
Lines 74558 74598 +40
==========================================
+ Hits 56699 56740 +41
+ Misses 14318 14317 -1
Partials 3541 3541
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThe PR updates dependencies, adds two fix documentation files (source/provision section detection in describe-affected, and AtmosConfig propagation through YAML function evaluation), enhances describe-affected to detect changes in source and provision sections, threads AtmosConfig through component description call chains, and introduces test fixtures and helpers for the new scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GetTerraformState
participant ExecuteDescribeComponent
participant YAMLResolver
Client->>GetTerraformState: GetTerraformState(atmosConfig)
GetTerraformState->>ExecuteDescribeComponent: ExecuteDescribeComponent(params with atmosConfig)
ExecuteDescribeComponent->>YAMLResolver: Resolve YAML functions (e.g., !terraform.state)
Note over YAMLResolver: Uses params.atmosConfig (BASE path)<br/>instead of nil (recreates from HEAD)
YAMLResolver-->>ExecuteDescribeComponent: Resolved values
ExecuteDescribeComponent-->>GetTerraformState: Component state
GetTerraformState-->>Client: Terraform outputs
sequenceDiagram
participant DescribeAffected
participant ComponentProcessor
participant SectionComparison
participant AffectedRegistry
DescribeAffected->>ComponentProcessor: Process component (HEAD vs BASE)
ComponentProcessor->>SectionComparison: Check metadata/vars/env
ComponentProcessor->>SectionComparison: Check source section (NEW)
alt source changed
SectionComparison->>AffectedRegistry: Mark with stack.source reason
end
ComponentProcessor->>SectionComparison: Check provision section (NEW)
alt provision changed
SectionComparison->>AffectedRegistry: Mark with stack.provision reason
end
AffectedRegistry-->>DescribeAffected: Affected components list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/exec/describe_affected_test.go`:
- Around line 1222-1234: The test currently only logs whether the prometheus
component was found (variable foundPrometheus) but never asserts, so failures
are silent; update the test after the loop over affected to assert that
foundPrometheus is true (e.g., use t.Fatalf or t.Errorf) with a clear message
mentioning prometheus and the affected slice length, ensuring the test fails
when the prometheus component is not detected by the code under test (keep the
existing t.Logf for extra context).
🧹 Nitpick comments (4)
internal/exec/component_describer_adapter.go (1)
14-36: AtmosConfig propagation through the adapter — solid.One small thing: line 15 uses
perf.Track(nil, ...)butparams.AtmosConfigis now available. Could pass it for consistency with the rest of the chain.Optional fix
- defer perf.Track(nil, "exec.componentDescriberAdapter.DescribeComponent")() + defer perf.Track(params.AtmosConfig, "exec.componentDescriberAdapter.DescribeComponent")()internal/exec/describe_affected_components.go (1)
301-322: Consider extracting the repeated source/provision check blocks.The source and provision checks are now copy-pasted across all three processors (Terraform, Helmfile, Packer), identical except for the component type and Spacelift parameters. The doc itself mentions Option B (generic section iteration) as a cleaner approach. Given the existing duplication in the file, this isn't urgent, but worth considering to avoid drift.
Also applies to: 411-432
internal/exec/describe_affected_test.go (2)
1335-1337: Preferrequire.NoErrorovert.Fatalffor consistency.The rest of the file uses
require.NoError(t, err)for fatal error checks. This one spot usest.Fatalfwith a manual format string.Suggested change
- if err != nil { - t.Fatalf("Unexpected error: %v", err) - } + require.NoError(t, err)
1365-1393: Mix oft.Errorfandassertfor the same kind of check.
foundVpcSource/foundVpcSourceWorkdir/foundWorkdirOnlyuset.Errorfwhen not found butassert.Equalwhen found. Consider usingrequire.Truefor the found checks to fail fast, thenassert.Equalfor the reason — keeps the pattern uniform.
- Use params.AtmosConfig in perf.Track for consistency - Replace t.Fatalf with require.NoError for consistency - Use require.True for found checks instead of t.Errorf - Clarify foundPrometheus comment (known limitation) - Add TestProcessComponentsSourceAndProvisionChanges covering source and provision section changes for terraform, helmfile, and packer components Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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)
NOTICE (1)
1-1:⚠️ Potential issue | 🟡 MinorNOTICE file is out of date per CI.
The pipeline reports this file needs regeneration. Run
./scripts/generate-notice.shlocally and commit the result. Based on learnings, this file is programmatically generated and should not be manually edited.
🧹 Nitpick comments (3)
internal/exec/describe_affected_components.go (1)
301-322: Helmfile and Packer source/provision checks are consistent.Both pass
false, nilfor Spacelift admin stacks, matching the existing convention in those functions. The three-way duplication is acknowledged vianolint:dupl— the doc's Option B (loop-based approach) could reduce this in the future if more sections are added.Also applies to: 411-432
internal/exec/describe_affected_test.go (2)
1129-1184: Good extraction of the common setup pattern.This reusable helper cuts duplication across the new test scenarios. The copy options and skip logic mirror the existing
setupDescribeAffectedTest— you could extract thecp.Optionsconstruction into a shared helper to eliminate the remaining duplication, but that's optional cleanup.
1279-1295: Error-matching logic is pragmatic but fragile.String-matching
"not provisioned"vs"Could not find"in the error message works for validating this specific bug fix, but if upstream error messages change, this test could silently pass or fail for the wrong reason. Consider defining sentinel errors or at least a comment noting the dependency on specific error wording.Not blocking — this is a reasonable tradeoff for an integration test verifying a specific fix.
|
These changes were released in v1.206.0-rc.3. |
2 participants
what
!terraform.state,!terraform.output) failing when processing BASE stacks that reference components not present in HEADdescribe affectednot detecting changes tosourceandprovisionsectionswhy
Fix 1: YAML functions ignore BASE paths
When running
describe affected, YAML functions like!terraform.statewould fail with "Could not find component" errors when:!terraform.stateor!terraform.outputRoot cause: YAML functions created a new
AtmosConfigurationfrom the current working directory (HEAD) instead of using the modified configuration pointing to BASE.Fix: Pass
AtmosConfigthrough the YAML function resolution chain instead ofnil.Fix 2: Source and provision sections not detected
Changes to
sourceandprovisionsections were silently ignored:source.versionchanges (e.g., upgrading vendored module versions) were not detectedprovision.workdir.enabledchanges were not detectedRoot cause: Only
metadata,vars,env,settings, and component folder files were checked.Fix: Add
sourceandprovisionsection checks with affected reasonsstack.sourceandstack.provision.references
docs/fixes/2026-02-07-describe-affected-yaml-functions-ignore-base-paths.mddocs/fixes/2026-02-07-describe-affected-source-provision-sections.mdSummary by CodeRabbit
Release Notes
Bug Fixes
Chores