Skip to content

Conversation

@dnephin
Copy link
Contributor

@dnephin dnephin commented May 2, 2017

Add a CI check for vendoring

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯
I wonder : should we run it only when vendor.conf or vendor/* files are changed, or always (as in this PR) ?

Copy link
Contributor

@gdevillele gdevillele left a comment

Choose a reason for hiding this comment

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

LGTM

but shouldn't we put the "vendor check" step before the "build+unit test" in circle.yml?

@dnephin
Copy link
Contributor Author

dnephin commented May 3, 2017

Running it only when vendor* is changed is just an optimization, right?

My feeling is that it's not super necessary, and there are two advantages to avoiding it.

  1. It means we don't have to worry about adding .git to any docker images. This lets us keep .git in .dockerignore for much faster builds. circleci doesn't support bind mount volumes with the remote engine, which we need to build images.
  2. I always felt it was annoying having to commit before running any of the hack/validate scripts. I always felt that they should run on the current files, not the git diff.

I think we should avoid the optimization for now. When we need it, we could do it only in the circle ci config, instead of the makefile.

@dnephin
Copy link
Contributor Author

dnephin commented May 3, 2017

but shouldn't we put the "vendor" step before the "build+unit test" in circle.yml

Ideally we would run them in parallel, but it's a bit more work to setup, and our build is probably fast enough right now that we can wait to do that work.

My reasoning for running it after build+test is that sometimes if you're modifying a dependency it is nice to be able to run tests with a custom version in vendor/ ahead of pushing it to master in the dependency repo. By putting it after binary and test, you can see if the change works, without having to commit it to the dependency repo.

@gdevillele
Copy link
Contributor

@dnephin got it. Thanks 🙂

@dnephin dnephin merged commit 328fa4b into docker:master May 3, 2017
@dnephin dnephin deleted the check-vendor branch May 3, 2017 20:01
@thaJeztah thaJeztah modified the milestone: 17.06.0 Jul 21, 2017
chris-crone pushed a commit to chris-crone/cli that referenced this pull request Jun 11, 2018
…ess-stream-friend

[18.03-ee] backport Fix panics when --compress and --stream are used together
nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025
[17.06] Re-vendor SwarmKit to 4b872cfac8ffc0cc7fff434902cc05dbc7612da5
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.

4 participants