[RFC] Setup a way to consistently manage go versions across scripts and go.mods #17876

MadhavJivrajani · 2024-04-25T08:17:50Z

This PR does the following:

scripts: default to using .go-version's version for tests and builds

Additionally, provide ability to opt-out of the .go-version and use a
custom one via env vars: FORCE_HOST_GO and GO_VERSION.

.*: sync go toolchain version and add ability to verify versions

This commit adds a script to sync the version present in .go-version
across all go.mod files as the toolchain directive. As part of that,
this commit also modifies go.mod files that did not have synced toolchain
directives.

Additionally, this also adds a script to verify all toolchain and go
directives against the version present in .go-version as follows:
(1) The go directive <= version in .go-version
(2) The toolchain directive == version in .go-version

The make target for this script is run as part of make verify which will automatically be run as a presubmit.

/assign @ahrtr @jmhbnz @serathius

ahrtr · 2024-04-25T11:45:27Z

scripts/sync_go_toolchain_directive.sh

+source ./scripts/test_lib.sh
+
+TARGET_GO_VERSION="${TARGET_GO_VERSION:-"$(cat "${ETCD_ROOT_DIR}/.go-version")"}"
+find . -name 'go.mod' -exec go mod edit -toolchain=go${TARGET_GO_VERSION} {} \;


There are three options,

only sync go line, e.g. go 1.22.2;

only sync toolchain line, e.g. toolchain go1.22.2;

sync both go and toolchain lines.

No matter which way we select, it will always use the highest go version by default.

For simplicity, can we go with option 1?

@ahrtr, if I understand correctly, the issue I see with option 1 and 3 is that whenever we allow bumping the go directive, we are also forcing developers of etcd and other projects that import etcd modules to bump their versions. From https://go.dev/doc/toolchain:

the go line sets the minimum required Go version necessary to use a module or workspace.

Yes, it's true. It's OK to go for option 2. But please also see my next related comment #17876 (comment)

ahrtr · 2024-04-29T11:43:00Z

scripts/verify_toolchain_directive.sh

+for mod in $(find . -name 'go.mod'); do
+    toolchain_version="$(go mod edit -json ${mod} | jq -r .Toolchain)"
+    if [[ "go${target_go_version}" != "${toolchain_version}" ]]; then
+        log_error "go toolchain directive out of sync for ${mod}, got: ${toolchain_version}"
+        out_of_sync="true"
+    fi
+done


The go lines in all etcd's go.mod files are also automatically bumped if any etcd's dependency bumped its go version. Should we also verify that the version in go lines <= .go-version?

@ahrtr that's a good point, however I'm not sure what the course of action is once the dependency bump PRs are merged. If a go line bump occurred and the PR is merged, do we just update the .go-version file then?

My main concern with allowing dependency bumps to also bump the go line is that it can cause an issue with projects like Kubernetes that import go.etcd.io... modules. If the go lines of these projects are behind those of the etcd modules, then they will be either forced to bump or they cannot update the dependency till they catch up (kubernetes/kubernetes#123744)

This behaviour won't be an issue in go1.23, but till that is released and we can adopt that, it is risky allowing the go line to be bumped at all, especially from a perspective of Kubernetes consuming etcd. For bumps related to security fixes or bugs that we see in etcd, we should probably discuss what the course of action should be, but we should probably we a little more aware for usual maintenance bumps.

they cannot update the dependency till they catch up

I think we should follow this.

For main branch, it isn't a problem because usually we keep using the latest go version (e.g. 1.22.2 for now).

For stable releases (3.5 and 3.4 for now), usually we keep using the latest patch of previous minor version (e.g. 1.21.9 for now). Usually, we don't proactively bump dependencies for etcd stable releases unless there are any CVEs or bugs that affect etcd. If bumping a dependency for a stable release leads to the go version being automatically bumped,

check whether there is an alternative. For example, if both 4.20.6 and 4.21.0 of a dependency resolve a CVE, but 4.20.6 requires go1.21.9 and 4.21.0 requires go 1.22.2, then we bump 4.20.6 for etcd stable releases (3.4/3.5).

if there is no alternative

if we have to bump the dependency, then we bump go version for etcd stable releases (3.4/3.5) as well.

if we don't have to bump the dependency (it's unlikely running into this case), we can hold off for now until we bump go version.

We need to clearly document the behaviour in https://github.com/etcd-io/etcd/blob/main/Documentation/contributor-guide/dependency_management.md#golang-versions

@ahrtr that sounds good, I've added the extra check.

Also, ack on documenting this.

ahrtr · 2024-04-29T11:45:52Z

Please also resolve the workflow failure: # shellcheck disable=SC2086

We also need to clearly document the behaviour.

MadhavJivrajani · 2024-05-02T12:29:45Z

@ahrtr on thinking about this a little more:
Whenever a dep bump changes the toolchain version, the version will be changed to whatever is the version of the local toolchain, from https://go.dev/doc/toolchain:

For repeatability, any command that updates the go line also updates the toolchain line to record its own toolchain name.

This means that automated PRs, if it runs ./scripts/test_lib.sh, will never accidentally change the toolchain line because we keep that in sync by setting the GOTOOLCHAIN env var as the version in .go-version. However, we still need to check it in case of manual bumps.

I've also changed the workflow to a presubmit instead of a periodic because now either case should not be allowed to be merged.

Makefile

MadhavJivrajani · 2024-05-06T06:36:49Z

@ahrtr can you PTAL? Thank you!

ahrtr · 2024-05-06T10:08:43Z

Overall looks good. Could you rebase this PR although github doesn't show any conflict? I see that it's still based on a commit of 2 weeks ago.

Additionally, provide ability to opt-out of the .go-version and use a custom one via env vars: FORCE_HOST_GO and GO_VERSION. Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

This commit adds a script to sync the version present in .go-version across all go.mod files as the toolchain directive. As part of that, this commit also modifies go.mod files that did not have synced toolchain directives. Additionally, this also adds a script to verify all toolchain and go directives against the version present in .go-version as follows: (1) The go directive <= version in .go-version (2) The toolchain directive == version in .go-version This script runs as part of the `make verify` target, making it run as a presbumit by default. Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

MadhavJivrajani · 2024-05-07T12:23:44Z

Done @ahrtr

ahrtr

Great work!

Thanks @MadhavJivrajani

Please also consider to update the doc as mentioned in #17876 (comment). It can be resolved in a separate PR.

ahrtr · 2024-05-08T05:41:06Z

cc @fuweid @ivanvc @jmhbnz @serathius

serathius · 2024-05-08T07:15:58Z

scripts/fix.sh

@@ -36,5 +36,7 @@ run_for_modules run ${GO_CMD} fmt || exit 2
 run_for_module tests bom_fix || exit 2
 bash_ws_fix || exit 2

+log_callout "Syncing go toolchain directives"
+run ./scripts/sync_go_toolchain_directive.sh || exit 2


Would prefer to move subroutines from ./scripts/fix.sh to makefile targets. As you already have a makefile target sync-toolchain-directive why not add it as subtarget to fix?

why not add it as subtarget to fix?

Yes, it's another option. Either way works for me.

We can even breakdown the existing ./script/fix.sh into a couple of Makefile targets

fix-go-dependency (in which we will run mod_tidy_fix and the sync_go_toolchain_directive.sh)

fix-gofmt

fix-script-whitespace

It can be discussed & resolved in a separate PR.

fuweid · 2024-05-08T16:25:11Z

Hi @ahrtr @serathius @MadhavJivrajani , do we consider to apply this to library project, like bbolt and raft? since we have discussion about this in bbolt project two days ago.

REF: etcd-io/bbolt#747

ahrtr · 2024-05-08T16:33:17Z

do we consider to apply this to library project, like bbolt and raft?

I think YES. Please see #17857 (comment), and please feel free to add comment in the issue.

k8s-ci-robot assigned ahrtr, jmhbnz and serathius Apr 25, 2024

MadhavJivrajani force-pushed the go-directive-periodic branch from a260bce to 7ae33c3 Compare April 25, 2024 08:44

ahrtr reviewed Apr 25, 2024

View reviewed changes

ahrtr reviewed Apr 29, 2024

View reviewed changes

MadhavJivrajani force-pushed the go-directive-periodic branch 2 times, most recently from 5b0aaa0 to 5f4b46c Compare May 2, 2024 12:26

MadhavJivrajani force-pushed the go-directive-periodic branch 4 times, most recently from e284a89 to e41b372 Compare May 2, 2024 13:13

ahrtr reviewed May 3, 2024

View reviewed changes

Makefile Show resolved Hide resolved

Makefile Show resolved Hide resolved

MadhavJivrajani force-pushed the go-directive-periodic branch 2 times, most recently from 59416d7 to 229e2fc Compare May 6, 2024 06:31

MadhavJivrajani force-pushed the go-directive-periodic branch 2 times, most recently from 63a7a70 to 5b3ec07 Compare May 6, 2024 09:27

MadhavJivrajani requested a review from ahrtr May 6, 2024 09:30

MadhavJivrajani added 2 commits May 7, 2024 17:52

scripts: default to using .go-version's version for tests and builds

e094139

Additionally, provide ability to opt-out of the .go-version and use a custom one via env vars: FORCE_HOST_GO and GO_VERSION. Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

MadhavJivrajani force-pushed the go-directive-periodic branch from 5b3ec07 to f6a35f8 Compare May 7, 2024 12:23

ahrtr approved these changes May 7, 2024

View reviewed changes

ahrtr mentioned this pull request May 8, 2024

Bump go 1.21.10 and 1.22.3 #17964

Open

serathius reviewed May 8, 2024

View reviewed changes

serathius approved these changes May 8, 2024

View reviewed changes

ahrtr merged commit 570370f into etcd-io:main May 8, 2024
44 checks passed

ahrtr mentioned this pull request May 8, 2024

Setup a consistent way to manage go version #17857

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Setup a way to consistently manage go versions across scripts and go.mods #17876

[RFC] Setup a way to consistently manage go versions across scripts and go.mods #17876

MadhavJivrajani commented Apr 25, 2024 •

edited

ahrtr Apr 25, 2024

MadhavJivrajani Apr 29, 2024

ahrtr Apr 29, 2024

ahrtr Apr 29, 2024

MadhavJivrajani May 2, 2024 •

edited

ahrtr May 2, 2024

MadhavJivrajani May 2, 2024

MadhavJivrajani May 2, 2024

ahrtr commented Apr 29, 2024

MadhavJivrajani commented May 2, 2024 •

edited

MadhavJivrajani commented May 6, 2024

ahrtr commented May 6, 2024

MadhavJivrajani commented May 7, 2024

ahrtr left a comment

ahrtr commented May 8, 2024

serathius May 8, 2024

ahrtr May 8, 2024

fuweid commented May 8, 2024

ahrtr commented May 8, 2024

[RFC] Setup a way to consistently manage go versions across scripts and go.mods #17876

[RFC] Setup a way to consistently manage go versions across scripts and go.mods #17876

Conversation

MadhavJivrajani commented Apr 25, 2024 • edited

scripts: default to using .go-version's version for tests and builds

.*: sync go toolchain version and add ability to verify versions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadhavJivrajani May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Apr 29, 2024

MadhavJivrajani commented May 2, 2024 • edited

MadhavJivrajani commented May 6, 2024

ahrtr commented May 6, 2024

MadhavJivrajani commented May 7, 2024

ahrtr left a comment

Choose a reason for hiding this comment

ahrtr commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid commented May 8, 2024

ahrtr commented May 8, 2024

MadhavJivrajani commented Apr 25, 2024 •

edited

MadhavJivrajani May 2, 2024 •

edited

MadhavJivrajani commented May 2, 2024 •

edited