ci: fix GHA CI build matrix

[milosgajdos]

It would appear that requesting Go 1.21 in the build matrix has no effect whatsoever on what go toolchain is actually used to build the project. See https://brandur.org/fragments/go-version-matrix.

Specifying 1.22.0 in the go.mod go directive forces the 1.22 toolchain to be used for building Go source files, ignoring whatever Go version we set in the GHA CI matrix.

You can see the "wrong" toolchain to be used in the screenshot below:

Luckily we can enforce the right setting by setting GOTOOLCHAIN=local which forces Go to use the locally available toolchain instead of downloading the one requested by go.mod.

If we want to be one minor version back compatible we need to drop the go.mod go directive to 1.21.0, too.

As for the image builds, the Docker image we use sets the GOTOOLCHAIN=local by default:

$ docker run -it golang:1.22.5 /bin/bash
root@f0dde6c33f17:/go# go env GOTOOLCHAIN
local

[thaJeztah]

Ah, yes, these are the fun ones. I think this is caused by #4347, which switched to the SemVer format, which forces it to download the version. I've tried to avoid using that format, but not sure how much longer Go allows the old format to be used.

Honestly, I think that the go-setup-action should also either set GOTOOLCHAIN=local or produce a big fat error when trying to specify a version without it being set.

[thaJeztah]

Does it still allow the version to be specified without .0 ? That would also avoid some of this.

More fun facts; it's likely that this also downgrades language version; i.e. even if you have a (say) _go1.22.go file to provide compatibility for multiple go versions, Go may now ignore those, and just fail to compile "must use goXXX". See opencontainers/runc#4277 (comment)

[milosgajdos]

it's likely that this also downgrades language version

I think that's what we want, right? If we want b/w compatibility with one minor version back we should make sure we don't use the language features that are incompatible with those

[thaJeztah]

Yeah, so the weird thing there was that, it's not even possible to produce support for both through common mechanisms; i.e., talking about constructs like;

foo_go1.21.go

//go:build !go1.22

// implementation for go < 1.22

foo_current.go

//go:build go1.22

// implementation for go >= 1.22

[milosgajdos]

If we mark go.mod with the minimum supported Go version and force the compilation using that very version as this PR attempts to do I am pretty happy with that outcome.

[thaJeztah]

Typo; tolchain

[Jamstah]

What's with the new certs in the vendor folder? Is that really an artifact of the go toolchain change?

[milosgajdos]

What's with the new certs in the vendor folder? Is that really an artifact of the go toolchain change?

I am not entirely sure. The certs were added as part of running go mod vendor. I remember looking into that at the time

I had a look at the dependency repo (github.com/google/s2a-go) and saw that the certs (used for testing only) were added in google/s2a-go#103 which was included in https://github.com/google/s2a-go/releases/tag/v0.1.1

Our deps are pinned to v0.1.4

distribution/go.mod

Line 63 in 0e85089

github.com/google/s2a-go v0.1.4 // indirect

So it's a bit strange. My theory at the time was there is a difference in how different toolchain versions handle dependency vendoring (i.e. go mod vendor command) 🤷‍♂️

Now, I'm thinking we should bump the Go version in the go.mod to 1.22 anyway, now that 1.23.X has been out for a bit, which will nuke these certs from the vendor

[thaJeztah]

My theory at the time was there is a difference in how different toolchain versions handle dependency vendoring (i.e. go mod vendor command) 🤷‍♂️

Yes, I think newer versions more aggressively pruning files from vendor. It already excluded testdata directories elsewhere, and for these ones already removed the .pem files. Perhaps the only reason it kept the directory was because it detected the .der files as binary files and kept them before;
https://github.com/google/s2a-go/tree/v0.1.4/internal/v2/remotesigner/testdata

[milosgajdos]

I think we should bump it back to 1.22 🤷‍♂️ WDYT @Jamstah

[wy65701436]

We can specify the Go version in the go.mod file and split the matrix into two jobs: one that uses the version defined in go.mod and another that uses the local version.

Like: prometheus/prometheus@1e01e97#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR44

[milosgajdos]

We can specify the Go version in the go.mod file and split the matrix into two jobs: one that uses the version defined in go.mod and another that uses the local version.

Like: prometheus/prometheus@1e01e97#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR44

Isnt that what this PR is actually doing? Am I missing something?

[milosgajdos]

I've decided to bump the Go version in go.mod and Dockerfiles

[wy65701436]

We can specify the Go version in the go.mod file and split the matrix into two jobs: one that uses the version defined in go.mod and another that uses the local version.
Like: prometheus/prometheus@1e01e97#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR44

Isnt that what this PR is actually doing? Am I missing something?

I mean, we don't use the matrix, and break them into two jobs.

[milosgajdos]

I mean, we don't use the matrix, and break them into two jobs.

I think the matrix works well. No need for unnecessary redundancy IMHO 🤷‍♂️

[wy65701436]

lgtm