-
Notifications
You must be signed in to change notification settings - Fork 188
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
chore: Define a new makefile command for running tfplugindocs #1829
Conversation
scripts/generate-doc.sh
Outdated
|
||
set -euo pipefail | ||
|
||
TF_VERSION="${TF_VERSION:-"1.6.6"}" # TF version to use when running tfplugindocs. Default: 1.6.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it'll be hard to remember to update this version but I understand we only have TF_VERSION_LATEST in GH not in local / makefile.
Can you try to see if it works with 1.6.x (for latest patch in 1.6) so we won't need to change it so often?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if the terraform version has much impact in generated docs. Having a fixed version does help ensure consistent results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good point, i have no idea. on the other hand there can be interesting improvements that we miss if we don't try with latest versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if the terraform version has much impact in generated docs. Having a fixed version does help ensure consistent results.
Unfortunately, their documentation does not explain how TF is used for the generation. I created hashicorp/terraform-plugin-docs#323 to get an answer from HashiCorp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try to see if it works with 1.6.x (for latest patch in 1.6) so we won't need to change it so often?
it did work indeed. Thanks! I will update the code
exporting schema from Terraform
compiling provider "mongodbatlas"
downloading Terraform CLI binary version from releases.hashicorp.com: 1.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
|
||
resource_name="$1" | ||
|
||
if [ ! -f "${TEMPLATE_FOLDER_PATH}/resources/${resource_name}.html.markdown.tmpl" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
knitpit: to have only one if statement with the 3 paths in an array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer leaving the code as it is as the last if
has a different behaviour and does not return an error if the template is missing since some resources do not have the plural form of the data source. Happy to create a follow-up PR if you have a strong opinion about this change. Thanks for the feedback!
|
||
tfplugindocs generate --tf-version "${TF_VERSION}" --website-source-dir "${TEMPLATE_FOLDER_PATH}" | ||
|
||
if [ ! -f "docs/resources/${resource_name}.html.markdown" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
knitpit: similar comment having only one if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer leaving the code as it is as the last if
has a different behaviour and does not return an error if the template is missing since some resources do not have the plural form of the data source. Happy to create a follow-up PR if you have a strong opinion about this change. Thanks for the feedback!
@@ -121,3 +121,8 @@ scaffold: | |||
@go run ./tools/scaffold/*.go $(name) $(type) | |||
@echo "Reminder: configure the new $(type) in provider.go" | |||
|
|||
|
|||
.PHONY: generate-doc | |||
generate-doc: ## Generate the resource documentation via tfplugindocs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this generated on demand? or we want to have something similar to test mocks to check if the user forgot to run the command by running it in a GH action and checking that there are not file changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, this can be useful to ensure docs and schema descriptions/changes are kept synced. This would however require an additional command that calls the docs generation for all resources which have documentation autogenerated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is a good idea. I believe it is still early to implement it as it should require moving all the resources to the auto-generation of the doc so that we don't have to keep track of what resources are using the gen tool and which are not. I will create a ticket to track this work once all the resources are migrated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Ticket: CLOUDP-221358
This PR defines the
make generate-doc
command to generate the resource tf doc.Type of change:
Required Checklist:
Testing
Running the new make command