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

go.mod: update to go v1.22 #1180

Merged
merged 1 commit into from
Jan 29, 2025
Merged

go.mod: update to go v1.22 #1180

merged 1 commit into from
Jan 29, 2025

Conversation

achilleas-k
Copy link
Member

  • Update the go.mod to use v1.22
  • Update all test workflows to install v1.22
  • Edit and run tools/prepare-source.sh

- Update the go.mod to use v1.22
- Update all test workflows to install v1.22
- Edit and run tools/prepare-source.sh
@achilleas-k achilleas-k requested a review from a team as a code owner January 29, 2025 10:46
@achilleas-k achilleas-k requested review from mvo5 and supakeen January 29, 2025 10:46
Copy link
Contributor

@schuellerf schuellerf left a comment

Choose a reason for hiding this comment

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

LGTM, although there are some places where we just dnf install go
which might cause inconsistent test results?
(e.g. test/scripts/install-dependencies or .github/workflows/test-osbuild-composer-integration.yml)
should we pin those too, in a followup PR?

@achilleas-k
Copy link
Member Author

achilleas-k commented Jan 29, 2025

No need. Those run on distros with new enough Go versions so there shouldn't be an issue.

Go version pinning isn't really important. It shows up in the GitHub actions because we use the action that supports pinning. Then there's the prepare-source.sh, which is pretty much a historical artifact now but serves mostly to avoid bumping the Go version accidentally when doing go mod stuff (which I think used to happen but doesn't anymore). As long as we have at least one GH action that make sure everything compiles correctly with our target Go version, it's fine.

@schuellerf
Copy link
Contributor

My thinking was rather that tests might behave differently e.g. a test could succeed in a newer go version but fails in the pinned/released older version

@supakeen
Copy link
Member

My thinking was rather that tests might behave differently e.g. a test could succeed in a newer go version but fails in the pinned/released older version

How do you mean? Go generally uses this version to enable/disable language features so they cannot be used; even when compiled with a newer Go.

@schuellerf
Copy link
Contributor

Probably saw to many odd behaviors with different compiler version just creating different results.
Searching a problem in production where the test (running the new compiler) was green but due to some odd bug only apparent when using the old compiler version in the release process feels scary, I guess.
This would probably fail in other tests as some might be repeated during the release process 🤔
If there wouldn't be the possibility for strange bugs like this, there wouldn't be releases of golang anymore X-D

@mvo5 mvo5 added this pull request to the merge queue Jan 29, 2025
Merged via the queue into osbuild:main with commit d5d247f Jan 29, 2025
18 checks passed
@achilleas-k achilleas-k deleted the go-1.22 branch January 29, 2025 14:59
@achilleas-k
Copy link
Member Author

Probably saw to many odd behaviors with different compiler version just creating different results. Searching a problem in production where the test (running the new compiler) was green but due to some odd bug only apparent when using the old compiler version in the release process feels scary, I guess.

But we do test with the supported version. It's just that we do manifest generation in Gitlab with whatever's in the Fedora repos. If compiling with a newer Go version generates a different manifest, that'd be a pretty serious compiler issue, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants