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

helper/schema: Add Resource type flags for disabling legacy type system protocol flags #1229

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Aug 8, 2023

Closes #1227

Introduces new Resource type EnableLegacyTypeSystemApplyErrors and EnableLegacyTypeSystemPlanErrors fields that prevent the PlanResourceChange and ApplyResourceChange protocol operation responses from setting the UnsafeLegacyTypeSystem flag. When Terraform encounters the UnsafeLegacyTypeSystem flag, it will demote certain data consistency issues from error diagnostics that would immediately prevent Terraform workflows from completing to warning logs, which can be difficult for provider developers to discover. Provider developers are not generally expected to try enabling these settings unless discovering these data consistency issues upfront is desired before migrating resources to terraform-plugin-framework.

This setting is on the individual resource level because these errors can be quite prevalent in existing terraform-plugin-sdk resources and therefore it would be problematic to not give provider developers more control over the behavior.

The settings are split between plan and apply because the protocol flag is on two separate protocol operations and because certain errors for one operation may be unavoidable, so it leaves provider developers the option to still raise errors for the other operation.

This change set includes new website documentation to discuss these data consistency issues and how to potentially resolve them more in-depth. It is expected that the framework migration guide will be updated to recommend enabling these settings before migrating to help provider developers understand the issues before migrating. The error resolution section has one particular error to start and it is expected that this section will grow over time as provider developers report the various data consistency errors that Terraform can raise.

@bflad bflad added the enhancement New feature or request label Aug 8, 2023
@bflad bflad added this to the v2.28.0 milestone Aug 8, 2023
@bflad bflad requested a review from a team as a code owner August 8, 2023 18:32
@bflad bflad requested a review from jbardin August 8, 2023 18:34
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switching the pull request view to ignore whitespace changes should make this file's changes easier to review (TestPlanResourceChange was switched to table-driven testing). 👍

@bflad

This comment was marked as outdated.

…stem protocol flags

Reference: #1227

Introduces new `Resource` type `EnableLegacyTypeSystemApplyErrors` and `EnableLegacyTypeSystemPlanErrors` fields that prevent the `PlanResourceChange` and `ApplyResourceChange` protocol operation responses from setting the `UnsafeLegacyTypeSystem` flag. When Terraform encounters the `UnsafeLegacyTypeSystem` flag, it will demote certain data consistency issues from error diagnostics that would immediately prevent Terraform workflows from completing to warning logs, which can be difficult for provider developers to discover. Provider developers are not  generally expected to try enabling these settings unless discovering these data consistency issues upfront is desired before migrating resources to terraform-plugin-framework.

This setting is on the individual resource level because these errors can be quite prevalent in existing terraform-plugin-sdk resources and therefore it would be problematic to not give provider developers more control over the behavior.

The settings are split between plan and apply because the protocol flag is on two separate protocol operations and because certain errors for one operation may be unavoidable, so it leaves provider developers the option to still raise errors for the other operation.

This change set includes new website documentation to discuss these data consistency issues and how to potentially resolve them more in-depth. It is expected that the framework migration guide will be updated to recommend enabling these settings before migrating to help provider developers understand existing issues before migrating. The error resolution section has one particular error to start and it is expected that this section will grow over time as provider developers report the various data consistency errors that Terraform can raise.
@bflad bflad force-pushed the bflad/legacy-type-system-env-var branch from 2bbfe8e to 405f5b2 Compare August 8, 2023 18:56
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

Overall lgtm, love the docs ❤️


The state value check will raise an error if there is an unexpected value difference, which is the same as if Terraform raised the error. Checking state values of configured values can be removed once the resource is [migrated to terraform-plugin-framework](/terraform/plugin/framework/migrating), as Terraform itself will raise any potential error.

## Resolving Data Consistency Errors
Copy link
Member

@austinvalle austinvalle Aug 8, 2023

Choose a reason for hiding this comment

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

Not sure we need to open the can of worms in this PR, but should we eventually add the other common error planned value <blah> does not match config value <blah> or prior value to this section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that would eventually be my goal from the PR description. I just don't have all those various errors handy right now and we'll need to figure out how we best want to discuss each one.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry! I missed that part of your PR description 😵‍💫

Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

bflad added a commit to hashicorp/terraform-plugin-framework that referenced this pull request Aug 9, 2023
@bflad bflad merged commit b374785 into main Aug 24, 2023
5 checks passed
@bflad bflad deleted the bflad/legacy-type-system-env-var branch August 24, 2023 17:17
bflad added a commit to hashicorp/terraform-plugin-framework that referenced this pull request Aug 24, 2023
@github-actions
Copy link

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 Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider Method for Disabling Legacy SDK Data Consistency Warning Log Demotion
4 participants