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

Consider adding (deprecated) support for blocks 😢 #85

Closed
Tracked by #158
paddycarver opened this issue Jul 28, 2021 · 6 comments · Fixed by #188
Closed
Tracked by #158

Consider adding (deprecated) support for blocks 😢 #85

paddycarver opened this issue Jul 28, 2021 · 6 comments · Fixed by #188
Assignees
Labels
design-doc Issues or pull requests about a design document, capturing design ideas and trade-offs. enhancement New feature or request schema Issues and pull requests about the abstraction for specifying schemas. sdkv2-parity Issues tracking feature parity with terraform-plugin-sdk v2 and PRs working towards it.
Milestone

Comments

@paddycarver
Copy link
Contributor

Module version

v0.2.0

Use-cases

We decided to model nested fields as nested attributes, because they're better in basically every way™, and I think that was a good decision.

However, for providers that currently exist and want to adopt the framework using mux, this presents two problems:

  1. Provider blocks need to match perfectly when muxing. But SDKv2 has no way to model nested attributes, and the framework has no way to model blocks, meaning if you have nested fields of any kind in your provider configuration, you just can't mux across SDKv2 and the framework. This is suboptimal, and makes migration to the framework much, much more difficult.
  2. Changing a group of nested fields from a block to an attribute is a breaking change, because it requires practitioners to add the = to their configuration. Meaning piecemeal migration of resources is not possible, as you can't model blocks in the framework, and need to migrate to nested attributes at that point, which is a practitioner-visible change.

Both of these problems severely limit how palatable our migration story can be.

Attempted Solutions

I thought about just relaxing mux to consider blocks and nested attributes the same for considering whether provider blocks are "equivalent" or not, as they'll both show up as objects in the config and can be accessed the same way. But that seems brittle, and would lead to weird situations where things aren't treated as sensitive properly, or where things can be expressed in the framework that SDKv2 doesn't know how to handle... it just feels like a really leaky abstraction and lossy mapping. It also doesn't address point 2 basically at all.

Proposal

I'm proposing that we add support to the framework for modeling blocks, and make it clear from its introduction that it is deprecated, we recommend everyone use nested attributes, and that it's only supported to ease migration from SDKv2. This isn't ideal, but I don't see a way to get a clean, piecemeal, non-practitioner-impacting migration strategy if we don't.

@paddycarver paddycarver added the enhancement New feature or request label Jul 28, 2021
@paddycarver paddycarver added the sdkv2-parity Issues tracking feature parity with terraform-plugin-sdk v2 and PRs working towards it. label Sep 9, 2021
@bflad
Copy link
Contributor

bflad commented Sep 10, 2021

To further hone on the proposal, I presume:

  • We should specifically only support tfprotov6.SchemaNestedBlockNestingModeList and tfprotov6.SchemaNestedBlockNestingModeSet since those were the two types of blocks supported in SDKv2?
  • We should explicitly not support tfprotov6.SchemaNestedBlockNestingModeMap (blocks with additional name label that serves as the map key) that's requested in Allow providers' schema to define named HCL blocks terraform-plugin-sdk#220, since we likely will never support that in SDKv2 due to the complexity of handling it in the underlying flatmap/field mapping and would feel weird with other block types being deprecated? I'm wondering if now is the best time to close that feature request.

@aareet aareet added the design-doc Issues or pull requests about a design document, capturing design ideas and trade-offs. label Sep 13, 2021
@aareet
Copy link
Contributor

aareet commented Sep 13, 2021

Next step: create an epic with more granular implementation so we can discuss with provider devs and get their input on which path is preferable.

@dpifke
Copy link

dpifke commented Sep 17, 2021

If deprecation of nested block syntax is planned, the documentation at https://www.terraform.io/docs/language/attr-as-blocks.html should probably be updated ASAP to no longer recommend it over nested attribute syntax.

Otherwise, providers using the framework will face confusion from users as to why the "recommended" syntax is not allowed.

Lack of support for nested blocks might also be worth a mention at https://www.terraform.io/docs/plugin/which-sdk.html.

@paddycarver paddycarver added the schema Issues and pull requests about the abstraction for specifying schemas. label Sep 21, 2021
@paddycarver
Copy link
Contributor Author

paddycarver commented Sep 21, 2021

I think that's a slightly confusing "we don't have good terms around this yet" case. SDKv2 can't currently use the recommended syntax, which is a framework-only feature.

Likewise, the "which SDK?" update may be more confusing than illuminating, as there's not really a difference in capability, just in syntax. Unsure, though. 🤔

@apparentlymart
Copy link

FWIW, the intent of that "Attributes as Blocks" page is only to describe the funny special-case behavior for the arguments which use that weird legacy mode. It's not a recommendation to do anything in particular. Its intended audience is end-users of providers that use the mechanism, to explain the end-user-visible consequences of using that special mode.

@bflad bflad self-assigned this Sep 29, 2021
bflad added a commit that referenced this issue Sep 29, 2021
Reference: #85

This implementation is analogous to the existing `Attributes` field on `Attribute`. While the framework handles the major differences at the protocol layer during conversion, it also must enforce the constraints of the underlying type system.

Some notable features include:

* Defining schemas is very similar to `Attributes`.
* Accessing and writing `Config`, `Plan`, and `State` data is no different than `Attributes` with the same nesting mode.
* Blocks are structural by Terraform's and cty's definition, meaning there is no concept of `Computed`, `Optional`, `Required`, or `Sensitive`. Checks are in place to enforce these constraints.

The primary purpose for supporting blocks is to allow previously existing schemas defined by the older Terraform Plugin SDK to not require practitioner breaking changes upon migrating to the framework (except the protocol version and therefore the minimum Terraform CLI version). This also allows this framework to be muxed with the older framework since the provider schema must match. It is expected that over time any schema definitions including `Blocks` will migrate to `Attributes`.

Provider developers should always opt for `Attributes` in new schema definitions.
bflad added a commit that referenced this issue Oct 1, 2021
bflad added a commit that referenced this issue Oct 29, 2021
bflad added a commit that referenced this issue Oct 29, 2021
Reference: #85

This implementation is analogous to the existing `Attributes` field on `Attribute`. While the framework handles the major differences at the protocol layer during conversion, it also must enforce the constraints of the underlying type system.

Some notable features include:

* Defining schemas is very similar to `Attributes`.
* Accessing and writing `Config`, `Plan`, and `State` data is no different than `Attributes` with the same nesting mode.
* Blocks are structural by Terraform's and cty's definition, meaning there is no concept of `Computed`, `Optional`, `Required`, or `Sensitive`. Checks are in place to enforce these constraints.

The primary purpose for supporting blocks is to allow previously existing schemas defined by the older Terraform Plugin SDK to not require practitioner breaking changes upon migrating to the framework (except the protocol version and therefore the minimum Terraform CLI version). This also allows this framework to be muxed with the older framework since the provider schema must match. It is expected that over time any schema definitions including `Blocks` will migrate to `Attributes`.

Provider developers should always opt for `Attributes` in new schema definitions.
@bflad bflad closed this as completed in #188 Nov 3, 2021
bflad added a commit that referenced this issue Nov 3, 2021
Reference: #85

The primary purpose for supporting blocks is to allow previously existing schemas defined by the older Terraform Plugin SDK to not require practitioner breaking changes upon migrating to the framework (except the protocol version and therefore the minimum Terraform CLI version unless a tfprotov5 server implementation is added to this framework as well). This also allows this framework to be muxed with the older framework since the provider schema must match. It is expected that over time any schema definitions including `Blocks` will migrate to `Attributes`.

Provider developers should always opt for `Attributes` in new schema definitions.

One notable exclusion to this initial block support is plan modification, both with attributes under blocks and blocks themselves. Support for this functionality can be tracked in #222.
@bflad bflad added this to the v0.5.0 milestone Nov 3, 2021
@github-actions
Copy link

github-actions bot commented Dec 4, 2021

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.

@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
design-doc Issues or pull requests about a design document, capturing design ideas and trade-offs. enhancement New feature or request schema Issues and pull requests about the abstraction for specifying schemas. sdkv2-parity Issues tracking feature parity with terraform-plugin-sdk v2 and PRs working towards it.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants