pulumi / pulumi-terraform-bridge

A library allowing providers built with the Terraform Plugin SDK to be bridged into Pulumi.
Apache License 2.0
184 stars 42 forks source link

Only validate IDs for PF resources #2029

Closed iwahbe closed 1 month ago

iwahbe commented 1 month ago

This PR is best reviewed commit by commit:

Fixes https://github.com/pulumi/pulumi-terraform-bridge/issues/2030

codecov[bot] commented 1 month ago

Codecov Report

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

Project coverage is 63.24%. Comparing base (9cf2ede) to head (875c5c0). Report is 2 commits behind head on master.

Files Patch % Lines
pf/tfgen/main.go 0.00% 7 Missing :warning:
pf/internal/check/not_supported.go 50.00% 1 Missing and 1 partial :warning:
pf/tfgen/gen.go 0.00% 1 Missing and 1 partial :warning:
pf/internal/check/checks.go 92.30% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2029 +/- ## ========================================== + Coverage 61.10% 63.24% +2.14% ========================================== Files 334 333 -1 Lines 44940 37387 -7553 ========================================== - Hits 27459 23647 -3812 + Misses 15960 12213 -3747 - Partials 1521 1527 +6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

iwahbe commented 1 month ago

It's not entirely clear to me that this needs to be in an internal package.

I'm not sure what you mean. Are you suggesting that it shouldn't be a package or that it shouldn't be internal?

Is it not sufficient to export Provider (and Provider only) if we have a new check package?

We do export Provider (and Provider only), but we only export them for tfgen. We don't want end users to be calling these functions. We don't have any backwards compatibility guarantees on names or signatures.

guineveresaenger commented 1 month ago

I see, you are saying end users should not be calling check.Provider() at all either.

I don't have a particularly strong feeling here; I would've left it at tfgen.checkProvider but usage wise internal makes sense too. Feel free to resolve.

iwahbe commented 1 month ago

Waiting on API token limits to get to a green CI and merge.