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 for dependency management #6118

Closed
wants to merge 1 commit into from
Closed

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Jul 15, 2019

This will close #5153

Move from dep to go mod for dependency management.

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@GeorgeMac GeorgeMac added area/packaging feature request Requests for new plugin and for new features to existing plugins wip labels Jul 15, 2019
@GeorgeMac GeorgeMac removed wip feature request Requests for new plugin and for new features to existing plugins labels Jul 15, 2019
@danielnelson
Copy link
Contributor

Sorry, I broke it in #6101

@GeorgeMac
Copy link
Contributor Author

GeorgeMac commented Jul 16, 2019

s'all good @danielnelson This is bound to conflict with many PRs.

@GeorgeMac
Copy link
Contributor Author

@danielnelson I bumped the gjson version to v1.3.0 and added the pretty package as per that PR to this one.

@glinton
Copy link
Contributor

glinton commented Jul 16, 2019

I seem to remember a discussion somewhere about holding off on go modules for telegraf until after the release of go 1.13 as there could/would be disruptions to modules functionality or something.

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

I don't see anything big coming up in the 1.13 draft release notes. I'm great at delaying but I suppose we might as well start this transition.

- cd "%GOPATH%\src\github.com\golang\dep"
- git checkout -q v0.5.0
- go install -ldflags="-X main.version=v0.5.0" ./cmd/dep
- choco install make
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, but do you know if this gets cached? If so, and especially if we can install specific versions, maybe we should use it for Go too?

Copy link
Contributor Author

@GeorgeMac GeorgeMac Jul 17, 2019

Choose a reason for hiding this comment

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

Im not sure how to get appveyors cache to speed up chocolatey installs. Though I watched a fair few appveyor builds while getting this to work and the majority of the build time seems to be builind telegraf itself. Also I had issues with the old gnu make installation. It just kept failing with a non-descript error code. As soon as I installed it via choco it just worked :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just had a look at there is a choco install golang 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we could cache the directory chocolately uses to download packages into https://help.appveyor.com/discussions/suggestions/592-speeding-up-chocolatey-downloads-and-installs and then invalidate it on appveyor change. We could then explicitly set golang version?
Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah let's try it

README.md Show resolved Hide resolved
go.mod Show resolved Hide resolved
@@ -33,7 +33,7 @@ all:

.PHONY: deps
deps:
dep ensure -vendor-only
go mod download
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to the previous -vendor-only option, we need to ensure that the command here does not modify the go.mod/go.sum files.

Also, we need to ensure that the commands in other targets never download or modify the go.mod/go.sum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like go mod download adhears to the current go.sum contents. Plus we do a go mod verify:

The 'go mod verify' command checks that
the cached copies of module downloads still match both their recorded
checksums and the entries in go.sum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do this in circleci after the module download.

Copy link
Contributor

Choose a reason for hiding this comment

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

If someone added a plugin with a new dependency but didn't add it to go.sum would this still pass? The cached copies would still match the go.sum.

Copy link
Contributor Author

@GeorgeMac GeorgeMac Jul 18, 2019

Choose a reason for hiding this comment

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

Good thought. Will experiment and check back in!

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 cannot find an equivalent to dep ensure --vendor-only. All the go mod commands appear to manipulate the go.sum file so I am adding a manual check using git diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙌 ahh great find

Copy link
Contributor Author

@GeorgeMac GeorgeMac Jul 19, 2019

Choose a reason for hiding this comment

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

Just had a play and basically you can put -mod=readonly on any non mod command. i.e. go build or go install. We would need to add that to every go compiling call (build,install,test). The result of which is that command will fail iff any dependencies in the mod file are not in the build cache. However, if you do a go mod download they will already be there. At which point it still update the go.sum file with the one it has in its build cache. So the result is still an updated go.sum file that is not checked in.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know we use go mod tidy in order to assert that the go.mod and go.sum are in sync with the dependencies that the codebase really uses.

We have a script that checks that go mod tidy didn't produce any change in mod/sum files: https://github.com/influxdata/flux/blob/master/etc/checktidy.sh. That script is used in CI in the step make checktidy: https://github.com/influxdata/flux/blob/master/Makefile#L58.

@danielnelson do you think that approach could solve your problem?

go.mod Show resolved Hide resolved
@@ -19,54 +25,55 @@ jobs:
steps:
- checkout
- restore_cache:
key: vendor-{{ checksum "Gopkg.lock" }}
key: go-mod-v1-{{ checksum "go.sum" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I reran the workflow on CircleCI, and it said:

No cache is found for key: go-mod-v1-5m8iTjU8woo3ixYf_mEKZ4D_98q5g9IaxeuniYq+mTU=

Is the cache working right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be. This is actualy how circle recommend configuring their cache for go modules.

https://circleci.com/blog/go-v1.11-modules-and-circleci/#caching

I can poke and prod a bit more. Perhaps our cache got invalidated for some reason.

@pgray
Copy link

pgray commented Nov 11, 2019

This PR is awesome. Any idea when it'll get merged? Looks like it's mostly there minus a rebase?

@danielnelson danielnelson modified the milestones: 1.13.0, 1.14.0 Nov 25, 2019
@glinton glinton removed their request for review December 19, 2019 20:31
@danielnelson
Copy link
Contributor

Updated in #6912

@danielnelson danielnelson deleted the gm/go-modules branch January 16, 2020 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert to go 1.11 modules
5 participants