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

vendor with go mod #3387

Merged
merged 3 commits into from
Jan 13, 2022
Merged

vendor with go mod #3387

merged 3 commits into from
Jan 13, 2022

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Dec 9, 2021

follow-up #3308

vendoring with go modules with nearly zero diff (ref moby/swarmkit#3046).

cc @thaJeztah @errordeveloper

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2021

Codecov Report

Merging #3387 (663f01b) into master (6fbf816) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3387   +/-   ##
=======================================
  Coverage   56.35%   56.35%           
=======================================
  Files         304      304           
  Lines       26830    26830           
=======================================
  Hits        15121    15121           
  Misses      10789    10789           
  Partials      920      920           

@crazy-max crazy-max marked this pull request as ready for review December 10, 2021 09:20
@AkihiroSuda
Copy link
Collaborator

Why is the filename vendor.mod, not go.mod?

@errordeveloper
Copy link
Contributor

errordeveloper commented Dec 10, 2021

Why is the filename vendor.mod, not go.mod?

Creating go.mod implies opting in for all the rules around SemVer, which this repo cannot abide by as it uses CalVer. So the idea is to start using go mod vendor instead of vndr, so that all dependencies that do use go.mod can be easily consumed (i.e. without having to patch vndr).

@AkihiroSuda
Copy link
Collaborator

Why is the filename vendor.mod, not go.mod?

Creating go.mod implies opting in for all the rules around SemVer, which this repo cannot abide by as it uses CalVer. So the idea is to start using go mod vendor instead of vndr, so that all dependencies that do use go.mod can be easily consumed (i.e. without having to patch vndr).

Thanks for explanation, could you add this background to the comment lines of vendor.mod ?

@errordeveloper
Copy link
Contributor

Thanks for explanation, could you add this background to the comment lines of vendor.mod ?

Good idea! @crazy-max would you mind doing that please? I'll update moby/swarmkit#3046 also.

@crazy-max
Copy link
Member Author

@errordeveloper Yes sure!

@thaJeztah
Copy link
Member

FWIW: I recalled we also had #3139 pending, but that never was finished / completed. I recalled I had a branch on my machine (but it may have still been "work in progress", as it was a quick "let's rip the remaining bits out as well" frenzy). Removing those k8s bits also helps reducing the list of dependencies considerably, so I just rebased that branch, and opened a draft PR to see what it would look like; #3389 (and if CI succeeds 😂)

vendor.mod Outdated Show resolved Hide resolved
vendor.mod Outdated Show resolved Hide resolved
@errordeveloper
Copy link
Contributor

Looks like this is good to go!

.gitignore Outdated Show resolved Hide resolved
@thaJeztah
Copy link
Member

Also opened docker/docker-ce-packaging#611 to do a quick check if the packaging scripts don't blow up 👍

scripts/vendor Outdated Show resolved Hide resolved
@crazy-max
Copy link
Member Author

scripts/vendor Outdated Show resolved Hide resolved
scripts/vendor Outdated
Comment on lines 27 to 28
if [ ! -x "$(command -v modvendor)" ]; then
echo "modvendor not found. Install with 'go install github.com/goware/modvendor@v0.5.0'"
exit 1
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend doing something similar to moby/swarmkit@a46370c, that way tools versions can be tracked in a separate go.mod file and will not interfere with actual runtime dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep sounds better with go build tools, will make some changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum will do that in a follow-up actually, there are other tools that would need the same behavior like goversioninfo.

Copy link
Contributor

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

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

Looks good overall! Just a few nits.

@crazy-max crazy-max force-pushed the vendor-gomod branch 2 times, most recently from caef84b to fc57f10 Compare December 14, 2021 16:19
@crazy-max crazy-max force-pushed the vendor-gomod branch 2 times, most recently from dcd21b4 to f49c568 Compare December 14, 2021 20:06
Comment on lines +2 to +3
vendor.mod linguist-language=Go-Module
vendor.sum linguist-language=Go-Checksums
Copy link
Member Author

@crazy-max crazy-max Dec 14, 2021

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@errordeveloper
Copy link
Contributor

errordeveloper commented Dec 15, 2021

@crazy-max should we request a review from someone who can merge this?

scripts/vendor Outdated Show resolved Hide resolved
scripts/vendor Show resolved Hide resolved
./scripts/vendor validate

.PHONY: mod-outdated
mod-outdated: ## check outdated dependencies
Copy link
Member

Choose a reason for hiding this comment

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

This is not yet in use, correct? (more of a "local dependabot check")

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is only intended for local check

Copy link
Member Author

Choose a reason for hiding this comment

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

Sample output:

$ DISABLE_WARN_OUTSIDE_CONTAINER=1 make mod-outdated
./scripts/vendor outdated
+ go-mod-outdated -update -direct
+ go list -mod=vendor -mod=readonly -modfile=vendor.mod -u -m -json all
+--------------------------------------+-----------------------------------------------------+------------------------------------+--------+------------------+
|                MODULE                |                       VERSION                       |            NEW VERSION             | DIRECT | VALID TIMESTAMPS |
+--------------------------------------+-----------------------------------------------------+------------------------------------+--------+------------------+
| github.com/containerd/console        | v1.0.2                                              | v1.0.3                             | true   | true             |
| github.com/containerd/containerd     | v1.5.5                                              | v1.5.8                             | true   | true             |
| github.com/creack/pty                | v1.1.11                                             | v1.1.17                            | true   | true             |
| github.com/docker/docker             | v20.10.3-0.20210811141259-343665850e3a+incompatible | v20.10.12+incompatible             | true   | true             |
| github.com/google/go-cmp             | v0.2.0                                              | v0.5.6                             | true   | true             |
| github.com/mitchellh/mapstructure    | v1.3.2                                              | v1.4.3                             | true   | true             |
| github.com/moby/buildkit             | v0.8.2-0.20210615162540-9f254e18360a                | v0.9.3                             | true   | true             |
| github.com/opencontainers/image-spec | v1.0.1                                              | v1.0.2                             | true   | true             |
| github.com/spf13/cobra               | v1.1.3                                              | v1.3.0                             | true   | true             |
| github.com/tonistiigi/fsutil         | v0.0.0-20210609172227-d72af97c0eaf                  | v0.0.0-20211208191308-f95797418e48 | true   | true             |
| github.com/tonistiigi/go-rosetta     | v0.0.0-20200727161949-f79598599c5d                  | v0.0.0-20201102221648-9ba854641817 | true   | true             |
| golang.org/x/sys                     | v0.0.0-20210823070655-63515b42dcdf                  | v0.0.0-20211216021012-1d35b9e2eb4e | true   | true             |
| golang.org/x/term                    | v0.0.0-20201117132131-f5c789dd3221                  | v0.0.0-20210927222741-03fcf44c2211 | true   | true             |
| golang.org/x/text                    | v0.3.3                                              | v0.3.7                             | true   | true             |
| k8s.io/api                           | v0.16.9                                             | v0.23.0                            | true   | true             |
| k8s.io/apimachinery                  | v0.16.9                                             | v0.23.0                            | true   | true             |
| k8s.io/client-go                     | v0.16.9                                             | v1.5.2                             | true   | true             |
+--------------------------------------+-----------------------------------------------------+------------------------------------+--------+------------------+

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Copy link
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

@tonistiigi PTAL

@crazy-max
Copy link
Member Author

PTAL @tonistiigi @silvin-lubecki @tiborvass thanks

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@thaJeztah
Copy link
Member

let's bring this one in

@thaJeztah thaJeztah merged commit a4787cf into docker:master Jan 13, 2022
@crazy-max crazy-max deleted the vendor-gomod branch January 13, 2022 14:15
@thaJeztah thaJeztah added this to the 22.06.0 milestone May 13, 2022
@crazy-max crazy-max mentioned this pull request Mar 24, 2023
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.

6 participants