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

[codegen/go] Fix Go resource registrations #6641

Merged
merged 1 commit into from
Mar 31, 2021
Merged

Conversation

justinvp
Copy link
Member

@justinvp justinvp commented Mar 29, 2021

We've been emitting calls to New<Resource> for resource registrations in Go, passing nil for args. However, some of those New<Resource> functions actually check for nil args and return an error if the resource has required arguments.

At first, I was looking for a way to check inside New<Resource> if the URN option was specified and in that case not error on nil args (like we do in other languages), but we don't provide a way to access the resource option values outside the Go SDK), so I don't think there is a way to do it this way for Go.

So instead, this change updates the registration code to call ctx.RegisterResource directly instead of New<Resource>, where we can pass a nil args.

Fixes pulumi/pulumi-eks#537

We've been emitting calls to `New<Resource>` for resource registrations
in Go, passing `nil` for args. However, some of those `New<Resource>`
functions actually check for `nil` args and return an error if the
resource has required arguments.

At first, I was looking for a way to check inside `New<Resource>` if
the `URN` option was specified and in that case not error on
`nil` args (like we do in other languages), but we don't provide a way
to access the resource option values outside the Go SDK), so I don't
think there is a way to do it this way for Go.

So instead, this change updates the registration code to call
`ctx.RegisterResource` directly instead of `New<Resource>`, where we can
pass a `nil` args.
@github-actions
Copy link

Diff for pulumi-random with merge commit ce338d8

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit ce338d8

@github-actions
Copy link

Diff for pulumi-azuread with merge commit ce338d8

@github-actions
Copy link

Diff for pulumi-gcp with merge commit ce338d8

@lblackstone
Copy link
Member

I confirmed that this fix worked by compiling new versions of pulumi-kubernetes, pulumi-eks, and pulumi-aws, and then running the following program:

package main

import (
	"os"

	"github.com/pulumi/pulumi-aws/sdk/v3/go/aws/iam"
	"github.com/pulumi/pulumi-eks/sdk/go/eks"
	"github.com/pulumi/pulumi/sdk/v2/go/pulumi"
)

func main() {
	pulumi.Run(func(ctx *pulumi.Context) error {

		eksCluster, err := eks.NewCluster(ctx, "eks-cluster", &eks.ClusterArgs{
			InstanceType:       pulumi.String("t2.medium"),
			DesiredCapacity:    pulumi.Int(1),
			MaxSize:            pulumi.Int(1),
			MinSize:            pulumi.Int(1),
			CreateOidcProvider: pulumi.Bool(true),
			ProviderCredentialOpts: &eks.KubeconfigOptionsArgs{
				ProfileName: pulumi.String(os.Getenv("AWS_PROFILE")),
			},
		})
		if err != nil {
			return err
		}

		oidcUrl := eksCluster.Core.OidcProvider().ApplyT(
			func(oidc *iam.OpenIdConnectProvider) pulumi.StringOutput { return oidc.Url })
		ctx.Export("oidcUrl", oidcUrl)

		return nil
	})
}

Screen Shot 2021-03-31 at 3 45 38 PM

@lblackstone lblackstone marked this pull request as ready for review March 31, 2021 21:47
@lblackstone lblackstone self-requested a review March 31, 2021 21:48
@github-actions
Copy link

Diff for pulumi-random with merge commit ce338d8

@github-actions
Copy link

Diff for pulumi-azuread with merge commit ce338d8

@justinvp justinvp added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Mar 31, 2021
@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit ce338d8

@justinvp
Copy link
Member Author

Thanks for the help, @lblackstone!

@github-actions
Copy link

Diff for pulumi-gcp with merge commit ce338d8

@github-actions
Copy link

Diff for pulumi-azure with merge commit ce338d8

@github-actions
Copy link

Diff for pulumi-aws with merge commit ce338d8

@github-actions
Copy link

Diff for pulumi-azure-native with merge commit ce338d8

@lblackstone lblackstone merged commit a9f0aea into master Mar 31, 2021
@pulumi-bot pulumi-bot deleted the justin/goregister branch March 31, 2021 22:50
@bothra90
Copy link

bothra90 commented Apr 1, 2021

Thanks for the fix, @justinvp!

lblackstone added a commit to pulumi/pulumi-kubernetes that referenced this pull request Apr 8, 2021
 - [sdk/go] Fix Go resource registrations (pulumi/pulumi#6641)
 - [sdk/python] Support `<Resource>Args` classes (pulumi/pulumi#6525)
lblackstone added a commit to pulumi/pulumi-kubernetes that referenced this pull request Apr 14, 2021
 - [sdk/go] Fix Go resource registrations (pulumi/pulumi#6641)
 - [sdk/python] Support `<Resource>Args` classes (pulumi/pulumi#6525)
lblackstone added a commit to pulumi/pulumi-kubernetes that referenced this pull request Apr 14, 2021
 - [sdk/go] Fix Go resource registrations (pulumi/pulumi#6641)
 - [sdk/python] Support `<Resource>Args` classes (pulumi/pulumi#6525)
lblackstone added a commit to pulumi/pulumi-kubernetes that referenced this pull request Apr 15, 2021
- [sdk/go] Fix Go resource registrations (pulumi/pulumi#6641)
 - [sdk/python] Support `<Resource>Args` classes (pulumi/pulumi#6525)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Go: Error when accessing CoreDataOutput of eks.Cluster
3 participants