Checking in vendored dependencies?

[timoreimann]

(I couldn't find any previous issues / pull requests on the following request, so I'm opening this one.)

I was wondering if the Traefik maintainers were open to checking in vendored dependencies into git. Right now, in order to build the project, developers first have to do glide install to bring in dependencies in the correct versions. For me, this is multiple unfavorable implications:

• Users need to have glide installed just to build the project (and ideally use the right version).
• Build times are significantly prolonged. While this is not so much of a problem when building locally (as dependencies shouldn't change too often, but see my other points), I noticed that large portions of CI time are consumed by installing dependencies. We seem to be speaking minute scale here.
• Developers need to run glide install on every git pull to guarantee consistency. One problem with this is that it's easy to forget and could lead to hard-to-trace bugs. The other concern is that you need to do it on every branch switch just to be safe.
• The build can break because of dependencies gone away, being renamed, Github failing, etc. Coincidentally, on my first Traefik PR, a ran into a Github glitch that caused my Travis build to fail.

All of these problems would go away if the vendor/ folder was checked into the VCS.

Of course, there are also downsides:

• Repo size increases. While this is a problem when using glide as-is, the popular glide plugin glide-vs can reduce the size significantly: With the current master, the vendor directory can be trimmed down from more than 300 MB to just 60 MB. Another 1-2 MB can be shaved off by also removing licence files, but that's somewhat controversial.
• PRs with dependency changes get harder to read since third-party and application sources are hard to distinguish in Github's PR UI. One way to deal with this is to isolate dependency changes into separate commits and review commit-wise. Only few PRs should be affected by this since dependency-changing PRs are usually in the minority.

I realize that there are numerous proponents for each of the commit/don't commit camps, and obviously I'm part of the commit camp. For me, the benefits clearly outnumber of drawbacks. I'd be curious to know how the Traefik project thinks about this matter and whether there's interest in "switching camps". :-)

Thanks!

[emilevauge]

Hi @timoreimann
You are talking about a key issue that is regularly discussed in the maintainers team ( @vdemeester, your time has come ^^ ).
There are pros and cons, and nobody agrees on what is the best solution.
I personally wonder how PR merging will looks like on Github ;)

One way to deal with this is to isolate dependency changes into separate commits and review commit-wise

Good luck with that!

Only few PRs should be affected by this since dependency-changing PRs are usually in the minority.

That's not really the case in fact. It sadly happens quite often.

The last time we talk about that, we agree to test vendoring dependencies. We just need a PR...

[timoreimann]

One way to deal with this is to isolate dependency changes into separate commits and review commit-wise

Good luck with that!

We have been doing this internally at my work place for quite some time. It's not ideal (mostly because the Github code review flow is inconsistent and messy at times) but yet the best we could find.

The last time we talk about that, we agree to test vendoring dependencies. We just need a PR...

I take this as an official invitation to provide a PR. :-)

[krancour]

This same subject has come up frequently at Deis and in various Kubernetes projects. Let me offer up one very heinous issue with vendoring dependencies... it's nearly impossible to assert when someone has modified the source of a dependency and committed that along with other changes. This doesn't even have to be malicious. I think we've all run into the situation before where we went and hacked up someone else's library locally to add more debugging. This is safe when the vendored code is untracked, but if tracked, those sort of changes can accidentally be committed... and they're not likely to be noticed by reviewers.

[vdemeester]

I personally wonder how PR merging will looks like on Github ;)

It works really ok (we do this on docker/docker). GitHub hide the content of the vendor code by default even so it's more than ok.

it's nearly impossible to assert when someone has modified the source of a dependency and committed that along with other changes. This doesn't even have to be malicious. I think we've all run into the situation before where we went and hacked up someone else's library locally to add more debugging. This is safe when the vendored code is untracked, but if tracked, those sort of changes can accidentally be committed... and they're not likely to be noticed by reviewers.

Yes, that's why we need a check on that. On docker/docker, if there is any change on the vendor/ folder, we run a vndr (here it would be glide install or whatever) to make sure the result is the same. If it's not, the check fails, the build fails. And thus, maintainers know there is something wrong.

I am 👍 multiplied by 💯 for this 👼

[timoreimann]

It works really ok (we do this on docker/docker). GitHub hide the content of the vendor code by default even so it's more than ok.

Nice, I didn't realize the suppressing-vendor-changes UI feature finally made it in. 🎉

Yes, that's why we need a check on that. On docker/docker, if there is any change on the vendor/ folder, we run a vndr (here it would be glide install or whatever) to make sure the result is the same. If it's not, the check fails, the build fails. And thus, maintainers know there is something wrong.

Is there some place in the docker build pipeline you could point me to where we could copy the idea from?

(Not sure if we need it from the beginning though.)

[vdemeester]

@timoreimann the check is here : https://github.com/docker/docker/blob/master/hack/validate/vendor … I did the first script (then it changed when we changed the tool we use for vendoring) so I can definitely help to put this in place.

(Not sure if we need it from the beginning though.)

(we do 😉 — it's not that big of a deal anyway)

[timoreimann]

👍

I have started to put together a PR. Will hopefully be able to push something soon.

One thing I noticed was that there are separate glide files for the primary project and the integration tests. The introducing commit said something about glide not being able to "read the version info from docker/docker". Any ideas what's up with that, and if we need to maintain the disparity?

FWIW, in my feature branch, I have removed the nested glide files and started to add the missing dependencies using glide one at a time, and the number of compile errors seems to drop steadily. Let me know if this is the wrong way to go though.

[emilevauge]

@timoreimann I think we definitely want to keep the separate integration tests glide file.
A lot of tests dependencies are not needed to build Traefik and we don't want to mess with that anymore.

The introducing commit said something about glide not being able to "read the version info from docker/docker"

Maybe glide is not able to read tags ? @errm do you remember ?

[timoreimann]

Glide should support pretty much all of semver specifiers, branches, tags, and commit IDs.

It also has a testImport part in the manifest file. Could that be helpful? I never fully grasped its purpose even after reading the brief blog announcement. Still wanted to mention it though.

[emilevauge]

@timoreimann

Glide should support pretty much all of semver specifiers, branches, tags, and commit IDs.

Yep, I know, we use semver tags with many deps. Maybe there is a specific issue with Docker, I don't know.

Nevermind, can I suggest to NOT merge both glide.yml files for now, and stick with vendoring in your PR?
I'm opened to changes on glide files, but not in the same PR if possible :)

[timoreimann]

@emilevauge yes, absolutely. Will try to be as KISS as possible. 😄