Bump Go to 1.17.5 and start using modules by errordeveloper · Pull Request #218 · docker/libkv

errordeveloper · 2021-12-15T12:46:33Z

thaJeztah

Was this to fix the vendor failure on that PR? So;

go: finding module for package camlistore.org/pkg/throttle
github.com/docker/docker/libnetwork imports
	github.com/docker/libkv/store/zookeeper imports
	github.com/samuel/go-zookeeper/zk tested by
	github.com/samuel/go-zookeeper/zk.test imports
	camlistore.org/pkg/throttle: module camlistore.org@latest found (v0.0.0-20171230002226-a5a65f0d8b22), but does not contain package camlistore.org/pkg/throttle
github.com/docker/docker/daemon/logger/gcplogs imports
	cloud.google.com/go/logging imports
	cloud.google.com/go tested by
	cloud.google.com/go.test imports
	cloud.google.com/go/bigquery imports
	cloud.google.com/go/internal/detect: module cloud.google.com/go@latest found (v0.99.0, replaced by cloud.google.com/go@v0.44.3), but does not contain package cloud.google.com/go/internal/detect

I'm not strictly against doing this, but overall, we should try to deprecate / replace this package;

the require lines in this repository likely shouldn't affect the Moby PR (as it would already default to picking "latest"), and (as mentioned) the replace directive is non-transitional, so would still be needed in the moby repository; wondering if we should start with adding (if needed) replace lines in the Moby PR 🤔

thaJeztah · 2021-12-15T13:38:07Z

go.mod

+
+require (
+	github.com/coreos/etcd v3.3.25+incompatible
+	github.com/hashicorp/consul/api v1.1.0


Comparing this with the current version used in moby; hashicorp/consul@v0.5.2...api/v1.1.0

Here is another problem, go mod tidy trips over this library because v0.5.2 doesn't use models, but new version of it does. Without and there is no issue with this library at all until you run go mod tidy...

github.com/docker/docker/libnetwork imports github.com/docker/libkv/store/consul imports github.com/hashicorp/consul/api loaded from github.com/hashicorp/consul@v0.5.2, but go 1.16 would fail to locate it: ambiguous import: found package github.com/hashicorp/consul/api in multiple modules: github.com/hashicorp/consul v0.5.2 (/Users/ilya/Code/moby/.gopath/pkg/mod/github.com/hashicorp/consul@v0.5.2/api) github.com/hashicorp/consul/api v1.1.0 (/Users/ilya/Code/moby/.gopath/pkg/mod/github.com/hashicorp/consul/api@v1.1.0)

@crazy-max and I tried a few different replace options, still the same issue...

thaJeztah · 2021-12-15T13:39:45Z

go.mod

+require (
+	github.com/coreos/etcd v3.3.25+incompatible
+	github.com/hashicorp/consul/api v1.1.0
+	github.com/samuel/go-zookeeper v0.0.0-20201211165307-7117e9ea2414


Comparing this one as well samuel/go-zookeeper@d0e0d8e...7117e9e

thaJeztah · 2021-12-15T13:40:41Z

go.mod

+	github.com/coreos/etcd v3.3.25+incompatible
+	github.com/hashicorp/consul/api v1.1.0
+	github.com/samuel/go-zookeeper v0.0.0-20201211165307-7117e9ea2414
+	github.com/stretchr/testify v1.7.0
+	go.etcd.io/bbolt v1.3.6


And this one; etcd-io/bbolt@v1.3.5...v1.3.6

thaJeztah · 2021-12-15T13:42:47Z

go.mod

+	sigs.k8s.io/yaml v1.3.0 // indirect
+)
+
+replace google.golang.org/grpc => google.golang.org/grpc v1.23.0


note that replace rules are non-transitive, so this line would still be needed in moby/moby

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

crazy-max · 2021-12-15T15:20:29Z

.travis.yml


 go:
-  - 1.8.7
+  - 1.17.5


CI seems broken, I will open a PR to fix it first if you want. SGTY @thaJeztah?

Yeah, although don't spend too much time on it; we really need to reduce use of this dependency (it's good to know if things are not broken of course)

Yeah that's a huge jump 1.8 > 1.17, better to do that in a follow-up 😅

I guess I'm just being very opportunistic and putting all my trust in Go's backwards compatibility promise 😹
And, to be fair, this has already been built in Moby using all mort recent versions of Go...

errordeveloper · 2021-12-15T15:40:11Z

Was this to fix the vendor failure on that PR?

Yes. Just trying to address the issue with zookeeper client and it's dead camlistore.org dependency... I'm still quite confused about these. Will report back once I have a slightly clearer idea.

errordeveloper · 2021-12-15T15:48:23Z

wondering if we should start with adding (if needed) replace lines in the Moby PR

I tried, and it didn't work. I think I'll need to update this repo to use the new client, make it work and go back to Moby.

It also looks like the issue in Moby PR comes to surface when go mod tidy runs and doesn't actually matter for any other steps of the build.

thaJeztah · 2021-12-15T15:53:10Z

I tried, and it didn't work. I think I'll need to update this repo to use the new client, make it work and go back to Moby.

Gotcha; so I didn't initially notice (thanks @crazy-max) that CI wasn't running on this PR, and thought no local changes were needed (besides the go module versions)

errordeveloper · 2021-12-15T15:55:35Z

Gotcha; so I didn't initially notice (thanks @crazy-max) that CI wasn't running on this PR, and thought no local changes were needed (besides the go module versions)

I was hoping CI would run... I ran go test ./... locally, but not sure if failures I'm seeing are to do with lack of proper setup or not. I can dig into it, but I thought of trying to make switch to new ZK client first and see if the code even builds without changes or now.

crazy-max · 2021-12-15T15:58:29Z

Gotcha; so I didn't initially notice (thanks @crazy-max) that CI wasn't running on this PR, and thought no local changes were needed (besides the go module versions)

I was hoping CI would run... I ran go test ./... locally, but not sure if failures I'm seeing are to do with lack of proper setup or not. I can dig into it, but I thought of trying to make switch to new ZK client first and see if the code even builds without changes or now.

Yeah zookeeper, etcd and consul bins seems to be required.

errordeveloper · 2021-12-15T16:10:17Z

Can we re-enable Travis for this repo?

errordeveloper · 2021-12-15T16:18:44Z

store/zookeeper/zookeeper.go

 	"github.com/docker/libkv"
 	"github.com/docker/libkv/store"
-	zk "github.com/samuel/go-zookeeper/zk"
+	zk "github.com/go-zookeeper/zk"


This seems to work without further changes!

errordeveloper · 2021-12-15T16:59:24Z

Coming for now in favour of #219. Switching to modules wasn't as helpful as I originally thought it might be :)

- use `vendor.mod` instead of `go.mod` to avoid issues to do with use of CalVer, not SemVer - ensure most of the dependency versions do not change - only zookeeper client has to change (via docker/libkv#218) as previously used version is no longer maintained and has missing dependencies

- use `vendor.mod` instead of `go.mod` to avoid issues to do with use of CalVer, not SemVer - ensure most of the dependency versions do not change - only zookeeper client has to change (via docker/libkv#218) as previously used version is no longer maintained and has missing dependencies Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

- use `vendor.mod` instead of `go.mod` to avoid issues to do with use of CalVer, not SemVer - ensure most of the dependency versions do not change - only zookeeper client has to change (via docker/libkv#218) as previously used version is no longer maintained and has missing dependencies Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah reviewed Dec 15, 2021

View reviewed changes

Bump Go to 1.17.5 and start using modules

a06a008

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

errordeveloper force-pushed the go-mod branch from 28d170e to a06a008 Compare December 15, 2021 14:23

crazy-max reviewed Dec 15, 2021

View reviewed changes

Use new Zookeeper library

0a1b3e6

errordeveloper commented Dec 15, 2021

View reviewed changes

errordeveloper mentioned this pull request Dec 15, 2021

vendor: use go mod moby/moby#43075

Closed

errordeveloper closed this Dec 15, 2021

Conversation

errordeveloper commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errordeveloper commented Dec 15, 2021

Uh oh!

errordeveloper commented Dec 15, 2021

Uh oh!

thaJeztah commented Dec 15, 2021

Uh oh!

errordeveloper commented Dec 15, 2021

Uh oh!

crazy-max commented Dec 15, 2021

Uh oh!

errordeveloper commented Dec 15, 2021

Uh oh!

errordeveloper Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errordeveloper commented Dec 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

errordeveloper commented Dec 15, 2021 •

edited

Loading

errordeveloper Dec 15, 2021 •

edited

Loading