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

migrate to go 1.14 + modules #2214

Merged
merged 14 commits into from Mar 3, 2020

Conversation

skriss
Copy link
Member

@skriss skriss commented Jan 22, 2020

closes #1886
xref #2127

This PR migrates Velero from dep to go modules for dependency management, and upgrades the go version to 1.13 1.14. Heavily inspired by @ashish-amarnath's work in #2127.

Probably easiest to review commit-by-commit, as I broke it up into small steps.

I believe the only thing that's not using modules yet is the Kubernetes code-generator tool - it's still being cloned into a $GOPATH-based dir and being run from there due to some compatibility issues (I'll update with links to issues).

I've tested all of the main workflows and things seemed to be working properly, but I would definitely appreciate some further testing before getting ready to merge this.

@carlisia
Copy link
Contributor

carlisia commented Jan 27, 2020

Hey @skriss I pulled this branch locally to run tests. Got some errors "not finding package" and am thinking there needs to be a go mod command run? I can google but if that's the case, should some documentation be added to the Contribute section?

@skriss
Copy link
Member Author

skriss commented Jan 27, 2020

Hey @skriss I pulled this branch locally to run tests. Got some errors "not finding package" and am thinking there needs to be a go mod command run? I can google but if that's the case, should some documentation be added to the Contribute section?

Hmm, not sure off the top of my head what issue you're running into -- can you provide some more detail (go version, is the code in $GOPATH, what's $GO111MODULE set to, what command did you run, full error output)?

@carlisia
Copy link
Contributor

Nvm, all I had to do was upgrade to 1.13.

carlisia
carlisia previously approved these changes Jan 27, 2020
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

This looks good to me.
Also, ran the tests using this branch and it's all good.

@skriss skriss marked this pull request as ready for review January 29, 2020 15:40
@skriss
Copy link
Member Author

skriss commented Jan 29, 2020

moving out of draft but I still have some followups

@nrb nrb added the On Hold label Feb 26, 2020
@nrb nrb added this to the v1.4 milestone Feb 26, 2020
@nrb
Copy link
Contributor

nrb commented Feb 26, 2020

Putting this on hold so it can be moved to post-1.3.

@skriss skriss changed the title migrate to go 1.13 + modules migrate to go 1.14 + modules Feb 27, 2020
@skriss
Copy link
Member Author

skriss commented Feb 27, 2020

rebased, addressed feedback, and updated to go 1.14. let's see if it passes CI..

@ashish-amarnath
Copy link
Contributor

The CI is passing 🎉

@skriss
Copy link
Member Author

skriss commented Feb 27, 2020

I'll do a little bit of retesting on this just to ensure nothing broke with the latest changes, but I think it's good to go.

We'll hold until just after we release 1.3.

Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Copy link
Contributor

@nrb nrb left a comment

Choose a reason for hiding this comment

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

During my manual testing of building and running through some sample backups/restores (not with restic), this appears to work as expected. I do have a question inline, though.

hack/build-image/Dockerfile Show resolved Hide resolved
$@

go run ${GOPATH}/src/sigs.k8s.io/controller-tools/cmd/controller-gen/main.go \
controller-gen \
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@skriss
Copy link
Member Author

skriss commented Mar 3, 2020

I retested the CI steps as well as the local dev stuff (go build/go test locally) inside and out of $GOPATH, and everything LGTM, so from my perspective this is ready to go. I will file a couple follow-up enhancement issues as well.

Copy link
Contributor

@nrb nrb left a comment

Choose a reason for hiding this comment

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

Thanks for shepherding this through @skriss and @ashish-amarnath! Hopefully all this work will make moving the plugins over much easier.

Copy link
Contributor

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@ashish-amarnath ashish-amarnath merged commit 3e1b8e0 into vmware-tanzu:master Mar 3, 2020
@skriss skriss deleted the final-go-mod-migrate branch March 3, 2020 22:13
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.

Migrate to go modules
5 participants