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

schema: fix unknown value validation bug #1047

Merged
merged 3 commits into from Sep 2, 2022

Conversation

kmoe
Copy link
Member

@kmoe kmoe commented Sep 2, 2022

See hashicorp/terraform#31730

The validate function should exit early in the case that a value is not wholly known. This is the intent of the comment and earlier code in this file - however, subsequent refactors introduced a bug in which one aspect of validation was run for unknown values: deprecation warnings.

Deprecation warnings for known values are generated by validateType lower down.

This fix ensures that deprecation warnings are not shown for unknown values, while still ensuring we exit validation early for all unknown values.

The validate function should exit early in the case that a value is not
wholly known. This is the intent of the comment and earlier code in this
file - however, subsequent refactors introduced a bug in which one
aspect of validation was run for unknown values: deprecation warnings.

Deprecation warnings for known values are generated by validateType
lower down.

This fix ensures that deprecation warnings are not shown for unknown
values, while still ensuring we exit validation early for all unknown
values.
@kmoe
Copy link
Member Author

kmoe commented Sep 2, 2022

Test fail is due to the content type issue fixed in #1045.

@bflad bflad added the bug Something isn't working label Sep 2, 2022
@bflad bflad added this to the v2.22.0 milestone Sep 2, 2022
bflad
bflad approved these changes Sep 2, 2022
Copy link
Member

@bflad bflad 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, @kmoe 🚀 Thanks so much for fixing this! Double checked that indeed there is deprecation warning handling later on and covering unit testing of null vs known values for it.

@bflad bflad merged commit 1288887 into hashicorp:main Sep 2, 2022
5 checks passed
bflad added a commit that referenced this pull request Sep 2, 2022
@bflad
Copy link
Member

bflad commented Sep 2, 2022

For what it's worth, folks will no longer have a feedback mechanism about setting these to unknown -> null before the attribute is fully removed, resulting in a configuration breaking change for them. I assume you folks think that tradeoff is worth it, which is why I opted to pull this in. The same change also needs to be done in terraform-plugin-framework, which is only checking an attribute value is not null directly:

https://github.com/hashicorp/terraform-plugin-framework/blob/47f5b81941b34c2260859513f62410e35e46e7c8/internal/fwserver/attribute_validation.go#L119-L137

bflad added a commit to hashicorp/terraform-plugin-framework that referenced this pull request Sep 2, 2022
…own values

Reference: hashicorp/terraform-plugin-sdk#1047
Reference: hashicorp/terraform#31730

This change is made to keep terraform-plugin-sdk and terraform-plugin-framework behaviors for deprecation warning handling in sync.
@bflad
Copy link
Member

bflad commented Sep 2, 2022

Framework change: hashicorp/terraform-plugin-framework#465

@jbardin
Copy link
Member

jbardin commented Sep 2, 2022

Note that this was put in specifically to help warn about users assigning values to deprecated attribute during validation. Without this validation will pass with no warnings at all. The logic of the check was that an unknown value indicates that the attribute has been assigned at some point in the configuration, therefore the user should be warned. Even if the value ends up being null, the warning serves to communicate to the user that the attribute is being removed and will result in an error in a future version. Without this warning, users can more easily end up on a situation where upgrades cause the configuration to become invalid, and rolling back may not be easy.

bflad added a commit to hashicorp/terraform-plugin-framework that referenced this pull request Sep 6, 2022
…own values (#465)

Reference: hashicorp/terraform-plugin-sdk#1047
Reference: hashicorp/terraform#31730

This change is made to keep terraform-plugin-sdk and terraform-plugin-framework behaviors for deprecation warning handling in sync.
@github-actions
Copy link

github-actions bot commented Oct 4, 2022

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 Oct 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants