null eval from output #19237
null eval from output #19237
Conversation
When replacing an instance, calculate a new proposed value from the null state and the config. This ensures that all unknown values are properly set.
The required value from an output is nil when it should be unknown
This allows us to see the results of the tests for all resources even if one of them fails.
Just as when we resolve single output values we must check to see if there is a planned new value for an output before using the value in state, because the planned new value might contain unknowns that can't be represented directly in the state (and would thus be incorrectly returned as null).
|
I found the cause of the issue here: the output change object in the plan was being written without an address, and so the evaluator couldn't find it. I found another problem along the way, which is that whole-resource references ( One of these changes seems to have regressed a few of the apply tests. I expect that these tests are not actually broken and were instead just relying on the implicit nullification of outputs, but I will investigate that more tomorrow. |
|
Nice! The new commits LGTM! |
|
This seems to have inverted the issue for some of the apply tests. One output is missing, and the others end up with unknown values after the fact. |
If plan and apply are both run against the same context then we still have the planned output values in memory while we're doing the apply walk, so we need to make sure to update them along with the state as we learn the final known values of each output. There were actually two different bugs here: - We weren't removing any existing planned change for an output when setting a new one. In retrospect a map would've been a better data structure for the output changes, rather than a slice to mimic what we do for resource instance objects, but for now we'll leave the structures alone and clean up as needed. (The set of outputs should be small for any reasonable configuration, so the main impact of this is some ugly code in RemoveOutputChange.) - RemoveOutputChange itself had a bug where it was iterating over the resource changes rather than the output changes. This didn't matter before because we weren't actually using that function, but now we are. This fix is confirmed by restoring various existing context apply tests back to passing again.
|
@jbardin in retrospect this is a bit weird because this was your PR but now I've pushed a bunch to it, but if this looks good to you then let me know and we can just go ahead and merge it even though technically GitHub won't let you approve your own PR. 😀 I pushed one more commit today to fix the last part of this problem, which was about updating the values in the changeset during apply. In retrospect the choice to use a slice for the output value changes (mimicking what we did for resources) doesn't seem like such a great choice right now but I chose to leave it alone both to minimize risk at this late stage and also thinking that our future changes to deal with outputs in a more first-class way during planning will cause us to revisit this soon enough. |
|
update LGTM, |
|
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
Failing minimal test for #19223
The issue is that the evaluation of the config (from
EvaluateBlock) returns aNullvalue for therequiredfield when referencing an attribute in a module output, rather thanUnknown.