search

bmc-toolbox / bmclib

Library to abstract Baseboard Management Controller interaction
Apache License 2.0
194 stars 37 forks source link

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

Closed jacobweinstock closed 6 months ago

jacobweinstock commented 6 months ago

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

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 commented 6 months ago

@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 commented 6 months ago

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)

chrisdoherty4 commented 6 months ago

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 commented 6 months ago

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