Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

Fix yq issues #1414

Merged
grahamwhaley merged 2 commits into from
Apr 8, 2019
Merged

Fix yq issues #1414

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7fa0390
ci: set $yq_version to 2.3.0
teawater Mar 25, 2019
4993dff
ci: Add a failed execution check for curl
teawater Mar 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions .ci/install-yq.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,11 @@ function install_yq() {
die "Please install curl"
fi

# Workaround to get latest release from github (to not use github token).
# Get the redirection to latest release on github.
yq_latest_url=$(curl -Ls -o /dev/null -w %{url_effective} "https://${yq_pkg}/releases/latest")
# 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
Copy link
Copy Markdown
Member

@egernst egernst Apr 3, 2019 •
edited
Loading

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...

Copy link
Copy Markdown
Contributor

@grahamwhaley grahamwhaley Apr 4, 2019

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 this yq version number should be wired into the versions.yaml files, and not hard wired here in a script??

Copy link
Copy Markdown
Member Author

@teawater teawater Apr 6, 2019

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.

Copy link
Copy Markdown
Contributor

@grahamwhaley grahamwhaley Apr 8, 2019

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 ;-)


local yq_url="https://${yq_pkg}/releases/download/${yq_version}/yq_${goos}_${goarch}"
curl -o "${yq_path}" -LSs ${yq_url}
curl -o "${yq_path}" -LSsf ${yq_url}
[ $? -ne 0 ] && die "Download ${yq_url} failed"
chmod +x ${yq_path}

if ! command -v "${yq_path}" >/dev/null; then
Expand Down