command/jsonstate: fix inconsistency with resource address#24256
command/jsonstate: fix inconsistency with resource address#24256mildwonkey merged 3 commits intomasterfrom
Conversation
Resource addresses in state output were not including index for instances created with for_each or count, while the index was appearing in the plan output. This PR fixes that inconsistency, adds tests, and updates the existing tests. Fixes #24110
vancluever
left a comment
vancluever
left a comment
There was a problem hiding this comment.
@mildwonkey thanks for looking into this!
The only feedback I think I can offer as someone who is not diving into this code on a regular basis is maybe we could get some degree of static fixture testing here to express testing of the final product in "layman's terms"... ie: less meta-code in the tests and more actual JSON.
Otherwise I'm guessing this looks good and I'll probably see it when we update the test fixtures in the Sentinel items 🙂
|
@vancluever that makes sense! The end to end tests, which include config and expected json output, are in the command package: https://github.com/hashicorp/terraform/tree/master/command/testdata/show-json Does that cover your request? I can (and will) go add a few more complicated examples that include resources created with for_each and count - and let me know if there's anything else you think is missing from those examples. |
|
Yeah e2e would be the prefect place. Did these not need modifying in their current form? |
|
The e2e tests did not need updates because none of them used (non-empty) prior state, and the state output is where the non-instanced addressing appeared. (I am adding to that now) |
mildwonkey
force-pushed
the
mildwonkey/json-state-resource-addr
branch
from
a16a753 to
095bf22
Compare
|
exciting! Unfortunately I'm unable to test beyond the example in my report, because I just have a json file that reproduces the more complex issue. |
| "root_module": { | ||
| "resources": [ | ||
| { | ||
| "address": "test_instance.test[0]", |
There was a problem hiding this comment.
@mwhooker I believe this is the important test for you :) And thank you again for reporting the bug!
There was a problem hiding this comment.
this looks good! It looks like the code covers this case, but there's also the issue of modules getting lost. I wonder if that's worth capturing in a test? up to you
There was a problem hiding this comment.
the issue of modules getting lost
Sorry, that's not ringing any bells - is there a related GH issue?
There are e2e tests that cover modules in configuration, but not prior state. I'll add (at least) one, and if you can share the module issue I can tailor a test to that specifically.
Codecov Report
@@ Coverage Diff @@
## master #24256 +/- ##
=========================================
Coverage ? 57.33%
=========================================
Files ? 656
Lines ? 59835
Branches ? 0
=========================================
Hits ? 34304
Misses ? 22486
Partials ? 3045
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I think @mwhooker has validated the fix 🙂 thanks again for including these @mildwonkey!
alisdair
left a comment
alisdair
left a comment
There was a problem hiding this comment.
This changeset makes sense to me, and the new e2e tests explains its effect very clearly 👏
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
4 participants
Resource addresses in state output were not including index for
instances created with for_each or count, while the index was appearing
in the plan output. This PR fixes that inconsistency, adds tests, and
updates the existing tests.
Fixes #24110