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

Only parse all-lowercase patterns as potential TF properties #1969

Merged
merged 1 commit into from
May 13, 2024

Conversation

guineveresaenger
Copy link
Contributor

@guineveresaenger guineveresaenger commented May 13, 2024

This pull request asserts that according to Terraform best practices 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, 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 shows multiple improvements. There are a few regressions but the main one is for a resource 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 shows even more improvements, with only V2modelsBotVersionLocaleSpecification having a single regression, because the upstream docs are erroneously using camelCase.
A comparison diff for GCP shows only improvements.
A comparison diff for Azure 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.

Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.56%. Comparing base (165111b) to head (1d76e74).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1969      +/-   ##
==========================================
+ Coverage   60.55%   60.56%   +0.01%     
==========================================
  Files         331      331              
  Lines       44722    44726       +4     
==========================================
+ Hits        27081    27090       +9     
+ Misses      16118    16114       -4     
+ Partials     1523     1522       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

LGTM

@guineveresaenger guineveresaenger merged commit d41e499 into master May 13, 2024
11 checks passed
@guineveresaenger guineveresaenger deleted the guin/fix-uppercase-bullet-points branch May 13, 2024 21:58
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.

clusterType values not listed
2 participants