Allow for nested fields with checkKey on ResourceDiffs. #18795
Conversation
When checking if a ResourceDiff key is valid, allow for keys that exist on sub-blocks. This allows things like diff.Clear to be called on sub-block fields, instead of just on top-level fields.
When using instance templates, if you use one of our image shorthands at the moment, you'll get a perma-diff. This is because the config gets resolved to another format before it is set in state, and so we need to set that other format in state. Unfortunately, resolving images requires network access, so we have to do this with CustomizeDiff. CustomizeDiff was having trouble (I think? More on this below) on setting a field as not ForceNew once the field was already set, so I moved the decision for whether a field was ForceNew or not into CustomizeDiff. I also resolved the old and new images, and if they were the same, cleared the diff. Unfortunately, you can't actually clear a field on a sub-block right now. You have to clear top-level fields only. So this will currently throw an error. I opened hashicorp/terraform#18795 to fix that. Once that's merged, and we vendor it here, this patch fixes the problem. If hashicorp/terraform#18795 doesn't get merged, the next best workaround is to keep track of _all_ the fields under `disk` with a diff in our CustomizeDiff, check whether they've all changed or not, and if they've all changed, clear the changes on `disk`, which I _think_ will resolve the issue. That's just a massive pain, unfortunately.
There was a problem hiding this comment.
This seems reasonable to me. I was trying to remember if there was a specific reason why we limited this only to top-level attributes but I can't think of one.
I'd like to also have a specific test for this to make sure we don't break it in future SDK updates (particularly once we start moving away from the internal flatmap representation of diffs), though I'm not sure what place is best to test it. It looks like there's a very basic CustomizeDiff test in resource_test.go, which could maybe be the basis for a new test that has a schema with a nested Resource and sets a value for it during customization?
|
@apparentlymart I added a few test cases to the existing tests for ResourceDiff.Clear (which calls into the checkKey helper I modified in this). Does that work as a testing strategy? |
You can't set individual items in lists, you can only set the list as a whole, so we shouldn't allow sub-blocks to SetNew or SetNewComputed.
|
New commit pushed. The executive summary is that we don't allow |
|
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
When checking if a ResourceDiff key is valid, allow for keys that exist
on sub-blocks. This allows things like diff.Clear to be called on
sub-block fields, instead of just on top-level fields.