-
Notifications
You must be signed in to change notification settings - Fork 43
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
Enforce .Elem.Fields (instead of .Fields) for object types #2309
Conversation
d5c0859
to
44e39ea
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2309 +/- ##
==========================================
+ Coverage 56.86% 56.93% +0.06%
==========================================
Files 365 365
Lines 49962 49957 -5
==========================================
+ Hits 28413 28444 +31
+ Misses 20004 19972 -32
+ Partials 1545 1541 -4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some tfgen type override integration tests? We need one for 2185 as well as some tests around Set, List, Object, Map[Object], Map etc.
32f875b
to
ca0ce3b
Compare
7d28415
to
0da65b6
Compare
I have added integration tests. See Add integration tests. |
0da65b6
to
567b0ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for adding the tests!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good - two last small comments
2b550fa
to
96ac411
Compare
96ac411
to
986aa53
Compare
This is a reversal of our previous stance that .Fields should be used. This aligns the fields argument with [schema generation](https://github.com/pulumi/pulumi-terraform-bridge/blob/72b0b7d33b6d9f0dce7c09b31951ade3711a025d/pkg/tfgen/generate.go#L395-L398). We *need* to choose `.Elem.Fields` as our 1 representation (instead of `.Fields`) because of an ambiguity in `shim.Schema`. From a comment in the PR: > To prevent confusion, users are barred from specifying > information on ps directly, they must set .Fields on ps.Elem: > ps.Elem.Fields. > > We need to make this choice (instead of having users set > information on .Fields (and forbidding ps.Elem.Fields) because > the [shim.Schema] representation is not capable of > distinguishing between Map[Object] and Object. SDKv{1,2} > providers are not able to express Map[Object], but Plugin > Framework based providers are. While this is technically a breaking change, I don't expect users to be broken. Any user that would break on this change would have been unable to addapt the last several bridge versions (for example: pulumiverse/pulumi-talos#99). Fixes #2185
986aa53
to
47aa692
Compare
This commit unifies the functionality of `Generator.Generate` and `GenerateWithOptions`, so that they now both call `ProviderInfo.Validate` (previously, only `Generator.Generate` called `ProviderInfo.Validate`). This caused 2 unrelated tests to fail. Both can be fixed by removing the invalid mappings they used to have.
This PR moves an invalid mapping in the `pkg/tf2pulumi/convert/testdata/mappings/renames.json` to a valid mapping. You can see the newly valid mapping take effect in the schema file: `pkg/tf2pulumi/convert/testdata/schemas/renames.json`. This PR exposes a bug where tf2pulumi doesn't properly propagate `MaxItems: 1` information. The bug is shown in the changes in `pkg/tf2pulumi/convert/testdata/renames/pcl/main.pp`.
47aa692
to
5c7dcd1
Compare
This is a reversal of our previous stance that .Fields should be used. This aligns the
fields argument with schema generation.
We need to choose
.Elem.Fields
as our 1 representation (instead of.Fields
) becauseof an ambiguity in
shim.Schema
. From a comment in the PR:While this is technically a breaking change, I don't expect users to be broken. Any user
that would break on this change would have been unable to addapt the last several bridge
versions (for example: pulumiverse/pulumi-talos#99).
Fixes #2185
I have tested this change on talos, AWS, GCP and Azure and it doesn't trigger an error.