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

Remove docs matching hack and add more correct strategies for docs discovery. #1882

Merged
merged 7 commits into from
Apr 22, 2024

Conversation

guineveresaenger
Copy link
Contributor

@guineveresaenger guineveresaenger commented Apr 18, 2024

This pull request includes changes to docs discovery that ensure we are matching entityDocs to the correct property field descriptions.

  • adds number pattern to regex so we can match "S3"
  • uses full parent path as docs path so we can properly map different "key" field descriptions to the correct parent field
  • adds new regex to nested header discovery
  • allows for dot-spaced keys in entityDocs for more precise lookup

Fixes #1789

Fixes #1876

Fixes #1875

Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.13%. Comparing base (a38a786) to head (26fd8df).
Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1882      +/-   ##
==========================================
- Coverage   60.99%   60.13%   -0.87%     
==========================================
  Files         303      327      +24     
  Lines       42457    44093    +1636     
==========================================
+ Hits        25898    26514     +616     
- Misses      15085    16090    +1005     
- Partials     1474     1489      +15     

☔ 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.

Looks good to me.

We are waiting on an OK from the AWS folks to merge, right?

pkg/tfgen/docs.go Outdated Show resolved Hide resolved
pkg/tfgen/docs.go Outdated Show resolved Hide resolved
pkg/tfgen/docs.go Outdated Show resolved Hide resolved
@guineveresaenger
Copy link
Contributor Author

guineveresaenger commented Apr 19, 2024

Yeah. I opened pulumi/pulumi-aws#3846 which only holds the schema diff when generated with d8ccf4c.

If we decide that that diff is too lossy for AWS, we have two options:

  1. add a flag here - by contrast, Azure, GCP, and Cloudflare docs all look like they are very happy with these changes so we should definitely roll this out elsewhere.
  2. implement Refine description matching when a description is unique #1886, see if that helps significantly for AWS and go through another round of checking with AWS squad.

Of course, we can also do 1 first and then implement 2 and remove the flag if deemed sufficiently better; if not, we could explore the option to flag resource-by-resource if top-used resources are negatively impacted.

@guineveresaenger guineveresaenger requested a review from a team April 19, 2024 18:16
@guineveresaenger guineveresaenger marked this pull request as ready for review April 19, 2024 18:16
@guineveresaenger guineveresaenger merged commit 64c7a9b into master Apr 22, 2024
11 checks passed
@guineveresaenger guineveresaenger deleted the guin/first-pass-correct-docs branch April 22, 2024 17:28
@guineveresaenger
Copy link
Contributor Author

The AWS folks say 🚢 since it this change removes a lot of incorrect docs, a net improvement for them. Thanks @t0yv0 for taking a look!

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