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

Revert "Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs" #11701

Merged
merged 1 commit into from Dec 21, 2022

Conversation

AaronFriel
Copy link
Member

@AaronFriel AaronFriel commented Dec 21, 2022

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 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:

if returnType, ok := data["outputs"]; ok {
isEmpty, err := emptyObject(returnType)
if err != nil {
return err
}
if !isEmpty {
if err := json.Unmarshal(returnType, &funcSpec.ReturnType); err != nil {
return err
}

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

@AaronFriel AaronFriel requested review from Zaid-Ajaj and iwahbe and removed request for Zaid-Ajaj December 21, 2022 01:48
@AaronFriel AaronFriel marked this pull request as ready for review December 21, 2022 01:48
@pulumi-bot
Copy link
Contributor

Changelog

[uncommitted] (2022-12-21)

Copy link
Member

@iwahbe iwahbe left a 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.

@iwahbe
Copy link
Member

iwahbe commented Dec 21, 2022

bors r+

bors bot added a commit that referenced this pull request Dec 21, 2022
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>
@bors
Copy link
Contributor

bors bot commented Dec 21, 2022

Build failed:

@iwahbe
Copy link
Member

iwahbe commented Dec 21, 2022

bors retry

@Zaid-Ajaj
Copy link
Contributor

For context, the fix would be implemented during the bind phase (pkg/codegen/schema/bind.go) in bindFunctionDef function where we currently have

if spec.ReturnType != nil {
   // bind return type
}

to handle Outputs being populated instead of ReturnType

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 PackageSpec

@bors
Copy link
Contributor

bors bot commented Dec 21, 2022

Build succeeded:

@bors bors bot merged commit 818d055 into master Dec 21, 2022
@bors bors bot deleted the revert-11418-simplified-invoke-gen branch December 21, 2022 11:07
bors bot added a commit that referenced this pull request Jan 11, 2023
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>
bors bot added a commit that referenced this pull request Jan 11, 2023
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>
bors bot added a commit that referenced this pull request Jan 12, 2023
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>
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.

Nodejs codegen regression impacting Azure provider
4 participants