Remove invalid warning during cleanup phase #35172
Conversation
There was a problem hiding this comment.
Hi @MicahKimel, thanks for this.
The overall approach looks good to me - I'd just replace the new function argument with a variable instead of the string so we can more easily document what behaviour is being changed with the argument.
Also, would you be able to add a small test for this? The testing framework is mostly testing in command/test_test.go. Here's an example of a test that runs terraform init and then terraform test and validates the output from the test command matches what we expect: https://github.com/hashicorp/terraform/blob/main/internal/command/test_test.go#L1955
You can add new test data in command/testdata/test, essentially we'd just need a very simple test case that references a variable that doesn't exist. Then validate the output produces the warning during the initial run and not during the cleanup phase.
Hopefully that makes sense! Let me know if you don't have time or it doesn't make sense. We don't need a big complicated test for this, so hopefully shouldn't be too difficult. Thanks again.
|
Thanks @liamcervante!! Updated solution and added test case! |
internal/command/testdata/test/invalid-cleanup-warnings/main.tftest.hcl
Outdated
Show resolved
Hide resolved
…he validation variable before cleanup phase
|
Thanks! How does this look |
There was a problem hiding this comment.
Your changes look perfect, but the test you've written has actually exposed another bug. Specifically around the line (and one more similar warning elsewhere). We should only be adding a single diagnostic here, and not two.
This is happening because of these lines: https://github.com/hashicorp/terraform/blob/main/internal/backend/local/test.go#L547-L551. You can see we are just needlessly re-adding and rechecking the original variableDiags into our main diagnostics which is causing the duplicates.
This isn't something caused by any changes in your PR but is another existing bug. Could you just delete those lines and update the new test you've added so it's not reporting it has found similar errors? This request is out of scope of the original bug, but would be nice to not add a test that is revealing another bug when it has such a simple fix.
If you don't have time we can file another issue tracking this and someone else can pick it up later - as I said it's out of scope for what you originally agreed to fix so we can merge this fix as is. Let me know what you want to do and I can approve or wait for the update as you want!
|
Thanks! Removed those lines and updated the test case. Thank you for me so much with this! This is my first time contributing to an open source project and I've really appreciated your help and feedback! If this looks good, I'll have to go find a new issue to work on! :) |
liamcervante
added
1.8-backport
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
* stop invalid warning during cleanup phase * stop invalid warning during cleanup phase * invalid warning during cleanup phase * remove unwanted comment * update GetVariables parameter for including warnings * invalid warning during cleanup phase test case * update invalid warnings in cleanup test case to throw a warning for the validation variable before cleanup phase * remove unwanted warnings
* stop invalid warning during cleanup phase * stop invalid warning during cleanup phase * invalid warning during cleanup phase * remove unwanted comment * update GetVariables parameter for including warnings * invalid warning during cleanup phase test case * update invalid warnings in cleanup test case to throw a warning for the validation variable before cleanup phase * remove unwanted warnings Co-authored-by: MicahKimel <[email protected]>
|
This has been merged and backported. It will be released as part of v1.8.5. |
Fixes #35061
Target Release
1.8.x
Draft CHANGELOG entry
NEW FEATURES | UPGRADE NOTES | ENHANCEMENTS | BUG FIXES | EXPERIMENTS
actioninto the GetVariables() function. This seemed to be the easiest way I could think of to know that theGetVariables()is being called byDestroy(). Let me know any feedback!