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

skip FixUpBlockAttrs when we can detect types from a new SDK #29624

Merged
merged 2 commits into from
Sep 24, 2021

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Sep 22, 2021

If structural types are being used, we can be assured that the legacy SDK SchemaConfigModeAttr is not being used, and the fixup is not needed. This prevents inadvertent mapping of blocks to structural attributes, and allows us to skip the fixup overhead when possible.

Here we have a passive alternative to #29572, requiring no input from the provider to prevent the collision of structural attributes and the block-to-attribute fixup. While we cannot use this technique to skip the fixup in all cases, the introduction of nested attribute types in the new SDK means that we will elide the fixup in the majority of cases with complex schemas.

The referenced PR contains some additional fixes, and a protocol 6.1 would still need to be cut to mark the deprecated fields, but those things can all be done in separate PRs.

If structural types are being used, we can be assured that the legacy
SDK SchemaConfigModeAttr is not being used, and the fixup is not needed.

This prevents inadvertent mapping of blocks to structural attributes,
and allows us to skip the fixup overhead when possible.
@jbardin jbardin requested a review from a team September 22, 2021 17:04
@bflad
Copy link
Contributor

bflad commented Sep 22, 2021

Definitely an edge case, but while we are here, would it be worth skipping on unsupported Block types as well? Terraform Plugin SDK version 2 only implements list and set blocks, unless hashicorp/terraform-plugin-sdk#62 / hashicorp/terraform-plugin-sdk#220 is ever implemented.

@jbardin
Copy link
Member Author

jbardin commented Sep 22, 2021

I was thinking about also extending the heuristic to skip on attribute types that could not have been generated by the legacy SDK, like objects, and maps of objects. I didn't consider invalid schema block types too (they aren't part of this fixup at all), but maybe they're worth adding to increase the likelihood of a future positive signal even if blocks are in the SDK at all (yet)?

@jbardin
Copy link
Member Author

jbardin commented Sep 22, 2021

Extended the heuristics to cover certain attribute types which were not generated by the legacy SDK. I did not add NestingMap block types to the check, because it currently doesn't appear in any schema, except for a number of internal tests using that type.

We can also rule out some attribute types as indicating something other
than the legacy SDK.

- Tuple types were not generated at all.
- There were no single objects types, the convention was to use a block
  list or set of length 1.
- Maps of objects were not possible to generate, since named blocks were
  not implemented.
- Nested collections were not supported, but when they were generated they
  would have primitive types.
@jbardin jbardin changed the title skip blocktoattr.FixUpBlockAttrs with nested types skip FixUpBlockAttrs when we can detect types from a new SDK Sep 24, 2021
@jbardin jbardin merged commit c8cd0b1 into main Sep 24, 2021
@jbardin jbardin deleted the jbardin/no-block-to-attr branch September 24, 2021 15:01
@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 Oct 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants