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

Go Modules #4109

Merged
merged 58 commits into from Mar 20, 2020
Merged

Go Modules #4109

merged 58 commits into from Mar 20, 2020

Conversation

EvanBoyle
Copy link
Contributor

@EvanBoyle EvanBoyle commented Mar 19, 2020

This PR enables four go modules at /pkg /sdk /tests and /examples. The end goal is to enable users to use go modules when developing pulumi programs with the go sdk.

The general strategy is as follows:

  1. move /cmd -> /pkg/cmd
  2. move all common code shared between /pkg and /sdk into /sdk/go/common
  3. Add remove the top level go.mod and add them for each of /pkg /sdk /tests and /examples.
  4. Update our build machinery to be aware of these go.mod files. Whenever there was a choice, I erred on the side towards less invasive changes. This means keeping /sdk/proto where it was, and keeping the language hosts where they are (more on that below).

Based on feedback from my first attempt at #4089, I tried to move the language hosts (pulumi-language-*) out of the SDKs, but noticed that they only brought a single dependency with them (github.com/Microsoft/go-winio v0.4.14`), so I wound up reverting that change and leaving them as is to reduce the churn on our make/build system.

The biggest delta between this set, and this one from @jen20 #4049, is that this change doesn't duplicate code from pkg. The best example to illustrate this is probably the shared marshalling code. In this changeset, pkg/resource/plugin is moved into common and in the one from James it is copied wholesale:

copied version of pkg/resource/plugin: https://github.com/pulumi/pulumi/blob/a1ccbb17eac288be5f07d52f8c76f60d47b3ea8f/sdk/go/pulumi/resource/marshal.go

used here via resource.Marshal rather than plugin.Marshal: https://github.com/pulumi/pulumi/blob/a1ccbb17eac288be5f07d52f8c76f60d47b3ea8f/sdk/go/pulumi/context.go

I did end up accidently bringing in some unnecessary code under the common package during my first attempt, and was able to eliminate it during this pass (deploy). The new set of dependencies isn't much smaller in the SDK go.mod (33 prev vs 31 for this iteration):

require (
	github.com/Microsoft/go-winio v0.4.14
	github.com/blang/semver v3.5.1+incompatible
	github.com/cheggaaa/pb v1.0.18
	github.com/codahale/hdrhistogram v0.0.0-20161010025455-3a0bb77429bd // indirect
	github.com/djherbis/times v1.2.0
	github.com/fatih/color v1.9.0 // indirect
	github.com/gofrs/flock v0.7.1
	github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b
	github.com/golang/protobuf v1.3.5
	github.com/grpc-ecosystem/grpc-opentracing v0.0.0-20180507213350-8e809c8a8645
	github.com/hashicorp/go-multierror v1.0.0
	github.com/mattn/go-colorable v0.1.6 // indirect
	github.com/mattn/go-runewidth v0.0.8 // indirect
	github.com/mitchellh/go-ps v1.0.0
	github.com/opentracing/basictracer-go v1.0.0 // indirect
	github.com/opentracing/opentracing-go v1.1.0
	github.com/pkg/errors v0.9.1
	github.com/spf13/cast v1.3.1
	github.com/spf13/cobra v0.0.6
	github.com/stretchr/testify v1.5.1
	github.com/texttheater/golang-levenshtein v0.0.0-20191208221605-eb6844b05fc6
	github.com/uber/jaeger-client-go v2.22.1+incompatible
	github.com/uber/jaeger-lib v2.2.0+incompatible // indirect
	golang.org/x/crypto v0.0.0-20200317142112-1b76d66859c6
	golang.org/x/net v0.0.0-20200301022130-244492dfa37a
	golang.org/x/sys v0.0.0-20200317113312-5766fd39f98d
	google.golang.org/grpc v1.28.0
	gopkg.in/cheggaaa/pb.v1 v1.0.28 // indirect
	gopkg.in/src-d/go-git.v4 v4.13.1
	gopkg.in/yaml.v2 v2.2.8
	sourcegraph.com/sourcegraph/appdash v0.0.0-20190731080439-ebfcffb1b5c0
)

If we'd like to reduce this set, we'll have to duplicate some of the shared code to trim the set of dependencies. Given that we've removed the biggest culprits (cloud sdks, terraform deps) I'm inclined to ship this as is and resort to further duplication if we see that this dependency set is untenable based on user feedback.

Notable Changes:

There were a couple of things with our build and engineering system that required rework due to moving towards modules:

  1. glog -> klog: glog uses func init() to parse flags. This causes a panic if the module is imported twice. Now that we have four different modules, we vendor into four folders and have the potential for this init to be called multiple times. To avoid this we moved to klog which is actively maintained by the kubernetes community and has worked around some of these issues. klog requires explicitly calling an explicit init function to parse command line args. I have updated our logging implementation to do this and verified that it works, but it currently has the restriction that the InitLogging function can only be called once. This wasn't previously the case, but nowhere in the code other than tests do we exercise this path. I added this issue to keep track of this: Reenable multiple calls to InitLogging #4121 4e44854 31d8f79 a2b3688 163444b
  2. Python build changes: For python we have a separate versioning script. The versions produced by this script aren't compatible with semver. Our python language plugin has always been minted with the python version, but the code that loads the language host checks that the version is semver compatible, and fails. I'm not sure why we're only seeing this issue after my refactor, we should've been running into this all along. The fix that I made was to use a VERSION that is semver compatible to mint the plugin and a PYPI_VERSION that is used for releasing the python SDK. Go Modules #4109 (comment) 5e1f597
  3. Duplicate the version package in both /sdk and /pkg. This is trivial code, so it shouldn't be a burden or cause future problems. I'm not sure why, but when I shared the package, the versions wouldn't get linked in properly, even though the makefile had been appropriately updated. I have a feeling that this has to do with that code being vendored causing some issue with the linking. 1fec569
  4. Go SDK builds now run tests: "test_all" no longer runs "test_fast" #4119 fa47d86

@EvanBoyle
Copy link
Contributor Author

Not sure why this is only appearing now, but I was seeing failures related to how we parse python versions. Ther version on my local machine was showing up as 1.14.0a1584648098+dirty, which fails the semver parse and prevents the language from initializing:

python/simple ] error: failed to load language plugin python: Invalid character(s) found in patch number "0a1584578188"

I've updated our versioning to include a . between the patch and the alpha/beta/rc character.

PEP440 is pretty loose with requirements, and it looks like this falls within them: https://www.python.org/dev/peps/pep-0440/

@EvanBoyle
Copy link
Contributor Author

so in the last two commits I did two things:

  1. reenable running go SDK tests during CI, which uncovered the next item:
  2. disable our logger test that calls InitLogging twice. This is a limitation I'm happy with taking on right now given that we don't do this in practice anywhere in the codebase. We can revisit later, but I'm concerned about the scope of changes here continuing to grow. Tracking issue: Reenable multiple calls to InitLogging #4121

@EvanBoyle
Copy link
Contributor Author

Un update on #4109 (comment) (python versioning)

I noticed that other repos distinguish between VERSION and PYPI_VERSION https://github.com/pulumi/pulumi-github/blob/ea8b48972534314b879f8c97547a07323bc5ddce/Makefile#L11-L12

It looks like this change never made it into pulumi/pulumi. I've reverted the change to the python versioning code, and updated the makefile to use VERSION and PYPI_VERSION appropriately.

Copy link
Member

@pgavlin pgavlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit and a question. Happy to address these in followup, though.

@@ -62,9 +62,13 @@ build_script:
- cmd: >-
set GO111MODULE=on

go mod tidy
pushd . && cd sdk && go mod tidy && go mod vendor && popd
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pushd . && cd sdk && go mod tidy && go mod vendor && popd
pushd sdk && go mod tidy && go mod vendor && popd

Same below, and in .github/workflows/windows-build.yml.

$(call STEP_MESSAGE)
ifeq ($(NOPROXY), true)
@echo "cd sdk && GO111MODULE=on go mod tidy"; cd sdk && GO111MODULE=on go mod tidy
@echo "cd sdk && GO111MODULE=on go mod vendor"; cd sdk && GO111MODULE=on go mod vendor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were switching to go mod download?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this with go mod download and got errors. I'll give this another try and open up a follow up PR.

@EvanBoyle EvanBoyle merged commit 1c4496a into master Mar 20, 2020
@pulumi-bot pulumi-bot deleted the evan/gomod branch March 20, 2020 19:01
metral added a commit to metral/pulumi-eks that referenced this pull request Mar 29, 2020
metral added a commit to metral/pulumi-eks that referenced this pull request Mar 29, 2020
metral added a commit to metral/pulumi-eks that referenced this pull request Mar 30, 2020
metral added a commit to pulumi/pulumi-eks that referenced this pull request Mar 30, 2020
metral added a commit to pulumi/pulumi-eks that referenced this pull request Mar 30, 2020
metral added a commit to pulumi/pulumi-eks that referenced this pull request Mar 30, 2020
metral added a commit to pulumi/pulumi-eks that referenced this pull request Mar 30, 2020
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.

None yet

2 participants