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

ci/cirrus: use Go 1.19.x not 1.19 #3814

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Apr 5, 2023

This variable is used in curl to download a go release, so we are using the initial Go 1.19 release in Cirrus CI, not the latest Go 1.19.x release.

From the CI perspective, it makes more sense to use the latest release, so let's use 1.19.x here.

Add some shell jq magic to extract the latest minor release information
from the download page, and use it.

This brings Cirrus CI jobs logic in line with all the others (GHA,
Dockerfile), where by 1.20 we actually mean "latest 1.20.x".

Previous commits touching this:

.cirrus.yml Outdated Show resolved Hide resolved
@kolyshkin kolyshkin force-pushed the go-1.19-minor branch 4 times, most recently from 3dbc526 to 5d27ae0 Compare April 5, 2023 23:00
@@ -118,7 +118,10 @@ task:
# Use --whatprovides since some packages are renamed.
rpm -q --whatprovides $RPMS
# install Go
curl -fsSL "https://dl.google.com/go/go${GO_VERSION}.linux-amd64.tar.gz" | tar Cxz /usr/local
PREFIX="https://go.dev/dl/"
Copy link
Member

Choose a reason for hiding this comment

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

If we have jq as option, we could use the JSON feed; https://go.dev/dl/?mode=json&include=stable

There's probably some docs somewhere about that URL (it looks like include=stable only returns stable versions, and the first entry would have the version at least (but first entry is "source", so we probably need to filter on os/arch).

There's some scripts used for the official Golang image on Docker Hub that may be useful; https://github.com/docker-library/golang/blob/e8e87a35e7de2343770e13ec1b6c62cafb95728d/versions.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment was marked as outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[kir@kir-rhat runc]$ PREFIX=https://golang.org/dl/
[kir@kir-rhat runc]$ GO_VERSION=1.20
[kir@kir-rhat runc]$ eval $(curl -fsSL "${PREFIX}?mode=json" | jq -r  --arg Ver "$GO_VERSION" '.[] | select(.version | startswith("go\($Ver)")) | .files[] | select(.os == "linux" and .arch == "amd64" and .kind == "archive") | "filename=\"" + .filename + "\""')
[kir@kir-rhat runc]$ echo $PREFIX$filename
https://golang.org/dl/go1.20.3.linux-amd64.tar.gz
[kir@kir-rhat runc]$ GO_VERSION=1.19
[kir@kir-rhat runc]$ eval $(curl -fsSL "${PREFIX}?mode=json" | jq -r  --arg Ver "$GO_VERSION" '.[] | select(.version | startswith("go\($Ver)")) | .files[] | select(.os == "linux" and .arch == "amd64" and .kind == "archive") | "filename=\"" + .filename + "\""')
[kir@kir-rhat runc]$ echo $PREFIX$filename
https://golang.org/dl/go1.19.8.linux-amd64.tar.gz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bash and friends version was not that bad though...

-    SUFFIX=$(curl -fsSL "$PREFIX" | grep ' href="/dl/go'$GO_VERSION'.*linux-amd64\.tar\.gz"' | head -1 | sed -e 's#^.* href="/dl/\([^"]*\)".*$#\1#g')
-    curl -fsSL "$PREFIX$SUFFIX" | tar Cxz /usr/local
+    eval $(curl -fsSL "${PREFIX}?mode=json" | jq -r  --arg Ver "$GO_VERSION" '.[] | select(.version | startswith("go\($Ver)")) | .files[] | select(.os == "linux" and .arch == "amd64" and .kind == "archive") | "filename=\"" + .filename + "\""')
+    curl -fsSL "$PREFIX$filename" | tar Cxz /usr/local

but jq is the king as it's much more structured and thus bulletproof.

Copy link
Member

Choose a reason for hiding this comment

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

found the docs! https://pkg.go.dev/golang.org/x/website/internal/dl

Ah! I was looking at the source code (spotted that @tianon added a link in that script). Was even considering if we could contribute a query-argument to filter the results by (major.minor) version. The code looks pretty straightforward, and I can envision such a query-parameter being useful for many purposes (including go-setup-action

Copy link
Member

Choose a reason for hiding this comment

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

The bash and friends version was not that bad though...

Agreed! I was mostly concerned it the HTML output was stable enough, and JSON is more structured / stable in that respect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

. Was even considering if we could contribute a query-argument to filter the results by (major.minor) version. The code looks pretty straightforward, and I can envision such a query-parameter being useful for many purposes (including go-setup-action

Yep, I'd rather use something like wget https://go.dev/dl/?version=1.20&os=linux&arch=amd64&kind=archive and be done with it.

Found a semi-relevant issue: golang/go#34864. Judging by it, it would be quite hard to move forward with a change like that.

Copy link
Member

Choose a reason for hiding this comment

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

Casually stealing that link and adding to the script so I can find it again later 😂 ❤️

Copy link
Member

Choose a reason for hiding this comment

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

it would be quite hard to move forward with a change like that.

Yeah, generally most proposals would end up with "Based on the discussion above, this proposal seems like a likely decline"

But just looking at the code, it feels like adding additional filters wouldn't do much harm (and for sure wouldn't break backward compatibility); https://github.com/golang/website/blob/41e922072f17ab2826d9479338314c025602a3a1/internal/dl/server.go#L173-L183

This variable is used in curl to download a go release, so we are using
the initial Go 1.19 release in Cirrus CI, not the latest Go 1.19.x
release.

From the CI perspective, it makes more sense to use the latest release.

Add some jq magic to extract the latest minor release information
from the download page, and use it.

This brings Cirrus CI jobs logic in line with all the others (GHA,
Dockerfile), where by 1.20 we actually mean "latest 1.20.x".

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Do not use echo, as this results in lines like this:

	...
	echo "-----"
	-----
	...

2. Move "cat /proc/cpuinfo" to be the last one, as the output is usually
   very long.

3. Add "go version" to CentOS jobs.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM!

@kolyshkin kolyshkin requested review from cyphar, hqhq and AkihiroSuda April 6, 2023 18:05
@kolyshkin kolyshkin added the backport/1.1-pr A backport PR to release-1.1 label Apr 6, 2023
@mrunalp mrunalp merged commit 941e592 into opencontainers:main Apr 6, 2023
@kolyshkin kolyshkin added backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 and removed backport/1.1-pr A backport PR to release-1.1 labels Aug 9, 2023
@kolyshkin
Copy link
Contributor Author

Whoops, this one had the wrong label set. Fixed now.

@kolyshkin
Copy link
Contributor Author

Backported to 1.1. in #3976.

@kolyshkin kolyshkin added backport/1.1-done A PR in main branch which has been backported to release-1.1 and removed backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 labels Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci backport/1.1-done A PR in main branch which has been backported to release-1.1 easy-to-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants