Skip to content

Start testing against Go 1.25 #1014

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.

Sign up for GitHub

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

Merged
brandur merged 1 commit into from
Aug 16, 2025
+22 −11
Merged

Start testing against Go 1.25 #1014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 13 additions & 5 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,20 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
# Run the 4 latest Postgres versions against the latest Go version:
go-version:
- "1.24"
postgres-version: [14, 15, 16, 17]
include:
# Also run previous Go version against the latest Postgres version:
# Run the 4 latest Postgres versions against the latest Go version:
- go-version: "1.25"
postgres-version: 17
- go-version: "1.25"
postgres-version: 16
- go-version: "1.25"
postgres-version: 15
- go-version: "1.25"
postgres-version: 14
Copy link
Copy Markdown
Contributor Author

@brandur brandur Aug 14, 2025

Choose a reason for hiding this comment

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

I ended up flattening this matrix out because I don't think it was working as intended before, or if it was, it was only doing so accidentally.

GitHub Actions matrixes operation quite unintuitively. I read those docs three times and still don't 100% understand it, but if I added only 1.24/17, it would buit it on all of Postgres 17/16/15/14, which is obviously not what was intended. Found it easier just to unroll the matrix like this which isn't too many more LOCs anwyay.

Copy link
Copy Markdown
Contributor

@bgentry bgentry Aug 16, 2025

Choose a reason for hiding this comment

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

I think the intention was to test the latest Go version against all recent supported Postgres versions, and then separately test each recent Go version against the latest Postgres. This was on the assumption that cross PG-Go version incompatibilities are super unlikely, whereas it's more likely that some dependency doesn't work as well in a particular Go version or against a certain PG version.

Copy link
Copy Markdown
Contributor Author

@brandur brandur Aug 16, 2025

Choose a reason for hiding this comment

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

Oh yeah, I totally understood that part of it. What I meant is just that the matrix functionality is kind of weirder than you'd expect. If you wanna play with it a bit, I'm sure there must be another way to get it working as intended, but my brain was too strained to figure it out.


# Also run a couple previous Go versions against the latest Postgres version:
- go-version: "1.24"
postgres-version: 17
- go-version: "1.23"
postgres-version: 17
Comment on lines +41 to 45
Copy link
Copy Markdown
Contributor

@bgentry bgentry Aug 13, 2025

Choose a reason for hiding this comment

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

Thoughts on removing the v1.23 from here? I don't think it adds much tbh.

Copy link
Copy Markdown
Contributor Author

@brandur brandur Aug 14, 2025

Choose a reason for hiding this comment

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

I'm kindaa thinking that we should lave that in for the time being. It's still the version of Go in all the go.mods, and we should be testing against what our ostensible lowest version is.

We could bump all the go.mod's to 1.24, but I don't think we should — only the last two versions of Go are supported in practice, but a lot of people get stuck behind, at least for a while. If there's a feature we really want in the latest or something we really wanted to drop, I can see the logic for an aggressive push for new Go versions, but given there isn't this time around ... may as well leave that lower version constraint as low as is convenient. At least for now.

Thoughts?

fail-fast: false
Expand Down
15 changes: 9 additions & 6 deletions cmd/river/rivercli/river_cli_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,9 @@ func TestBaseCommandSetIntegration(t *testing.T) {

buildInfo, _ := debug.ReadBuildInfo()

require.Equal(t, strings.TrimSpace(fmt.Sprintf(`
River version (unknown)
// `devel` on 1.25, `unknown` on versions previous to that
require.Regexp(t, strings.TrimSpace(fmt.Sprintf(`
River version \((devel|unknown)\)
Built with %s
`, buildInfo.GoVersion)), strings.TrimSpace(bundle.out.String()))
})
Expand All @@ -195,8 +196,9 @@ Built with %s

buildInfo, _ := debug.ReadBuildInfo()

require.Equal(t, strings.TrimSpace(fmt.Sprintf(`
River version (unknown)
// `devel` on 1.25, `unknown` on versions previous to that
require.Regexp(t, strings.TrimSpace(fmt.Sprintf(`
River version \((devel|unknown)\)
Built with %s
`, buildInfo.GoVersion)), strings.TrimSpace(bundle.out.String()))
})
Expand Down Expand Up @@ -502,8 +504,9 @@ func TestVersion(t *testing.T) {

buildInfo, _ := debug.ReadBuildInfo()

require.Equal(t, strings.TrimSpace(fmt.Sprintf(`
River version (unknown)
// `devel` on 1.25, `unknown` on versions previous to that
require.Regexp(t, strings.TrimSpace(fmt.Sprintf(`
River version \((devel|unknown)\)
Built with %s
`, buildInfo.GoVersion)), strings.TrimSpace(bundle.buf.String()))
})
Expand Down