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

Adding docs for migrating from SDKv2 to the Framework #461

Merged
merged 30 commits into from
Sep 9, 2022

Conversation

bendbennett
Copy link
Contributor

Closes: #459

Copy link
Contributor

@robin-norwood robin-norwood left a comment

Choose a reason for hiding this comment

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

It's almost there! A few small things to fix.

website/data/plugin-framework-nav-data.json Outdated Show resolved Hide resolved
website/docs/plugin/framework/migrating/schema.mdx Outdated Show resolved Hide resolved

This page explains how to migrate an attribute from SDKv2 to the plugin Framework.

## SDKv2
Copy link
Contributor

Choose a reason for hiding this comment

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

If you get a chance, you can try running markdownlint CLI against these files to catch things like this:

Suggested change
## SDKv2
## SDKv2

And further down with additional newlines after code fences. 👍

Copy link
Contributor Author

@bendbennett bendbennett Aug 30, 2022

Choose a reason for hiding this comment

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

Have installed and run markdownlint --disable MD013 -- ./website/docs/plugin/framework/migrating/*.

I'm ignoring the MD013/line-length Line length notifications.

In terms of the remaining notifications, I'm interested to hear feedback from @robin-norwood and @laurapacilio as I thought this was intentional but happy to fix however suits:

markdownlint --disable MD013 -- ./website/docs/plugin/framework/migrating/*
./website/docs/plugin/framework/migrating/data-sources.mdx:107 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "### SDKv2"]
./website/docs/plugin/framework/migrating/data-sources.mdx:140 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "### Framework"]
./website/docs/plugin/framework/migrating/provider.mdx:111 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "### SDKv2"]
./website/docs/plugin/framework/migrating/provider.mdx:138 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "### Framework"]
./website/docs/plugin/framework/migrating/provider.mdx:208 MD036/no-emphasis-as-heading/no-emphasis-as-header Emphasis used instead of a heading [Context: "SDKv2"]
./website/docs/plugin/framework/migrating/provider.mdx:233 MD036/no-emphasis-as-heading/no-emphasis-as-header Emphasis used instead of a heading [Context: "Framework"]
./website/docs/plugin/framework/migrating/provider.mdx:278 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "### SDKv2"]
./website/docs/plugin/framework/migrating/provider.mdx:293 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "### Framework"]
./website/docs/plugin/framework/migrating/provider.mdx:313 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "### Migration Notes"]
./website/docs/plugin/framework/migrating/provider.mdx:321 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "### Example"]
./website/docs/plugin/framework/migrating/provider.mdx:335 MD036/no-emphasis-as-heading/no-emphasis-as-header Emphasis used instead of a heading [Context: "SDKv2"]
./website/docs/plugin/framework/migrating/provider.mdx:358 MD036/no-emphasis-as-heading/no-emphasis-as-header Emphasis used instead of a heading [Context: "Framework"]
./website/docs/plugin/framework/migrating/testing.mdx:109 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "### Example"]
./website/docs/plugin/framework/migrating/testing.mdx:119 MD036/no-emphasis-as-heading/no-emphasis-as-header Emphasis used instead of a heading [Context: "SDKv2"]
./website/docs/plugin/framework/migrating/testing.mdx:146 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "### Framework"]

Copy link
Contributor

Choose a reason for hiding this comment

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

Forgive me - is the problem that we have duplicate headings? My opinion is that they are needed here, but let me know if you disagree!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 2 things that are being flagged by the linter are:

  • Duplicate headings (e.g., where we have ### SDKv2 multiple times).
  • Emphasis used instead of heading (e.g., SDKv2) rather than #### SDKv2, such as in the ### Example sections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following the removal of **SDKv2** and **Framework**, we now have the following:

terraform-plugin-framework % markdownlint --disable MD013 -- ./website/docs/plugin/framework/migrating/*
./website/docs/plugin/framework/migrating/testing.mdx:108 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "### Example"]
./website/docs/plugin/framework/migrating/testing.mdx:118 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "#### SDKv2"]
./website/docs/plugin/framework/migrating/testing.mdx:144 MD024/no-duplicate-heading/no-duplicate-header Multiple headings with the same content [Context: "#### Framework"]

return &schema.Resource{
Schema: map[string]*schema.Schema{
"length": {
ValidateDiagFunc: validation.ToDiagFunc(validation.IntAtLeast(1)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is showing a "predefined" validator, should we get an example using a custom validation function already? e.g. here's some examples from the AWS provider

https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/service/rds/validate.go

website/docs/plugin/framework/migrating/schema.mdx Outdated Show resolved Hide resolved
Comment on lines 10 to 12
When you update a resource's implementation in your provider, some changes may not be compatible with old versions. You
can create state upgraders to help users migrate resources provisioned with old schema configurations. Refer to
[State Upgrade](/plugin/framework/resources/state-upgrade) in the Framework documentation for details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do state upgraders "help users migrate resources" or do they do it automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Have reworded as:

You can create state upgraders to automatically migrate resources provisioned with old schema configurations.

Comment on lines 24 to 25
You must also update the [provider factories](/plugin/framework/migrating/testing/provider-factories) to use
the Framework.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did the testing/provider-factories page not get migrated? I don't see it in the PR. (Also should testing.mdx be testing/index.mdx? @laurapacilio

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Provider Factories are included within the testing.mdx page. I've update the link.

Let me know about testing.mdx => /testing/index.mdx`.

@bendbennett bendbennett marked this pull request as ready for review September 9, 2022 14:37
@bendbennett bendbennett requested a review from a team as a code owner September 9, 2022 14:37
@bendbennett bendbennett merged commit 8ce2dcd into main Sep 9, 2022
@bendbennett bendbennett deleted the bendbennett/issues-459 branch September 9, 2022 14:38
@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 10, 2022
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.

Add Website Documentation for Migrating Providers from SDKv2 to Framework
4 participants