ROX-22250: Update version string in builds for fast stream #11187

kylape · 2024-05-21T02:11:52Z

Description

When calculating the output of make tag, pipe the output of the git describe command through a new script called tag.py, which will make the following two modifications:

Change the .x- to .0-
Bump the minor release by one

These changes are intended to support the versioning strategy Fast Stream Releases.

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps
Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

Here I tell how I validated my change

TODO(replace-me)
Use this space to explain how you validated that your change functions exactly how you expect it.
Feel free to attach JSON snippets, curl commands, screenshots, etc. Apply a simple benchmark: would the information you
provided convince any reviewer or any external reader that you did enough to validate your change.

It is acceptable to assume trust and keep this section light, e.g. as a bullet-point list.

It is acceptable to skip testing in cases when CI is sufficient, or it's a markdown or code comment change only.
It is also acceptable to skip testing for changes that are too taxing to test before merging. In such case you are
responsible for the change after it gets merged which includes reverting, fixing, etc. Make sure you validate the change
ASAP after it gets merged or explain in PR when the validation will be performed.
Explain here why you skipped testing in case you did so.

Have you created automated tests for your change? Explain here which validation activities you did manually and why so.

Reminder for reviewers

In addition to reviewing code here, reviewers must also review testing and request further testing in case the
performed one does not seem sufficient. As a reviewer, you must not approve the change until you understand the
performed testing and you are satisfied with it.

rhacs-bot · 2024-05-21T02:40:59Z

Images are ready for the commit at ef042ef.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.4.0-817-gef042ef321.

tools/tag.py

tommartensen · 2024-05-21T09:26:23Z

tools/tag.py

+
+if __name__ == "__main__":
+    v = Version.fromstring(orig)
+    v.minor = str(int(v.minor) + 1)


This feels wrong, considering we still have the a.b.x GitHub tag, it will produce a.(b+1).0-....

I am asking myself, can we bump the a.b.x to the next minor release?

https://github.com/stackrox/stackrox/blob/master/.github/workflows/start-release.yml#L157-L162

I'm not sure I follow the concern. Would this (tag.py) script be used to create the version string for build on the release branches as well? Based on a quick look it seems like BUILD_TAG is set in downstream builds, thus overriding what this script would produce.

I'm with Tom, the release automation scripting should place A.(B+1).x tag instead of A.B.x (which it currently does) on master when release-A.B branch is cut.

Would this (tag.py) script be used to create the version string for build on the release branches as well?

It would be used for all commits, except tagged ones.

There is one slight caveat with doing the A.(B+1).x approach. We tag the commit where we branch off the release branch. Any commit on the release branch before the first release candidate on the .0 release (which is the A.B.0-rc.1 tag), would have A.(B+1).x-n-g123456. This might be ignored, because those commits are not built - they are updating the CHANGELOG, cherry-picks for the first RC and pushed with an empty commit. See https://github.com/stackrox/stackrox/commits/release-4.4/?before=53dd371292a8277e1049815c4f408d04a041f317+105 - 7fc7385 is pushed, preceding commits until 1f75c80 are not built.

A "solution" would be to add an empty commit with the tag to the master branch after the branching point - that leaves the question how to tag if the branching point is not the latest commit on master - the branching may be done couple of hours/days after the code freeze at a specific commit.

Is there a reason not to tag the very first commit on the release-A.B branch with A.B.0-rc.0? Also, the very first commit could be an empty placeholder so that there's always something.

Is there a reason not to tag the very first commit on the release-A.B branch with A.B.0-rc.0? Also, the very first commit could be an empty placeholder so that there's always something.

Not really. Tagged commits have a few additional checks, like confirming that COLLECTOR_VERSION and SCANNER_VERSION are SemVer, but failure here may be informative, for the release engineers - if they look at the status of this tag at all.

codecov · 2024-05-21T17:04:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.99%. Comparing base (9181396) to head (acef514).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #11187   +/-   ##
=======================================
  Coverage   47.98%   47.99%           
=======================================
  Files        2330     2330           
  Lines      166754   166761    +7     
=======================================
+ Hits        80013    80030   +17     
+ Misses      80388    80379    -9     
+ Partials     6353     6352    -1

Flag	Coverage Δ
go-unit-tests	`47.99% <ø> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Makefile

msugakov · 2024-05-22T15:08:51Z

tools/tag.py

+
+if __name__ == "__main__":
+    v = Version.fromstring(orig)
+    v.minor = str(int(v.minor) + 1)


I'm with Tom, the release automation scripting should place A.(B+1).x tag instead of A.B.x (which it currently does) on master when release-A.B branch is cut.

Creating a minor release branch (let's call the new minor release version "y") should go like this: * Create a new release branch based on selected commit in master (no change here) * Push an empty commit to the release branch * Tag that empty commit on the release branch with "4.y.0-rc.0" * Push an empty commit to the tip of master (it is ok if this empty commit does not directly follow the branching commit) * Tag that empty commit on master with "4.<y+1>.x", e.g. if the minor version is "4", the tag would be "4.5.x". The last change is to update the output of "make tag" to replace the ".x-" with ".0-" using the sed command provided by Misha in the PR.

msugakov · 2024-05-30T10:49:01Z

OSCI jobs have failed, e.g. here https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/stackrox_stackrox/11187/pull-ci-stackrox-stackrox-master-gke-nongroovy-e2e-tests/1796024033389580288#1:build-log.txt%3A605

because images were not built. For some reason, I see build-and-push-main as skipped https://github.com/stackrox/stackrox/actions/runs/9296116570/job/25584351007?pr=11187

It is not unexpected that the tag change .x -> .0 may have cascading effects. We need to make sure everything will be fine.

Update: Looks like, this time it's just a GitHub thing and not caused by this change.

I triggered a re-run in GitHub.

msugakov · 2024-05-30T10:51:26Z

/retest

msugakov · 2024-05-30T10:59:25Z

.github/workflows/variables.yml

+          major, minor = tuple(int(i) for i in m.group('release').split('.'))
+          next_release = f'{major}.{minor+1}'


I suppose you can try redefine RELEASE=r'(?P<release>\d+\.\d+)' to be nested:

RELEASE=r'(?P<release>(?P<major>\d+)\.(?P<minor>\d+))'

Then, this will turn into:

Suggested change

major, minor = tuple(int(i) for i in m.group('release').split('.'))

next_release = f'{major}.{minor+1}'

major, minor = int(m.group('major')), int(m.group('minor'))

next_release = f'{major}.{minor+1}'

I've addressed this in https://github.com/stackrox/test-gh-actions/pull/158

msugakov · 2024-05-30T11:02:32Z

.github/workflows/variables.yml

+      next-release:
+        description: Next Release (A.`B+1`)
+        value: ${{jobs.parse.outputs.next-release}}
      next-release-patch:
        description: Next Release.patch (A.B.`C+1`)
        value: ${{format('{0}.{1}', jobs.parse.outputs.release, jobs.parse.outputs.next-patch)}}


[nit] Output names next-release, next-release-patch and next-named-release-patch don't stack against each other any more because the new next-release means a different thing now.

One option that comes to mind is to rename
next-release-patch -> next-patch
and next-named-release-patch -> next-named-patch.
This way, they don't refer to next-release.

What do you think about this:

next-release -> next-minor-release
next-release-patch -> next-patch-release
next-named-release-patch -> next-named-patch-release

Looks ok.
I feel tiny bit uneasy about minor because it alludes to a small amount of changes, but the alternative next-y-stream-release looks a bit ugly. Therefore, what you suggest is actually ok.

I've addressed this in https://github.com/stackrox/test-gh-actions/pull/158

msugakov · 2024-05-30T11:08:16Z

.github/workflows/start-release.yml

+          git commit --allow-empty --message \
+            "Initial commit to release branch ${{needs.variables.outputs.release}}"


In good StackRox tradition, I suggest to tweak the message to:

Suggested change

git commit --allow-empty --message \

"Initial commit to release branch ${{needs.variables.outputs.release}}"

git commit --allow-empty --message \

"Empty commit to diverge ${{needs.variables.outputs.release}} from master"

There's nothing wrong about the current message, I just somehow feel attached to the old one :-)

I've addressed this in https://github.com/stackrox/test-gh-actions/pull/158

msugakov · 2024-05-30T11:25:23Z

.github/workflows/start-release.yml

@@ -174,10 +170,21 @@ jobs:
            git commit --message "Changelog for ${{inputs.version}}"
            echo "\`CHANGELOG.md\` has been updated on the release branch." >> "$GITHUB_STEP_SUMMARY"
          fi
+      - name: Tag master for next release
+        if: steps.check-existing.outputs.branch-exists == 'false'


[no-op] I wouldn't say that the condition is the perfect guard here. If making the workflow idempotent, I'd suggest to move the check inside the script and check for presence of a ${{needs.variables.outputs.next-release}} tag. Similarly, other steps (e.g. Create and annotate branch ${{needs.variables.outputs.branch}}) can check for presence of a thing (x.y.0-rc.0 tag) before adding it.

However, the current step is consistent with other steps so I don't insist on changing that.

msugakov · 2024-05-30T11:25:57Z

.github/workflows/start-release.yml

+          git commit --allow-empty --message \
+            "Bumping release tag to ${{needs.variables.outputs.next-release}}"
+          git tag --annotate --message "Upstream automation" \
+            "${{needs.variables.outputs.next-release}}" HEAD


This must be

Suggested change

"${{needs.variables.outputs.next-release}}" HEAD

"${{needs.variables.outputs.next-release}}.x" HEAD

I've addressed this in https://github.com/stackrox/test-gh-actions/pull/158

msugakov · 2024-05-30T11:28:30Z

.github/workflows/start-release.yml

+          git commit --allow-empty --message \
+            "Bumping release tag to ${{needs.variables.outputs.next-release}}"


[nitpicking, but] I'd love to suggest a more festive message here but currently feel my muse is away. How about something like "Work on the release ${{needs.variables.outputs.next-release}} begins from here"?

Also, it would be nice if you add a comment saying that there could be several commits between the starting point of the release branch and this new commit on master tagged with the next-release tag. This is ok because if we'd put the next-release tag we would change make tag output for those existing commits which is like overwriting history and is worse (than having these few tagged with the previous release tag).

I've addressed this in https://github.com/stackrox/test-gh-actions/pull/158

tommartensen

@kylape Please consider https://github.com/stackrox/test-gh-actions as the place to develop and test these changes. It has all release related workflows, some tags, no make tag (yet!)

You'll also be missing builds and E2E tests, but for getting the workflow correct, it might be better to start there.

msugakov · 2024-05-31T07:54:40Z

Makefile

@@ -27,7 +27,7 @@ SILENT ?= @
 UNIT_TEST_IGNORE := "stackrox/rox/sensor/tests|stackrox/rox/operator/tests|stackrox/rox/central/reports/config/store/postgres|stackrox/rox/central/complianceoperator/v2/scanconfigurations/store/postgres|stackrox/rox/central/auth/store/postgres|stackrox/rox/scanner/e2etests"

 ifeq ($(TAG),)
-TAG=$(shell git describe --tags --abbrev=10 --dirty --long --exclude '*-nightly-*')$(MAIN_TAG_SUFFIX)


One thing I'd also ask is to adjust operator's Makefile which does not need to handle cases like 4.5.x and 3.0.62.1.

stackrox/operator/Makefile

Lines 6 to 8 in a8df2c2

# The version reported by the root Makefile is converted to be compatible with SemVer. Specifically, inner-zero is

# dropped (e.g. 3.0.61.1 -> 3.61.1) and development version ".x" is changed to ".0" (e.g. 3.0.61.x-123 -> 3.0.61.0-123).

VERSION ?= $(shell $(MAKE) --quiet --no-print-directory -C .. tag | sed -E 's@^(([[:digit:]]+\.)+)x(-)?@\10\3@g' | sed -E 's@^3.0.([[:digit:]]+\.[[:digit:]]+)(-)?@3.\1\2@g')

It can simply become

VERSION ?= $(shell $(MAKE) --quiet --no-print-directory -C .. tag)

since the Makfile in repo root now does handling of .x and versions like 3.0.y.z are long gone.

kylape · 2024-05-31T14:10:55Z

/retest

Revert "Add next-release output" This reverts commit acef514. Revert "Fix quoting issue" This reverts commit ad6630c. Revert "Update release script for fast stream versioning" This reverts commit 76a0705.

No longer needed because main makefile now does handling of `.x` and versions like 3.0.y.z are long gone.

kylape · 2024-05-31T17:03:40Z

Github workflow changes have been moved to https://github.com/stackrox/test-gh-actions/pull/158.

openshift-ci · 2024-05-31T18:52:28Z

@kylape: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ocp-4-12-operator-e2e-tests	`ef042ef`	link	false	`/test ocp-4-12-operator-e2e-tests`
ci/prow/gke-nongroovy-e2e-tests	`ef042ef`	link	false	`/test gke-nongroovy-e2e-tests`
ci/prow/ocp-4-12-qa-e2e-tests	`ef042ef`	link	false	`/test ocp-4-12-qa-e2e-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

kylape requested a review from a team May 21, 2024 02:13

tommartensen reviewed May 21, 2024

View reviewed changes

tools/tag.py Outdated Show resolved Hide resolved

tommartensen reviewed May 21, 2024

View reviewed changes

msugakov reviewed May 22, 2024

View reviewed changes

kylape added 4 commits May 29, 2024 17:37

ROX-22250: Update version string in builds for fast stream

d58a9d6

Change 'release' to 'build'

7d764a4

Remove tag.py and only change x->0 using sed

5672699

kylape force-pushed the klape/rox-22250 branch from 54dc0d5 to 76a0705 Compare May 30, 2024 02:26

github-actions bot added the area/ci label May 30, 2024

kylape added 2 commits May 29, 2024 21:31

Fix quoting issue

ad6630c

Add next-release output

acef514

msugakov reviewed May 30, 2024

View reviewed changes

msugakov added the ci-build-race-condition-debug Build a `-race` image tagged with `-rcd`. Required for `/test gke-race-condition-qa-e2e-tests`. label May 30, 2024

tommartensen reviewed May 30, 2024

View reviewed changes

msugakov reviewed May 31, 2024

View reviewed changes

kylape added 2 commits May 31, 2024 11:59

Revert commits that modify github release workflows

7422f79

Revert "Add next-release output" This reverts commit acef514. Revert "Fix quoting issue" This reverts commit ad6630c. Revert "Update release script for fast stream versioning" This reverts commit 76a0705.

Remove sed command from operator's version string calculation

ef042ef

No longer needed because main makefile now does handling of `.x` and versions like 3.0.y.z are long gone.

kylape requested a review from a team as a code owner May 31, 2024 17:02

github-actions bot added the area/operator label May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROX-22250: Update version string in builds for fast stream #11187

ROX-22250: Update version string in builds for fast stream #11187

kylape commented May 21, 2024

rhacs-bot commented May 21, 2024 •

edited

tommartensen May 21, 2024

kylape May 21, 2024

msugakov May 22, 2024

tommartensen May 22, 2024

msugakov May 22, 2024

tommartensen May 23, 2024

codecov bot commented May 21, 2024 •

edited

msugakov May 22, 2024

msugakov commented May 30, 2024 •

edited

msugakov commented May 30, 2024

msugakov May 30, 2024

kylape May 31, 2024

msugakov May 30, 2024

kylape May 30, 2024

msugakov May 31, 2024

kylape May 31, 2024

msugakov May 30, 2024 •

edited

kylape May 31, 2024

msugakov May 30, 2024

msugakov May 30, 2024

kylape May 31, 2024

msugakov May 30, 2024

msugakov May 30, 2024

kylape May 31, 2024

tommartensen left a comment

msugakov May 31, 2024

kylape commented May 31, 2024

kylape commented May 31, 2024

openshift-ci bot commented May 31, 2024

		major, minor = tuple(int(i) for i in m.group('release').split('.'))
		next_release = f'{major}.{minor+1}'

		git commit --allow-empty --message \
		"Initial commit to release branch ${{needs.variables.outputs.release}}"

	"${{needs.variables.outputs.next-release}}" HEAD
	"${{needs.variables.outputs.next-release}}.x" HEAD

		git commit --allow-empty --message \
		"Bumping release tag to ${{needs.variables.outputs.next-release}}"

	# The version reported by the root Makefile is converted to be compatible with SemVer. Specifically, inner-zero is
	# dropped (e.g. 3.0.61.1 -> 3.61.1) and development version ".x" is changed to ".0" (e.g. 3.0.61.x-123 -> 3.0.61.0-123).
	VERSION ?= $(shell $(MAKE) --quiet --no-print-directory -C .. tag \| sed -E 's@^(([[:digit:]]+\.)+)x(-)?@\10\3@g' \| sed -E 's@^3.0.([[:digit:]]+\.[[:digit:]]+)(-)?@3.\1\2@g')

ROX-22250: Update version string in builds for fast stream #11187

Are you sure you want to change the base?

ROX-22250: Update version string in builds for fast stream #11187

Conversation

kylape commented May 21, 2024

Description

Checklist

Testing Performed

Here I tell how I validated my change

Reminder for reviewers

rhacs-bot commented May 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 21, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

msugakov commented May 30, 2024 • edited

msugakov commented May 30, 2024

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

msugakov May 30, 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

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

tommartensen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylape commented May 31, 2024

kylape commented May 31, 2024

openshift-ci bot commented May 31, 2024

rhacs-bot commented May 21, 2024 •

edited

codecov bot commented May 21, 2024 •

edited

msugakov commented May 30, 2024 •

edited

msugakov May 30, 2024 •

edited