fix(output): stop after-* hooks from corrupting backend.tf.json when backend uses !terraform.state#2358
Merged
aknysh merged 9 commits intocloudposse:mainfrom Apr 23, 2026
Conversation
…ions through execute() Prep for cloudposse#2356 fix. Adds WithBackendGenerator option so tests can assert artifact-regeneration behavior, and adds a processYamlFunctions parameter to execute() plumbed through all four internal call sites. No behavior change — all current callers pass values equivalent to the old behavior.
…processed Guards Step 4/Step 5 of execute() behind the new processYamlFunctions flag. When after-* hooks read outputs with SkipInit=true && authManager==nil, DescribeComponent is intentionally called with ProcessYamlFunctions=false, which leaves literal '!terraform.state ...' strings in config.Backend and config.Providers. Regenerating backend.tf.json / providers_override.tf.json from those un-rendered sections overwrote a correctly-rendered file from the init/apply phase with a broken one, causing subsequent tofu output to fail with 'Backend configuration block has changed'. Fixes cloudposse#2356.
…ns in SkipInit tests Strengthens the four SkipInit tests to explicitly verify that GenerateBackendIfNeeded and GenerateProvidersIfNeeded are called iff YAML functions were processed. Locks in the cloudposse#2356 fix and the opposite invariant (auth present -> regen still happens).
…ruption Integration-level assertion: a pre-written backend.tf.json remains byte-identical on disk after GetOutputWithOptions(SkipInit=true) runs against a component whose sections contain literal YAML-function strings. Fails without the execute() guard; passes with it.
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. |
Contributor
📝 WalkthroughWalkthroughExecutor.execute now accepts a new processYamlFunctions boolean and will only invoke backend/provider regeneration when that flag is true. Executor gained an injectable BackendGenerator and WithBackendGenerator option; call sites propagate the flag. Tests and generated mocks updated to cover the new behavior and regression. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Executor
participant BackendGenerator
participant FS
Caller->>Executor: GetOutputWithOptions(..., SkipInit, ProcessYamlFunctions)
Executor->>Executor: determine processYamlFunctions
alt processYamlFunctions == true
Executor->>BackendGenerator: GenerateBackendIfNeeded(config, comp, stack, auth)
BackendGenerator->>FS: write backend.tf.json / providers_override.tf.json
BackendGenerator-->>Executor: success / error
Executor->>BackendGenerator: GenerateProvidersIfNeeded(config, auth)
BackendGenerator->>FS: write providers_override.tf.json
BackendGenerator-->>Executor: success / error
else processYamlFunctions == false
Executor-->>Executor: skip backend/provider regeneration
end
Executor->>FS: terraform init/apply or read cached outputs
Executor-->>Caller: outputs or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/terraform/output/executor_regression_test.go (1)
86-88: Assert the executor used the backend file’s workdir.The test compares
backendPath, but the runner factory ignoresworkdir. Capture it and assert it equalscomponentDir; otherwise a path-resolution change could make this test pass while checking the wrong file.Test hardening
+ var capturedWorkdir string customFactory := func(workdir, executable string) (TerraformRunner, error) { + capturedWorkdir = workdir return mockRunner, nil } @@ require.NoError(t, err) + require.Equal(t, componentDir, capturedWorkdir, + "regression test must exercise the same workdir as the backend file under assertion") // Assert the file on disk is byte-identical.Also applies to: 133-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/terraform/output/executor_regression_test.go` around lines 86 - 88, The test's customFactory closure currently ignores its workdir parameter so the test never verifies the executor used the backend file's workdir; update the closure used for Terraform runner factory (customFactory) to capture the incoming workdir argument (e.g., assign to a local variable like capturedWorkdir) and add an assertion that capturedWorkdir == componentDir (or use testify/require as used elsewhere) to ensure the factory is called with the backend file's directory; repeat the same change for the other factory instance referenced in the test (the one around the 133-139 range) so both factories verify the workdir parameter instead of ignoring it.
🤖 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/terraform/output/executor_regression_test.go`:
- Around line 86-88: The test's customFactory closure currently ignores its
workdir parameter so the test never verifies the executor used the backend
file's workdir; update the closure used for Terraform runner factory
(customFactory) to capture the incoming workdir argument (e.g., assign to a
local variable like capturedWorkdir) and add an assertion that capturedWorkdir
== componentDir (or use testify/require as used elsewhere) to ensure the factory
is called with the backend file's directory; repeat the same change for the
other factory instance referenced in the test (the one around the 133-139 range)
so both factories verify the workdir parameter instead of ignoring it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f18a6d16-ab46-4781-8fc2-08c1682c0c16
📒 Files selected for processing (4)
pkg/terraform/output/executor.gopkg/terraform/output/executor_regression_test.gopkg/terraform/output/executor_test.gopkg/terraform/output/mock_executor_test.go
The custom-gcl linter flagged TestExecutor_Execute_SkipsArtifactRegen_WhenYamlFunctionsNotProcessed as structurally duplicate of TestExecutor_Execute_SkipInit_DirectCall. The two tests are intentionally parallel — they assert the processYamlFunctions=true vs =false invariants at the same execute() call site — so sharing scaffolding via a helper would obscure the red/green contrast this PR is establishing. No behavior change.
…oviders regen - TestExecutor_Execute_PropagatesBackendGenError: asserts GenerateBackendIfNeeded errors propagate unchanged out of execute() in the processYamlFunctions=true branch. Locks in the only newly-reachable error path introduced by the guard. - TestExecutor_Execute_PropagatesProvidersGenError: symmetric coverage for GenerateProvidersIfNeeded, and also confirms execute() short-circuits on the first generator error (no downstream runner calls). - TestExecutor_Regression_Issue2356_...: extended to also pre-write a rendered providers_override.tf.json, add a providers section with literal '!terraform.state ...' strings, and assert the file is byte-identical after the SkipInit path. Manual revert-check confirms both backend and providers assertions fire if the guard is removed. Addresses review findings on error-path and providers coverage gaps.
…ator contract Addresses review feedback on the cloudposse#2356 fix: - Expand the Step 4/5 guard comment to explain *why* execute() regenerates artifacts in the first place (static remote state, cross-stack !terraform.output, auth-present output fetch). Prevents a future reader from concluding regen is redundant with apply and deleting it. - Add log.Debug when the guard skips regeneration, so operators debugging hook-related issues can distinguish 'skipped by design (cloudposse#2356)' from 'never reached this code path'. - WithBackendGenerator godoc now states explicitly that BackendGenerator is an internal test seam, not a stable external extension point. - ExecuteWithSections gains a caller contract docblock: the sections must have ProcessYamlFunctions-evaluated values; literal YAML-function strings would bypass the execute() guard (which runs with processYamlFunctions=true on this path) and cause the same corruption cloudposse#2356 fixed. No behavior change — same guarded flow, same public API surface.
CodeRabbit flagged: the customFactory closure was discarding its workdir parameter, so if upstream path resolution ever changed the byte-compare could pass vacuously against an untouched file while the real generator wrote elsewhere. Capture the workdir the executor asks for and require it equals componentDir before asserting on file contents.
Contributor
Author
|
Thanks @coderabbitai — addressed in ddc1653. The Note: there's only one |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
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)
pkg/terraform/output/executor_test.go (1)
1318-1375:⚠️ Potential issue | 🟡 MinorThe
fakeAuthManager := "fake-auth-manager"string will fail the type assertion infetchAndCacheOutputsbeforeDescribeComponentis reached.
GetAllOutputscallsfetchAndCacheOutputs, which validates that authManager is a realauth.AuthManager(lines 204–209 of executor.go). The test will returnErrInvalidAuthManagerTypeearly, never reaching the DescribeComponent call it's meant to exercise.Replace the string with a mock
auth.AuthManager(useNewMockAuthManager(ctrl)or equivalent) so the test exercises the actual YAML-function processing path it claims to verify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/terraform/output/executor_test.go` around lines 1318 - 1375, The test uses a plain string for fakeAuthManager which triggers the auth type check in fetchAndCacheOutputs and returns ErrInvalidAuthManagerType before DescribeComponent runs; replace fakeAuthManager := "fake-auth-manager" with a proper mock implementing auth.AuthManager (e.g. fakeAuthManager := NewMockAuthManager(ctrl)) so GetAllOutputs calls into fetchAndCacheOutputs with a valid auth.AuthManager and the DescribeComponent path (params.ProcessYamlFunctions) is exercised as intended.
🧹 Nitpick comments (1)
pkg/terraform/output/executor_regression_test.go (1)
188-221: Good helper — the inline comments explaining why the literal!terraform.state ...strings matter (Line 197-201, 208-211) turn this from "test data" into documentation of the bug's surface area.One minor nit: the helper is unexported and only used by the one regression test in this file. If more
#2356-adjacentregression tests land later (e.g. for!terraform.outputor other auth-backed functions), consider parameterizing the YAML-function name so each scenario gets its own canonical fixture without copy-paste. Not a blocker for this PR — just flagging for the follow-up tracked in#2357.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/terraform/output/executor_regression_test.go` around lines 188 - 221, The helper regressionSectionsWithLiteralYamlFunctions should be parameterized to accept the YAML-function identifier (e.g. yamlFunc string) instead of hardcoding "!terraform.state" so tests can reuse it for other YAML functions; update the function signature regressionSectionsWithLiteralYamlFunctions(yamlFunc string) and replace the inline literals (used by GenerateBackendIfNeeded and GenerateProvidersIfNeeded test scenarios) with formatted strings that prepend "!" + yamlFunc (e.g. "!"+yamlFunc+" ..."), and update the single regression test call to pass "terraform.state" (future tests can pass "terraform.output" etc.).
🤖 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 `@pkg/terraform/output/executor_test.go`:
- Around line 1318-1375: The test uses a plain string for fakeAuthManager which
triggers the auth type check in fetchAndCacheOutputs and returns
ErrInvalidAuthManagerType before DescribeComponent runs; replace fakeAuthManager
:= "fake-auth-manager" with a proper mock implementing auth.AuthManager (e.g.
fakeAuthManager := NewMockAuthManager(ctrl)) so GetAllOutputs calls into
fetchAndCacheOutputs with a valid auth.AuthManager and the DescribeComponent
path (params.ProcessYamlFunctions) is exercised as intended.
---
Nitpick comments:
In `@pkg/terraform/output/executor_regression_test.go`:
- Around line 188-221: The helper regressionSectionsWithLiteralYamlFunctions
should be parameterized to accept the YAML-function identifier (e.g. yamlFunc
string) instead of hardcoding "!terraform.state" so tests can reuse it for other
YAML functions; update the function signature
regressionSectionsWithLiteralYamlFunctions(yamlFunc string) and replace the
inline literals (used by GenerateBackendIfNeeded and GenerateProvidersIfNeeded
test scenarios) with formatted strings that prepend "!" + yamlFunc (e.g.
"!"+yamlFunc+" ..."), and update the single regression test call to pass
"terraform.state" (future tests can pass "terraform.output" etc.).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b4d32ab-f310-4502-bc98-cf3950830ee9
📒 Files selected for processing (3)
pkg/terraform/output/executor.gopkg/terraform/output/executor_regression_test.gopkg/terraform/output/executor_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/terraform/output/executor.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2358 +/- ##
==========================================
- Coverage 77.35% 76.95% -0.41%
==========================================
Files 1089 1089
Lines 102828 135605 +32777
==========================================
+ Hits 79547 104354 +24807
- Misses 18909 26880 +7971
+ Partials 4372 4371 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
gofumpt moved //nolint:dupl below its intended site and left an orphan 'by design —' lowercase sentence that godot flagged. Restructure so the nolint directive sits directly above the func and the explanatory comment reads as a complete sentence.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/terraform/output/executor_test.go (2)
1326-1374: Use a typed auth manager for this regression test.This now asserts the auth-present path, but
fakeAuthManageris astring. That only proves “non-nil flips the flag” and conflicts with nearby invalid-auth tests. Prefer a real/mockauth.AuthManagerso the test covers the path users can actually hit. As per coding guidelines, “Test behavior, not implementation. Never test stub functions. Avoid tautological tests.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/terraform/output/executor_test.go` around lines 1326 - 1374, The test uses a string as fakeAuthManager which only verifies non-nil behavior; replace it with a real typed auth.AuthManager mock (e.g., create mockAuthMgr := mock_auth.NewMockAuthManager(ctrl) or a concrete auth.AuthManager test double) and pass that into exec.GetAllOutputs so the DescribeComponent expectation asserts the actual typed manager; update the DescribeComponent DoAndReturn to assert params.AuthManager equals mockAuthMgr and adjust any imports/setup to use the mock auth package and controller.
1479-1484: Fix the duplicate-test rationale.This says the duplicate tests contrast
processYamlFunctions=truevsfalse, but this test andTestExecutor_Execute_SkipInit_DirectCallboth passfalse. Please update the comment to point at the auth-present/generator-called coverage, or remove the duplicate if it no longer adds a separate invariant.Suggested wording
-// design — the two tests contrast the processYamlFunctions=true vs =false -// invariants at the same execute() call site, so sharing scaffolding via a -// helper would obscure the side-by-side comparison. +// design — this test isolates the processYamlFunctions=false invariant at +// execute(), while the auth-present tests cover the processYamlFunctions=true +// regeneration path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/terraform/output/executor_test.go` around lines 1479 - 1484, The duplicate-test rationale is incorrect: both this test and TestExecutor_Execute_SkipInit_DirectCall pass processYamlFunctions=false, so update the comment above this test to accurately explain what the test adds (e.g., it covers the "auth present / generator called" behavior difference versus the other test) or if the test is redundant remove it; specifically edit the comment that references processYamlFunctions to instead reference the auth/generator invariant (or delete the duplicated test) and keep the //nolint:dupl line only if you retain intentional duplication for that corrected rationale.
🤖 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/terraform/output/executor_test.go`:
- Around line 1326-1374: The test uses a string as fakeAuthManager which only
verifies non-nil behavior; replace it with a real typed auth.AuthManager mock
(e.g., create mockAuthMgr := mock_auth.NewMockAuthManager(ctrl) or a concrete
auth.AuthManager test double) and pass that into exec.GetAllOutputs so the
DescribeComponent expectation asserts the actual typed manager; update the
DescribeComponent DoAndReturn to assert params.AuthManager equals mockAuthMgr
and adjust any imports/setup to use the mock auth package and controller.
- Around line 1479-1484: The duplicate-test rationale is incorrect: both this
test and TestExecutor_Execute_SkipInit_DirectCall pass
processYamlFunctions=false, so update the comment above this test to accurately
explain what the test adds (e.g., it covers the "auth present / generator
called" behavior difference versus the other test) or if the test is redundant
remove it; specifically edit the comment that references processYamlFunctions to
instead reference the auth/generator invariant (or delete the duplicated test)
and keep the //nolint:dupl line only if you retain intentional duplication for
that corrected rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e011343-5301-49f2-bfd6-790a7e5cc115
📒 Files selected for processing (1)
pkg/terraform/output/executor_test.go
aknysh
approved these changes
Apr 23, 2026
Contributor
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.
Summary
Fixes #2356. The
after-terraform-applystore hook path regeneratedbackend.tf.json/providers_override.tf.jsonfrom un-renderedcomponent sections when the backend referenced
!terraform.state,overwriting a correctly-rendered file with literal YAML-function strings:
The hook then failed its
tofu outputcall with:Why
Regression introduced in v1.216.0 by #2309 (commit
3c0e748ce) +follow-up commit
c7ef142a9("fix: skip-init should skip yaml functionevaluation").
c7ef142a9added a guard disabling YAML-functionevaluation when
SkipInit && authManager == nilto avoid failing onauth-requiring functions in the post-hook context. The guard is overly
broad — it also disables evaluation of non-auth functions like
!terraform.state— so sections returned fromDescribeComponentretainliteral YAML-function strings.
execute()then extractsconfig.Backendfrom those sections and writes them to disk via
GenerateBackendIfNeeded.Fix
Thread
processYamlFunctions boolthroughexecute()inpkg/terraform/output/executor.goand guard the artifact-regenerationblock (Step 4 / Step 5) behind it. When YAML functions were not
evaluated upstream,
execute()must not regenerate artifacts from theun-rendered sections. The backend file on disk from the init/apply phase
is already correct; leaving it alone is always safe. Output reading
(
tofu output) still works via the on-disk state.Minimal, localized diff — four commits:
refactor(output): inject BackendGenerator and thread processYamlFunctions through execute()— pure DI plumbing, no behavior change.fix(output): skip artifact regeneration when YAML functions were not processed— the actual guard.test(output): assert backend-generator calls match processYamlFunctions in SkipInit tests— locks in the invariant in four existing SkipInit tests.test(output): regression test for #2356 backend.tf.json corruption— byte-identical integration assertion.Test plan
TestExecutor_Execute_SkipsArtifactRegen_WhenYamlFunctionsNotProcessed(demonstrably red before the guard, green after).TestExecutor_GetAllOutputs_SkipInit_WithAuthManager_ProcessesYamlFunctions:GenerateBackendIfNeeded+GenerateProvidersIfNeededcalled exactly once when auth is present.TestExecutor_Regression_Issue2356_BackendFileUnchangedInSkipInitPath: writes a renderedbackend.tf.json, drivesGetOutputWithOptions(SkipInit=true, authManager=nil), asserts the file is byte-identical. Fails without the guard; passes with it.go test ./pkg/terraform/output/... -count=1green.make lint/./custom-gcl run --new-from-rev=origin/mainclean (oneduplwarning on the new test vs the existing SkipInit test is suppressed with//nolint:dupl+ justification — they test contrasting invariants at the same call site; extracting shared scaffolding would obscure the red/green comparison).ignore/issues/post-apply-hook-backend-racecondition/repro.shin the branch, referenced from after-terraform-apply hook corrupts backend.tf.json when backend uses !terraform.state #2356). Exits 0 withFIX VERIFIEDon this branch; backend file byte-diff is empty after the after-apply hook.Follow-up
The
processYamlFunctions = falseguard inGetOutputWithOptions/fetchAndCacheOutputsis the deeper design issue — auth availabilityshould not gate evaluation of non-auth YAML functions. Tracked in #2357.
This PR is the minimal regression fix for v1.216.x.
Release
fix:conventional commit → patch release (v1.216.1).Summary by CodeRabbit
Bug Fixes
Tests