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

chore(docs): use default template where possible for Fabric docs #764

Merged
merged 6 commits into from
Sep 9, 2024

Conversation

ctreatma
Copy link
Contributor

@ctreatma ctreatma commented Aug 28, 2024

This removes custom templates for some Fabric data sources and resources to avoid missed docs changes due to hard-coded docs templates, which partially addresses #724.

Changes made here are: in cases where there is a single example file,

  1. rename that example to align with conventional paths for examples in tfplugindocs
  2. remove the custom template so that the default template is used
  3. update the resource/data source description in code to match what was in the previous docs

@ctreatma
Copy link
Contributor Author

@equinix/governor-digin-fabric for your reference (and/or review). I noticed that #762 makes changes to the schema but doesn't include the corresponding docs updates because these templates are all currently hard-coded.

ocobles
ocobles previously approved these changes Aug 30, 2024
Copy link
Contributor

@ocobles ocobles left a comment

Choose a reason for hiding this comment

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

lgtm! However there few golangci-lint errors that we may want to address in a different PR

@ctreatma
Copy link
Contributor Author

lgtm! However there few golangci-lint errors that we may want to address in a different PR

Rather than bypass the required status check, since it's already limited to files that are changed in the PR, I fixed the issues.

Took a little more back-and-forth than I would have liked due to linter config. I have a separate PR to adjust linter config here: #766

---

# equinix_fabric_clouder_router (Data Source)
Copy link
Member

Choose a reason for hiding this comment

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

it rhymes

Copy link
Member

@displague displague left a comment

Choose a reason for hiding this comment

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

Aside from some callouts, the move to a fully generated doc brought all-around improvement. Enums and fields that were missed are now documented and will stay documented through schema changes.

subcategory: "Fabric"
description: |-
Copy link
Member

Choose a reason for hiding this comment

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

Can we add description into the resource definition to get matching markdown results? This is a small description, but we could miss some handy documentation links or greater details if they were specified on other resources (I'll keep an eye out in this review).

I presume the default page_title is as good or better.

Copy link
Member

Choose a reason for hiding this comment

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

The descriptions are in the body, they are not being rendered in the markdown frontmatter.

https://github.com/equinix/terraform-provider-equinix/pull/764/files?diff=split&w=1#diff-329a375d2de207d4b7529b502ca8930f5636802bc6aa8934c848ed2972a7260aR32-R36

Is this intended? The first line of non-title markdown body text ends up getting the same treatment as a description frontmatter so it would be redundant to capture it as front-matter?

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 description frontmatter is not used by the registry: https://developer.hashicorp.com/terraform/registry/providers/docs#yaml-frontmatter

The page_title is used, but based on current page titles in the registry, the value we are specifying in data source and resource pages is equivalent to not specifying that attribute at all.

### Nested Schema for `a_side.access_point`

Read-Only:

- `account` (Set of Object) (see [below for nested schema](#nestedobjatt--a_side--access_point--account))
- `authentication_key` (String)
- `gateway` (Set of Object, Deprecated) **Deprecated** `gateway` Use `router` attribute instead (see [below for nested schema](#nestedobjatt--a_side--access_point--gateway))
Copy link
Member

Choose a reason for hiding this comment

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

we lose a documented deprecation flag. Is the field not marked as deprecated? (also in z_side docs)

Copy link
Contributor

Choose a reason for hiding this comment

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

It is marked appropriately, but because it is a nested field the descriptions and further details aren't picked up by tfplugindocs. Just the name of the attribute and the type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, there may be another reason for this. The data_source isn't showing descriptions for any of the nested attributes even though the resource schema displays them. Might be because of the way we're changing attribute behavior to computed and then setting the data_source value.

Resource attributes are showing nested descriptions.

@@ -60,7 +55,7 @@ resource "equinix_fabric_cloud_router" "new_cloud_router"{

- `description` (String) Customer-provided Fabric Cloud Router description
- `href` (String) Fabric Cloud Router URI information
- `order` (Block Set, Min: 1, Max: 1) Order information related to this Fabric Cloud Router (see [below for nested schema](#nestedblock--order))
Copy link
Member

Choose a reason for hiding this comment

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

Min:1 will be lost. Not included in spec? Intended? (perhaps the documentation template isn't reflecting it)

Copy link
Contributor

Choose a reason for hiding this comment

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

Order is optional so it shouldn't have a minimum of 1 anyway. This is intended.

Copy link
Contributor

@thogarty thogarty left a comment

Choose a reason for hiding this comment

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

LGTM. Can be shipped.

### Nested Schema for `a_side.access_point`

Read-Only:

- `account` (Set of Object) (see [below for nested schema](#nestedobjatt--a_side--access_point--account))
- `authentication_key` (String)
- `gateway` (Set of Object, Deprecated) **Deprecated** `gateway` Use `router` attribute instead (see [below for nested schema](#nestedobjatt--a_side--access_point--gateway))
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, there may be another reason for this. The data_source isn't showing descriptions for any of the nested attributes even though the resource schema displays them. Might be because of the way we're changing attribute behavior to computed and then setting the data_source value.

Resource attributes are showing nested descriptions.

@@ -60,7 +55,7 @@ resource "equinix_fabric_cloud_router" "new_cloud_router"{

- `description` (String) Customer-provided Fabric Cloud Router description
- `href` (String) Fabric Cloud Router URI information
- `order` (Block Set, Min: 1, Max: 1) Order information related to this Fabric Cloud Router (see [below for nested schema](#nestedblock--order))
Copy link
Contributor

Choose a reason for hiding this comment

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

Order is optional so it shouldn't have a minimum of 1 anyway. This is intended.

This renames example terraform files in cases where there is exactly
one example for a Fabric resource or data source.  Renaming these
files brings them fully in line with the tfplugindocs conventional
paths, which means we no longer need a custom template for each
resource or data source.
@ctreatma ctreatma merged commit 4b0785b into main Sep 9, 2024
8 of 11 checks passed
@ctreatma ctreatma deleted the fabric-default-template branch September 9, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants