Fix verifications #1197

didierofrivia · 2025-03-10T19:44:53Z

This PR re introduces the make targets that were asserting the validity of manifests such as bundle, controller-gen and helm-charts in order to be used in the main verify workflow. Asserting the healthiness of manifests on every PR to main or triggered manually with workflow_dispatch. A second workflow that will be triggered for push or PR to a release branch, will be running the battery of scripts for asserting the validity of a release. There's also a refactor the the original test workflow, that will be running the tests and code validations for every PR and push to release branches and main. The reason behind is that for releases we run the prepare-release target, which executes the battery of scripts for preparing the manifests for a release, that involves GH API calls to its dependencies and other assertions that are not needed for a non release PR.

So, it looks like this:

Verify Release:
- Push to release branch
- PR to release branch
Verify Main
- Push to main branch
- PR to main branch
Tests:
- Every PR
- Push to either release or main branches

codecov · 2025-03-10T19:47:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (release-process-enhancement@d985f86). Learn more about missing BASE report.

Additional details and impacted files

@@                      Coverage Diff                       @@
##             release-process-enhancement    #1197   +/-   ##
==============================================================
  Coverage                               ?   83.58%           
==============================================================
  Files                                  ?       82           
  Lines                                  ?     7117           
  Branches                               ?        0           
==============================================================
  Hits                                   ?     5949           
  Misses                                 ?      939           
  Partials                               ?      229

Flag	Coverage Δ
bare-k8s-integration	`23.71% <ø> (?)`
controllers-integration	`74.25% <ø> (?)`
envoygateway-integration	`41.05% <ø> (?)`
gatewayapi-integration	`20.06% <ø> (?)`
istio-integration	`43.95% <ø> (?)`
unit	`19.08% <ø> (?)`

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

Components	Coverage Δ
api/v1beta1 (u)	`90.78% <0.00%> (?)`
api/v1beta2 (u)	`∅ <0.00%> (?)`
pkg/common (u)	`∅ <0.00%> (?)`
pkg/istio (u)	`62.06% <0.00%> (?)`
pkg/log (u)	`93.18% <0.00%> (?)`
pkg/reconcilers (u)	`24.50% <0.00%> (?)`
pkg/rlptools (u)	`∅ <0.00%> (?)`
controllers (i)	`86.80% <0.00%> (?)`

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

* 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]>

branches * Instead of calling it from verification workflows Signed-off-by: dd di cesare <[email protected]>

Boomatang

A part from the question I am asking in the other comments there is one question that I have that does not fit neatly to any line change. Why is there two more workflows being added? They don't seem to give any value that the test.yaml can not give. They are increasing the stuff that would need to be maintained. So are they really needed?

Boomatang · 2025-03-14T14:27:11Z

.github/workflows/test.yaml

-  merge_group:
-    types: [ checks_requested ]


Why the removal of this?

Boomatang · 2025-03-14T14:34:54Z

make/verify.mk

+.PHONY: verify-manifests ## Verify controller-gen, bundle and helm charts manifests.
+verify-manifests: ## Verify manifests update.
+	make verify-controller-manifests
+	make verify-bundle
+	make verify-helm-charts


Why is this being done this way?

Suggested change

.PHONY: verify-manifests ## Verify controller-gen, bundle and helm charts manifests.

verify-manifests: ## Verify manifests update.

make verify-controller-manifests

make verify-bundle

make verify-helm-charts

.PHONY: verify-manifests ## Verify controller-gen, bundle and helm charts manifests.

verify-manifests: ## Verify manifests update.

make manifests

make bundle

make helm-charts

git diff --exit-code .

But I would also look at my other comments

Boomatang · 2025-03-14T15:06:08Z

make/verify.mk

+.PHONY: verify-controller-manifests
+verify-controller-manifests: manifests ## Verify controller-gen manifests update.
+	git diff --exit-code ./config
+	[ -z "$$(git ls-files --other --exclude-standard --directory --no-empty-directory ./config)" ]
+
+.PHONY: verify-bundle
+verify-bundle: bundle ## Verify bundle update.
+	git diff --exit-code ./bundle
+	[ -z "$$(git ls-files --other --exclude-standard --directory --no-empty-directory ./bundle)" ]
+
+.PHONY: verify-helm-charts
+verify-helm-charts: helm-build ## Verify helm charts update.
+	git diff --exit-code ./charts
+	[ -z "$$(git ls-files --other --exclude-standard --directory --no-empty-directory ./charts)" ]


There is a number of things here that we need to discussed.
The two main things are:

The use of the bundle make target was only left in the codebase to not break the nightly pipelines. And the next action should be getting the nightly pipelines to support the release.yaml file. Which could mean the removal of these base make targets completely.

The second thing that is interesting is around the developer experiences within the project. On a number of occasions now I have being asked why is there so many make targets within the project. They say it is hard to know what should be done, what order to do things in, where to even start. These comments came from people within the project, but also external to the project. They are the more interesting ones.

So I am going to ask. Is there a different way that this can be done that reduces the number of make targets, and stream lines the experiences of others.

Boomatang · 2025-03-14T15:08:55Z

.github/workflows/test.yaml

-  verify-release:
-    name: Validate release data 
-    if: needs.pre-job.outputs.should_skip != 'true'
-    needs: pre-job
-    runs-on: ubuntu-latest
-    steps:
-      - name: Set up Go 1.22.x
-        uses: actions/setup-go@v4
-        with:
-          go-version: 1.22.x
-        id: go
-      - name: Checkout code at git ref
-        uses: actions/checkout@v4
-      - name: Check bundles and manifests
-        run: make verify-prepare-release


Why remove this from all PRs?

Boomatang · 2025-03-14T15:26:26Z

.github/workflows/verify-main.yaml

+      - name: Checkout code at git ref
+        uses: actions/checkout@v4
+      - name: Check bundles and manifests
+        run: make verify-manifests


Why use this make target over the verify-prepare-release? If the verification calls to github checking the existing of versions seems to expensive I would argue that is not a good enough reason. There are will be external call to quay by opm render when the bundle make target is called.

There is also the feature of the scripts used in the prepare-release make target to grep and only run a given script. If the cost of a check was that high problematic (thinking getting rate limited), it would make more sense to add the feature to filter out scripts over trying to maintain two versions. Then how I would see that being used would be something like make verify-prepare-release --exclude script-a script-b. Stating the list of scripts that would be exclude also makes it easier for someone in 6 months, a year to understand what is and is not happening.

didierofrivia force-pushed the fix-verifications branch from 6ad8c1e to d5f630f Compare March 11, 2025 09:48

didierofrivia marked this pull request as ready for review March 11, 2025 10:24

didierofrivia requested a review from a team as a code owner March 11, 2025 10:24

didierofrivia self-assigned this Mar 11, 2025

didierofrivia requested review from maleck13, eguzki and Boomatang March 11, 2025 13:15

didierofrivia force-pushed the fix-verifications branch 3 times, most recently from bfb1432 to e0c52d5 Compare March 11, 2025 17:40

didierofrivia mentioned this pull request Mar 13, 2025

Release process enhancement #1185

Draft

4 tasks

eguzki approved these changes Mar 14, 2025

View reviewed changes

didierofrivia added 4 commits March 14, 2025 15:28

[make] Restore original verification targets

5cc7861

* With some modifications. verify-manfiests calls the targets verify bundle, controller-gen and helm Signed-off-by: dd di cesare <[email protected]>

[ci] Separated workflows for PR and push to main and releases

4564ba5

Signed-off-by: dd di cesare <[email protected]>

[refactor] Splitting workflows, test.yaml as workflow_call

8384d2a

Signed-off-by: dd di cesare <[email protected]>

[refactor] Tests running on every PR and pushes to main and release

4c06e91

branches * Instead of calling it from verification workflows Signed-off-by: dd di cesare <[email protected]>

didierofrivia force-pushed the fix-verifications branch from 9ad97f8 to 4c06e91 Compare March 14, 2025 14:28

eguzki approved these changes Mar 14, 2025

View reviewed changes

Boomatang reviewed Mar 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix verifications #1197

Fix verifications #1197

didierofrivia commented Mar 10, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading

Boomatang left a comment

Boomatang Mar 14, 2025

Boomatang Mar 14, 2025

Boomatang Mar 14, 2025

Boomatang Mar 14, 2025

Boomatang Mar 14, 2025

Fix verifications #1197

Are you sure you want to change the base?

Fix verifications #1197

Conversation

didierofrivia commented Mar 10, 2025 • edited Loading

codecov bot commented Mar 10, 2025 • edited Loading

Codecov Report

Boomatang left a comment

Choose a reason for hiding this comment

Boomatang Mar 14, 2025

Choose a reason for hiding this comment

Boomatang Mar 14, 2025

Choose a reason for hiding this comment

Boomatang Mar 14, 2025

Choose a reason for hiding this comment

Boomatang Mar 14, 2025

Choose a reason for hiding this comment

Boomatang Mar 14, 2025

Choose a reason for hiding this comment

didierofrivia commented Mar 10, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading