Issue 26320 - lookup(): only treat map as unknown if it is wholly unknown #26427
Conversation
There was a problem hiding this comment.
Hi @OwenTuz ! Thank you for working on this.
I really appreciate you adding test coverage, so please don't take this as criticism, just a request:
I would prefer to see test coverage in the lookup tests rather than the e2e tests - the e2e tests are primarily focused on the workflow and generally only run in circleCI, whereas the unit tests are something one can run locally and quickly.
Here's a test case that you could add to the lookup tests (in collection_test.go) which fails today and passes with your change:
{
[]cty.Value{
mapWithUnknowns,
cty.StringVal("foo"),
},
cty.StringVal("bar"),
false,
},
Thank you so much for submitting on this!
|
No problem at all, thanks for reviewing! I'm out today but will add that
tomorrow.
…On Thu, 1 Oct 2020, 14:06 Kristin Laemmert, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Hi @OwenTuz <https://github.com/OwenTuz> ! Thank you for working on this.
I really appreciate you adding test coverage, so please don't take this as
criticism, just a request:
I would prefer to see test coverage in the lookup tests rather than the
e2e tests - the e2e tests are primarily focused on the *workflow* and
generally only run in circleCI, whereas the unit tests are something one
can run locally and quickly.
Here's a test case that you could add to the lookup tests (in
collection_test.go) which fails today and passes with your change:
{
[]cty.Value{
mapWithUnknowns,
cty.StringVal("foo"),
},
cty.StringVal("bar"),
false,
},
Thank you so much for submitting on this!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26427 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABYSNPEULLV4EWN4AQXANETSIR5FJANCNFSM4R7A5YNA>
.
|
Fix for issue hashicorp#26320 - this allows us to derive known values from partially known maps where we can, and may prevent unnecessary destroy/rebuild cycles during apply in some cases.
OwenTuz
force-pushed
the
issue-26320-prevent-some-resources-incorrectly-being-labelled-unknown
branch
from
04cbaeb to
28cb0ab
Compare
Codecov Report
|
|
This is merged and the fix will be in Terraform v0.14 🎉 |
|
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 as resolved and limited conversation to collaborators
Fix for issue #26320 - this allows us to derive known values from partially known maps where we can, and may prevent unnecessary destroy/rebuild cycles during apply in some cases.
Based on @apparentlymart 's suggestions in the issue thread.
Most of this PR is changes to
primary_test.goandautomation_test.goas I have added a very basic regression test to the primary workflow. Happy to change or adjust this if you feel it would be cleaner/preferable to have a separate HCL file and test set for this.