Repository navigation
fix: CI job summary shows NO_CHANGE badge for output-only Terraform changes - #2306
Merged
Merged
Conversation
…hanges When Terraform exits with code 2 (changes detected) but only outputs changed (no resource changes), the CI job summary incorrectly displayed a gray NO_CHANGE badge. This happened because HasChanges() only checked resource counts, ignoring output changes entirely. - Add HasOutputChanges field and HasResourceChanges() method to context - Update ParsePlanJSON and ParsePlanOutput to detect output-only changes - Add distinct OUTPUT_CHANGE badge (blue) to plan and apply templates - Differentiate headings: "Resource Changes Found" vs "Output Changes Found" - Add "Changes to Outputs:" as plan output marker for preamble stripping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
April 9, 2026 21:10
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
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 |
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (3)
📝 WalkthroughWalkthroughDetects Terraform plan output-only changes separately from resource changes, adds Changes
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.
🧹 Nitpick comments (3)
pkg/ci/plugins/terraform/parser_test.go (1)
968-972: Consider adding a zero-count edge case.The
buildOutputChangeSummary(0)path would return "0 outputs to change" which may or may not be intentional. While unlikely to be called with 0 in practice, documenting the behavior with a test case could prevent surprises.Optional test addition
func TestBuildOutputChangeSummary(t *testing.T) { + assert.Equal(t, "0 outputs to change", buildOutputChangeSummary(0)) assert.Equal(t, "1 output to change", buildOutputChangeSummary(1)) assert.Equal(t, "2 outputs to change", buildOutputChangeSummary(2)) assert.Equal(t, "5 outputs to change", buildOutputChangeSummary(5)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ci/plugins/terraform/parser_test.go` around lines 968 - 972, Add a zero-count test case to TestBuildOutputChangeSummary to document and lock in the behavior for the edge case where buildOutputChangeSummary(0) is called; update the test in pkg/ci/plugins/terraform/parser_test.go (function TestBuildOutputChangeSummary) to include an assertion like assert.Equal(t, "0 outputs to change", buildOutputChangeSummary(0)) so the expected output for 0 is explicit.pkg/ci/plugins/terraform/context.go (1)
46-48: Clarify field semantics in the comment.The field
HasOutputChangesis set totruewhenever outputs exist (line 72), regardless of whether resource changes also exist. The current comment says "without resource changes" which could be misleading. Templates would need to checkHasOutputChanges && !HasResourceChanges()for output-only scenarios.Consider updating the comment to reflect actual semantics:
Suggested comment clarification
- // HasOutputChanges indicates if there are terraform output value changes (without resource changes). + // HasOutputChanges indicates if there are terraform output value changes. + // For output-only changes (no resource changes), check HasOutputChanges && !HasResourceChanges(). HasOutputChanges bool🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ci/plugins/terraform/context.go` around lines 46 - 48, The comment for the struct field HasOutputChanges is misleading — update it to state that HasOutputChanges is true whenever Terraform outputs exist (regardless of resource changes), rather than implying outputs-only; mention that callers must combine HasOutputChanges with HasResourceChanges() (e.g., check HasOutputChanges && !HasResourceChanges()) to detect output-only changes; modify the comment above the HasOutputChanges field and keep references to HasOutputChanges and HasResourceChanges() so readers know how to interpret the flag.pkg/ci/plugins/terraform/template_test.go (1)
543-554: Test name could be clearer.The test case "output changes only does not count" doesn't actually set
HasOutputChanges: trueon the context. SinceHasResourceChanges()only checks resource counts (notHasOutputChanges), the test passes correctly — but the name implies output changes are being tested when they aren't.Consider renaming to "empty resources" or adding a dedicated test case that explicitly sets
HasOutputChanges: trueto confirm the method ignores it.♻️ Optional: Add explicit output-only test case
tests := []struct { name string resources plugin.ResourceCounts + hasOutputChanges bool want bool }{ {"no changes", plugin.ResourceCounts{}, false}, {"create only", plugin.ResourceCounts{Create: 1}, true}, - {"output changes only does not count", plugin.ResourceCounts{}, false}, + {"empty resources", plugin.ResourceCounts{}, false}, + {"output changes only does not count as resource changes", plugin.ResourceCounts{}, false}, // HasOutputChanges would be true but irrelevant } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx := &TerraformTemplateContext{Resources: tt.resources} + ctx := &TerraformTemplateContext{ + Resources: tt.resources, + HasOutputChanges: tt.name == "output changes only does not count as resource changes", + } assert.Equal(t, tt.want, ctx.HasResourceChanges()) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ci/plugins/terraform/template_test.go` around lines 543 - 554, The test name "output changes only does not count" is misleading because the table entry doesn't set TerraformTemplateContext.HasOutputChanges; either rename that case to "empty resources" or add a new table case that sets TerraformTemplateContext{Resources: plugin.ResourceCounts{}, HasOutputChanges: true} to assert HasResourceChanges() still returns false; update the test table and t.Run entries accordingly so TerraformTemplateContext and its HasResourceChanges() behavior are explicitly and accurately covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/ci/plugins/terraform/context.go`:
- Around line 46-48: The comment for the struct field HasOutputChanges is
misleading — update it to state that HasOutputChanges is true whenever Terraform
outputs exist (regardless of resource changes), rather than implying
outputs-only; mention that callers must combine HasOutputChanges with
HasResourceChanges() (e.g., check HasOutputChanges && !HasResourceChanges()) to
detect output-only changes; modify the comment above the HasOutputChanges field
and keep references to HasOutputChanges and HasResourceChanges() so readers know
how to interpret the flag.
In `@pkg/ci/plugins/terraform/parser_test.go`:
- Around line 968-972: Add a zero-count test case to
TestBuildOutputChangeSummary to document and lock in the behavior for the edge
case where buildOutputChangeSummary(0) is called; update the test in
pkg/ci/plugins/terraform/parser_test.go (function TestBuildOutputChangeSummary)
to include an assertion like assert.Equal(t, "0 outputs to change",
buildOutputChangeSummary(0)) so the expected output for 0 is explicit.
In `@pkg/ci/plugins/terraform/template_test.go`:
- Around line 543-554: The test name "output changes only does not count" is
misleading because the table entry doesn't set
TerraformTemplateContext.HasOutputChanges; either rename that case to "empty
resources" or add a new table case that sets TerraformTemplateContext{Resources:
plugin.ResourceCounts{}, HasOutputChanges: true} to assert HasResourceChanges()
still returns false; update the test table and t.Run entries accordingly so
TerraformTemplateContext and its HasResourceChanges() behavior are explicitly
and accurately covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef862e8c-9416-4783-9ac2-ed652f8b5d2d
📒 Files selected for processing (11)
pkg/ci/plugins/terraform/context.gopkg/ci/plugins/terraform/parser.gopkg/ci/plugins/terraform/parser_test.gopkg/ci/plugins/terraform/plugin.gopkg/ci/plugins/terraform/template_test.gopkg/ci/plugins/terraform/templates/apply.mdpkg/ci/plugins/terraform/templates/plan.mdpkg/ci/plugins/terraform/testdata/golden/plan_creates_only.mdpkg/ci/plugins/terraform/testdata/golden/plan_destroys_warning.mdpkg/ci/plugins/terraform/testdata/golden/plan_with_warnings.mdpkg/ci/plugins/terraform/testdata/plan_json/success_output_only.json
The plan template was changed to use "Resource Changes Found" instead of "Changes Found" for resource-level changes, but test assertions and golden files were not updated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2306 +/- ##
==========================================
+ Coverage 77.19% 77.22% +0.03%
==========================================
Files 1070 1070
Lines 101511 101547 +36
==========================================
+ Hits 78359 78418 +59
+ Misses 18835 18812 -23
Partials 4317 4317
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Reverse EnsureWorkspace to try WorkspaceSelect before WorkspaceNew, fixing a Windows race condition where WorkspaceNew creates a new empty workspace instead of failing with "already exists" after init -reconfigure. Add cache reset functions (ResetOutputsCache, ResetStateCache) and clear caches at test start to prevent cross-test contamination via shared package-level sync.Map caches. Add Windows-specific retry in ReadTerraformBackendLocal for state file visibility after apply. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/exec/yaml_func_terraform_output_test.go (1)
19-20: Pair the pre-test reset with a cleanup reset.This clears shared cache state on entry, but the test repopulates the cache before exiting. Register the same helper in
t.Cleanupso later tests don't inherit this run's outputs.Suggested fix
tfoutput.ResetOutputsCache() + t.Cleanup(func() { + tfoutput.ResetOutputsCache() + })Based on learnings: "cloudposse/atmos: pkg/utils now exposes ResetPathMatchCache() and ResetGlobMatchesCache() (export_test.go). In tests, prefer these helpers with t.Cleanup over direct global var assignment to avoid races and inter-test coupling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/yaml_func_terraform_output_test.go` around lines 19 - 20, Add a matching cleanup call so the pre-test cache reset doesn't leak state: after calling tfoutput.ResetOutputsCache() in the test setup, register a t.Cleanup that calls tfoutput.ResetOutputsCache() again (use the test's t and t.Cleanup to ensure the cache is reset after the test finishes); if other cache helpers are available in this package (e.g., ResetPathMatchCache or ResetGlobMatchesCache), prefer using those helper functions both at setup and inside t.Cleanup to avoid races and inter-test coupling.internal/exec/yaml_func_utils_test.go (1)
237-239: Reset these caches again int.Cleanup.This gives you a clean start, but the test repopulates both caches and leaves shared state behind for whatever runs next. Add the same resets in
t.Cleanupso isolation is bidirectional.Suggested fix
ResetStateCache() tfoutput.ResetOutputsCache() + t.Cleanup(func() { + ResetStateCache() + tfoutput.ResetOutputsCache() + })Based on learnings: "cloudposse/atmos: pkg/utils now exposes ResetPathMatchCache() and ResetGlobMatchesCache() (export_test.go). In tests, prefer these helpers with t.Cleanup over direct global var assignment to avoid races and inter-test coupling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/yaml_func_utils_test.go` around lines 237 - 239, Add cache resets into t.Cleanup so the test cleans up shared state after it runs: call ResetStateCache() and tfoutput.ResetOutputsCache() inside t.Cleanup to mirror the initial clears, or if available prefer the exported helpers ResetPathMatchCache() and ResetGlobMatchesCache() (and tfoutput.ResetOutputsCache()) in the t.Cleanup closure; update the test to register those cleanup calls using t.Cleanup to avoid leaking state between tests.internal/exec/yaml_func_terraform_state_test.go (1)
19-21: Add post-test cache cleanup for stronger isolation.Lines 19-21 clear shared caches only at test start. Add
t.Cleanupto clear them again after the test so this test doesn’t leak state into later tests.Suggested patch
func TestYamlFuncTerraformState(t *testing.T) { // Clear caches to ensure isolation from other tests that may have run first. ResetStateCache() tfoutput.ResetOutputsCache() + t.Cleanup(func() { + ResetStateCache() + tfoutput.ResetOutputsCache() + }) if _, lookErr := exec.LookPath("tofu"); lookErr != nil {Based on learnings: cloudposse/atmos tests should use cache reset helpers with
t.Cleanupto avoid races and inter-test coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/yaml_func_terraform_state_test.go` around lines 19 - 21, The test currently calls ResetStateCache() and tfoutput.ResetOutputsCache() only at the start causing possible leakage; add post-test cleanup by registering t.Cleanup callbacks that call ResetStateCache() and tfoutput.ResetOutputsCache() so both caches are reset after the test finishes (use t.Cleanup(func() { ResetStateCache() }) and t.Cleanup(func() { tfoutput.ResetOutputsCache() }) adjacent to the existing setup calls).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exec/terraform_state_utils.go`:
- Around line 21-24: ResetStateCache currently rebinds terraformStateCache to a
new sync.Map which races with concurrent GetTerraformState Load/Store calls;
instead, iterate over the existing terraformStateCache (use
terraformStateCache.Range) and call terraformStateCache.Delete(key) for each
entry so the same sync.Map instance is cleared in place; update ResetStateCache
to perform an in-place clear and keep references used by GetTerraformState and
other callers.
In `@pkg/terraform/output/executor.go`:
- Around line 47-50: The ResetOutputsCache function currently rebinds the shared
terraformOutputsCache to a new sync.Map which races with concurrent readers;
instead, clear the existing sync.Map in place by iterating over
terraformOutputsCache and deleting each key (use terraformOutputsCache.Range and
terraformOutputsCache.Delete) so goroutines holding references to the map see it
emptied rather than swapped; keep the existing perf.Track defer and replace the
assignment terraformOutputsCache = sync.Map{} with in-place clearing logic
inside ResetOutputsCache.
In `@pkg/terraform/output/workspace.go`:
- Around line 63-85: The WorkspaceSelect error handling currently treats any
error as "missing" and calls WorkspaceNew; add a helper
isWorkspaceMissingError(err error) that mirrors the existing
isWorkspaceExistsError pattern to detect explicit "workspace not found" messages
from terraform-exec, then change the select-fallback logic in the function that
calls runner.WorkspaceSelect(ctx, workspace) to only call runner.WorkspaceNew
when isWorkspaceMissingError(err) is true and otherwise return/wrap the original
error (using the same wrapErrorWithStderr/errUtils.Build flow). Also add a unit
test that simulates a non-missing select error (e.g., errors.New("permission
denied")) and asserts WorkspaceNew is not invoked and the original error is
returned to cover the negative-path behavior.
---
Nitpick comments:
In `@internal/exec/yaml_func_terraform_output_test.go`:
- Around line 19-20: Add a matching cleanup call so the pre-test cache reset
doesn't leak state: after calling tfoutput.ResetOutputsCache() in the test
setup, register a t.Cleanup that calls tfoutput.ResetOutputsCache() again (use
the test's t and t.Cleanup to ensure the cache is reset after the test
finishes); if other cache helpers are available in this package (e.g.,
ResetPathMatchCache or ResetGlobMatchesCache), prefer using those helper
functions both at setup and inside t.Cleanup to avoid races and inter-test
coupling.
In `@internal/exec/yaml_func_terraform_state_test.go`:
- Around line 19-21: The test currently calls ResetStateCache() and
tfoutput.ResetOutputsCache() only at the start causing possible leakage; add
post-test cleanup by registering t.Cleanup callbacks that call ResetStateCache()
and tfoutput.ResetOutputsCache() so both caches are reset after the test
finishes (use t.Cleanup(func() { ResetStateCache() }) and t.Cleanup(func() {
tfoutput.ResetOutputsCache() }) adjacent to the existing setup calls).
In `@internal/exec/yaml_func_utils_test.go`:
- Around line 237-239: Add cache resets into t.Cleanup so the test cleans up
shared state after it runs: call ResetStateCache() and
tfoutput.ResetOutputsCache() inside t.Cleanup to mirror the initial clears, or
if available prefer the exported helpers ResetPathMatchCache() and
ResetGlobMatchesCache() (and tfoutput.ResetOutputsCache()) in the t.Cleanup
closure; update the test to register those cleanup calls using t.Cleanup to
avoid leaking state between tests.
🪄 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: 1d7dbd71-ad4c-49df-99da-27d05894b96d
📒 Files selected for processing (9)
internal/exec/terraform_state_utils.gointernal/exec/yaml_func_terraform_output_test.gointernal/exec/yaml_func_terraform_state_test.gointernal/exec/yaml_func_utils_test.gointernal/terraform_backend/terraform_backend_local.gopkg/terraform/output/executor.gopkg/terraform/output/executor_test.gopkg/terraform/output/workspace.gopkg/terraform/output/workspace_test.go
…test isolation
- Fix cache reset race: ResetStateCache and ResetOutputsCache now use
Range+Delete to clear entries in place instead of reassigning
sync.Map{}, which races with concurrent Load/Store calls.
- Add t.Cleanup cache resets to yaml_func_terraform_output_test.go,
yaml_func_terraform_state_test.go, and yaml_func_utils_test.go for
bidirectional test isolation (clear before AND after each test).
- Guard WorkspaceSelect→WorkspaceNew fallback: only fall back to
WorkspaceNew when isWorkspaceMissingError confirms the workspace
doesn't exist. Other errors (auth, backend, permission) now fail
fast instead of silently creating empty workspaces that produce
wrong outputs. New isWorkspaceMissingError helper checks for
Terraform "doesn't exist" and OpenTofu "does not exist" patterns.
- Fix existing tests to use realistic Terraform error messages instead
of generic strings. Add negative-path test confirming non-missing
select errors (e.g. "permission denied") fail fast without triggering
WorkspaceNew.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/terraform/output/workspace.go`:
- Around line 126-132: The isWorkspaceMissingError helper is too broad (it
treats any error containing "does not exist"/"doesn't exist" as a missing
workspace), so restrict the matcher to workspace-specific messages (e.g.,
require patterns like "workspace .* does not exist", "workspace .* doesn't
exist", or the exact select/terraform output format such as `No workspace named
"<name>"`/`workspace "<name>" not found`) inside isWorkspaceMissingError to
avoid matching bucket/config errors; update the function to check for those
workspace-specific substrings or regexes and add a regression test that passes a
non-workspace error (e.g., "bucket does not exist") to ensure
isWorkspaceMissingError returns false and that WorkspaceNew does not run in that
case.
🪄 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: 8fc75db7-6121-4d80-9eea-d1f44eb8f29b
📒 Files selected for processing (7)
internal/exec/terraform_state_utils.gointernal/exec/yaml_func_terraform_output_test.gointernal/exec/yaml_func_terraform_state_test.gointernal/exec/yaml_func_utils_test.gopkg/terraform/output/executor.gopkg/terraform/output/workspace.gopkg/terraform/output/workspace_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/exec/yaml_func_utils_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/terraform/output/workspace_test.go
- Require "workspace" in the error message alongside "doesn't exist" / "does not exist" so non-workspace errors like "bucket does not exist" don't trigger WorkspaceNew fallback. - Fix executor_test.go: replace generic "workspace not found" with realistic Terraform error message matching the tightened guard. - Add regression tests for "bucket does not exist" and "resource does not exist" confirming they are NOT treated as missing-workspace errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
Apr 11, 2026
Contributor
|
These changes were released in v1.215.0-rc.7. |
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
OUTPUT_CHANGEbadge for output-only plans/applies, distinct from the grayNO_CHANGEbadge and the resource-levelCREATE/CHANGE/REPLACE/DESTROYbadgeswhy
HasChanges()method only checked resource counts, so output-only changes were incorrectly reported as "NO_CHANGE" in CI job summaries. This was misleading because Terraform considers these real changes that need to be applied.references
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation