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

Support Go Modules #25

Merged
merged 1 commit into from
Sep 10, 2019
Merged

Support Go Modules #25

merged 1 commit into from
Sep 10, 2019

Conversation

SamWhited
Copy link
Contributor

@SamWhited SamWhited commented Sep 6, 2019

Attempting to support modules for containerd projects. Specifically this PR adds a go.mod file for this repo and updates the vendor validation script to support go modules alongside vndr by checking for the presence of vendor.conf and using vndr if it exists or go mod vendor if it does not.

@estesp
Copy link
Member

estesp commented Sep 9, 2019

Sorry that we hadn't removed the release-tool from this repo yet; I just opened a PR #27 to correct this. Can you move this work to the new release-tool repo?

@SamWhited
Copy link
Contributor Author

SamWhited commented Sep 9, 2019

Sure thing, as soon as your PR is merged I'll remove everything from this PR except the validation script fix, and then after that's merged I can update the release-tools repo to use Go Modules.

@SamWhited
Copy link
Contributor Author

Done, PTAL.

@dmcgowan
Copy link
Member

dmcgowan commented Sep 9, 2019

This was the previous method used in some PRs and we suggested adding here #26

@SamWhited
Copy link
Contributor Author

SamWhited commented Sep 9, 2019

Adding tidy sounds good, but we probably don't want to check the value of GO111MODULE because "on" will be the default value when Go 1.14 is released and if it's defaulting to on it still won't show up in go env GO111MODULE. It could also be set to auto and we could be outside of GOPATH in which case we may still want to use module mode. Since this is repo specific I don't think we can depend on an environment variable, but the go.mod file being present (or the lack of a vendor.conf file) is probably a good indicator that we intended to use modules with this repo.

EDIT: Added go mod tidy as well.

Signed-off-by: Sam Whited <sam@samwhited.com>
@estesp
Copy link
Member

estesp commented Sep 9, 2019

This method seems to make sense to me, and helps have an easy "switch" for transition--when the vendor.conf is removed from a repo, you get automatic enablement of the go mod checks.

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@kzys
Copy link
Member

kzys commented Sep 9, 2019

I thought we would have a transition period where we still use vndr but make sure go mod is working. Dependencies like gotest.tools should be updated before adding go.mod.

If we don't have the transition period, we can just use either vndr or go mod, as this PR proposed.

@SamWhited
Copy link
Contributor Author

SamWhited commented Sep 9, 2019

A transition period seems like you'd just wind up with the vendor.conf and go.mod being out of sync somehow, then when you switch over fully to go.mod you accidentally downgrade a dependency. You would possibly also want to add a test that always checked that they were in sync on each PR to make sure this didn't happen by mistake.

This seems like a lot of work to me instead of just upgrading repo-by-repo to go.mod.

@dmcgowan
Copy link
Member

Sure, let's try this approach, it seems very reasonable and requires no extra configuration from the caller.

LGTM

@dmcgowan dmcgowan merged commit e453310 into containerd:master Sep 10, 2019
@SamWhited SamWhited deleted the modules branch September 10, 2019 21:27
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

4 participants