-
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
Fix schema path elem traversal #1757
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1757 +/- ##
==========================================
- Coverage 59.79% 59.30% -0.50%
==========================================
Files 300 307 +7
Lines 42025 42418 +393
==========================================
+ Hits 25129 25154 +25
- Misses 15474 15840 +366
- Partials 1422 1424 +2 ☔ 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.
Looks sensible but do we have enough coverage here given the previous regressions? I don't want to block on this if you are certain of the fix but it'd seem prudent to add some coverage if we missed this last time.
case shim.Resource: // object element type | ||
return schemaPathToPropertyPath(basePath, schemaPath[1:], e.Schema(), schemaInfo.Fields) | ||
elem := schemaInfo.Elem |
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.
Does this also need a test? Looks like the tests only cover the other direction.
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.
The test is bi-directional. Both changes are covered.
I actually only noticed that this needed to be fixed because the test I added was still failing.
We have coverage over the material that this PR changed. In terms of general coverage of I expect that closing #1282 will give us lots of coverage over |
case shim.Resource: // object element type | ||
return propertyPathToSchemaPath(basePath.Element(), elemPP, e.Schema(), schemaInfo.Fields) | ||
elem := schemaInfo.Elem |
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.
Yeah, very annoying omission. For this case schemaInfo.Elem.Fields makes so much more sense.
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.
Thank you!
Regression tests for #1843 Issue fixed in pulumi/pulumi-terraform-bridge#1757
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`.
Fixes pulumi/pulumi-gcp#1794
This is the second attempt at fixing pulumi/pulumi-gcp#1794, so strict scrutiny would be appreciated.
The core of the issue is correctly interpreting this block:
pulumi-terraform-bridge/pkg/tfshim/shim.go
Lines 102 to 127 in 07b2c1c
From what I can tell, both
propertyPathToSchemaPathInner
andschemaPathToPropertyPathInner
confuse case 3 for case 2:propertyPathToSchemaPathInner
andschemaPathToPropertyPathInner
correctly handle case 2 ((schema, ps) ~ {x: T}
), traversing(schema.Elem().(shim.Resource).Schema(), ps.Fields)
. This is correct, as the.Elem()
in theshim
path is a fiction of representation.ps.Fields
correctly represents that(schema, ps)
is the object ({x: T}
) in question.Currently, both functions incorrectly handle case 3 (
(schema, ps) ~ List[{x: T}], (schema.Elem(), ps.Elem) ~ {x: T}
), treating it identically to case 2. The correct traversal for the nested object is(schema.Elem().(shim.Resource).Schema(), ps.Elem.Fields)
(note the additional.Elem
afterps
). This PR makes that change, and adds a test case to lock it in.