Hi @jwendell. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

we should not do this and we need to make sure the pr itself doesn’t bloat up the repo size

Here's the mailing list thread for those that're interested.

Creating the PR doesn't hurt anything and helps drive the discussion forward, since it'd stalled in the mailing list.

Gave the single submodule for the whole vendor dir a quick try this morning - go get will totally fetch it. This will work but it feels like the wrong approach still. Whether your dependencies are in the same project or in a different repository you will end up downloading all of the code to your machine regardless. A repositories dependencies are just as important to the project history as its own source - so I don't mind dependency only commits being in the same repo.

Side note: when all of these dependencies were originally vendored with dep it seems like we failed to run dep prune (I can tell because I see a whole bunch of _test files in each of the dependencies). So the project could potentially drop quite a few LOC just by running that.

I'm experimenting with submoduling vendor.

Here's an issue: When you run dep it will delete and recreate the vendor dir, thus erasing the submodule status, making vendor dir an ordinary dir.

I'm afraid that, in order to workaround situations like this, we'll have to create an unnecessary complex Makefile/scripts solutions.

Also, we should make sure that releases would include vendor dir by default in tarballs.

Do you know of a golang project that has vendor as a submodule? I guess that if it's so good as it sounds, projects like kubernetes would use it, no?

@jwendell would you also mind running the dep prune on your PR. I'd love to know how many files we are going to drop.

@zachgersh thanks for the dep prune hint. It indeed saved a lot of bytes!

Before prune:

19515 files changed, 7844506 insertions(+), 1 deletion(-)

$ du -csh vendor/
363M	vendor/

After prune:

5291 files changed, 1627481 insertions(+), 2 deletions(-)

$ du -csh vendor/
65M	vendor/

I've made this change and force-pushed to this PR.

It's worth to note, in this PR, only the first commit (Add vendor to SCM) is the giant one, others are easy to review :-).

Here's an issue: When you run dep it will delete and recreate the vendor dir, thus erasing the submodule status, making vendor dir an ordinary dir.

I'm afraid that, in order to workaround situations like this, we'll have to create an unnecessary complex Makefile/scripts solutions.

We need a target / automation to update the vendor either way (basically dep ensure + commit script). I don't see how it'd be more complicated to do so in a submodule than the base. With the benefit of not trashing the main repo with hundreds of file changes each time we rev up a dependency

@jwendell PR needs rebase

cc @geeknoid

If offline and reproducible are the main motivations then there will need to be a story on a build that doesn't make queries to git (mostly via bin/get_workspace_status). Ideally the behaviors of an on-line and off-line build have minimal differences--few people will test offline builds, and even fewer will check that the result of the build is the same. I suppose it's possible to have make run get_workspace_status and store the results in a file somewhere that's consumed by the other components but I don't have any decent ideas on how/when to trigger a refresh of the file.

This vendor/ directory might only address the go part of the build, as the docker part seems to pull from remote locations (via actual pull, or "apt get"), though I'm not sure if it's up to istio or proxies to make this work offline.

I'm not strictly for or against at this point--I haven't done any analysis yet since presently I'm still focused on getting things migrated over to make (& addressing regressions, not all of which are attributable to make) while trying to keep a partial eye out for other changes that break things or complicate things (reproducibility or otherwise).

@jwendell PR needs rebase

air-gapped builds are a must as @costinm points out. However, as @ldemailly points out, I'm not a fan of checking in thousands of files that may change in the main repo creating a monstrous repository over the long term in terms of size and review requirements.

Someone in this thread suggested checking with the Kubernetes team if they would make the same choice again or if they would suggest a different approach. This seems like a pretty valid idea to me. Would someone mind following up on that request?

Cheers
-steve

here is the alternative: #3348 - 8mbytes sub module - PTAL
(and please help with makefile changes to take advantage of it)

@jwendell PR needs rebase

It appears this comes down to a question of trade-offs, i.e. is it worth checking in the vendor directory to istio/istio and keeping things simple or modifying the build process to support sub-modules and coordinating releases across multiple repos.

We were supposed to discuss this in yesterday's call but did not manage to do so as it was pushed down the agenda, it would be good to make this a high priority issue on next week's agenda if we cannot get a consensus through these discussions.

there is no “modifying the build process” difference between submodule or direct checkin - either way a small change is needed

@ldemailly Unfortunately I believe you are missing some important aspects but I will try to elaborate on the problems with submodules on the other PR.

I also don't believe they both require changes, with the submodule approach you will have to modify the scripts to use git rather than dep however when committing to the same repo they should work without any necessary changes. Of course submodules also changes the developer flow which could lead to more serious issues but please take a look at the other PR and comment there.

the current istio build has a bunch of dep run logic which will need to be removed/moved to a separate target either way (when a dependency update is needed instead of mandatory to build) - more detailed reply in #3348 (comment)

@jwendell PR needs rebase

@jwendell: The following tests failed, say /retest to rerun them all:

@jwendell PR needs rebase

Thanks for having brought up the issue !

Fixed through #3348 #3678 #3697
and the growing https://github.com/istio/istio/wiki/Vendor-FAQ