core: move import validation into the validate graph #35543
Conversation
Import blocks were originally static and had no use for dynamic evaluation. Once we added dynamic IDs and for_each expansion however, validation should have moved into the validation graph for evaluation.
Have the resource validation node check if config generation is possible when there is no config given. We can't check the output path here because that is a plan option, so a final suggestion to use -generate-config-out will have to wait until plan.
This functionality was never developed further, no need to leave the comment here.
Remove the redundant validation from the config transformers, making sure most of the validation is now done by NodeValidatableResource. Inset Validate calls into import tests to ensure both that existing errors are still found, and that existing plans don't fail the new validation code.
| } | ||
| } | ||
| */ | ||
| // BUG(paddy): we're not validating provider_meta blocks on EvalValidate right now |
There was a problem hiding this comment.
ProviderMetas are not being developed further, and we can removed these large block comments
| @@ -28,11 +28,6 @@ func evaluateImportIdExpression(expr hcl.Expression, target addrs.AbsResourceIns | |||
| }) | |||
| } | |||
|
|
|||
| diags = diags.Append(validateImportSelfRef(target.Resource.Resource, expr)) | |||
There was a problem hiding this comment.
validateImportSelfRef is now being done during validation, so we can remove the call along with the extra target argument
| if diags.HasErrors() { | ||
| return diags | ||
| } | ||
| diags = diags.Append(n.validateImportTargetExpansion(to, imp.Config.To)) |
There was a problem hiding this comment.
is it worth also adding this new validation to the equivalent part of the plan? It only matters in situations where the expansion is unknown during the validation, and I guess we might get a different error message about the resource being missing or something.
Just wondering if it's worth being consistent?
There was a problem hiding this comment.
That's a good point, the plan error will just say the resource is missing, and these errors are probably more helpful. I initially wanted to do this all statically (which I think is nearly possible except when the use has any typed variables involved) but carrying the type info forward to determine what each.key is is not so easy.
There was a problem hiding this comment.
We can often still get the type of the for_each expression when it's unknown, and we have the To traversal. It would be easy to validate if the To expression only allowed indexing with each.key but in reality it can use anything from each, so we need to find and extract the resource index key expression and apply that path to the for_each type to determine if we have a numeric or string index. We're so close to being able to statically verify this, but the required code just felt too big and cumbersome to throw in for these tiny edge cases and still not cover the cases where the type is unknown.
Instead added the same validation checks into the plan node too for the better error messages.
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
When import blocks were first introduced they had to be fully static, which allowed them to be validated during the configuration loading process. This early static validation was carried forward as more features were introduced, but that led to some validation in awkward places like the graph builder, and some validation which could not be done like validating the
toexpression.Moving the import validation into the validate graph allows us to more fully validate the block contents, in the same way as normal resource configuration and mostly unifies where validation happens. We still have a couple outliers due to the possibility of config generation, which requires some checks in the config transformer for entirely missing modules when the resource has no config, and the plan node to suggest config generation if the resource has no configuration.
Now that
Validatehandles most of the actual validation, I insertedValidatecalls before the importPlancalls in tests, or replace thePlancalls entirely when it was clear that the desired diagnostics had moved intoValidate. This was to leverage the existing tests both for the new code, and to verify there are no test regressions sinceValidate` would always be called during a plan from the command line.Fixes #35516