Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

CI: Really fix xurls URL issue #843

Closed

Conversation

jodh-intel
Copy link
Contributor

Effectively reverted #839 as the real problem appears to be that although the CI is downloading and installing the correct version of golang, it's not actually using it.

I noticed this because xurls has recently changed the minimum golang version needed to run it to 1.10.3+:

Testing locally, the old go get for xurls works fine but not with versions of go < 1.10.3.

However, although this PR resolves the issue in this repo, we still need to update golang to 1.10.3+ so this PR will fail until the following one lands:

Use a variable in the golang install script to avoid repetition.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
The script used to install golang was downloading and installing the
binary package, but not actually using the new golang version.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@jodh-intel jodh-intel force-pushed the ci-fix-xurls-url-harder branch from 555f180 to 353e6d8 Compare October 23, 2018 12:27
@jodh-intel
Copy link
Contributor Author

/test

@jodh-intel
Copy link
Contributor Author

Hmm - the go "fix" may actually be bogus since the Jenkins job does set PATH correctly:

In which case, either that isn't propagating to the static checks script or this isn't the problem with xurls ;(

gorootbin="${GOROOT}/bin"
[ -d "${gorootbin}" ] || die "failed to find expected golang binary path ${gorootbin} (from ${archive})"

export PATH="${gorootbin}:$PATH"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iiuc, this script is called (nor sourced) from the jenkins script - so, these exports will be happening in a sub-shell, and not propagate up/out will they?
Maybe there needs to be a GOPATH type fix in either the jenkins script (caller) or the static script (user) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - see comment above. I'm honestly not sure currently where the fix would go as the Jenkins script looks fine - any thoughts?

/cc @chavafg.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hold on - you say we need golang >= 1.10.3 - but, our versions.yaml is not fixed for that yet (as you noted on the runtime repo PR).
So, we should expect this not to work until we get golang updated, with the pending PRs.

Let me go check that PR - last I remember upstream crio got patched so we could make the version bump, let me see where we are stuck....

Undo URL change from [1] since the xurls install issue was caused by
the CI using a version of golang that is too old for xurls.

Fixes kata-containers#841.

---
[1] - kata-containers#839

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@jodh-intel jodh-intel force-pushed the ci-fix-xurls-url-harder branch from 353e6d8 to 3324e20 Compare October 23, 2018 13:57
@jodh-intel
Copy link
Contributor Author

Re-pushed now that kata-containers/runtime#744 has landed.

@jodh-intel
Copy link
Contributor Author

/test

@jodh-intel
Copy link
Contributor Author

From the Travis log:

INFO: Installing xurls utility
Fetching https://mvdan.cc/xurls/cmd/xurls?go-get=1
Parsing meta tags from https://mvdan.cc/xurls/cmd/xurls?go-get=1 (status code 200)
get "mvdan.cc/xurls/cmd/xurls": found meta tag get.metaImport{Prefix:"mvdan.cc/xurls", VCS:"git", RepoRoot:"https://github.com/mvdan/xurls"} at https://mvdan.cc/xurls/cmd/xurls?go-get=1
get "mvdan.cc/xurls/cmd/xurls": verifying non-authoritative meta tag
Fetching https://mvdan.cc/xurls?go-get=1
Parsing meta tags from https://mvdan.cc/xurls?go-get=1 (status code 200)
mvdan.cc/xurls (download)
Fetching https://mvdan.cc/xurls?go-get=1
Parsing meta tags from https://mvdan.cc/xurls?go-get=1 (status code 200)
get "mvdan.cc/xurls": found meta tag get.metaImport{Prefix:"mvdan.cc/xurls", VCS:"git", RepoRoot:"https://github.com/mvdan/xurls"} at https://mvdan.cc/xurls?go-get=1
mvdan.cc/xurls
mvdan.cc/xurls/cmd/xurls
INFO: Checking documentation
INFO: Checking local branch for changed documents only
INFO: No documentation to check

All that xurls spew is good by the way (but ugly as sin :)

@grahamwhaley
Copy link
Contributor

You could chronic it to hide the mess :-)

@jodh-intel
Copy link
Contributor Author

Ah - sorry, I should have explained - I actually wanted to see this output so I added the -v to go get - I just didn't want us to hide all the spew given all the trouble this package has caused us ;)

@chavafg
Copy link
Contributor

chavafg commented Oct 23, 2018

lgtm

Approved with PullApprove

@jodh-intel
Copy link
Contributor Author

The fact that #838 is now showing passed CI jobs suggests that this PR is indeed bogus - the real issue being that xurls changed the minimum version of golang it requires to work so it wasn't an issue with the xurls projects URL:

The CI installs golang version languages.golang.meta.newest-version (not languages.golang.version):

Previous to kata-containers/runtime#744 landing, the CI would have been installing golang 1.10 which is too old for the latest xurls which needs 1.10.3):

I'm going to close this PR but if we get any more issues with xurls, we should consider versioning it in:

@jodh-intel jodh-intel closed this Oct 23, 2018
jodh-intel added a commit to jodh-intel/documentation that referenced this pull request Oct 23, 2018
Travis appears to be providing a version of golang that is too old for
https://mvdan.cc/xurls/cmd/xurls, which is used by the CI scripts in the
tests repo.

See:

- kata-containers/runtime#744
- kata-containers/tests#843 (comment)

Fixes kata-containers#281.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
jodh-intel added a commit to jodh-intel/documentation that referenced this pull request Oct 23, 2018
Travis appears to be providing a version of golang that is too old for
https://mvdan.cc/xurls/cmd/xurls, which is used by the CI scripts in the
tests repo.

See:

- kata-containers/runtime#744
- kata-containers/tests#843 (comment)

Fixes kata-containers#281.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
jodh-intel added a commit to jodh-intel/documentation that referenced this pull request Oct 23, 2018
Travis appears to be providing a version of golang that is too old for
https://mvdan.cc/xurls/cmd/xurls, which is used by the CI scripts in the
tests repo.

See:

- kata-containers/runtime#744
- kata-containers/tests#843 (comment)

Fixes kata-containers#281.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
jodh-intel added a commit to jodh-intel/documentation that referenced this pull request Oct 23, 2018
Travis appears to be providing a version of golang that is too old for
https://mvdan.cc/xurls/cmd/xurls, which is used by the CI scripts in the
tests repo.

See:

- kata-containers/runtime#744
- kata-containers/tests#843 (comment)

Fixes kata-containers#281.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
jodh-intel added a commit to jodh-intel/documentation that referenced this pull request Oct 23, 2018
Travis appears to be providing a version of golang that is too old for
https://mvdan.cc/xurls/cmd/xurls, which is used by the CI scripts in the
tests repo.

See:

- kata-containers/runtime#744
- kata-containers/tests#843 (comment)

Fixes kata-containers#281.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
jodh-intel added a commit to jodh-intel/documentation that referenced this pull request Oct 23, 2018
Travis appears to be providing a version of golang that is too old for
https://mvdan.cc/xurls/cmd/xurls, which is used by the CI scripts in the
tests repo.

See:

- kata-containers/runtime#744
- kata-containers/tests#843 (comment)

Required adding a NOP makefile to avoid Travis from trying to build
this repo using `go`.

Fixes kata-containers#281.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants