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

Nodejs codegen regression impacting Azure provider #11699

Closed
AaronFriel opened this issue Dec 21, 2022 · 1 comment · Fixed by #11701
Closed

Nodejs codegen regression impacting Azure provider #11699

AaronFriel opened this issue Dec 21, 2022 · 1 comment · Fixed by #11701
Assignees
Labels
area/codegen SDK-gen, program-gen, convert impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec p0 Bugs severe enough to interrupt existing work resolution/fixed This issue was fixed
Milestone

Comments

@AaronFriel
Copy link
Member

The Azure SDK generates invalid codegen for plain invokes such as azure:automation/getAccount:getAccount, returning Promise<void> instead of a return type.

Before:

export function getAccount(args: GetAccountArgs, opts?: pulumi.InvokeOptions): Promise<GetAccountResult> {
    if (!opts) {
        opts = {}
    }

    opts = pulumi.mergeOptions(utilities.resourceOptsDefaults(), opts);
    return pulumi.runtime.invoke("azure:storage/getAccount:getAccount", {
        "minTlsVersion": args.minTlsVersion,
        "name": args.name,
        "resourceGroupName": args.resourceGroupName,
    }, opts);
}

After:

export function getAccount(args: GetAccountArgs, opts?: pulumi.InvokeOptions): Promise<void> {

    opts = pulumi.mergeOptions(utilities.resourceOptsDefaults(), opts || {});
    return pulumi.runtime.invoke("azure:storage/getAccount:getAccount", {
        "minTlsVersion": args.minTlsVersion,
        "name": args.name,
        "resourceGroupName": args.resourceGroupName,
    }, opts);
}

The return type declaration GetAccountResult is also omitted.

This appears to be a regression introduced in #11418, determined by bisection.

@AaronFriel AaronFriel changed the title Nodejs codegen regression Nodejs codegen regression impacting Azure provider Dec 21, 2022
@AaronFriel AaronFriel added kind/bug Some behavior is incorrect or out of spec p0 Bugs severe enough to interrupt existing work area/codegen SDK-gen, program-gen, convert labels Dec 21, 2022
@AaronFriel AaronFriel added this to the 0.82 milestone Dec 21, 2022
@AaronFriel
Copy link
Member Author

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

bors bot added a commit that referenced this issue 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 bors bot closed this as completed in 818d055 Dec 21, 2022
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Dec 21, 2022
@lukehoban lukehoban added the impact/regression Something that used to work, but is now broken label Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/codegen SDK-gen, program-gen, convert impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec p0 Bugs severe enough to interrupt existing work resolution/fixed This issue was fixed
Projects
None yet
3 participants