Vendor dependencies #1144

timoreimann · 2017-02-08T23:21:39Z

This PR switches the way we handle dependencies by committing the vendor folders (main and integration) into the VCS.

Some noteworthy changes:

Committing the vendor folders populated by glide install [--strip-vendor] as-is results in more than 400 MB of dependency files. That's why we also --skip-tests and run glide-vc afterwards to trim down deps to 62 MB. (Note that I have decided to keep the licencing information in place for reasons described here.)
We can only strip vendor folders with the main dependencies; the test build breaks if we try to do the same on the integration deps.
The latest versions of glide-vc use glide list to determine the files which can be eliminated. What I found out though is that this even removes needed files. This might be a bug in glide; for now, I have resorted to using the glide.lock file as the basis for what can be trimmed (--use-glide-lock switch on glide-vc).
All of this seems too complicated to be done correctly by a human. Hence, I created the helper/wrapper script glide.sh that should be used whenever we need to work with dependencies (install/update/get/trim).

I separated the vendor folder changes into dedicated commits so that the actual changes to the project infrastructure can be reviewed more easily (the first commit).

Fixes #1097.

timoreimann · 2017-02-08T23:21:45Z

NB: I am having issues making the Travis CI run complete in time on my fork: The integration tests keep failing and must be retried until they pass. Once that happens, there's not enough time left to finish cross-compiling within the 50 minute timeout that Travis imposes. I'm not entirely sure whether this is a consequence of my PR or just flakiness, but I'm slightly leaning towards the latter.

Feedback on this part is appreciated!

timoreimann · 2017-02-09T01:49:53Z

Looks like it went green on the first try. :-)

@emilevauge @errm @vdemeester: PTAL.

emilevauge · 2017-02-09T07:20:32Z

Wow! Great job @timoreimann !
This is not a full review for now.
I have a major concern on this:

Committing the vendor folders populated by glide install [--strip-vendor] as-is results in more than 500 MB of dependency files. That's why we also --skip-tests and run glide-vc afterwards to trim down deps to 62 MB. (Note that I have decided to keep the licencing information in place for reasons described here.)

I don't think this respect some deps' licences sadly. I know this is annoying, and I'm not a lawyer, but I think we can't modify what in vendor/...

timoreimann · 2017-02-09T09:25:43Z

@emilevauge

Disclaimer: I'm not a lawyer either. :-)

Yes, that's indeed somewhat of a disputed topic. FWIW, the Google lawyers think it's okay, and govendor follows a similar approach by just holding on to *.go and licencing files by default. It'll be interesting to see what conclusions golang/dep#120 will eventually draw in this regard.

That said, I realize that neither of the mentioned entities will defend you, me, or the Traefik project when in doubt. :-) Therefore, I'll push another commit to remove the contentious part.

emilevauge · 2017-02-09T10:53:19Z

On this, WDYT @containous/traefik ?

timoreimann · 2017-02-09T12:57:48Z

Travis CI fails right now due to the new validate-vendor claiming there's a file I didn't check in. However, I see it being committed locally.

Will need to investigate.

timoreimann · 2017-02-09T23:59:18Z

For some reason, the build container behaves slightly differently than my host system: Running glide install --strip-vendor during the validate-vendor script inside the container produces one vendored file vendor/bitbucket.org/ww/goautoneg/.hg_archival.txt diffing by one line:

root@39c722483812:/go/src/github.com/containous/traefik# glide --quiet install --strip-vendor
root@b483094ac340:/go/src/github.com/containous/traefik# git diff vendor
diff --git a/vendor/bitbucket.org/ww/goautoneg/.hg_archival.txt b/vendor/bitbucket.org/ww/goautoneg/.hg_archival.txt
index b9a2ff9..3c3827a 100644
--- a/vendor/bitbucket.org/ww/goautoneg/.hg_archival.txt
+++ b/vendor/bitbucket.org/ww/goautoneg/.hg_archival.txt
@@ -3,4 +3,3 @@ node: 75cd24fc2f2c2a2088577d12123ddee5f54e0675
 branch: default
 latesttag: null
 latesttagdistance: 5
-changessincelatesttag: 5

If I install the dependencies on my host machine (Mac) using glide, that last line is there in the file as can be seen on the committed file.

I suspected different glide versions but they are equal (except for the varying architectures).

I have been debugging this issue for a few hours now and start to run out of things to try next. If anyone has an idea, I'd be happy to hear.

errm · 2017-02-10T00:12:22Z

sounds like something to do with the version of hg(mercurial)?

Is there a difference in the output of hg version?

timoreimann · 2017-02-10T00:35:17Z

@errm good point: I have 3.9 on my host, the jessie-backed golang Docker image is on a fairly old 3.1.

There's 3.9 available on jessie-backports. I'll try to upgrade to that one in the container.

Thanks!

timoreimann · 2017-02-10T01:20:00Z

Looks like it was the mercurial version: Validation runs to a successful completion. 🎉

Crossing fingers for the Travis CI run now...

timoreimann · 2017-02-10T06:05:45Z

Build went green. 😀

I think we are ready for review again.

(if / once this PR gets approval, please do not merge yourself; I'd like to squash commits first.)

timoreimann · 2017-02-13T21:56:52Z

@emilevauge @vdemeester @errm any chance to get a review and ideally an LGTM? :-)

klausenbusk · 2017-02-20T11:26:38Z

From a outsider just looking around :). Is there any reason you need to add all the files, couldn't you just use something like submodule?

vdemeester · 2017-02-20T11:32:56Z

@klausenbusk using git submodule is the same as not vendoring, and has shortcommings too

as for not vendoring, if a repository is gone, you can't build anymore
if the repository is not a git repository, you are screwed 👼

The thing with vendoring is : do we clean the mess or not — i.e. do we only keep packages we use, without the test files, and keeping LICENCE and other important files (and limit a lot what we store) ; or do we not clean and store waaayy too much file.

I'm for the first (and there is a few project who do that).

klausenbusk · 2017-02-20T11:47:49Z

@klausenbusk using git submodule is the same as not vendoring, and has shortcommings too

Oh yeh, you correct. I didn't through of that, for my small "hobby"-project (a few hundred lines) I just use submodules, but is has some shortcoming defiantly.

timoreimann · 2017-02-20T12:33:18Z

@vdemeester since you're around anyway :) Any chance for a review of my PR? Thanks!

vdemeester · 2017-02-20T12:59:04Z

I'm in 💯 on vendoring, but I'm really not a huge fan of glide and I really think we should clean the deps (strip vendor, remove test files and package that are not used — not sure how glide manage all of these). I understand removing some files from vendor might no respect some license and that's why I don't like transitive dependencies either (because you don't control those).

So, from me it would be a LGTM with removing some file to keep only what's needed. But I'm a little biased on the subject (@containous/traefik maintainers knows my point of view 😅) so I'll let other maintainer give their opinion.

timoreimann · 2017-02-20T14:38:21Z

I think I can answer those glide behavior questions:

vendor stripping: Happens when you pass the -v parameter. Traefik already does this for the main repo but not for the integration one. (But even if we wanted to, it doesn't work for the latter right now. See my PR description comment.)
test file removal: Needs an extra switch. My initial version of this PR had it and it worked fine.
Removing unused files: Needs the glide-vc (vendor cleaning) plugin, glide only removes whole unused package dependencies. Again, the initial PR commits contain it.

timoreimann · 2017-03-01T05:51:56Z

Following my discussion with @emile, this PR is back to where it used to be in the beginning, with dependency pruning enabled. The overall {integration}/vendor/ folder size is 62 MB.

Do we agree to move forward with vendoring as-is? @vdemeester?

emilevauge

@timoreimann Awesome work 👏
LGTM!
/cc @containous/traefik

emilevauge · 2017-03-03T14:43:07Z

build.Dockerfile

@@ -1,5 +1,12 @@
 FROM golang:1.7

+# Install a more recent version of mercurial to avoid mismatching results
+# between glide run on a decently updated host system and the build container.


Oh god 😢

The golang builder is based on Debian jessie. 🙁

emilevauge · 2017-03-03T14:44:46Z

.gitignore

@@ -1,15 +1,13 @@
 /dist
-gen.go
+/autogen/gen.go


I think we could now include generated files in git. This will allow to go get Traefik. WDYT? But that wan be done in another PR.

I'd love to get rid of the extra generator step. I haven't worked a lot with generated code, however, so no idea if checking that in is a good practice or not.

(I know the Kubernetes folks are trying hard to undo committing generated code, but that might be because they are playing in a different league.)

Either way, discussing / considering in a separate issue / PR sounds good. 👍

timoreimann · 2017-03-03T15:11:42Z

@vdemeester with pruning now part of this PR, can I get your LGTM? :)

emilevauge · 2017-03-07T14:12:04Z

Ping @containous/traefik

vdemeester

LGTM 🐮

dtomcej

LGTM

- Add helper script to simplify glide usage. - Add validation script for unwanted changes to vendoring. - Relax/tighten up .{git,docker}ignore to cover vendored files properly. - .validate: Protect from unbound variable in case of nounset setting. - Install more recent hg version in the build container. - Remove glide installation steps from Dockerfile. - Update documentation.

timoreimann force-pushed the vendor-dependencies branch from 52bc335 to 65676b0 Compare February 9, 2017 12:58

emilevauge modified the milestone: 1.3 Feb 17, 2017

timoreimann force-pushed the vendor-dependencies branch 8 times, most recently from a2ae03e to 5ce0f31 Compare February 25, 2017 20:30

emilevauge added the status/1-needs-design-review label Feb 28, 2017

emilevauge added the status/2-needs-review label Feb 28, 2017

timoreimann force-pushed the vendor-dependencies branch 3 times, most recently from 24fd82f to 1618fd8 Compare March 3, 2017 12:24

emilevauge approved these changes Mar 3, 2017

View reviewed changes

emilevauge removed the status/1-needs-design-review label Mar 3, 2017

timoreimann force-pushed the vendor-dependencies branch from 1618fd8 to dcde0ae Compare March 3, 2017 17:53

vdemeester approved these changes Mar 8, 2017

View reviewed changes

dtomcej approved these changes Mar 8, 2017

View reviewed changes

timoreimann force-pushed the vendor-dependencies branch from dcde0ae to 5417f06 Compare March 8, 2017 21:21

timoreimann added 2 commits March 9, 2017 13:13

Vendor main dependencies.

dd5e3fb

Vendor integration dependencies.

55b57c7

timoreimann force-pushed the vendor-dependencies branch from 5417f06 to 55b57c7 Compare March 9, 2017 12:13

timoreimann merged commit 91bf627 into traefik:master Mar 9, 2017

timoreimann deleted the vendor-dependencies branch March 9, 2017 15:23

timoreimann added status/3-needs-merge and removed status/2-needs-review labels Mar 9, 2017

ldez removed the status/3-needs-merge label Nov 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendor dependencies #1144

Vendor dependencies #1144

timoreimann commented Feb 8, 2017 •

edited

Loading

timoreimann commented Feb 8, 2017 •

edited

Loading

timoreimann commented Feb 9, 2017

emilevauge commented Feb 9, 2017 •

edited

Loading

timoreimann commented Feb 9, 2017

emilevauge commented Feb 9, 2017

timoreimann commented Feb 9, 2017

timoreimann commented Feb 9, 2017 •

edited

Loading

errm commented Feb 10, 2017

timoreimann commented Feb 10, 2017

timoreimann commented Feb 10, 2017 •

edited

Loading

timoreimann commented Feb 10, 2017 •

edited

Loading

timoreimann commented Feb 13, 2017

klausenbusk commented Feb 20, 2017

vdemeester commented Feb 20, 2017

klausenbusk commented Feb 20, 2017

timoreimann commented Feb 20, 2017

vdemeester commented Feb 20, 2017

timoreimann commented Feb 20, 2017

timoreimann commented Mar 1, 2017 •

edited

Loading

emilevauge left a comment

emilevauge Mar 3, 2017

timoreimann Mar 7, 2017

emilevauge Mar 3, 2017

timoreimann Mar 3, 2017

timoreimann commented Mar 3, 2017

emilevauge commented Mar 7, 2017

vdemeester left a comment

dtomcej left a comment

Vendor dependencies #1144

Vendor dependencies #1144

Conversation

timoreimann commented Feb 8, 2017 • edited Loading

timoreimann commented Feb 8, 2017 • edited Loading

timoreimann commented Feb 9, 2017

emilevauge commented Feb 9, 2017 • edited Loading

timoreimann commented Feb 9, 2017

emilevauge commented Feb 9, 2017

timoreimann commented Feb 9, 2017

timoreimann commented Feb 9, 2017 • edited Loading

errm commented Feb 10, 2017

timoreimann commented Feb 10, 2017

timoreimann commented Feb 10, 2017 • edited Loading

timoreimann commented Feb 10, 2017 • edited Loading

timoreimann commented Feb 13, 2017

klausenbusk commented Feb 20, 2017

vdemeester commented Feb 20, 2017

klausenbusk commented Feb 20, 2017

timoreimann commented Feb 20, 2017

vdemeester commented Feb 20, 2017

timoreimann commented Feb 20, 2017

timoreimann commented Mar 1, 2017 • edited Loading

emilevauge left a comment

Choose a reason for hiding this comment

emilevauge Mar 3, 2017

Choose a reason for hiding this comment

timoreimann Mar 7, 2017

Choose a reason for hiding this comment

emilevauge Mar 3, 2017

Choose a reason for hiding this comment

timoreimann Mar 3, 2017

Choose a reason for hiding this comment

timoreimann commented Mar 3, 2017

emilevauge commented Mar 7, 2017

vdemeester left a comment

Choose a reason for hiding this comment

dtomcej left a comment

Choose a reason for hiding this comment

timoreimann commented Feb 8, 2017 •

edited

Loading

timoreimann commented Feb 8, 2017 •

edited

Loading

emilevauge commented Feb 9, 2017 •

edited

Loading

timoreimann commented Feb 9, 2017 •

edited

Loading

timoreimann commented Feb 10, 2017 •

edited

Loading

timoreimann commented Feb 10, 2017 •

edited

Loading

timoreimann commented Mar 1, 2017 •

edited

Loading