This repository has been archived by the owner on Aug 19, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @coreydaley ! But now the tags for the PR images sound a bit confusing to me :/
Also, I think that with the changes here, once this PR is merged, the
next-container-build
workflow would end up creating images like this:quay.io/janus-idp/operator:0.0.1-latest-latest-$sha
orquay.io/janus-idp/operator:0.0.1-latest-next-$sha
, which will make it more confusing to me.Maybe we should just build and push the tag for the base version (
0,0,1
, which would be considered thelatest
) besides the other tags? So I'd suggest keeping the base version as it is, but changing https://github.com/janus-idp/operator/blob/main/.github/workflows/next-container-build.yaml#L88-L114 to push the tag for the base version. WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point
I think we should not have "0.0.1" version yet as it is not released yet but we probably need to have "latest" aliased to the last PR's image.
I.e. something like this (for this example):
operator:pr-200-ae0f223 (having 0.0.1 prefix looks redundant to me)
and
operator:latest - pointed to the same image (latest for the time)
And when we release 0.0.1
operator:0.0.1 (plus we may have some alias like "stable" for the latest released)
WDYT?
CC @nickboldt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC,
make release-build
does require a semantic version, so withMajor.Minor.Patch
elements:I think that's why we added the additional labels and build metadata to the version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rm3l I see, I missed this point.
So we can keep it as it is (0.0.1-pr-200-ae0f223) for PR release + tag "latest" for the latest one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gazarenkov we can not simply use VERSION := latest as otherwise we have similar semantic version error:
"Error: invalid command options: latest is not a valid semantic version: No Major.Minor.Patch elements found"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So after discussing this, we think it would make sense to:
VERSION
in the Makefile formain
and1.1.x
branches.next-container-build
workflow to also push the tag corresponding to thisVERSION
fieldThis way, this PR won't be needed, and
make deploy
would work out of the box.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rm3l This sounds good to me. Further considerations for discussion:
a) In the master branch, set VERSION ?= 1.1.0-dev (In this way the images from master and release branches will never have conflict).
b) In the release branch 1.1.x, set VERSION ?= 1.1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jianrongzhang89 Following the discussion yesterday about versioning of the CR, I'm wondering if we should not still keep
0
as major here as well (to make it clear that it is not stable version yet - (only RHDH version is 1.x.y))?So:
0.0.1
as it is in the release branch1.1.x
0.0.1-dev
onmain
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rm3l it makes sense to use major version 0, but I think we should use a minor version instead of a patch version:
0.1.0 as it is in the release branch (and we should rename the branch to 0.1.x to match them),
but 0.1.0-dev on main.
Releases with bugfixes can be released as 0.1.1 etc, releases with new features can be 0.2.0 etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. But I would keep the
1.1.x
release branch name as it is for now, as I'm not sure about the implications on the downstream build pipelines..