[codegen/go] Improve optional params in invoke #8839

AaronFriel · 2022-01-26T18:54:48Z

As described in #8821, docs generated for helper functions can be incorrect because optional arguments to parameter objects are not correctly handled.

Previously, code would be generated like so:

    policyDocument, err := iam.GetPolicyDocument(ctx, &iam.GetPolicyDocumentArgs{
      Statements: []iam.GetPolicyDocumentStatement{
        iam.GetPolicyDocumentStatement{
          Sid: "1",
          //...

However "Sid" is of type *string.

This four helper conversion functions, to handle the primitive types we lower from, and modifies codegen to recursively apply these functions inside of an invoke call.

In the new code generation, the above is instead rendered as:

    policyDocument, err := iam.GetPolicyDocument(ctx, &iam.GetPolicyDocumentArgs{
      Statements: []iam.GetPolicyDocumentStatement{
        iam.GetPolicyDocumentStatement{
          Sid: pulumi.StringRef("1"),
          //...

github-actions · 2022-01-26T18:57:14Z

Diff for pulumi-random with merge commit 4352872

github-actions · 2022-01-26T18:57:58Z

Diff for pulumi-azuread with merge commit 4352872

github-actions · 2022-01-26T19:00:22Z

Diff for pulumi-kubernetes with merge commit 4352872

github-actions · 2022-01-26T19:01:31Z

Diff for pulumi-gcp with merge commit 4352872

github-actions · 2022-01-26T19:05:55Z

Diff for pulumi-azure with merge commit 4352872

github-actions · 2022-01-26T19:09:10Z

Diff for pulumi-aws with merge commit 4352872

codecov · 2022-01-26T19:17:51Z

Codecov Report

Merging #8839 (ef6e697) into master (eb97581) will decrease coverage by 15.09%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           master    #8839       +/-   ##
===========================================
- Coverage   59.35%   44.26%   -15.10%     
===========================================
  Files         641      635        -6     
  Lines       99407    97214     -2193     
  Branches     1389     1389               
===========================================
- Hits        59006    43033    -15973     
- Misses      37038    51198    +14160     
+ Partials     3363     2983      -380

Impacted Files	Coverage Δ
pkg/codegen/go/gen_program_expressions.go	`0.00% <0.00%> (-78.00%)`	⬇️
pkg/codegen/go/gen_program_optionals.go	`0.00% <0.00%> (-94.21%)`	⬇️
pkg/codegen/internal/test/program_driver.go	`0.00% <ø> (-52.71%)`	⬇️
sdk/go/pulumi/type_conversions.go	`0.00% <0.00%> (ø)`
pkg/codegen/docs.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/codegen/go/gen_spill.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/codegen/dotnet/templates.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/codegen/hcl2/model/format/func.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/codegen/go/utilities.go	`0.00% <0.00%> (-90.00%)`	⬇️
pkg/codegen/go/gen.go	`0.00% <0.00%> (-89.71%)`	⬇️
... and 126 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb97581...ef6e697. Read the comment docs.

github-actions · 2022-01-26T19:48:48Z

Diff for pulumi-azure-native with merge commit 4352872

AaronFriel · 2022-01-27T22:35:57Z

@t0yv0 or @pgavlin would appreciate your insight on why this panics

t0yv0 · 2022-01-27T22:39:25Z

Let me return to this tomorrow

mikhailshilkov · 2022-01-28T13:57:41Z

pkg/codegen/internal/test/testdata/aws-get-policy-document-pp/aws-get-policy-document.pp

+}
+
+
+resource "aws_iam_policy" "example" {


I think it panics because "example" is not a valid resource type. It tries to lookup a provider which is supposed to come from that resource type.

Suggested change

resource "aws_iam_policy" "example" {

resource aws_iam_policy "aws:iam:Policy" {

mikhailshilkov · 2022-01-28T14:00:42Z

pkg/codegen/internal/test/testdata/aws-get-policy-document-pp/aws-get-policy-document.pp

@@ -0,0 +1,53 @@
+data "aws_iam_policy_document" "example" {


This likely needs to be an invoke, see

pulumi/pkg/codegen/internal/test/testdata/aws-fargate-pp/aws-fargate.pp

Line 5 in ae247f8

subnets = invoke("aws:ec2:getSubnetIds", {

for example

Ah, for my understanding this is because the .pp files are in PCL?

Yes, I think pp = pulumi program.

t0yv0 · 2022-01-28T22:50:42Z

Apologies, slipping again, will look on Monday

mikhailshilkov · 2022-01-28T22:53:03Z

@t0yv0 I think I responded to the initial question, I'm happy to help @AaronFriel here

github-actions · 2022-02-01T03:24:21Z

Diff for pulumi-random with merge commit a3209b4

github-actions · 2022-02-01T03:24:31Z

Diff for pulumi-azuread with merge commit a3209b4

github-actions · 2022-02-01T03:26:43Z

Diff for pulumi-kubernetes with merge commit a3209b4

github-actions · 2022-02-01T03:27:10Z

Diff for pulumi-gcp with merge commit a3209b4

github-actions · 2022-02-01T03:33:21Z

Diff for pulumi-azure with merge commit a3209b4

github-actions · 2022-02-01T03:35:32Z

Diff for pulumi-aws with merge commit a3209b4

AaronFriel · 2022-02-01T03:41:09Z

@t0yv0 I've rewritten the optional spiller to track state as it recurses into an invoke, then an intrinsic __convert, then an object constructor. I attempted to use replace clauses in go.mod to regenerate the AWS SDK to validate, and it appears that (unrelated to my change) I can't seem to build exactly what's in pulumi-aws/sdk/go. Even with a clean version of pulumi SDK, I get many thousands of lines of diffs.

That said, I did run it and verified by hand that positive changes were visible in doc comments to remove hoisted variables:

  pulumi.Run(func(ctx *pulumi.Context) error {
    _, err := aws.GetRegions(ctx, &GetRegionsArgs{
      AllRegions: pulumi.BoolRef(true),
    }, nil)

     _, err = lb.LookupListener(ctx, &lb.LookupListenerArgs{
       LoadBalancerArn: pulumi.StringRef(selected.Arn),
       Port:            pulumi.IntRef(443),
     }, nil)

This looks to be a positive change everywhere I can see, but again, having difficulty even locally rebuilding the AWS provider SDK reproducibly, so I'd be hesitant to merge this without being able to do that.

github-actions · 2022-02-01T04:09:12Z

Diff for pulumi-azure-native with merge commit a3209b4

CHANGELOG_PENDING.md

mikhailshilkov · 2022-02-01T08:51:30Z

pkg/codegen/go/gen_program_test.go

-				"github.com/pulumi/pulumi/sdk/v3/go/pulumi",
-				"../../../../../../sdk")},
+				"github.com/pulumi/pulumi/sdk/v3",
+				"../../../../../../../sdk")},


Why has this changed?

I think the Go test were running via fluke before, resolving a different "github.com/pulumi/pulumi/sdk/v3" than the local copy. Until I changed this, tests were running using the published version of our SDK, I believe? I don't know Go module resolution well enough to say, but I do know that until I changed this, tests weren't running using my local changes.

Digging into the tests, they temporarily create a go.mod in a subdirectory like below, then run go mod tidy, then add a replacement for the Pulumi SDK. But I think that last step was a no-op.

testdata ├── aws-optionals-pp │ ├── go │ │ ├── aws-optionals.go │ │ └── go.mod # generated at test runtime

From the directory containing go.mod up to sdk is 7 levels:

testdata/aws-optionals-pp/go on  AaronFriel/issue8821 [$?⇕] via  v1.17.6 ❯ ls ../../../../../../../sdk README.md dotnet go go.mod go.sum nodejs proto python

AaronFriel · 2022-02-01T09:37:56Z

Todo:

Split changelog codegen & new Go SDK APIs into two parts.

github-actions · 2022-02-01T09:40:05Z

Diff for pulumi-random with merge commit fb98f44

AaronFriel · 2022-02-01T19:41:39Z

@t0yv0 removing your draft and I believe with this nit commit, all tests should be green again.

github-actions · 2022-02-01T19:43:28Z

Diff for pulumi-random with merge commit 95fdae9

github-actions · 2022-02-01T19:43:53Z

Diff for pulumi-azuread with merge commit 95fdae9

As described in #8821, docs generated for helper functions can be incorrect because optional arguments to parameter objects are not correctly handled. Previously, code would be generated like so: ```go policyDocument, err := iam.GetPolicyDocument(ctx, &iam.GetPolicyDocumentArgs{ Statements: []iam.GetPolicyDocumentStatement{ iam.GetPolicyDocumentStatement{ Sid: "1", //... ``` However "Sid" is of type `*string`. This four helper conversion functions, to handle the primitive types we lower from, and modifies codegen to recursively apply these functions inside of an invoke call. In the new code generation, the above is instead rendered as: ```go policyDocument, err := iam.GetPolicyDocument(ctx, &iam.GetPolicyDocumentArgs{ Statements: []iam.GetPolicyDocumentStatement{ iam.GetPolicyDocumentStatement{ Sid: pulumi.StringRef("1"), //... ```

github-actions · 2022-02-01T19:46:29Z

Diff for pulumi-kubernetes with merge commit 95fdae9

github-actions · 2022-02-01T19:47:06Z

Diff for pulumi-random with merge commit bd241b3

github-actions · 2022-02-01T19:47:32Z

Diff for pulumi-gcp with merge commit 95fdae9

github-actions · 2022-02-01T19:47:39Z

Diff for pulumi-azuread with merge commit bd241b3

github-actions · 2022-02-01T19:50:23Z

Diff for pulumi-kubernetes with merge commit bd241b3

github-actions · 2022-02-01T19:50:25Z

Diff for pulumi-gcp with merge commit bd241b3

github-actions · 2022-02-01T19:51:47Z

Diff for pulumi-azure with merge commit 95fdae9

github-actions · 2022-02-01T19:55:54Z

Diff for pulumi-azure with merge commit bd241b3

github-actions · 2022-02-01T19:57:38Z

Diff for pulumi-aws with merge commit 95fdae9

github-actions · 2022-02-01T19:58:12Z

Diff for pulumi-aws with merge commit bd241b3

github-actions · 2022-02-01T20:28:27Z

Diff for pulumi-azure-native with merge commit 95fdae9

github-actions · 2022-02-01T20:30:33Z

Diff for pulumi-azure-native with merge commit bd241b3

github-actions · 2022-02-01T21:00:32Z

Diff for pulumi-random with merge commit 13fd9de

github-actions · 2022-02-01T21:01:09Z

Diff for pulumi-azuread with merge commit 13fd9de

github-actions · 2022-02-01T21:02:36Z

Diff for pulumi-kubernetes with merge commit 13fd9de

github-actions · 2022-02-01T21:04:07Z

Diff for pulumi-gcp with merge commit 13fd9de

github-actions · 2022-02-01T21:10:31Z

Diff for pulumi-azure with merge commit 13fd9de

github-actions · 2022-02-01T21:12:18Z

Diff for pulumi-aws with merge commit 13fd9de

github-actions · 2022-02-01T21:41:20Z

Diff for pulumi-azure-native with merge commit 13fd9de

t0yv0 self-requested a review January 27, 2022 16:25

mikhailshilkov reviewed Jan 28, 2022

View reviewed changes

AaronFriel force-pushed the AaronFriel/issue8821 branch from e22f9c3 to cc97c7f Compare February 1, 2022 03:18

AaronFriel changed the title ~~wip: panic on running tests?~~ [codegen/go] Improve optional params in invoke Feb 1, 2022

AaronFriel force-pushed the AaronFriel/issue8821 branch from cc97c7f to fa22ee8 Compare February 1, 2022 03:21

mikhailshilkov reviewed Feb 1, 2022

View reviewed changes

AaronFriel force-pushed the AaronFriel/issue8821 branch from fa22ee8 to 6cb2adb Compare February 1, 2022 09:37

AaronFriel marked this pull request as ready for review February 1, 2022 19:41

AaronFriel added 3 commits February 1, 2022 11:44

nit: omit other languages in optionals behavior test

3648c0b

nit: fix the other nit, use Skip to skip test on platforms except Go

ef6e697

AaronFriel force-pushed the AaronFriel/issue8821 branch from 0966242 to ef6e697 Compare February 1, 2022 19:44

Merge branch 'master' into AaronFriel/issue8821

75fc102

AaronFriel merged commit 0295ce9 into master Feb 1, 2022

pulumi-bot deleted the AaronFriel/issue8821 branch February 1, 2022 20:58

	resource "aws_iam_policy" "example" {
	resource aws_iam_policy "aws:iam:Policy" {

[codegen/go] Improve optional params in invoke #8839

[codegen/go] Improve optional params in invoke #8839

Conversation

AaronFriel commented Jan 26, 2022 • edited Loading

github-actions bot commented Jan 26, 2022

github-actions bot commented Jan 26, 2022

github-actions bot commented Jan 26, 2022

github-actions bot commented Jan 26, 2022

github-actions bot commented Jan 26, 2022

github-actions bot commented Jan 26, 2022

codecov bot commented Jan 26, 2022 • edited Loading

Codecov Report

github-actions bot commented Jan 26, 2022

AaronFriel commented Jan 27, 2022

t0yv0 commented Jan 27, 2022

mikhailshilkov Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

mikhailshilkov Jan 28, 2022

Choose a reason for hiding this comment

AaronFriel Jan 31, 2022

Choose a reason for hiding this comment

mikhailshilkov Jan 31, 2022

Choose a reason for hiding this comment

t0yv0 commented Jan 28, 2022

mikhailshilkov commented Jan 28, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

AaronFriel commented Feb 1, 2022 • edited Loading

github-actions bot commented Feb 1, 2022

mikhailshilkov Feb 1, 2022

Choose a reason for hiding this comment

AaronFriel Feb 1, 2022

Choose a reason for hiding this comment

AaronFriel commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

AaronFriel commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

github-actions bot commented Feb 1, 2022

AaronFriel commented Jan 26, 2022 •

edited

Loading

codecov bot commented Jan 26, 2022 •

edited

Loading

mikhailshilkov Jan 28, 2022 •

edited

Loading

AaronFriel commented Feb 1, 2022 •

edited

Loading