Skip to content

Use go mod#2957

Closed
dperny wants to merge 4 commits intomoby:masterfrom
dperny:use-go-mod
Closed

Use go mod#2957
dperny wants to merge 4 commits intomoby:masterfrom
dperny:use-go-mod

Conversation

@dperny
Copy link
Copy Markdown
Collaborator

@dperny dperny commented Jun 4, 2020

As an experiment, migrated from vndr to go mod. This included doing go mod vendor to vendor all packages.

Then, bumped go to version 1.14, because there are changes to vendoring in that version.

@thaJeztah
Copy link
Copy Markdown
Member

is this using the same versions of dependencies and before?

I would recommend holding off with updating to Go 1.14, as there's still known issues with the runtimes (runc, containerd) when using go 1.14.

@thaJeztah
Copy link
Copy Markdown
Member

Also might want to add a vendor validation check, to make sure the vendor directory matches the expected content. here's an example; https://github.com/containerd/project/blob/master/script/validate/vendor

@dperny
Copy link
Copy Markdown
Collaborator Author

dperny commented Jun 4, 2020

Mostly the same versions. Some of them have been updated in one way or another, like etcd from version 3.3.12 to 3.3.22.

@dperny
Copy link
Copy Markdown
Collaborator Author

dperny commented Jun 4, 2020

This is an experiment, not something I'm committing to.

github.com/google/go-cmp v0.4.1 // indirect
github.com/gorilla/mux v1.7.4 // indirect
github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
github.com/hashicorp/go-immutable-radix v1.2.0 // indirect
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might need a replace rule for this on (as we still use a fork;

github.com/hashicorp/go-immutable-radix 826af9ccf0feeee615d546d69b11f8e98da8c8f1 git://github.com/tonistiigi/go-immutable-radix.git

trying to get rid of that fork though; see the discussion on hashicorp/go-immutable-radix#29

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We don't need this fork anymore, from Swarmkit's standpoint. The added Seek method is unused by Swarmkit or any of its dependencies.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When vendored in docker/docker, that fork (and older versions of the related dependencies) will be used, which may be something to take into account if that causes incompatibilities?

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 4, 2020

FWIW; the bbolt failure on Go 1.14 was fixed in v1.3.4 (containerd/containerd#4134), but introduced a regression, so in containerd we had to revert to v1.3.3 (containerd/containerd#4156)

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 4, 2020

Codecov Report

Merging #2957 into master will decrease coverage by 1.11%.
The diff coverage is 24.85%.

@@            Coverage Diff             @@
##           master    #2957      +/-   ##
==========================================
- Coverage   61.77%   60.66%   -1.12%     
==========================================
  Files         142      142              
  Lines       23005    20286    -2719     
==========================================
- Hits        14211    12306    -1905     
+ Misses       7303     6519     -784     
+ Partials     1491     1461      -30     

@thaJeztah
Copy link
Copy Markdown
Member

@dperny needs a rebase

@AkihiroSuda
Copy link
Copy Markdown
Member

ping @dperny could you rebase?

Signed-off-by: Drew Erny <derny@mirantis.com>
Signed-off-by: Drew Erny <derny@mirantis.com>
Signed-off-by: Drew Erny <derny@mirantis.com>
Signed-off-by: Drew Erny <derny@mirantis.com>
@dperny
Copy link
Copy Markdown
Collaborator Author

dperny commented Oct 15, 2020

Rebased, we'll see how it goes through CI.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

@AkihiroSuda @cpuguy83 ok with you as well?

@@ -0,0 +1,60 @@
module github.com/docker/swarmkit
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

actually one thing I'm just thinking of; if we want to move this repository to the moby org; would it be better to move and change the module name at the same time (so move first, then add module?)

@AkihiroSuda you recently renamed some repos; any recommendation?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I suggest renaming module name as well

@rm -Rf .vendor.bak
@mv vendor .vendor.bak
@$(VNDR)
@go mod vendor
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps also

Suggested change
@go mod vendor
@go mod tidy
@go mod vendor

Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

LGTM (IANAM)

@thaJeztah
Copy link
Copy Markdown
Member

This has been done now, so let me close this one

@thaJeztah thaJeztah closed this Nov 24, 2022
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.

3 participants