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

Documentation for mapping between SDKv2 and the Framework #660

Merged
merged 8 commits into from
Feb 10, 2023

Conversation

bendbennett
Copy link
Contributor

@bendbennett bendbennett commented Feb 8, 2023

Closes: #664
Closes: #659
Closes: #657
Closes: #650

…ramework attribute and data source, provider and resource validators (#659)
@bendbennett bendbennett added documentation Improvements or additions to documentation enhancement New feature or request labels Feb 8, 2023
@bendbennett bendbennett requested a review from a team as a code owner February 8, 2023 09:05

| SDK Attribute Field | Framework Attribute Validator | Framework Data Source, Provider and Resource Validators |
|---------------------|---------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------|
| AtLeastOneOf | {TYPE}validator.AtLeastOneOf() | [datasourcevalidator.AtLeastOneOf()](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-framework-validators/datasourcevalidator#AtLeastOneOf), [providervalidator.AtLeastOneOf()](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-framework-validators/providervalidator#AtLeastOneOf), [resourcevalidator.AtLeastOneOf()](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-framework-validators/resourcevalidator#AtLeastOneOf) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Would some html be useful here to have a list for the cases where there are several translation available: https://stackoverflow.com/questions/19950648/how-to-write-lists-inside-a-markdown-table

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how well our Markdown rendering engine for the website plays with intermixed HTML/Markdown syntax. Another option here could be having separate table columns for data source, provider, and resource to prevent the listing in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have split into 3 columns for data source, provider and resource.

@bflad
Copy link
Contributor

bflad commented Feb 8, 2023

I'm wondering if we might also want a table on https://developer.hashicorp.com/terraform/plugin/framework/migrating/attributes-blocks/fields that accounts for all the schema.Schema since that may be where folks first wind up when looking for this type of information. That framework side of that table could point folks to either the direct answer (e.g. Required for Required) or link to the other migration pages:

SDK Schema Field Framework
Required Required field on attribute
ValidateFunc Predefined Validators and Custom Validators

It may also be worth considering something similar for https://developer.hashicorp.com/terraform/plugin/framework/migrating/attributes-blocks/blocks since they have a differing migration path -- e.g. #650

Copy link
Contributor

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

I think this is shaping up nicely so far!

| Description | `Description` field on attribute |
| InputDefault | [PlanModifiers](/framework/migrating/resources/plan-modification#framework) field on attribute or implementation of [ResourceWithModifyPlan](/plugin/framework/migrating/resources/plan-modification#framework) interface |
| StateFunc | Requires implementation of bespoke logic before storing state, for instance in resource [Create method](plugin/framework/migrating/resources/crud#framework-1) |
| Elem | `ElementType` on [ListAttribute](/plugin/framework/migrating/attributes-blocks/types), [MapAttribute](/plugin/framework/migrating/attributes-blocks/types) or [SetAttribute](/plugin/framework/migrating/attributes-blocks/types) |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be good to call out that any schema.Resource in Elem should refer to the blocks documentation instead. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to include call out.

@@ -14,6 +14,17 @@ This page explains how to migrate nested blocks that are not computed (i.e., do
[Blocks with Computed Fields](/plugin/framework/migrating/attributes-blocks/blocks-computed) for more details
about migrating nested blocks that contain fields that are computed.

Refer to [Attribute Fields](/plugin/framework/migrating/attributes-blocks/fields) for details regarding the relationship
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there are a variety of differences between blocks and attributes, I think this page might warrant its own table. The prior SDK allowed developers to do unsupported things, like declare Sensitive on blocks (hashicorp/terraform-plugin-sdk#819) and we already have special callouts in paragraph form about Computed and Required. Therefore I think we should walk through the various fields and how they relate to blocks:

SDK Schema Field Framework
Computed Link to computed block page
Optional N/A - no implementation required
Required listvalidator.IsRequired() and setvalidator.IsRequired() (let's omit any discussion on single nested blocks, they weren't possible to declare before)
Sensitive N/A - only supported on attributes

etc. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added a table into the Blocks page.

Copy link
Contributor

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

This looks really good to me 🚀 Nice work mapping everything out.

@bendbennett bendbennett merged commit d8f5d5a into main Feb 10, 2023
@bendbennett bendbennett deleted the bendbennett/issues-659 branch February 10, 2023 17:06
@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 Mar 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
3 participants