-
Notifications
You must be signed in to change notification settings - Fork 375
Conversation
teawater
commented
Mar 25, 2019
- Set version to 2.3.0
- Check the fail of curl
/test |
/test |
The CI failed on fedora is the same with the failed on kata-containers/agent#490, I'm not sure is this failed related with that PR, I'll have a look on this failed later. |
According what I said in kata-containers/tests#1349, use the last verion is not a good choice. This commit set it to 2.3.0. Fixes: kata-containers#1411 Signed-off-by: Hui Zhu <[email protected]>
There is no checks for curl get 404 or something else. Add a check for it. Fixes: kata-containers#1411 Signed-off-by: Hui Zhu <[email protected]>
/test |
# The redirected url should include the latest release version | ||
# https://github.com/mikefarah/yq/releases/tag/<VERSION-HERE> | ||
yq_version=$(basename "${yq_latest_url}") | ||
local yq_version=2.3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grahamwhaley - can you put your @jodh-intel hat on and PTAL? I know we have versions.yaml around, and wondering if we should be utilizing that instead of hardcoding in the installation script itself?
I don't want to necessarily hold up a fix, but...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK, to pin, to get stability. What I don't quite follow is that the curl of github release latest
should be picking up the latest released version (which is 2.3.0 for yq right now), and not any HEAD
version.
Thus, I actually think:
- this change is currently benign
- but in the long term, pinning to a known version probably gets us more stability than not pinning - but at the cost that we have to track.
Hmm, actually then, @chavafg - do we think thisyq
version number should be wired into theversions.yaml
files, and not hard wired here in a script??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@egernst @grahamwhaley I agree with put the request version to versions.yaml.
But yq is used to parse versions.yaml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teawater - heh, hah!
In which case, I have no issue with this - as long as we remember it is hard wired ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
let's see how it flies.
I got an issue on Ubuntu 20 x86_64 - I am using gollvm :
In my case
Ivan |