-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Release process enhancement #1185
base: main
Are you sure you want to change the base?
Conversation
Adjust the release document to suit the new release process. Signed-off-by: Jim Fitzpatrick <[email protected]>
From talks and calls, the release process has being moved to a more local process, less automated to allow for manual checks of the data before releasing. Signed-off-by: Jim Fitzpatrick <[email protected]>
- fix naming issue - refine process steps Signed-off-by: Jim Fitzpatrick <[email protected]>
The release script has being design to use a toml file for the version and is to be run locally. Signed-off-by: Jim Fitzpatrick <[email protected]>
Remove the reference to bundles from the format. It is suspected that the released version of the operator will never be different from the bundles. There is also an overlap with helm which does not have the concept of bundles. Signed-off-by: Jim Fitzpatrick <[email protected]>
The release workflow has being modified to only be triggered when a pull request is merged to branches that match `release-vX.Y`. The workflow will only make the GitHub release objects. Signed-off-by: Jim Fitzpatrick <[email protected]>
Set up a make target for verifying that the prepare-release target was ran, and there is no uncommitted changes. There has also being a workflow set up to run this check when a Pull Request is made against the release branches. Signed-off-by: Jim Fitzpatrick <[email protected]>
Slight change to the form that is being used. Signed-off-by: Jim Fitzpatrick <[email protected]>
[ci] Setting OLM channel depending on prerelease input [ci] Fixing yq cmd updating release.yaml [ci] Fixing branching, prepare release scripts and dependencies [ci] Creating CR release branch on `Create Pull Request` step * Running Create Pull Request only if not triggered by ACT * In order to set the desired channel and default channel * Changed consolePlugin default to `0.0.0` * Also descriptions of inputs to semver [ci] Updated sed command to also work with `+build` tags Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Refactor workflows to only use the verify-prepare-release target. This removed one of the workflows which was no longer required. Signed-off-by: Jim Fitzpatrick <[email protected]>
In the release.yaml 0.0.0 can now be used to mark a version should be the latest version. Signed-off-by: Jim Fitzpatrick <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1185 +/- ##
==========================================
+ Coverage 83.38% 83.46% +0.07%
==========================================
Files 82 82
Lines 7120 7117 -3
==========================================
+ Hits 5937 5940 +3
+ Misses 949 946 -3
+ Partials 234 231 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: dd di cesare <[email protected]>
* If it so, exit the release process Signed-off-by: dd di cesare <[email protected]>
* Until we have some scripts in that directory Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
* In order to avoid rate limiting and empty json Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
The workflow steps has no field needs Signed-off-by: Jim Fitzpatrick <[email protected]>
Set the commitish-target to the prerlease branch. The branch will be the in the following format: "release-vMAJOR.MINO" Signed-off-by: Jim Fitzpatrick <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
* In order to be able to trigger other workflows with push tag Signed-off-by: dd di cesare <[email protected]>
* With some modifications. verify-manfiests calls the targets verify bundle, controller-gen and helm Signed-off-by: dd di cesare <[email protected]>
Signed-off-by: dd di cesare <[email protected]>
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.
These changes all look good.
/lgtm
Let's 👁️ this. Please, wait for my review before merging it |
@@ -0,0 +1,14 @@ | |||
kuadrant-operator: |
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.
I wonder if this file should be named something like release.yaml.tmpl
or release.yaml.in
in the main
branch, and renamed/duplicated as release.yaml
only in the release branches.
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.
What would the advantage of doing that be?
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.
Mainly communication, to call the file for what it is. This file should only exist in release branches. The only meaning it has in main
is as a template.
Plus, we don't want people modifying and committing this file in main
. Some distraction and it may end up with some of values set something other than 0.0.0
, which is ugly, to say the least.
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.
I would not say it should only exist on release branches, there are possible advantages to how the nightly pipelines could make use of the file. None of that work has being done as of yet. There is also a hope to get make local-setup
to be able to consume that file, so it is not truly about releases.
|
||
log "RUNNING: $dir_name" | ||
|
||
tasks=($(find "$script_dir/$dir_name" -mindepth 1 -maxdepth 1 -perm -001 | sort)) |
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.
Should the names of the task files be prefixed with an ordering number (e.g.: 1-verify_dependencies.sh
, 2-verify_release_file.sh
, ...)?
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.
This is something that can be done if there is a required order that the scripts most proceed as. For now there is no hard requirement on the ordering. While it is not requirement lets not introduce a number schema to maintain.
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.
Why sort
then? Deterministic output?
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 question. Initial the order did matter, and there was numbering prefixes. This is something could be removed, but I currently don't see the harm in having it there. The deterministic output is a nice side affect. I don't think there is a need for a change here. More of an understanding of what is happening.
verify-bundle: | ||
name: Verify bundle | ||
if: needs.pre-job.outputs.should_skip != 'true' | ||
verify-manifests: |
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.
I think it is better to have N verify jobs. Two reasons:
- the UX is better in Github. You know very quickly what failed
- Each tests is run in isolation: one test cannot interfere the other test. For instance,
make bundle
could be changing files from helm charts and the helm charts test could be failing due to bundle test. Test isolation is always a desired property.
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.
I am not sure I fully agree with the each test running in isolation. There is a strong interaction between the bundles and helm charts when it comes to releasing. Would see this test more of an integration test over a unit test. When the make prepare-release
would resolve both issue I don't see the need to have them split.
You might know very quickly what failed, but does that tell you as quickly how to fix it. I also think the make target in the test should not be used, but instead use make verify-prepare-release
.
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.
The source of truth is in golang code and /config
plus few other places like Makefile. Those are verification steps to make sure the source of truth is consistent with the shipped manifests (bundle or helm charts). So, IMO, independent and isolated tests are good to have. I can live without it as well.
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.
In the future, the Makefile will no longer be a source of truth.. it will be replaced by release.yaml. Because now there are two ways to generate the manifests and eventually we need to merge into 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.
Agreed eventually it needs to merge into one. The next step on the road I see is how the nightly test pipeline generate their builds.
dns_version=$(mod_version $(yq '.dependencies.dns-operator' $env/release.yaml)) | ||
limitador_version=$(mod_version $(yq '.dependencies.limitador-operator' $env/release.yaml)) | ||
|
||
AUTHORINO_OPERATOR_GITREF=$authorino_version envsubst < $env/config/dependencies/authorino/kustomization.template.yaml > $env/config/dependencies/authorino/kustomization.yaml |
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.
I see env
is being set at utils/release/release.sh:49
. Should we add a default value for it to the project dir at the beginning of each task script file?
I'm afraid one who inadvertently tries to run one of these script files locally, individually, i.e. without the initialisation by release.sh
, could accidentally target /config/…
.
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.
I would not be in favor of setting a default value. However I can see your point about accidentally targetting /config/...
, would a early error termination of the script solve the issue in a satisfactory manner?
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.
would a early error termination of the script solve the issue in a satisfactory manner?
I don't think it's enough. /config
was just an example. This is a problem that affects all scripts, not just this one. Imagine if it was referring to /bin
.
I think we can come up with plenty of other examples where this can go wrong, even if it doesn't fail with an error.
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 then I am hear that for now the best step forward is to document the use of the release.sh
.
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.
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.
I think the best step is to set a default.
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.
For now this is a low risk issue. We can iterate on the problem. The scope of the PR has already blow out of scale. The issue #1219 is the follow up issue for this. I think that is where we should describe the change what wanted to be seen in an iteration.
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Makefile: Release section
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.
Looking good
filling the following fields: | ||
- gitRef: Select the branch/tag/commit where you want to cut a release from. | ||
- kuadrantOperatorVersion: the [Semantic Version](https://semver.org/) of the desired release. | ||
- authorinoOperatorVersion: Authorino Operator bundle version (X.Y.Z) |
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.
- authorinoOperatorVersion: Authorino Operator bundle version (X.Y.Z) | |
- authorinoOperatorVersion: Authorino Operator version (X.Y.Z) |
- gitRef: Select the branch/tag/commit where you want to cut a release from. | ||
- kuadrantOperatorVersion: the [Semantic Version](https://semver.org/) of the desired release. | ||
- authorinoOperatorVersion: Authorino Operator bundle version (X.Y.Z) | ||
- limitadorOperatorVersion: Limitador Operator bundle version (X.Y.Z) |
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.
- limitadorOperatorVersion: Limitador Operator bundle version (X.Y.Z) | |
- limitadorOperatorVersion: Limitador Operator version (X.Y.Z) |
- kuadrantOperatorVersion: the [Semantic Version](https://semver.org/) of the desired release. | ||
- authorinoOperatorVersion: Authorino Operator bundle version (X.Y.Z) | ||
- limitadorOperatorVersion: Limitador Operator bundle version (X.Y.Z) | ||
- dnsOperatorVersion: DNS Operator bundle version (X.Y.Z) |
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.
- dnsOperatorVersion: DNS Operator bundle version (X.Y.Z) | |
- dnsOperatorVersion: DNS Operator version (X.Y.Z) |
- dnsOperatorVersion: DNS Operator bundle version (X.Y.Z) | ||
- wasmShimVersion: WASM Shim version (X.Y.Z) | ||
- consolePluginVersion: ConsolePlugin version (X.Y.Z) | ||
- olmChannel: This will set the OLM `channels` and `default-channel` annotations |
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 enough for now.
While channel is mandatory setting for a bundle, default channel is not. The default values if not set on the workflow form should be:
channel
:stable
(or whatever is in the workspace)- default-channel: not set.
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 thing is that we can modify that on the actual PR, so there's nothing committed
utils/release/load_github_envvar.sh
Outdated
|
||
releaseBody="**This release enables installations of Authorino Operator v$authorinoOperatorVersion, Limitador Operator v$limitadorOperatorVersion, DNS Operator v$dnsOperatorVersion, WASM Shim v$wasmShimVersion and ConsolePlugin $consolePluginURL**" | ||
|
||
kuadratantOperatorTag="v$(yq '.kuadrant-operator.version' $ROOT/release.yaml)" |
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.
kuadratantOperatorTag="v$(yq '.kuadrant-operator.version' $ROOT/release.yaml)" | |
kuadrantOperatorTag="v$(yq '.kuadrant-operator.version' $ROOT/release.yaml)" |
1. Create the `release-vX.Y` branch, if the branch does not already exist. | ||
2. Push the `release-vX.Y` to the remote (kuadrant/kuadrant-operator) | ||
3. Create the `release-vX.Y.Z-rc(n)` branch with `release-vX.Y` as the base. | ||
4. Cherry-pick commits to the `kudrant-vX.Y.Z-rc(n)` from the relevant sources, i.e. `main`. | ||
5. Update the applicable version in the [release.yaml](https://github.com/Kuadrant/kuadrant-operator/blob/main/RELEASE.md#release-file-format). |
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.
Why merge -rc
branches into release-vX.Y
ones instead of just building directly from release-vX.Y.Z
named branches? Patch releases checkpoints will be saved inside x.y.z branches, and you won't need to do some extra steps and checks during this release process.
But I guess you'll need to sync minor release branch after each patch release instead. Or if you don't care at all about saving patch releases? This flow makes more sense to me though
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.
Image building is broken.
The image building system is implemented in .github/workflows/build-images-for-tag-release.yaml
. This workflow still assumes the existence of release.mk
file, which no longer exists. So Image build fails. One example run failing: https://github.com/Kuadrant/kuadrant-operator/actions/runs/13954148232/job/39060935703
I (tried to) fix it partially in #1223. But the catalog and bundle build is very likely to fail as well.
I can open another PR with the fix for the bundle and catalog builds or leave someone else to do this.
Add note on release note generation failure. Signed-off-by: Jim Fitzpatrick <[email protected]>
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
prepare-release: make default-channel optional
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
workflows/build-images-for-tag-release.yaml: fix operator image build
Closes #1122
This PR introduces a new process for releasing the Kuadrant operator. It replaces the poorly documented manual way and the error prone automated workflow, by a new automated workflow and a well documented manual one. Both of the processes work around a new manifest named
release.yaml
, which is the source of truth for our kustomize configs, bundles and helm charts.release.yaml file format.
This example of the
release.yaml
file uses tagv1.0.1
as reference.The
kuadrant-operator
section relates to the release version of the kuadrant operator.While the
olm
section relates to fields required for building the olm catalogs.And the
dependencies
section relates to the released versions of the subcomponents that will be included in a release.There are validation steps during the
make prepare-release
that require the dependencies to be release before generating the release of the Kuadrant operator.How to release Kuadrant Operator
To release a version “vX.Y.Z” of Kuadrant Operator in GitHub and Quay.io, there are two options, a manual and an automated process.
For both processes, first make sure every Kuadrant Operator dependency has been already released.
Automated Workflow
filling the following fields:
channels
anddefault-channel
annotationsit will also build the images and packages and publish them on Quay, Helm repository.
Notes
Manual Workflow
Local steps
release-vX.Y
branch, if the branch does not already exist.release-vX.Y
to the remote (kuadrant/kuadrant-operator)release-vX.Y.Z-rc(n)
branch withrelease-vX.Y
as the base.kudrant-vX.Y.Z-rc(n)
from the relevant sources, i.e.main
.make prepare-release
on therelease-vX.Y.Z-rc(n)
. If you run into rate limit errors, set the envGITHUB_TOKEN
with your PAT.Remote steps
release-vX.Y
branch with the changes fromrelease-vX.Y.Z-rc(n)
branch../bundle.Dockerfile
./bundle
./config
./charts/
release-vX.Y
. This does the following:TODO
needs
directive from steps