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

Replace old doc generation process with new tfplugindocs tool #356

Merged
merged 43 commits into from
Jan 29, 2021

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Jan 25, 2021

Hashicorp have deleted a Makefile target (website-provider-test) which we were depending on to generate our Terraform provider's documentation (this was historically handled by the CI action file .github/workflows/pr.yml and the website job defined there). This means all our Terraform provider PRs will be failing (inc. any new PRs that get opened).

NOTES: I need to restructure the directories for documentation in order to use https://github.com/hashicorp/terraform-plugin-docs. I also removed some obsolete files that were related to when the terraform platform was hosted using Ruby.

Because the new tfplugindocs tool is using the Description field from our own Schema code to generate the schema documentation output, it meant that I needed to manually copy the live documentation values into the code because there was a difference (in a lot of places) between the code and the original manually curated/written documentation.

Contrived example:

- "service_id": "service Id"            // original code
+ "service_id": "The ID of the Service" // description in the manually written docs.

NOTE: the biggest change in this PR (outside of renaming directories and files) is scripts/generate-docs.go which is what now generates the documentation.

CAVEATS: I believe the original CI action was validating the content (e.g. broken links). The new tool, although it has a validate command, appears to only validate the directory structure and not the content.

This means I've deleted the CI website job and the related Makefile targets because they are no longer usable for our purposes. Hopefully in the future the tfplugindocs validate command will become richer in functionality and so we can try running it in CI.

FOLLOW-UP PR: implement a new .github/workflows/pr.yml job that will run make generate-docs and then diff the output against what's been changed to be sure any changes to the code results in new documentation being generated.

REFERENCES: https://www.terraform.io/docs/registry/providers/docs.html

@Integralist Integralist changed the title new terraform documentation tool Replace old doc generation process with new tfplugindocs tool Jan 26, 2021
@Integralist Integralist force-pushed the integralist/20210125_new_doc_tool branch from eb4f92f to 1766d34 Compare January 26, 2021 09:17
@Integralist Integralist force-pushed the integralist/20210125_new_doc_tool branch from 0967738 to d3cb7dc Compare January 26, 2021 11:20
@Integralist
Copy link
Collaborator Author

@phamann looks like I may have made some progress 🎉

$ tfplugindocs validate
detected static docs directory, running checks
                                                                                                                                                                                                                                              
$ tfplugindocs generate
rendering website for provider "terraform-provider-fastly"
exporting schema from Terraform
compiling provider "fastly"
generating missing resource content
generating template for "fastly_service_dictionary_items_v1"
generating template for "fastly_service_dynamic_snippet_content_v1"
generating template for "fastly_service_v1"
generating template for "fastly_service_waf_configuration"
generating template for "fastly_user_v1"
generating template for "fastly_service_acl_entries_v1"
generating template for "fastly_service_compute"
generating missing data source content
generating template for "fastly_ip_ranges"
generating template for "fastly_waf_rules"
generating missing provider content
generating template for "terraform-provider-fastly"
cleaning rendered website dir
rendering templated website to static markdown
rendering "data-sources/ip_ranges.md.tmpl"
rendering "data-sources/waf_rules.md.tmpl"
rendering "index.md.tmpl"
rendering "resources/service_acl_entries_v1.md.tmpl"
rendering "resources/service_compute.md.tmpl"
rendering "resources/service_dictionary_items_v1.md.tmpl"
rendering "resources/service_dynamic_snippet_content_v1.md.tmpl"
rendering "resources/service_v1.md.tmpl"
rendering "resources/service_waf_configuration.md.tmpl"
rendering "resources/user_v1.md.tmpl"

See the changes to the generated docs in the latest commit (00f2482)

This suggests two things:

  1. We don't need scripts/website/parse-templates.go as tfplugindocs generates generates the required Markdown.
  2. The new Markdown is significantly different in the sense that we lose contextual information...
- cidr_blocks - The lexically ordered list of ipv4 CIDR blocks.
+ cidr_blocks (List of String)

Another example is...

Screenshot 2021-01-26 at 11 39 07

But if I check the Terraform code I can see there are descriptions defined...

Screenshot 2021-01-26 at 11 39 18

So we need to figure out how can we get that context put back in (as we're losing a lot of useful info otherwise) 🤔

Two other outstanding questions are:

  1. How do we preview these changes? Looks like there is now only a 'basic' preview tool (here).
  2. What other features are missing from the original website-provider-test Makefile target? e.g. I thought we had link checkers etc.

/cc @kailan @doramatadora

Copy link
Member

@phamann phamann left a comment

Choose a reason for hiding this comment

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

Looking good, I think there is some confusion as to what the tool is doing, hence why you're seeing large diffs in the content.

The new Markdown is significantly different in the sense that we lose contextual information...

I believe this is because the tool is not finding our templates, as you're using docs_src directory instead of templates and thus it is generating its own ones from the provider schema. From the how it works read me of the tool:

  • Generate resource template files, if missing

How do we preview these changes? Looks like there is now only a 'basic' preview tool (here).

I'm pretty sure serving is on the roadmap for the new too. In fact you can see a stub here. Therefore I suggest we just wait for that and live without it in the meantime.

I thought we had link checkers etc.

Per above, I assume this feature will come in a later version of the tool.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Integralist Integralist force-pushed the integralist/20210125_new_doc_tool branch from 170c700 to 0433b0a Compare January 26, 2021 13:39
@Integralist Integralist force-pushed the integralist/20210125_new_doc_tool branch from d073156 to 9862c36 Compare January 26, 2021 17:45
@Integralist Integralist force-pushed the integralist/20210125_new_doc_tool branch 13 times, most recently from 547ae87 to a9224d2 Compare January 27, 2021 18:00
@Integralist Integralist force-pushed the integralist/20210125_new_doc_tool branch from a9224d2 to b454bb4 Compare January 27, 2021 18:04
@Integralist
Copy link
Collaborator Author

OK @phamann I think this is ready for a review now.

As mentioned in the PR description: I'm pushing back the creation of a new CI job to a separate PR.

Copy link
Member

@phamann phamann left a comment

Choose a reason for hiding this comment

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

Looking good, thank you for doing this, updating all of the schema descriptions must have been painful but a big win.

It would be great if we can find a way to programatically import the description data from the open-api schema 🤔?

Just a couple of outstanding queries.

.github/workflows/pr.yml Show resolved Hide resolved
docs/data-sources/ip_ranges.md Outdated Show resolved Hide resolved
docs/resources/service_compute.md Show resolved Hide resolved
docs/resources/service_v1.md Show resolved Hide resolved
fastly/block_fastly_service_v1_package.go Outdated Show resolved Hide resolved
scripts/generate-docs.go Show resolved Hide resolved
scripts/generate-docs.go Outdated Show resolved Hide resolved
scripts/generate-docs.go Show resolved Hide resolved
Copy link
Member

@phamann phamann left a comment

Choose a reason for hiding this comment

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

👍🏻 LGTM

I now see the original implementation didn't name the files correctly either. I still find it weird/inconsistent that the outputed filenames, and thus the generated URLs on the registry, don't match the name of the resource. Would be good to see what the convention with other providers is here - but not a blocker.

@Integralist
Copy link
Collaborator Author

Ran the full test suite and it's passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants