Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not use 'go get' for retrieving assets; use git, wget, curl, etc. instead #823

Closed
umarcor opened this issue Sep 29, 2020 · 4 comments
Closed
Labels
area/ci Issues affecting the continuous integration bug Incorrect behaviour good-first-issue Small and simple task for new contributors lang/shell Shell script (bash)

Comments

@umarcor
Copy link

umarcor commented Sep 29, 2020

Description of problem

  • go get is NOT for retrieving assets, but for adding Go (build) dependencies.
  • git, wget, curl, etc. might be used for retrieving assets.
  • In this project, go get is used incorrectly for retrieving assets.
  • Enabling go modules is broken because the testing infrastructure relies on go get downloading assets to specific locations. See https://github.com/kata-containers/kata-containers/blob/2.0-dev/ci/lib.sh#L17.
  • Travis CI is passing because go modules are NOT being used.
  • Some maintainers seem to think that go modules are properly supported just because go.mod files were added to the repo.

kata-containers/runtime#1911 (comment)
@bergwolf:
We should not use the new go get to download code or binary inside a go module directory, -- it would fetch the code and add it to your go.mod dependency. git clone is your friend ;)

Expected result

Running Travis CI with go modules enabled should be successful.

Actual result

Please, see kata-containers/runtime#1420 (comment).

Further information

@umarcor umarcor added bug Incorrect behaviour needs-review Needs to be assessed by the team. labels Sep 29, 2020
@ariel-adam ariel-adam added good-first-issue Small and simple task for new contributors bug Incorrect behaviour area/ci Issues affecting the continuous integration and removed bug Incorrect behaviour needs-review Needs to be assessed by the team. labels Jan 19, 2021
@jodh-intel
Copy link
Contributor

If anyone wants to do this, step 1 is to grep -r "go get" and change each occurrence to use curl -sLO $URL instead (no wget please).

@ariel-adam
Copy link
Contributor

We will leave this as a parent issue not assigned to anyone specific.

@umarcor
Copy link
Author

umarcor commented Jan 19, 2021

@jodh-intel, last time I had a look at this, the problem was not replacing go get with curl|wget|git. That is rather easy, since it is a mechanical replacement. The actual problem is that all the scripts rely on assets being installed in some specific location in the GOPATH. That location changes when go modules are enabled; so the scripts cannot find them. Installing assets into the GOPATH without using go is likely to produce more headaches than help solve the issue. Please, see the detailed explanation in kata-containers/runtime#1420 (comment) about the problems derived from just changing from go get to curl.

Therefore, three things need to be modified at the same time: how are assets retrieved, where are assets downloaded to, how do scripts find assets (sometimes hardcoded). I could and can "fix" the two first points, but the latter requires an expertise/knowledge about this project's testing infrastructure as a whole. As a result, I believe that modifying this needs to be guided by someone with a good understanding of the organisation and the overall testing architecture. I wouldn't call this a "good first issue".

@liubin
Copy link
Member

liubin commented Dec 1, 2022

This should be closed via kata-containers/tests#5224

@liubin liubin closed this as completed Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci Issues affecting the continuous integration bug Incorrect behaviour good-first-issue Small and simple task for new contributors lang/shell Shell script (bash)
Projects
None yet
Development

No branches or pull requests

4 participants