Changes to support Go SDK #519

viveklak · 2021-01-26T13:40:40Z

These changes are based on @justinvp's branch here: #457.

I have split things up to separate commits to ease review: https://github.com/pulumi/pulumi-eks/pull/519/commits

provider/cmd/pulumi-gen-eks/main.go

examples/cluster-go/go.mod

provider/cmd/pulumi-gen-eks/main.go

justinvp

LGTM otherwise

justinvp · 2021-01-26T17:04:19Z

examples/cluster-go/main.go

+package main
+
+import (
+	"github.com/pulumi/pulumi-eks/go/eks"


So, one thing I've been wondering...

All our other providers put the SDK files under <repo>/sdk, e.g.:

pulumi/pulumi-aws/sdk/dotnet

pulumi/pulumi-aws/sdk/go

pulumi/pulumi-aws/sdk/nodejs

pulumi/pulumi-aws/sdk/python

However, since this repo (eks) was originally only Node.js, it didn't have an sdk dir -- all the Node.js files were in a nodejs dir in the root fo the repo (pulumi/pulumi-eks/nodejs).

When I went to add Python and .NET, I contemplated moving the existing Node.js files under a new sdk dir, but ended up holding off and instead creating python and dotnet dirs in the root of the repo (with the idea that I'd eventually go back and clean this up). In hindsight, I probably should have just gone ahead and created the sdk dir and moved all the sdk files under it.

The structure of the repo doesn't matter much for using these language SDKs, but with Go, you're importing paths in the repo, and all our other repos put the go files under sdk.

I'm wondering if we should just go ahead and do that for the Go SDK now, before anyone has taken a dependency on the import path.

I think this means reworking the existing go.mod/sum in the root of the repo, something like:

Add sdk dir

Generate Go files under sdk/go.

Remove go.mod/sum in repo root.

Add go.mod/sum in sdk dir (with just dependencies needed for the SDK)

Add go.mod/sum under examples (with just dependencies needed to run the tests)

Move utils/utils.go to examples/utils/utils.go

Add go.mod/sum under provider (with just dependencies necessary for the provider and code generator program)

Ideally, we'd then also move the nodejs, python, and dotnet dirs under sdk, but we can do that later, as a separate change (will need to ensure all the paths are fixed up across the tests, GHA, etc.).

I agree that we should move Go at least to live in the sdk directory since codegen has that assumption baked in several places (e.g., the PkgVersion function regex).

Sounds good. Will try this out and update the PR shortly.

I updated things as you recommended @justinvp. I haven't quite run the tests yet after the reorg but things build. Will look to add a simple starting go test integration test wrapper as well.

CHANGELOG.md

provider/cmd/pulumi-gen-eks/main.go

justinvp · 2021-01-26T23:31:37Z

RE: utils/testvpc/index.ts → examples/utils/testvpc/index.ts

Note the tests depend on the path to the testvpc. This will need to be updated:

pulumi-eks/examples/examples_test.go

Lines 66 to 71 in a1031f9

    
           // Test build of testvpc plugin. 
        
           testPluginDir := filepath.Join("..", "utils", "testvpc") 
        
           absTestPluginDir, err := filepath.Abs(testPluginDir) 
        
           if err != nil { 
        
           	return "", err 
        
           }

And the Makefile:

pulumi-eks/Makefile

Lines 87 to 88 in a1031f9

    
           test_build:: 
        
           	cd utils/testvpc && yarn install && yarn run tsc

Co-authored-by: Justin Van Patten <jvp@justinvp.com>

stack72

Left some feedback about unnecessary package renames as well as the go tests not working in CI

stack72 · 2021-01-27T01:27:40Z

examples/examples_dotnet_test.go

@@ -13,7 +13,7 @@
 // limitations under the License.
 // +build dotnet all

-package examples


This should stay as examples

Something I failed to revert after running into some go mod issues. Fixed now across the examples.

stack72 · 2021-01-27T01:28:03Z

examples/examples_go_test.go

+// limitations under the License.
+// +build go all
+
+package main


This should be package examples

stack72 · 2021-01-27T01:28:16Z

examples/examples_nodejs_test.go

@@ -13,7 +13,7 @@
 // limitations under the License.
 // +build nodejs all

-package examples
+package main


Why is this changing?

stack72 · 2021-01-27T01:28:36Z

examples/examples_py_test.go

@@ -13,7 +13,7 @@
 // limitations under the License.
 // +build python all

-package examples
+package main


Same question as above

stack72 · 2021-01-27T01:29:30Z

examples/cluster-go/go.mod

+	github.com/pulumi/pulumi/sdk/v2 v2.18.2
+)
+
+replace github.com/pulumi/pulumi-eks/sdk => ../../sdk


I'm pretty sure this won't run in CI as it creates a temp GOPATH and it won't be able to traverse the ../../ notation

I'm pretty sure this won't run in CI as it creates a temp GOPATH and it won't be able to traverse the ../../ notation

The reason for this to exist currently is because we don't actually have github.com/pulumi/pulumi-eks/sdk - this change adds it. I added a TODO. I could also exclude the example and add it in a separate PR. Let me know if you have a strong preference either way.

TODO works for me. 👍

stack72 · 2021-01-27T01:35:39Z

examples/examples_go_test.go

+			"aws:region": region,
+		},
+		Dependencies: []string{
+			"Pulumi.Eks",


This is specific to DotnEt

We'll need to fix this up, but we can do that when we hook up the test to CI, separately.

I made the change but I expect to need to fiddle here as part of CI work.

justinvp

LGTM

Vivek Lakshmanan added 4 commits January 26, 2021 05:25

Provider and build changes to support go SDK

f6bef32

Adding basic go example

e636fa4

Adding generated Go SDK

06d927d

Add provider.go back

76f19e9

viveklak requested review from justinvp and lblackstone January 26, 2021 13:40

viveklak added the area/multi-language-components label Jan 26, 2021

Add changelog

f325f87

lblackstone requested changes Jan 26, 2021

View reviewed changes

provider/cmd/pulumi-gen-eks/main.go Show resolved Hide resolved

examples/cluster-go/go.mod Outdated Show resolved Hide resolved

provider/cmd/pulumi-gen-eks/main.go Outdated Show resolved Hide resolved

justinvp reviewed Jan 26, 2021

View reviewed changes

justinvp mentioned this pull request Jan 26, 2021

Initial support for Go #457

Closed

2 tasks

Split into multiple go modules

d343323

lblackstone approved these changes Jan 26, 2021

View reviewed changes

Vivek Lakshmanan and others added 2 commits January 26, 2021 16:53

Update CHANGELOG.md

8697cb9

Co-authored-by: Justin Van Patten <jvp@justinvp.com>

Fix utils path

8b667b1

viveklak force-pushed the vl/SquashedMultilangGo branch from 4447629 to 8b667b1 Compare January 27, 2021 00:54

stack72 reviewed Jan 27, 2021

View reviewed changes

Vivek Lakshmanan added 2 commits January 26, 2021 17:43

Use example as package instead of main

bca9fbe

Add a TODO to remove local module replace

61d34fa

justinvp approved these changes Jan 27, 2021

View reviewed changes

Fix dependency for go test.

b1d3f50

viveklak merged commit 2129c31 into master Jan 27, 2021

pulumi-bot deleted the vl/SquashedMultilangGo branch January 27, 2021 04:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to support Go SDK #519

Changes to support Go SDK #519

viveklak commented Jan 26, 2021 •

edited

Loading

justinvp left a comment

justinvp Jan 26, 2021

lblackstone Jan 26, 2021

viveklak Jan 26, 2021

viveklak Jan 26, 2021

justinvp commented Jan 26, 2021

stack72 left a comment

stack72 Jan 27, 2021

viveklak Jan 27, 2021

stack72 Jan 27, 2021

stack72 Jan 27, 2021

stack72 Jan 27, 2021

stack72 Jan 27, 2021

viveklak Jan 27, 2021

justinvp Jan 27, 2021

stack72 Jan 27, 2021

justinvp Jan 27, 2021

viveklak Jan 27, 2021

justinvp left a comment

Changes to support Go SDK #519

Changes to support Go SDK #519

Conversation

viveklak commented Jan 26, 2021 • edited Loading

justinvp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinvp commented Jan 26, 2021

stack72 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinvp left a comment

Choose a reason for hiding this comment

viveklak commented Jan 26, 2021 •

edited

Loading