Switch to Go modules for dependency management #8041
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shoenig
force-pushed
the
x-gomod
branch
9 times, most recently
from
f7d8cd7
to
e0725a8
Compare
shoenig
force-pushed
the
x-gomod
branch
2 times, most recently
from
4cf993f
to
df0988e
Compare
shoenig
changed the title
rboyer
reviewed
Jun 1, 2020
tgross
approved these changes
Jun 2, 2020
GNUmakefile
Outdated
| .PHONY: dev | ||
| dev: GOOS=$(shell go env GOOS) | ||
| dev: GOARCH=$(shell go env GOARCH) | ||
| dev: GOPATH=$(shell go env GOPATH) | ||
| dev: DEV_TARGET=pkg/$(GOOS)_$(GOARCH)/nomad | ||
| dev: vendorfmt changelogfmt hclfmt ## Build for the current development platform | ||
| dev: sync changelogfmt hclfmt ## Build for the current development platform |
There was a problem hiding this comment.
This seems to be a significant difference in behavior for something we probably run a lot in local development. When we were doing vendorfmt that's more the equivalent of go mod tidy, whereas now we have the side-effect of potentially making a bunch of changes to what's been vendored on every local build.
There was a problem hiding this comment.
How fast is make sync? I use make dev frequently for compiling, and would love to keep the overhead somewhat minimal. vendorfmt is basically instantaneous.
There was a problem hiding this comment.
Good catch @tgross ! Seems like we can maintain the previous behavior by breaking out this sync target into 2: tidy and sync. The dev target would only reference tidy, which would be close to vendorfmt (with the exception that tidy removes unused modules, but I think that's okay). sync would then just be a convenience target to run both go mod tidy and go mod vendor, which is handy when updating the go.mod with a new dependency/version.
notnoop
approved these changes
Jun 2, 2020
There was a problem hiding this comment.
This looks great! Thank you for the effort and getting this across the line.
I'm unsure about all the vendored changes, so it might be nice doing some quick check of how the go mod versions compare with govendor and if they lead to significant results (e.g. minor change to align on releases vs accidentally went back to a super old version).
GNUmakefile
Outdated
| .PHONY: dev | ||
| dev: GOOS=$(shell go env GOOS) | ||
| dev: GOARCH=$(shell go env GOARCH) | ||
| dev: GOPATH=$(shell go env GOPATH) | ||
| dev: DEV_TARGET=pkg/$(GOOS)_$(GOARCH)/nomad | ||
| dev: vendorfmt changelogfmt hclfmt ## Build for the current development platform | ||
| dev: sync changelogfmt hclfmt ## Build for the current development platform |
There was a problem hiding this comment.
How fast is make sync? I use make dev frequently for compiling, and would love to keep the overhead somewhat minimal. vendorfmt is basically instantaneous.
| github.com/cyphar/filepath-securejoin v0.2.3-0.20190205144030-7efe413b52e1 // indirect | ||
| github.com/docker/cli v0.0.0-20200303215952-eb310fca4956 | ||
| github.com/docker/distribution v2.7.1+incompatible | ||
| github.com/docker/docker v17.12.0-ce-rc1.0.20200330121334-7f8b4b621b5d+incompatible |
There was a problem hiding this comment.
Managing recent docker packages have been troublesome, due to x/sys/windows changes (e.g. #8042). I see that Windows build in CI passes, so that's great :). Was that an issue during the change? how did you deal with it?
There was a problem hiding this comment.
Really the only troublemaker was github.com/NYTimes/gziphandler, which made some backwards incompatible changes in the way it parses gzip headers (but still compiled just fine). Luckily we have a volume migration integration test that caught the problem early, but I don't think we can ever upgrade from v1.0.0
This PR switches the Nomad repository from using govendor to Go modules for managing dependencies. Aspects of the Nomad workflow remain pretty much the same. The usual Makefile targets should continue to work as they always did. The API submodule simply defers to the parent Nomad version on the repository, keeping the semantics of API versioning that currently exists.
|
This is a manifest of the changes from packages to modules. It tries to pull the version if it exists, otherwise uses the revision. generated from this doodad |
|
@shoenig yay!!! Thanks for all of your hard work on this!
Can you file an issue on our side to track that? |
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR switches the Nomad repository from using
govendorto Go modules for managing dependencies. Aspects of the Nomad workflow remain pretty much the same. The usual Makefile targets continue to work as they previously did. The API module (which now becomes a submodule) continues to be un-versioned, keeping the semantics of API versioning that currently exists.One difference:
go installondarwinrequires runningmake devfirst, which copies some CGO files into the right place. The reason being that Go modules purges non-Go files out of modules (including from thevendordirectory). With any luck, the upstream will be fixed soon in shirou/gopsutil#885Non
vendorfiles changed (i.e. candidates for review)