vendor: eliminate some of the `replace` directives by errordeveloper · Pull Request #3048 · moby/swarmkit

errordeveloper · 2021-12-15T09:29:57Z

codecov · 2021-12-15T09:53:26Z

Codecov Report

Merging #3048 (4b579ac) into master (234cb83) will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3048      +/-   ##
==========================================
+ Coverage   62.14%   62.29%   +0.14%     
==========================================
  Files         155      155              
  Lines       24533    24533              
==========================================
+ Hits        15247    15282      +35     
+ Misses       7682     7651      -31     
+ Partials     1604     1600       -4

errordeveloper · 2021-12-15T10:20:14Z

--- FAIL: TestRaftQuorumRecovery (10.55s)
    testutils.go:97: 
        	Error Trace:	testutils.go:97
        	            				raft_test.go:343
        	Error:      	Received unexpected error:
        	            	did not find a ready leader in member list
        	            	github.com/docker/swarmkit/manager/state/raft/testutils.WaitForCluster.func1
        	            		/home/circleci/.go_workspace/src/github.com/docker/swarmkit/manager/state/raft/testutils/testutils.go:74
        	            	github.com/docker/swarmkit/testutils.PollFuncWithTimeout
        	            		/home/circleci/.go_workspace/src/github.com/docker/swarmkit/testutils/poll.go:22
        	            	github.com/docker/swarmkit/testutils.PollFunc
        	            		/home/circleci/.go_workspace/src/github.com/docker/swarmkit/testutils/poll.go:36
        	            	github.com/docker/swarmkit/manager/state/raft/testutils.WaitForCluster
        	            		/home/circleci/.go_workspace/src/github.com/docker/swarmkit/manager/state/raft/testutils/testutils.go:60
        	            	github.com/docker/swarmkit/manager/state/raft_test.TestRaftQuorumRecovery
        	            		/home/circleci/.go_workspace/src/github.com/docker/swarmkit/manager/state/raft/raft_test.go:343
        	            	testing.tRunner
        	            		/usr/local/go/src/testing/testing.go:1259
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1581
        	            	polling failed
        	            	github.com/docker/swarmkit/testutils.PollFuncWithTimeout
        	            		/home/circleci/.go_workspace/src/github.com/docker/swarmkit/testutils/poll.go:28
        	            	github.com/docker/swarmkit/testutils.PollFunc
        	            		/home/circleci/.go_workspace/src/github.com/docker/swarmkit/testutils/poll.go:36
        	            	github.com/docker/swarmkit/manager/state/raft/testutils.WaitForCluster
        	            		/home/circleci/.go_workspace/src/github.com/docker/swarmkit/manager/state/raft/testutils/testutils.go:60
        	            	github.com/docker/swarmkit/manager/state/raft_test.TestRaftQuorumRecovery
        	            		/home/circleci/.go_workspace/src/github.com/docker/swarmkit/manager/state/raft/raft_test.go:343
        	            	testing.tRunner
        	            		/usr/local/go/src/testing/testing.go:1259
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1581
        	Test:       	TestRaftQuorumRecovery
FAIL
coverage: 70.8% of statements
FAIL	github.com/docker/swarmkit/manager/state/raft	39.571s

Could this be an issue with upgraded deps or is it a flake? 🤔

errordeveloper · 2021-12-15T10:23:40Z

Could this be an issue with upgraded deps or is it a flake? 🤔

Ok, looks like a flake as it all tests passed on a previous commit.

crazy-max

Great! Minor nit, also squash commits

vendor.mod

Files in GOPATH have read-only persmissions, after simply copying them can result in an access error. Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

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

errordeveloper · 2021-12-17T15:22:37Z

@thaJeztah I think we should merge this, when I update Moby I'll make sure to use 234cb83 in the first update.

crazy-max · 2022-01-20T08:43:05Z

@errordeveloper moby/moby#43101 has been merged. I think we can move forward with this one.

thaJeztah · 2022-01-20T11:21:35Z

vendor.mod

-	gopkg.in/fsnotify.v1 v1.4.7 // indirect
 	gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
+	gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c // indirect


Was wondering why this didn't use a tagged release, but looks like no v3 releases exist yet; https://github.com/go-yaml/yaml/tags

eh... also gopkg.in does some magic, which I never had the time to try and understand, very few libs use it anyway.

thaJeztah · 2022-01-20T11:27:39Z

vendor.mod

 replace (
-	github.com/Microsoft/go-winio => github.com/Microsoft/go-winio v0.4.15
-	github.com/akutz/gosync => github.com/akutz/gosync v0.1.0
 	github.com/containerd/containerd => github.com/containerd/containerd v1.5.2


Was this replace still needed? Wondering if we can just update the indirect dependency to the latest v1.5.x release

I should note that if we need to keep some of the replace rules, then it might be good to add a comment in the corresponding require section, and to add a comment that "the actual version is overridden in the replace section", similar to https://github.com/moby/buildkit/blob/390c6886f4a310af5ee0d45eaab35dc36b21f23e/go.mod#L22-L23

It's easy to overlook that there's a replace rule for a dependency, and people may then forget to update (or remove) them when updating dependencies.

As the time of the PR states, the goal is to remove only some of the replace directives, i.e. seemingly trivial version bumps that won't affect downstream in any way. So runc and containerd are untouched for the purpose of this PR, as I cannot assume those are trivial and won't affect Moby.

thaJeztah · 2022-01-20T11:28:20Z

vendor.mod

-	github.com/matttproud/golang_protobuf_extensions => github.com/matttproud/golang_protobuf_extensions v1.0.1
-	github.com/onsi/ginkgo => github.com/onsi/ginkgo v1.8.0
-	github.com/onsi/gomega => github.com/onsi/gomega v1.5.0
 	github.com/opencontainers/runc => github.com/opencontainers/runc v1.0.0-rc95


Same for this one; if we update containerd, this may not be needed (I see containerd 1.5.x now uses run v1.0.2); https://github.com/containerd/containerd/blob/v1.5.9/go.mod#L43

thaJeztah · 2022-01-20T11:30:36Z

vendor.mod

@@ -120,9 +104,3 @@ replace (
 // After updating GRPC, if integration test failures occur, verify that the
 // string matching there is correct.
 replace google.golang.org/grpc => google.golang.org/grpc v1.23.0


Do we need to / should we copy the replace rules from the containerd version we're using? Or does it break with v1.27.1 ? There's some other related replace in the containerd go.mod as well; https://github.com/containerd/containerd/blob/v1.5.9/go.mod#L75-L80

github.com/gogo/googleapis => github.com/gogo/googleapis v1.3.2 github.com/golang/protobuf => github.com/golang/protobuf v1.3.5 // urfave/cli must be <= v1.22.1 due to a regression: https://github.com/urfave/cli/issues/1092 github.com/urfave/cli => github.com/urfave/cli v1.22.1 google.golang.org/genproto => google.golang.org/genproto v0.0.0-20200224152610-e50cd9704f63 google.golang.org/grpc => google.golang.org/grpc v1.27.1

This is another non-trivial one, so I'd leave it for another PR :)

Yeah this is the hard one that blocks buildkit vendoring on moby.

Yeah, we can do in a follow-up. I was looking at the moby code, which already included those replace rules (linking to the vendor.conf file for convenience) and versions of containerd/runc

https://github.com/moby/moby/blob/79651b7a979b40e26af353ad283ca7ea5d67a855/vendor.conf#L83

https://github.com/moby/moby/blob/79651b7a979b40e26af353ad283ca7ea5d67a855/vendor.conf#L76

For these I was more concerned about not having replace rules for some of them, but CI is green so at least currently it works 😂

errordeveloper · 2022-01-20T13:49:44Z

@thaJeztah thanks for the review!

thaJeztah

LGTM

thaJeztah · 2022-01-20T19:05:31Z

@dperny PTAL 🤗

thaJeztah · 2022-11-24T21:26:32Z

These have now all been removed in #3094

errordeveloper mentioned this pull request Dec 15, 2021

vendor: use go mod #3046

Merged

errordeveloper force-pushed the go-mod branch 3 times, most recently from 0efc05e to 02fb360 Compare December 15, 2021 09:40

errordeveloper force-pushed the go-mod branch 3 times, most recently from 0c2ce7f to a9ada03 Compare December 15, 2021 11:37

crazy-max suggested changes Dec 15, 2021

View reviewed changes

vendor.mod Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

errordeveloper force-pushed the go-mod branch from dd36e92 to 43f7d64 Compare December 15, 2021 19:55

errordeveloper added 3 commits December 16, 2021 10:58

vendor: fix issue with permissions

1646833

Files in GOPATH have read-only persmissions, after simply copying them can result in an access error. Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

vendor: fix dependency path resolution in make go-mod-vendor

4ee4f47

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

vendor: eliminate some of the replace directives

25e88b5

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

errordeveloper force-pushed the go-mod branch from 43f7d64 to 25e88b5 Compare December 16, 2021 10:58

errordeveloper marked this pull request as ready for review December 16, 2021 10:58

misc: add .gitattributes

4b579ac

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

errordeveloper requested a review from crazy-max December 16, 2021 11:22

thaJeztah reviewed Jan 20, 2022

View reviewed changes

errordeveloper requested a review from thaJeztah January 20, 2022 13:49

thaJeztah approved these changes Jan 20, 2022

View reviewed changes

crazy-max approved these changes Jan 30, 2022

View reviewed changes

thaJeztah closed this Nov 24, 2022

Conversation

errordeveloper commented Dec 15, 2021

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

errordeveloper commented Dec 15, 2021

Uh oh!

errordeveloper commented Dec 15, 2021

Uh oh!

crazy-max left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

errordeveloper commented Dec 17, 2021

Uh oh!

crazy-max commented Jan 20, 2022

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 Jan 20, 2022

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 20, 2022

Uh oh!

thaJeztah commented Nov 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 15, 2021 •

edited

Loading