-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Revert "Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs" #11701
Conversation
…r dotnet and nodejs"
Changelog[uncommitted] (2022-12-21) |
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.
Custom marshaling is indeed ignored if the schema is not marshaled, but instead assembled by hand.
In the future, I think we should introduce a schema.Builder
for creating and mutating pre-bound schemas (which the binder would use internally), since AFAIK it is currently finicky to build/mutate a raw schema.Package
. schema.*Spec
should be made internal, since it is unpleasant to use and because it exposes us to this kind of foot gun.
Right now, we can revert.
bors r+ |
11701: Revert "Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs" r=iwahbe a=AaronFriel Reverts #11418. Fixes #11699. The addition of the `ReturnType` field on `schema.FunctionSpec` broke the API contract between Pulumi's codegen package and tfbridge and other tools. From my comment on #11699. > I was able to bisect pu/pu to a commit that had the issue, but couldn't reproduce the behavior with a synthetic schema. PR #11418 should be reverted because it introduces the breaking change, and before that PR is reintroduced, the breaking change to the schema should be addressed. > > Notes on root causing: > > Adding an unserialized `ReturnType` field on the `schema.FunctionSpec` type was a breaking change in the API contract with the tfgen bridge and any other tools that generate a PackageSpec directly (without marshaling/unmarshaling) before binding it and calling `GeneratePackage`. > > In [cd6f658](cd6f658) we can see that a regression test fails to catch this case, the generated code is correct and matches the "before" case! That's because the regression tests in Pulumi load schemas from JSON, so the `ReturnType` field is populated. That occurs in the implementation of `unmarshalFunctionSpec` at line 1656: > > https://github.com/pulumi/pulumi/blob/78c9bf4ee4def5130bfd5b29346ece7c61cec08a/pkg/codegen/schema/schema.go#L1649-L1658 > > This function in the bridge is unaware of the new field, and so it only populates `Outputs`. > > https://github.com/pulumi/pulumi-terraform-bridge/blob/36b10921bfd25a5cabf0a342d0e449278a256a8b/pkg/tfgen/generate_schema.go#L607-L638 Co-authored-by: Aaron Friel <mayreply@aaronfriel.com>
Build failed: |
bors retry |
For context, the fix would be implemented during the bind phase ( if spec.ReturnType != nil {
// bind return type
} to handle if spec.ReturnType != nil {
// bind from return type spec
} else if spec.Outputs != nil {
// bind from outputs
} Note when bringing this back: add SDK-tests of which the schema is generated from |
Build succeeded: |
11753: Revert "Revert "Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs"" r=Zaid-Ajaj a=Zaid-Ajaj Reverts #11701 and fixes the problem which caused the issue in the first place: - Binding `Outputs` field from `FunctionSpec` into `Function.ReturnType` when available - Adding a test that verifies the above works as expected Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com>
11753: Revert "Revert "Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs"" r=Zaid-Ajaj a=Zaid-Ajaj Reverts #11701 and fixes the problem which caused the issue in the first place: - Binding `Outputs` field from `FunctionSpec` into `Function.ReturnType` when available - Adding a test that verifies the above works as expected Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com>
11753: Revert "Revert "Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs"" r=Zaid-Ajaj a=Zaid-Ajaj Reverts #11701 and fixes the problem which caused the issue in the first place: - Binding `Outputs` field from `FunctionSpec` into `Function.ReturnType` when available - Adding a test that verifies the above works as expected Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com>
Reverts #11418.
Fixes #11699. The addition of the
ReturnType
field onschema.FunctionSpec
broke the API contract between Pulumi's codegen package and tfbridge and other tools.From my comment on #11699.