Keep distinct Fn::GetStackOutput values distinct - #259
Open
owevertonguedes wants to merge 1 commit into
Open
Conversation
Every Fn::GetStackOutput resolved to the same constant
ResolvedValue::Dynamic { reason: "cross-stack output" }, so an array
holding several reads of different stack outputs looked like a list of
identical values and tripped the duplicate-values warning.
Carry the arguments that identify which output is read (StackName,
Region, OutputName) into the resolved value, the way the ImportValue arm
already folds in the export name. Only arguments that resolve to a
concrete string contribute; when none does, the reason stays the plain
constant. The parts are emitted in a fixed key order so the same call
written with its arguments in a different order stays equal, and each
value is quoted so a separator inside a value cannot read as a field
boundary.
RoleArn is left out on purpose: two calls differing only there still
name the same output of the same stack, so they are the same value and
should still be reported as a duplicate.
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
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.
Closes #247
The cause
The
GetStackOutputarm insrc/template-model/src/resolver.rsresolved its arguments only to keepcurrent_pathin step, discarded the results withlet _ = ..., and returned a constantResolvedValue::Dynamic { reason: "cross-stack output" }.StackName,RegionandOutputNamenever reached the resolved value, so every call in a template compared equal and W9007 saw an array of identical entries.The change
The arm now mirrors the
ImportValuearm directly above it and folds the arguments that identify which output is read into the resolved value. Three details are deliberate:"cross-stack output"constant, the same fallbackImportValueuses for a non-concrete argument.resolve_get_stack_output_distinct_sources_differcovers this.Measured, before and after
Both engines, on the template from the issue and on the two boundary cases below, counting W9007 diagnostics at
--level debug:mainregomaincelRefOutputNameRegionand one omitting itTwo scope decisions, both separable
RoleArnis excluded from the identity. The issue namesStackName+Region+OutputName, and I kept to that. Two calls that differ only inRoleArnstill name the same output of the same stack, so treating them as distinct would suppress a duplicate W9007 should report.resolve_get_stack_output_distinct_sources_differpins that decision, so if you wantRoleArnin, it is a one-line change plus that assertion.I kept
ResolvedValue::Dynamicrather than switching toTypedDynamic. The issue suggests consideringTypedDynamicwithPARAM_TYPE_STRINGfor parity withImportValue. I did not, becausedescribe_resolutioninsrc/schema-validator/src/validate.rsrendersTypedDynamicasformat!("parameter '{}' (type {})", name, typ), so the diagnostic'sresolutionSourcewould readparameter 'cross-stack output: StackName="VpcStack", ...' (type String), which is wrong for a cross-stack read. Happy to make the switch if you would rather change that rendering too.Two limitations I did not fix
Both are visible in the table above and I would rather state them than have you find them.
An identifying argument that is itself deploy-time cannot contribute. Two reads whose
OutputNamecomes from differentRefs still collapse and still fire W9007. This is not a regression,mainfires there too, and it is the same behaviourImportValuehas with a non-concrete argument. Closing it would mean putting a symbolic description of an unresolved value into a user-visible string, which felt like a separate decision rather than part of this fix.An omitted
Regionno longer matches an explicit one. Onmainthese two entries collapsed and W9007 fired, which happened to be right when the deployment region is the one written out. On this branch they stay distinct and W9007 is silent. I left it that way on purpose: the engine cannot know the deployment region, so it cannot show that the two entries read the same output. If you would rather have the absentRegionnormalised to the assumed region, that is a small follow-up, but it puts a region the template never wrote intoresolutionSourcefor every such call.Tests
src/resources/templates/gh-issues/issue-247.jsonholds the issue's own snippet, following theissue-52.jsonprecedent.issue_247_no_w9007_on_distinct_getstackoutput,issue_247_each_identifying_argument_distinguishes_on_its_own(the three rows of the probe table in the issue,StackName,RegionandOutputNameeach on its own), andissue_247_w9007_still_fires_on_repeated_getstackoutputas the positive boundary.resolve_get_stack_output_carries_source_identityandresolve_get_stack_output_distinct_sources_differat the resolver level, matching the pair that already exists forImportValue.Reverting the production arm to the constant reason fails both resolver tests and two of the three integration tests.
issue_247_w9007_still_fires_on_repeated_getstackoutputpasses either way by construction, exactly as itsissue_52counterpart does, since the old constant reason also made identical calls compare equal.Checks
cargo test --workspacepasses: 44 test binaries, 1801 tests, 0 failures.cargo fmt --allandcargo clippy --locked --all-targets --workspace -- -D warningsare clean, andgit diff --checkis empty. The golden was regenerated withcargo run --release -p resources --example generate_golden, which reports 595 templates andEngine parity verified: rego == cel on all 595 templates. Its only content change outside the new fixture is oneresolutionSourcestring that now carries the stack, region and output name.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.