Migrate to go modules (deprecate 'vendor')

[umarcor]

This is an attempt to migrate from dep and a vendor directory to go modules, and hopefully close #1331. Ref #1338.

The first commit does remove vendor and Gopkg.*; go.mod and go.sum are added instead. .travis.yml is updated accordingly.

The build is successful, but static-checks fail because the testing infrastructure assumes that go packages installed through go get are located in $GOPATH/src: https://travis-ci.com/umarcor/kata-runtime/builds/105771341. That's not true anymore. With go modules, packages are located in $GOPATH/pkg/mod.

In the second commit, static-checks are disabled, just to ensure that other steps are successful: https://travis-ci.com/umarcor/kata-runtime/builds/105771550

The third commit is an attempt at fixing the issue with kata-runtime/tests. The repository is downloaded through curl -fsSL https://codeload.github.com/kata-containers/tests/tar.gz/master | tar xzf - -C ../tests --strip-components=1 instead of go get. So, export tests_repo_dir="$(pwd)/../tests". There is no problem with downloading and calling the scripts in it, but it fails because scripts in kata-runtime/tests do not check their own location: https://travis-ci.com/umarcor/kata-runtime/builds/105772078 (it is hardcoded https://github.com/kata-containers/tests/blob/master/.ci/lib.sh#L30-L31).

I could make a dirty hack by overwritting the file after downloading the tarball. But then I got another error related to the same issue: golangci-lint is installed through go get (which is quite strange I must say). Therefore, scripts assume that it is available in GOPATH/src.... That's not true now. I removed it from https://github.com/kata-containers/tests/blob/master/versions.yaml#L52-L62 and installed it in the before_install field of .travis.yml. At some point it worked, but linting errors were produced: https://travis-ci.com/umarcor/kata-runtime/jobs/187675554. I tried to rebase on top of #1372. Rebasing produced no conflicts, but linting issues were not solved. Furthermore, in #1372 golangci-lint is defined in a different versions.yaml the specified version does not match the one kata-containers/tests: https://github.com/kata-containers/runtime/blob/f4428761cbd0418c8024f0356897f10417ab38bd/versions.yaml.

As a result, I think that it would be desirable that someone with better knowledge about kata-containers/test could think about the issue. How much effort is required to avoid using hardcoded locations in the tests? Why are packages such as golangci-lint installed through go get instead of go install?

Also, related to .travis.yml, I wonder why install and before_script are used instead of script and after_success.

[raravena80]

@umarcor nudge. any updates?

[umarcor]

Hi @raravena80! Thanks for coming back to this. As commented in the first message:

As a result, I think that it would be desirable that someone with better knowledge about kata-containers/test could think about the issue. How much effort is required to avoid using hardcoded locations in the tests? Why are packages such as golangci-lint installed through go get instead of go install?

Overall, there is no issue at all with replacing dep with 'go modules', as far as dependency management is concerned. However, the testing infrastructure in the project as a whole relies on some hardcoded paths which are broken with this change. I believe that this is an indicator of the fragility of the testing environment. Thus, those hardcoded paths should be fixed. However, since that is out of the scope of this PR, I am waiting for some thoughts, comments or help about it.

[umarcor]

I rebased on top of master. This is the result with static-checks disabled: https://travis-ci.com/umarcor/kata-runtime/builds/108843843

[jodh-intel]

@umarcor - since there seem to be various pieces we need to change to switch to go mod, I've created a tracking issue:

• switch vendoring from dep to go modules kata-containers#42

I've tried to distill your fairly detailed explanations, but please update the issue above if I've missed anything. If you would like to help out with some of these preparatory tasks, please do! 😄

[raravena80]

@umarcor any updates? Thx.

[umarcor]

Hi @raravena80. Not really. Some clarificative comments were posted in kata-containers/kata-containers#42, but there is no further progress AFAIK.

[grahamwhaley]

@jodh-intel - did you say you were contemplating looking at this as well, or did I dream that? :-)

[jodh-intel]

That would have been a dream 😄

I like the idea but no spare cycles to do this atm. Any volunteers?

[caoruidong]

I think we can do this when golang 1.13 is out

[raravena80]

Waiting for golang 1.13...

[grahamwhaley]

@ganeshmaharaj @amshinde - were you working on this on new PRs now? If so, can this one be closed do we think?

[caoruidong]

ping @umarcor Can we close it ?

[umarcor]

@caoruidong, #1911 is a kind of duplicate of this. Hence, you can either close that or this. However, note that there is a relevant difference. Here, I proposed to remove vendor, since it should not be required with go modules. However, in #1911 the vendor directory is updated, not removed.

I am not familiar with using repositories outside of GOPATH, with go modules enabled and with a vendor subdir. How does this work when there is a version of a package in the system, another one in the cache of the go modules and a last one in the vendor dir? Is the expected approach for each kata-containers repo to vendor everything (probably duplicating lots of common dependencies), instead of potentially having advantage of a built-in mechanism?

See #1331:

What is happening instead is that you can copy absolutely all the dependencies to vendor and then tell go tools to use it exclusively. This is quite nonesense, because it effectively treats each repository as a completely independent project and the same dependencies are duplicated unnecessarily. At this point, I don't know if there is any benefit over using git submodules, provided that all the go dependencies are available as git repos.

Ideally, checking vendor into the repo should not be required neither with dep nor with go mod. The vendor directory should be a local temporal resource that can be 'generated' unequivocally with go.mod, Gopkg.lock and/or Gopkg.toml.

[caoruidong]

Leaving vendor in repo will benefit in downloading process. User can directly run make. I think both ways are OK. The community now are thinking about merging several repos into two or three.

[umarcor]

Leaving vendor in repo will benefits in downloading process. User can directly run make. I think both ways are OK. The community now are thinking about merging several repos into two or three.

That's equivalent to go mod download (which can be added to the make target), isn't it?

[caoruidong]

Yes, you're right. Maybe it can avoid some network issue. Different version of packages should follow a priority rule during building. Some k8s related projects hold vendor directory too. So it may not be a big problem. We can discuss further in #1911.

[umarcor]

@caoruidong, after having a better look at #1911, I can provide some further insight:

The title of this PR is Replace 'dep' (vendor) with go modules, but what I actually propose is to migrate to go modules (in a proper idiomatic way). The title of #1911 is wip: Move to Go modules, but @amshinde is only replacing the mechanism to update vendor. Therefore, we should probably swtich the titles.

Since @amshinde adds export GO111MODULE=on to make vendor, and because make vendor is NOT executed in .travis.yml, none of the CI tests is using go modules, in practice. I am afraid that it will fail in any environment where GO111MODULE=on, from the beginning. Please, see this job, which is a single commit ahead of #1911. By the same token, here I set GO111MODULE globally, so every task in the CI job uses go modules.

As a result, all (if not all) the issues with the unidiomatic way of handling the tests_repo, setup.sh, lib.sh, etc. still apply; no matter if #1911 is merged only, this one, or both of them. Please, read kata-containers/kata-containers#42 (comment) and #1420 (comment) carefully.

Summarizing, it is ok if you want to merge #1911 first, as it will work for now. However, I will keep this PR open, I will update it and I will update the title. I believe this is the way to go in the mid term. Not because of vendoring, but because the current repo structure of kata-containers as a project is broken from a 'go modules workspace' perspective.

[caoruidong]

This is just a weekly ping, not a force merge or close. I think repo structure may change some day. Your PR may be the final resolution. You can keep this PR open anyway.

[umarcor]

I think repo structure may change some day.

I think this is the relevant issue here. I don't mind if it is this PR or any other. But I believe that kata-containers/kata-containers#42 deserves some love from the community of maintainers and contributors of kata-containers.

[caoruidong]

FYI, it's #1413

[umarcor]

FYI, it's #1413

I'm sorry, but I don't understand the relation. #1413 is about unifying all the repositories in a monorepo. I commented there.

What I mean with 'the structure of the project' here is not to unify, but to think about it. For example: why are tests and other non-build dependencies installed through go get, which is precisely meant to handle build dependencies? It might be ok to use go install, should the tests repo be a go tool. Otherwise, curl, git, wget, etc. should be used. These are standard tools to retrieve assets; and tests are assets. See 761534e for a taste of what I mean.

[caoruidong]

Got it. It is another choice. Personally I'm OK with it. Let's hear from the community.

[raravena80]

@umarcor any updates on this PR? Thx!

[umarcor]

Hi @raravena80! Unfortunately... no. This issue is likely to be stalled until anyone tries to reorganise the repo/testing layout: https://github.com/kata-containers/tests. Since that's a (not pleasant) chore, and not a new feature or an improvement, I assume that nothing will be done until things break really badly and the whole project is blocked.

[raravena80]

@umarcor any updates?

Your weekly Kata herder 😄

[umarcor]

@raravena80, see #1911 (comment)

[raravena80]

Btw, one of the downsides of go modules is that in some cases the build might take longer due to the dependencies being pulled across the network.

[umarcor]

Agree. A mechanism to explicitly define a local cache folder (a la vendor but without checking it into git) would be really useful.

[grahamwhaley]

as this is effectively replaced by #1911 , can we close this one @umarcor @raravena80

[umarcor]

@grahamwhaley, as discussed in the comments above, and as explained in #1911 (comment) , #1911 is likely to be a (probably broken) subset of this PR. I'm ok with closing this PR, as it didn't get enough attention in almost a year. Moreover, I'm ok with @amshinde taking over this PR. Nonetheless, #1911 does NOT solve the problem that has prevented this PR from going forward.

[grahamwhaley]

Ah, thanks for the clarification @umarcor - there were a lot of comments to read through, and I'm not a go mod expert ;-).
Let's leave this here for a little while longer then - now us gatekeepers should see this note and not nudge you again for a while!

[raravena80]

@umarcor this is old, should we close this and move the work to 2.0?

[umarcor]

@raravena80, as explained, the blocking factor for this PR is unrelated to the runtime. Hence, I think that closing this PR and opening a new one in kata-containers/kata-containers is just a cosmetic change. The problem is with kata-containers/tests, which did not change as far as I can tell. Precisely:

https://github.com/kata-containers/tests#prepare-an-environment

Clone the kata-container/tests repository:

$ go get -d github.com/kata-containers/tests

go get is NOT a command to "clone a repository". go get is for ADDING DEPENDENCIES and/or INSTALLING TOOLS. Assets should (need to) be installed using git, wget, curl... or ANY OTHER mechanism.

IMHO, unless this organization stops using go get to retrieve assets, it is NOT possible to use go modules.

[bergwolf]

We have already moved to go mod in kata 2.0, see https://github.com/kata-containers/kata-containers/blob/2.0-dev/src/runtime/go.mod.

IMO we can close this unless it is strongly desired to use go mod in 1.x as well.

[umarcor]

@bergwolf is kata-containers/tests used in kata-containers/kata-containers? Or are all tests in the new repo written in golang?

[umarcor]

@bergwolf, @raravena80, @grahamwhaley I did a quick test to prove that the problem still exists in v2.0. It's exactly the same problem that was explained in this thread, in #1331, in #42 and in #1911. Very precisely, go modules are NOT enabled in Travis by default, so neither #1911 nor 2.0-dev in kata-containers/kata-containers are using go modules. Those branches/repos have a go.mod file which is ignored. You can easily check that in any of the CI runs of the repo: https://travis-ci.org/github/kata-containers/kata-containers/jobs/709153391#L190

I just added GO111MODULE=on (see umarcor/kata-containers@a659b54) and everything broke (https://travis-ci.com/github/umarcor/kata-containers/jobs/363292420#L252) because of kata-containers/tests. This is because, again, go get is NOT a procedure to retrieve assets, and hardcoding the default checkout path of pre-gomodules everywhere is a BROKEN procedure.

@bergwolf, see #1911 (comment)

[amshinde]

This has been fixed in our 2.0 branch instead. Will not fix in 1.x for now.

[umarcor]

@amshinde, as I said 19 months ago, and I've been repeating each time some maintainer tried to closed this as "fixed", this will NOT be fixed until ALL improper usages of go get are removed. At this point, I'm starting to feel the maintenance to be quite disrespectful towards me trying to just explain a problem that exists because of an inadequate design (structural) decision. So, once again:

Enabling GO111MODULE breaks 2.0. Travis CI is passing just because go modules are NOT used in practice.

See umarcor/kata-containers@1cc0749 and https://travis-ci.com/github/umarcor/kata-containers/jobs/392554689.

Maintainers, please read #1420 (comment) CAREFULLY. I'd be so glad to reword any explanation, should the problem be not clear enough.

[amshinde]

@umarcor I understand that this is still an outstanding issue and needs to be resolved with not only moving to go modules but also addressing all erroneous invocations of go get, replacing them with a wget or curl instead.
I simply closed this as I am trying to close any stale PRs that are not actively being worked on. With 2.0, we are consolidating all repos into kata-containers/kata-containers repo including runtime, tests, packaging among others. While we do want to backport important bugs and features, I think resolving all the issues you mentioned including changing the package management tooling would be worth doing in our 2.0 repo.
I closed this PR since I was under the impression that you are not actively working on this PR and considering we have an issue open in 2.0 repo to track remaining changes that need to be done.

[umarcor]

@amshinde, thanks for clarifying. I think it is ok to have this PR closed. The title is outdated and the content of branch umarcor:go-mod is not going to be merged. However, this was also a placeholder for acknowledging that a problem with the testing infrastructure exists. Closing it without explicitly keeping track of that requirement is what I considered unfair.

Should the plan be to fix the tests repo after everything is merged in a monolithic approach, that's absolutely acceptable. Please, just be careful for it not to be diluted.

Anyway, I opened kata-containers/kata-containers#823 with an as short and specific description as I could write.