Implement N-1 support policy for Go versions #4655

perhapsmaple · 2023-08-13T16:16:59Z

Which problem is this PR solving?

Resolves #4653

Description of the changes

Current files tracked are go.mod, all yml files in ./github/workflows/ and ./docker/Makefile. If there are any more build scripts to track, I would be happy to update the PR.
Added scripts/check-go-version.sh to verify that all our build scripts are using the same Go version N, but go.mod is N-1 and fail if not, with an error message and the source files along with their respective versions.
Added scripts/update-go-version.sh to update those scripts to latest version, to make these upgrades easier in the future. Also bumps go.mod to version N-1.
Updated README explaining the support policy.

How was this change tested?

I have tested the changes locally by modifying go versions

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

- Fixes #4653. Implements linter check for go versions. - Current files checked are all yml files in ./github/workflows/ and ./docker/Makefile. Signed-off-by: Harish Shan <[email protected]>

…olicy - Fixes #4653. Implements convenience script to update references of go version. - Current files checked are all yml files in ./github/workflows/ and ./docker/Makefile. Signed-off-by: Harish Shan <[email protected]>

Signed-off-by: Harish Shan <[email protected]>

yurishkuro · 2023-08-13T17:08:17Z

README.md

+
+Starting with the release of Go 1.21, support for Go versions will be updated as follows:
+
+1. The first release after the release of a new Go minor version `N` will add build and tests steps for the new Go minor version.


I am not sure we want to promise that such changes will be precisely in the "next release after Go release". Let's change to a bit more flexible phrasing like "soon after Go release"

yurishkuro · 2023-08-13T17:12:10Z

scripts/check-go-version.sh

+go_mod_version=$(grep -oP "go\s+\K$version_regex+" $go_mod_path)
+
+# Ensure go.mod uses go_latest_version - 1
+if [ "${go_mod_version%.*}" != "${go_latest_version%.*}" ] || [ $((10#${go_latest_version#*.} - 10#${go_mod_version#*.})) != 1 ]; then


I don't think this is what we want. With this check the CI will start failing immediately after Go release, which is unfair to PR authors. My intention was that the script checks that all other build scripts are on some version N and the go.mod is on N-1, but N does not have to be latest. Latest can be printed, but not enforced. It should only be used in the update script.

yurishkuro · 2023-08-13T17:15:07Z

scripts/update-go-version.sh

@@ -0,0 +1,42 @@
+#!/bin/bash


This file has a large overlap with the check script, they need to be in sync regarding all the places they check. It would make sense to combine them into one and have an optional "update" behavior. Eg check script can take -u flag, and update script simply calls 'check -u'

- Moved all logic into ./scripts/check-go-version.sh with optional update flag - check-go-version.sh does not enforce version N to be latest now - Update script now calls ./scripts/check-go-version.sh -u - Updated README Signed-off-by: Harish Shan <[email protected]>

perhapsmaple · 2023-08-14T18:38:43Z

Hi @yurishkuro , I have updated the scripts according. I'm wondering if the update script is required, since it just calls the check-go-version script with a flag.

yurishkuro · 2023-08-14T20:34:39Z

README.md

+Starting with the release of Go 1.21, support for Go versions will be updated as follows:
+
+1. Soon after the release of a new Go minor version `N`, updates will be made to the build and tests steps to accommodate the latest Go minor version. 
+2. Soon after the release of a new Go minor version `N`, support for Go version `N-2` will be removed.


Suggested change

2. Soon after the release of a new Go minor version `N`, support for Go version `N-2` will be removed.

2. Soon after the release of a new Go minor version `N`, support for Go version `N-2` will be removed and version `N-1` will become the minimum required version.

yurishkuro · 2023-08-14T20:37:00Z

scripts/check-go-version.sh

+
+# If update, set go.mod version as latest version - 1 (N-1)
+if [ "$update" = true ]; then
+    go_mod_updated="${go_latest_version%.*}.$((10#${go_latest_version#*.} - 1))"


why double parentheses in the 2nd part?

go_mod_updated is odd name, perhaps go_mod_desired

yurishkuro · 2023-08-14T20:40:48Z

scripts/check-go-version.sh

+go_mod_path="./go.mod"
+go_mod_version=$(grep -oP "go\s+\K$version_regex+" $go_mod_path)
+
+# If update, set go.mod version as latest version - 1 (N-1)


could we add a parameter so that the version can be specified in the command line instead of always being inferred from the latest?

yurishkuro · 2023-08-14T20:42:00Z

scripts/check-go-version.sh

+    if [ "$update" = true ]; then
+        if [ "${version_string##*: }" != "$go_latest_version" ]; then
+            updated_version="${version_string%%:*}: ${go_latest_version}"
+            sed -i "s/$version_string/$updated_version/g" "$build_script"


how do you know this will not update some other part of the file? E.g. some GH actions are included via hash, which could in theory match the string you're substituting

yurishkuro · 2023-08-14T20:43:22Z

scripts/check-go-version.sh

+    if [ "${go_alpine_version##*:}" != "$go_latest_version" ]; then
+        sed -i "s/${go_alpine_version}/golang:${go_latest_version}/" "$alpine_makefile_path"
+        {
+            printf "Build script: %-60s | Previous Go Version: %s | Updated Go Version: %s\n" "$alpine_makefile_path" "${go_alpine_version##*:}" "${go_latest_version}"


can you make a function for this?

yurishkuro · 2023-08-14T20:44:05Z

scripts/check-go-version.sh

+fi
+
+# Ensure N-1 support policy
+if [ "$update" != true ]; then


this is not really making the code shorter, you still have completely independent code paths

yurishkuro · 2023-08-14T20:44:36Z

scripts/update-go-version.sh

@@ -0,0 +1,3 @@
+#!/bin/bash
+
+./scripts/check-go-version.sh -u


I'm fine to drop this script

codecov · 2023-09-04T16:34:53Z

Codecov Report

Patch has no changes to coverable lines.

❗ Current head b26467e differs from pull request most recent head 14f368d. Consider uploading reports for the commit 14f368d to get more accurate results

📢 Thoughts on this report? Let us know!.

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2023-09-04T22:58:29Z

This did not fully resolve the ticket because there is no linter check, but let's first start with manual updates.

perhapsmaple added 4 commits August 13, 2023 09:31

Add shell script to ensure N-1 support policy for Go versions

053d3e7

- Fixes #4653. Implements linter check for go versions. - Current files checked are all yml files in ./github/workflows/ and ./docker/Makefile. Signed-off-by: Harish Shan <[email protected]>

Add Go Version Compatibility section to README

bdd1c1b

Signed-off-by: Harish Shan <[email protected]>

Fix exit condition

3363b95

Signed-off-by: Harish Shan <[email protected]>

perhapsmaple requested a review from a team as a code owner August 13, 2023 16:16

perhapsmaple requested a review from albertteoh August 13, 2023 16:17

perhapsmaple changed the title ~~Check go version~~ Implement N-1 support policy for Go versions Aug 13, 2023

yurishkuro reviewed Aug 13, 2023

View reviewed changes

perhapsmaple added 2 commits August 14, 2023 14:33

Merge branch 'main' into check-go-version

8824400

perhapsmaple requested a review from yurishkuro August 14, 2023 18:40

yurishkuro reviewed Aug 14, 2023

View reviewed changes

Merge branch 'main' into check-go-version

b26467e

yurishkuro added 2 commits September 4, 2023 18:53

clean up

948d2bd

Signed-off-by: Yuri Shkuro <[email protected]>

fix

14f368d

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro approved these changes Sep 4, 2023

View reviewed changes

yurishkuro merged commit fb8eb7d into jaegertracing:main Sep 4, 2023
27 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement N-1 support policy for Go versions #4655

Implement N-1 support policy for Go versions #4655

perhapsmaple commented Aug 13, 2023

yurishkuro Aug 13, 2023

yurishkuro Aug 13, 2023

yurishkuro Aug 13, 2023

perhapsmaple commented Aug 14, 2023

yurishkuro Aug 14, 2023

yurishkuro Aug 14, 2023

yurishkuro Aug 14, 2023

yurishkuro Aug 14, 2023

yurishkuro Aug 14, 2023

yurishkuro Aug 14, 2023

yurishkuro Aug 14, 2023

yurishkuro Aug 14, 2023

codecov bot commented Sep 4, 2023 •

edited

Loading

yurishkuro commented Sep 4, 2023


		Starting with the release of Go 1.21, support for Go versions will be updated as follows:

		1. The first release after the release of a new Go minor version `N` will add build and tests steps for the new Go minor version.

	2. Soon after the release of a new Go minor version `N`, support for Go version `N-2` will be removed.
	2. Soon after the release of a new Go minor version `N`, support for Go version `N-2` will be removed and version `N-1` will become the minimum required version.

Implement N-1 support policy for Go versions #4655

Implement N-1 support policy for Go versions #4655

Conversation

perhapsmaple commented Aug 13, 2023

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perhapsmaple commented Aug 14, 2023

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

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

codecov bot commented Sep 4, 2023 • edited Loading

Codecov Report

yurishkuro commented Sep 4, 2023

codecov bot commented Sep 4, 2023 •

edited

Loading