Skip to content

[ENGINE-978] Change revision scheme per ENGINE-970 requirements#5

Open
psaintlaurent wants to merge 2 commits intoMirantis:masterfrom
psaintlaurent:ENGINE-978
Open

[ENGINE-978] Change revision scheme per ENGINE-970 requirements#5
psaintlaurent wants to merge 2 commits intoMirantis:masterfrom
psaintlaurent:ENGINE-978

Conversation

@psaintlaurent
Copy link

@psaintlaurent psaintlaurent commented Aug 28, 2024

This is a PR in order to change the versioning scheme to utilize the upstream buildx version along with the PRERELEASE and REVISIONS schemes described in ENGINE-970.

Signed-off-by: plaurent <patrick@saint-laurent.us>
tests/version.go Outdated
Comment on lines 48 to 49
regex, err := regexp.CompilePOSIX("^v[0-9]+.[0-9]+.[0-9]+(m[0-9]+|m[0-9]+-rc[0-9]+|m[0-9]+-tp[0-9]+)$")
require.True(t, err == nil && regex.MatchString(version), "Second field was not valid: %+v", version)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
regex, err := regexp.CompilePOSIX("^v[0-9]+.[0-9]+.[0-9]+(m[0-9]+|m[0-9]+-rc[0-9]+|m[0-9]+-tp[0-9]+)$")
require.True(t, err == nil && regex.MatchString(version), "Second field was not valid: %+v", version)
regex := regexp.MustCompilePOSIX("^v[0-9]+.[0-9]+.[0-9]+(m[0-9]+|m[0-9]+-rc[0-9]+|m[0-9]+-tp[0-9]+)$")
require.True(t, regex.MatchString(version), "Second field was not valid: %+v", version)

hack/build Outdated
Comment on lines 9 to 11
: "${PRERELEASE=$(./hack/git-meta version | rev | cut -c3- | rev | cut -d'-' -f 2 | grep -E '(^tp|^rc).*')}"
: "${REVISION_NUMBER=1}"
VERSION=$(printf "v%sm%s%s" "$(./hack/git-meta version | rev | cut -c3- | rev | cut -d'-' -f 1 | cut -c2-)" "${REVISION_NUMBER}" "-${PRERELEASE}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please explain what this is doing and why it is necessary? Why does the m-version need to be sourced from an environment variable? Why can't we just include the m-version part in our git tags?

Copy link
Author

@psaintlaurent psaintlaurent Aug 28, 2024

Choose a reason for hiding this comment

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

By m-version, I assume you mean the REVISION_NUMBER. I was under the impression that was going to be included from the pipeline going forward.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The package revision is going to be provided from the packaging pipeline going forward. I don't think it makes sense for the packaging pipeline to pass the software version into the build script piecemeal. @aepifanov's release process proposal -- which you reviewed with the rest of the team -- is for the packaging pipeline to take a Git tag name as a parameter and use the tag name as the version of the software being built. Since the tag is going to encode the full software version; revision, prerelease and all; there would be little value in passing the version components separately. The pipeline would have to parse the version string, only to have the build script re-concatenate it into an identical string.

Note that we will be creating multiple tags which reference the same commit, e.g. to rebuild an RC as a GA. That means we can't rely on the hack/git-meta version command. It looks for which tags reference the commit, but if more than one tag references the commit it might choose the wrong tag. This hack/build script already affords overriding the automatically inferred software version by setting the VERSION environment variable. Why not simply leverage that, and have our packaging script set VERSION to the git tag name when invoking the build?

Copy link
Author

Choose a reason for hiding this comment

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

The proposal doesn't have much detail regarding buildx.

Signed-off-by: plaurent <patrick@saint-laurent.us>
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.

2 participants