Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

generate precise resource types during validate #29862

Merged
merged 2 commits into from Nov 3, 2021

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Nov 2, 2021

Allow GetResource to return correctly typed values during validation, rather than relying on cty.DynamicVal as a placeholder. This allows other dependent expressions to be more correctly evaluated.

There is no reason we must use the stored state to find the provider address for evaluation. We don't need the exact provider instance here, since we are only looking for the schema and the schema lookup is done with a simple provider type.

Once we have the resource type, we can return correctly shaped unknown values for validation based on the expansion mode of the resource configuration.

Fixes the specific case shown in #29809

Allow `GetResource` to return correct types values during validation,
rather than relying on `cty.DynamicVal` as a placeholder. This allows
other dependent expressions to be more correctly evaluated.
We only lookup providers by provider type to get the schema, so there's
no reason to generate anything more specific.
@jbardin jbardin requested a review from a team November 2, 2021 19:39
@jbardin jbardin self-assigned this Nov 2, 2021
// since the plan was not yet created during validate, the values we
// collected here may not correspond with configuration, so they must be
// unknown.
if d.Operation == walkValidate {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section was redundant with the default case before we checked the instance state, since there is no instance state during validation. Verified that this condition was no longer reached during tests.

@@ -1,4 +1,4 @@
resource "aws_instance" "web" {
foo = "${aws_instance.web.*.foo}"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

improved evaluation found a very old invalid test which was assigning a list(list(string)) to a list(string) (though that could have been flattened in the legacy type system, but was never caught post-0.12)

Copy link
Member

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

I'm assume that since this is only adding more information to validation there shouldn't be anything new this will catch that wouldn't have been subsequently caught during planning before. That therefore makes this consistent with our compatibility promises, where we promised compatibility only for configurations that could previously get through the full validate/plan/apply sequence without encountering any errors (because we always consider it an improvement to catch an existing problem in an earlier phase than we were able to before).

@jbardin jbardin added the 1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Nov 3, 2021
@jbardin jbardin merged commit 913f9cc into main Nov 3, 2021
@jbardin jbardin deleted the jbardin/validate-resource-values branch November 3, 2021 12:59
@github-actions
Copy link

github-actions bot commented Dec 4, 2021

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants