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

Use Go Modules #15

Merged
merged 1 commit into from
Sep 11, 2019
Merged

Use Go Modules #15

merged 1 commit into from
Sep 11, 2019

Conversation

SamWhited
Copy link
Contributor

Please consider also adding a semver compatible tag so this can be used from other packages that support Go Modules, eg. to start you could tag this commit as v0.0.1.

@crosbymichael
Copy link
Member

We usually don't vendor package dependencies. That is why you don't see a vendor.conf here. We let the calling application/project pin the deps. With this in mind, do you think we should add go mod here?

@dmcgowan
Copy link
Member

I think it is worth adding go.mod to all our (Go) projects.

Can you add the vendor line so it can test the projects is working (just merged that in)

  - ../project/script/validate/vendor

@SamWhited
Copy link
Contributor Author

@crosbymichael it definitely makes sense not to vendor deps in a library, but adding a go.mod would be good because it makes it more likely that projects which use this select a compatible version of any dependencies that are required by this library.

@dmcgowan done

@dmcgowan
Copy link
Member

start you could tag this commit as v0.0.1

Maybe after the release tool is updated to support go mods. I am not a huge fan of arbitrarily creating tagged versions just because some tooling doesn't like hashes, if we tag we should follow a unified release process.

@SamWhited
Copy link
Contributor Author

@dmcgowan oh, I didn't think about this, but the files will always be modified because nothing is vendored right now. What is it exactly you were hoping to validate?

@codecov-io
Copy link

codecov-io commented Sep 10, 2019

Codecov Report

Merging #15 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #15   +/-   ##
======================================
  Coverage    64.1%   64.1%           
======================================
  Files           1       1           
  Lines          78      78           
======================================
  Hits           50      50           
  Misses         21      21           
  Partials        7       7

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7312978...a54a737. Read the comment docs.

@dmcgowan
Copy link
Member

dmcgowan commented Sep 10, 2019

@SamWhited By using go 1.12 and having the go.mod available, it is then safe to assume that the tests will be using that defined version? I wanted to validate we were testing against the correct version (previously that would require vendoring), but seems like that may be redundant now.

@SamWhited
Copy link
Contributor Author

SamWhited commented Sep 10, 2019

@dmcgowan yes, unless modules are turned off somewhere or we're inside the GOPATH with 1.12. Probably best to just manually set GO111MODULE=on for 1.12.

EDIT: done.

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

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

LGTM

@crosbymichael
Copy link
Member

LGTM

@crosbymichael crosbymichael merged commit 5eb2502 into containerd:master Sep 11, 2019
@SamWhited SamWhited deleted the module branch September 11, 2019 14:26
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