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

Revert to Go 1.18 in go.mod and make policy around this official #390

Merged
merged 2 commits into from
May 6, 2024

Conversation

jacobweinstock
Copy link
Member

@jacobweinstock jacobweinstock commented May 3, 2024

What does this PR implement/change/remove?

We don't seem to have an official strategy for the Go version in go.mod. I believe that, as a library, we should only bump this version if there are dependencies we use that would require us to move to this version (ref). I don't believe that we have any dependencies that warrant a bump. Bumping this version will require user to update to this version. While it is a good practice to build with the latest version of Go, this requirement will be making a decision for users that should be theirs and not ours. We are a library not a binary to be built. In this aspect, we are using the version in go.mod incorrectly, in my opinion, by doing this. This assumes we don't have a known use case where bumping this version if needed. I don't know of any at the moment. If there are any we should bring them up.

Apologies, i know i approved the PR to update this. The affects of this didn't dawn on me then.

The other side of the coin

There can be reasons to bump this version. I think before we bump the version we should have use cases the bump and more carefully consider the affects. From the official Go docs:
"At go 1.21 or higher:
The go line declares a required minimum version of Go to use with this module.
The go line must be greater than or equal to the go line of all dependencies.
The go command no longer attempts to maintain compatibility with the previous older version of Go.
The go command is more careful about keeping checksums of go.mod files in the go.sum file."
- ref

Checklist

  • Tests added
  • Similar commits squashed

The HW vendor this change applies to (if applicable)

The HW model number, product name this change applies to (if applicable)

The BMC firmware and/or BIOS versions that this change applies to (if applicable)

What version of tooling - vendor specific or opensource does this change depend on (if applicable)

Description for changelog/release notes

@joelrebel
Copy link
Member

joelrebel commented May 6, 2024

@jacobweinstock based on the Go release policy, we'd want to be on a release that is not at its EOL?
Each major Go release is supported until there are two newer major releases
ref: https://go.dev/doc/devel/release#policy

@ofaurax
Copy link
Collaborator

ofaurax commented May 6, 2024

Note that is the minimum version supported.

By setting it to 1.18, we don't prevent people from using latest version.
But by setting it to 1.22 (or oldest not EOL), we just force people to upgrade without technical justification other than "keep your go secure" (which is not this project's mission).

I see several options here: we could stay on the smallest go version of dependencies, or follow the base go version available for some platforms (e.g. min of go version available in oldest supported debian/redhat/ubuntu)

@@ -88,7 +88,6 @@ golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.19.0 h1:q5f1RH2jigJ1MoAWp2KTp3gm5zAGFUTarQZ5U386+4o=
golang.org/x/sys v0.19.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
golang.org/x/term v0.19.0 h1:+ThwsDv+tYfnJFhF4L8jITxu1tdTWRTZpdsWgEgjL6Q=
golang.org/x/term v0.19.0/go.mod h1:2CuTdWZ7KHSQwUzKva0cbMg6q2DMI3Mmxp+gKJbskEk=
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this an effect of the downgrading?

@chrisdoherty4
Copy link
Collaborator

chrisdoherty4 commented May 6, 2024

I agree with the OP: libraries should honor the definition of the go directive and specify what they need (apps probably should also, particularly as of 1.21 which introduces toolchain).

I see several options here: we could stay on the smallest go version of dependencies, or follow the base go version available for some platforms (e.g. min of go version available in oldest supported debian/redhat/ubuntu)

I think the only reason to care about supported platforms is in implementation detail because you wouldn't want to use a language feature > than the oldest supported platform.

Assuming there aren't any language feature issues I would stick with the definition of the go directive and select the oldest version that can be used for the language features leveraged.

@joelrebel
Copy link
Member

Those are fair points, thanks for raising them. If we can get a rebase on main here then this can be merged.

We don't seem to have an offical
strategy for the Go version in
go.mod. I believe that, as a library,
we should only bump this version if
there are dependencies we use that
would require us to move to this version.
I don't believe that we have any dependencies
that warrant a bump.

Signed-off-by: Jacob Weinstock <[email protected]>
This makes official our philosophy
and policy for the Go version in go.mod.

Signed-off-by: Jacob Weinstock <[email protected]>
@jacobweinstock jacobweinstock removed the request for review from chrisdoherty4 May 6, 2024 16:37
@mergify mergify bot merged commit 77eee83 into bmc-toolbox:main May 6, 2024
5 checks passed
@jacobweinstock jacobweinstock deleted the gomod-version branch May 6, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants