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

Implement migrate sub-command #314

Merged
merged 19 commits into from
Jan 11, 2024
Merged

Implement migrate sub-command #314

merged 19 commits into from
Jan 11, 2024

Conversation

SBGoods
Copy link
Contributor

@SBGoods SBGoods commented Dec 20, 2023

Implements a new migrate subcommand that migrates existing provider docs using the old website directory structure (website/docs/) to a terraform-plugin-docs-supported templates directory.

The migrate subcommand takes the following actions:

  • Copies the --old-website-source-dir folder to the --tempates-dir folder (will create this folder if it doesn't exist)
  • Renames docs/d/ and docs/r/ subdirectories to data-sources/ and resources/ respectively
  • Change file suffixes for template files from .html.markdown to .md.tmpl
  • Extracts code blocks from website docs to create individual example files in --examples-dir (will create this folder if it doesn't exist)
  • replace extracted example code in website templates with tfplugindocs template code referring to example files.
  • Copies non-template files to --templates-dir folder

@SBGoods SBGoods requested a review from a team as a code owner December 20, 2023 19:02
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.

Overall this is looking pretty good! Some high level comments/questions and then its probably good to go! 👍

.changes/unreleased/FEATURES-20231220-141244.yaml Outdated Show resolved Hide resolved
internal/cmd/migrate.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
internal/cmd/migrate.go Outdated Show resolved Hide resolved
internal/provider/migrate.go Outdated Show resolved Hide resolved
internal/provider/migrate.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
Co-authored-by: Brian Flad <bflad417@gmail.com>
@bflad
Copy link
Contributor

bflad commented Dec 21, 2023

I do also wonder if we should have a flag (or bake the behavior in) to remove the old website/docs/... files. I presume the developer workflow for this command is a one-time operation that would be submitted as a pull request and removing the old files would save a lot of time. For additional context, the public registry does not support having both website/docs/... and docs/... layouts at the same time -- it will take precedence with the legacy file layout over the newer layout (for whatever reason), so its important to get rid of the old files to prevent developer confusion.

@bflad bflad added the enhancement New feature or request label Dec 21, 2023
@bflad bflad added this to the v0.17.0 milestone Dec 21, 2023
@SBGoods SBGoods requested a review from bflad January 10, 2024 22:07
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.

This is awesome @SBGoods! I left some random thoughts/questions but everything LGTM :shipit:

internal/cmd/migrate.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved

--examples-dir <ARG> examples directory based on provider-dir (default: "examples")
--provider-dir <ARG> relative or absolute path to the root provider code directory when running the command outside the root provider code directory
--templates-dir <ARG> new website templates directory based on provider-dir; files will be migrated to this directory (default: "templates")
Copy link
Member

Choose a reason for hiding this comment

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

At first glance, it wasn't immediately obvious what the input directories were vs. the output directory without reading the help text. Would it be confusing/misleading to name the --templates-dir as --output-dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the name should be the same to keep it in parity with the generate command with the same flag. If we changed the flag to something like --output-dir it might not be obvious to the user that they would need to specify generate --template-dir to that directory after migration.

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay that's helpful context! Makes sense to me 👍🏻

README.md Outdated Show resolved Hide resolved
internal/provider/migrate.go Outdated Show resolved Hide resolved
internal/provider/migrate.go Outdated Show resolved Hide resolved
internal/provider/migrate.go Outdated Show resolved Hide resolved
internal/provider/migrate.go Outdated Show resolved Hide resolved
SBGoods and others added 2 commits January 11, 2024 12:20
Co-authored-by: Austin Valle <austinvalle@gmail.com>
@SBGoods SBGoods merged commit 068a226 into main Jan 11, 2024
6 checks passed
@SBGoods SBGoods deleted the SBGoods/migrate-command branch January 11, 2024 18:24
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 May 23, 2024
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.

3 participants