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

Add tfgen time validation for ProviderInfo.Resources #1758

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Mar 13, 2024

While discussing #1757, there was some uncertainty on how to align *SchemaInfo with the underlying shim.Schema. This PR clarifies by introducing a user error during make tfgen time if the user provides a misaligned or nonsensical *SchemaInfo.

@iwahbe iwahbe self-assigned this Mar 13, 2024
@iwahbe iwahbe requested a review from t0yv0 March 13, 2024 14:06
@iwahbe iwahbe marked this pull request as draft March 13, 2024 14:06
Copy link

codecov bot commented Mar 13, 2024

Codecov Report

Attention: Patch coverage is 76.19048% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 60.07%. Comparing base (f64f0d8) to head (cff9ca4).

Files Patch % Lines
pkg/tfbridge/validate.go 78.21% 20 Missing and 2 partials ⚠️
pkg/tfgen/generate.go 25.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1758      +/-   ##
==========================================
- Coverage   60.57%   60.07%   -0.50%     
==========================================
  Files         302      310       +8     
  Lines       42213    42694     +481     
==========================================
+ Hits        25569    25649      +80     
- Misses      15173    15572     +399     
- Partials     1471     1473       +2     

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

var (
errNoCoorospondingField = fmt.Errorf("overriding non-existent field")
errNoElemToOverride = fmt.Errorf("overriding non-existent elem")
errCannotSpecifyFieldsOnListOrSet = fmt.Errorf("cannot specify .Fields on a List[T] or Set[T] type")
Copy link
Member

Choose a reason for hiding this comment

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

Nice

@t0yv0
Copy link
Member

t0yv0 commented Mar 14, 2024

This is very valuable! Would be curious to run downstream checks perhaps we can preemptively fixup non-compliant providers before merging to main. For good form could throw in some tests to cover this code but it's not the top priority for coverage obviously, looks nice and straightforward.

@t0yv0 t0yv0 self-requested a review March 14, 2024 14:31
@iwahbe iwahbe force-pushed the iwahbe/validate-ProviderInfo.Resources branch 3 times, most recently from bf01cbf to 4d763c1 Compare March 26, 2024 14:38
@iwahbe
Copy link
Member Author

iwahbe commented Mar 26, 2024

I added tests, which caught one set of false negatives. I'm blasting out a test run on downstream now: https://github.com/pulumi/pulumi-terraform-bridge/actions/runs/8438306439.

If that returns 🟢 or all failures are genuine, I'll merge.

@iwahbe
Copy link
Member Author

iwahbe commented Mar 26, 2024

Running a inspection of the generated workload, we see:

Results pending:

Failed:

All tests passed:

@iwahbe iwahbe force-pushed the iwahbe/validate-ProviderInfo.Resources branch from 4d763c1 to cff9ca4 Compare March 27, 2024 13:36
iwahbe added a commit to pulumi/pulumi-datadog that referenced this pull request Mar 27, 2024
This fixes a bug in schema traversal, unblocking
pulumi/pulumi-terraform-bridge#1758.
iwahbe added a commit to pulumi/pulumi-cloudflare that referenced this pull request Mar 27, 2024
iwahbe added a commit to pulumi/pulumi-azure that referenced this pull request Mar 27, 2024
iwahbe added a commit to pulumi/pulumi-aws that referenced this pull request Mar 27, 2024
This is a breaking change for Go and Typescript if users were reading the `PolicyDocument`
field on LogResourcePolicyArgs or on LogResourcePolicy.

This was discovered as part of
pulumi/pulumi-terraform-bridge#1758.
@iwahbe iwahbe marked this pull request as ready for review March 27, 2024 16:51
iwahbe added a commit to pulumi/pulumi-aws that referenced this pull request Mar 27, 2024
- `aws_cloudwatch_log_resource_policy.policy_document`
- `aws_ecr_registry_policy.policy`
- `aws_sns_topic_policy.policy`

This is a breaking change for Go and Typescript if users were reading the `PolicyDocument`
field on LogResourcePolicyArgs or on LogResourcePolicy.

This was discovered as part of
pulumi/pulumi-terraform-bridge#1758.
iwahbe added a commit to pulumi/pulumi-azure that referenced this pull request Mar 27, 2024
iwahbe added a commit to pulumi/pulumi-cloudflare that referenced this pull request Mar 27, 2024
iwahbe added a commit to pulumi/pulumi-datadog that referenced this pull request Mar 27, 2024
This fixes a bug in schema traversal, unblocking
pulumi/pulumi-terraform-bridge#1758.
@t0yv0 t0yv0 self-requested a review March 27, 2024 17:28
Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

🚢

@iwahbe iwahbe merged commit 3a2ed13 into master Mar 27, 2024
9 checks passed
@iwahbe iwahbe deleted the iwahbe/validate-ProviderInfo.Resources branch March 27, 2024 17:57
iwahbe added a commit to pulumi/pulumi-aws that referenced this pull request Mar 28, 2024
…3743)

This is a breaking change for Go and Typescript if users were reading
the `PolicyDocument` field on LogResourcePolicyArgs or on
LogResourcePolicy. If we don't want to take the change, we can remove
the override entirely (semantically equivalent to what we have now in
master).

This was discovered as part of
pulumi/pulumi-terraform-bridge#1758.
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.

None yet

2 participants