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

Allow providers to re-validate the final resource config #21555

Merged
merged 3 commits into from
Jun 7, 2019

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jun 1, 2019

The provider's validate methods are called along with the static config validation early on, but will contain many unknown values that can't be checked for content.

By re-running the provider validate functions before plan (and read for data sources), the provider gets a chance to see the complete config, and return diagnostics about any new values which may not be valid.

Fixes #21225.
Fixes #21436, which was partially fixed by an earlier commit that prevented unknown values from going through validation, but as a side effect prevented their validation altogether.
This also closes #21315, since it will allow the provider to reject unexpected null values that would have otherwise been skipped due to being unknown during Validate.

A dynamic block is always going to be a single value during validation,
but should not fail when min-items > 1 is specified.
The config is statically validated early on for structural issues, but
the provider can't validate any inputs that were unknown at the time.
Run ValidateResourceTypeConfig during Plan, so that the provider can
validate the final config values, including those interpolated from
other resources.
Just like resources, early data soruce validation will contain many
unknown values. Re-validate once we have the config config.
@jbardin jbardin requested a review from a team June 1, 2019 16:53
Copy link
Contributor

@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.

Looks good to me!

I think this was a pre-existing bug prior to Terraform v0.12 too, right? If that is true, my instinct would be to hold this until after v0.12.1 is out just to keep that release limited only to v0.12.0 regression fixes, and then land this shortly afterwards.

@jbardin
Copy link
Member Author

jbardin commented Jun 1, 2019

The primary motivation for this was completing #21436, which is on the priority list, it just turned out that it was finished by the same fix as the others. The current SDK will now skip a lot more early validation (from #21443), so this also better maintains validation once providers update.

I don't think this is critical if there is some concern about the extra validation call however, so we can review this prior to release.

@apparentlymart
Copy link
Contributor

I think I'd originally flagged #21436 because dynamic blocks are the upgrade path for the previous (unreliable) workaround of treating block type names as attributes and so it would be effectively a regression (even though dynamic blocks are a new feature) if there were any situation where assigning an unknown list value to a block type (using attribute syntax) would've worked in Terraform 0.11.

However, thinking about this again freshly I'm not sure that's actually true: one of the reasons why the "treat block type as attribute" workaround wasn't reliable was that if you assigned an unknown list to it then it would return "should be a list". While that's not the same error about the MinItems violation, switching from one error message to another is not really a regression in real terms.

With that said, my previous remark was coming from an abundance of caution about keeping the 0.12.1 fix as focused as possible on very targeted bug fixes. This fix isn't so "targeted" in that it also addresses some pre-existing bugs, and thus risks unintended consequences, but if I'm wrong above and there are regressions behind #21436 then I expect including this in 0.12.1 wouldn't be the end of the world.

@jbardin
Copy link
Member Author

jbardin commented Jun 3, 2019

If you also don't see any pressing need, this can certainly be in a follow up release. The current SDK won't block the config from working, so any improvements can quickly be added in core.

@ghost
Copy link

ghost commented Jul 25, 2019

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 ghost locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants