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

clusterType values not listed #1507

Closed
Tracked by #1794 ...
brucemackenzie opened this issue Aug 22, 2023 · 5 comments · Fixed by #1969
Closed
Tracked by #1794 ...

clusterType values not listed #1507

brucemackenzie opened this issue Aug 22, 2023 · 5 comments · Fixed by #1969
Assignees
Labels
area/docsgen Issues with docs capture or example rendering, historically part of pkg/tfgen kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed

Comments

@brucemackenzie
Copy link

File: themes/default/content/registry/packages/mongodbatlas/api-docs/cluster/_index.md

Looking in the src code I found these values possible for clusterType:
REPLICASET, GEOSHARDED, SHARDED

@github-actions github-actions bot added the needs-triage Needs attention from the triage team label Aug 22, 2023
@interurban interurban added the area/docs Improvements or additions to docs for this repo label Aug 23, 2023
@interurban interurban transferred this issue from pulumi/pulumi-hugo Aug 23, 2023
@interurban
Copy link

Thank you @brucemackenzie, we will look into why these values are missing.

@toriancrane toriancrane added kind/bug Some behavior is incorrect or out of spec and removed needs-triage Needs attention from the triage team labels Oct 25, 2023
@toriancrane toriancrane self-assigned this Oct 30, 2023
@cnunciato
Copy link
Member

cnunciato commented Nov 6, 2023

Adding a bit of detail. The issue is that these values are present in the TF provider docs, but missing in ours:

image

It looks like they never made it into the schema:

https://github.com/pulumi/pulumi-mongodbatlas/blob/36c49e17ec8bc365a76e23753c6bb06641dbd151/provider/cmd/pulumi-resource-mongodbatlas/schema.json#L13973

image

So it looks like this is a bug in the doc-parsing code. Transferring to pulumi-terraform-bridge for the team to confirm.

@cnunciato cnunciato transferred this issue from pulumi/registry Nov 6, 2023
@toriancrane toriancrane removed their assignment Nov 6, 2023
@toriancrane toriancrane added the needs-triage Needs attention from the triage team label Dec 5, 2023
@iwahbe
Copy link
Member

iwahbe commented Dec 6, 2023

Yep, this is a bug in our docs parsing code. Thanks for reporting it.

@iwahbe iwahbe removed the needs-triage Needs attention from the triage team label Dec 6, 2023
@guineveresaenger
Copy link
Contributor

I believe this is, if not a duplicate, then at least related to pulumi/pulumi-libvirt#335.

@iwahbe iwahbe added area/docsgen Issues with docs capture or example rendering, historically part of pkg/tfgen and removed area/docs Improvements or additions to docs for this repo labels May 10, 2024
@guineveresaenger guineveresaenger self-assigned this May 13, 2024
@guineveresaenger
Copy link
Contributor

Update: We can in fact fix this by more strictly parsing potential properties as presenting in snake_case only.

#1969 will not address situations such as pulumi/pulumi-libvirt#335, since in that case we're seeing only lowercase values.

guineveresaenger added a commit that referenced this issue May 13, 2024
This pull request asserts that [according to Terraform best
practices](https://www.terraform-best-practices.com/naming) we should
not parse backticked values containing uppercase letters as potential
schema properties.

While it is a convention to use `snake_case` for TF properties, it is a
strong one. The AWS provider [requires
it](https://github.com/hashicorp/terraform-provider-aws/blob/main/docs/naming.md),
and other providers follow this convention strictly as well.

The reason I would like to bring this change is because it allows us to
detect documentation sections that are not actually nested property
types, but rather lists of possible values, as described in #1507 in
pulumi-mongodbatlas. Rather than add an ever-elusive regex for "Possible
values include" et al, we can rely with reasonable confidence on the
fact that TF properties are in general lowercase. See sample comparison
diffs below.

A [comparison diff for
mongodbatlas](https://github.com/pulumi/pulumi-mongodbatlas/compare/missing-settings?expand=1)
shows multiple improvements. There are a few regressions but the main
one is for [a
resource](https://www.pulumi.com/registry/packages/mongodbatlas/api-docs/getthirdpartyintegrations/)
whose upstream doc is so unconventional that our representation of it is
already flawed. Overall, we gain a lot more information than we're
losing - the cost is that in a couple of places we get a nonsensical
appendage of a real TF description. This should be fixed with docs
overrides or by opening an upstream PR.
A [comparison diff for
AWS](https://github.com/pulumi/pulumi-aws/compare/missing-settings?expand=1)
shows even more improvements, with only
`V2modelsBotVersionLocaleSpecification` having a single regression,
because the upstream docs are erroneously using camelCase.
A [comparison diff for
GCP](https://github.com/pulumi/pulumi-gcp/compare/missing-settings?expand=1)
shows only improvements.
A [comparison diff for
Azure](https://github.com/pulumi/pulumi-azure/compare/missing-descriptions?expand=1)
shows only improvements.

**Note** I refactored the tests slightly because I wanted to be able to
run individual tests and leave hints as to what they were testing.

Fixes #1507.
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docsgen Issues with docs capture or example rendering, historically part of pkg/tfgen kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants