Open
Conversation
…iants Problem: The build workflow was building operator images separately for RHACS_BRANDING and STACKROX_BRANDING (4 builds total: 2 brandings × 2 architectures), even though the Docker layers are identical. The only difference between builds was the target registry (quay.io/rhacs-eng vs quay.io/stackrox-io). Investigation showed: - operator/prebuilt.Dockerfile contains no ROX_PRODUCT_BRANDING - Operator Go code has no branding references - Binary builds are identical regardless of branding - Only the registry tag differs between variants Solution: Build once per architecture, tag and push to both registries. Changes: - Matrix definition: removed branding dimension from build_and_push_operator, added registry dimension to push_operator_multiarch_manifests - build-and-push-operator job: build with IMAGE_REPO=stackrox to create local tag, then push to both quay.io/rhacs-eng and quay.io/stackrox-io - Added push_operator_image_to_registry() function that accepts registry parameter instead of deriving it from branding - Added push_operator_image_manifest_lists() function for registry-based manifest push (follows scanner-build.yaml pattern) Benefits: - Reduces operator builds from 4 to 2 (50% reduction) - Saves ~5-8 minutes per workflow run - Images remain available at same registry locations (no breaking changes) Follows existing pattern from scanner-build.yaml which already uses registry-based matrix instead of branding matrix. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Both
push_operator_image_to_registryandpush_operator_image_manifest_listsre-implement tag/major-minor logic that already exists in other helpers (e.g., the older operator/scanner push functions); consider extracting a common helper to avoid duplication and keep the tagging behavior consistent across images. - The operator jobs now hardcode
ROX_PRODUCT_BRANDING=RHACS_BRANDING; if any future operator build behavior diverges by branding, this may be a surprising coupling—consider adding a short comment in the workflow or wiring branding in a way that makes the assumption explicit or easier to change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both `push_operator_image_to_registry` and `push_operator_image_manifest_lists` re-implement tag/major-minor logic that already exists in other helpers (e.g., the older operator/scanner push functions); consider extracting a common helper to avoid duplication and keep the tagging behavior consistent across images.
- The operator jobs now hardcode `ROX_PRODUCT_BRANDING=RHACS_BRANDING`; if any future operator build behavior diverges by branding, this may be a surprising coupling—consider adding a short comment in the workflow or wiring branding in a way that makes the assumption explicit or easier to change.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19820 +/- ##
=======================================
Coverage 49.60% 49.60%
=======================================
Files 2763 2763
Lines 208271 208271
=======================================
+ Hits 103309 103312 +3
+ Misses 97294 97292 -2
+ Partials 7668 7667 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe build workflow transitions from branding-based operator image pushing to registry-based pushing. Operator images are now built once with Changes
Sequence Diagram(s)sequenceDiagram
participant WF as GitHub Workflow
participant Build as Build System
participant Reg1 as quay.io/rhacs-eng
participant Reg2 as quay.io/stackrox-io
participant Manifest as Manifest Service
WF->>Build: build_and_push_operator (IMAGE_REPO=stackrox)
Build->>Build: Create stackrox/stackrox-operator:tag
WF->>Reg1: push_operator_image_to_registry(registry=rhacs-eng, arch=amd64)
Reg1->>Reg1: Login & retag to registry
Reg1->>Reg1: Push with arch tag
Reg1->>Reg1: Conditional: Push latest-amd64
WF->>Reg2: push_operator_image_to_registry(registry=stackrox-io, arch=amd64)
Reg2->>Reg2: Login & retag to registry
Reg2->>Reg2: Push with arch tag
Reg2->>Reg2: Conditional: Push latest-amd64
WF->>Manifest: push_operator_image_manifest_lists(registry=rhacs-eng)
Manifest->>Reg1: Generate & publish manifest list
Manifest->>Reg1: Conditional: Publish latest, major.minor variants
WF->>Manifest: push_operator_image_manifest_lists(registry=stackrox-io)
Manifest->>Reg2: Generate & publish manifest list
Manifest->>Reg2: Conditional: Publish latest, major.minor variants
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yaml (1)
75-76:⚠️ Potential issue | 🔴 CriticalRemove the empty
nameaxis from the operator matrices.
build_and_push_operatorandpush_operator_multiarch_manifestshavename: []. In GitHub Actions, an empty array instrategy.matrixcauses a validation error and prevents the job from expanding at all—operator images and manifests will not be built or published. Remove the unusednamekey.Fix
- "build_and_push_operator": { "name":[], "arch":[] }, - "push_operator_multiarch_manifests": { "name":[] }, + "build_and_push_operator": { "arch":[] }, + "push_operator_multiarch_manifests": { "registry":[], "archs":[] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yaml around lines 75 - 76, The operator job matrices include an empty name axis which breaks matrix expansion; remove the unused name: [] entries under the "build_and_push_operator" and "push_operator_multiarch_manifests" matrix definitions in .github/workflows/build.yaml so the matrix no longer contains an empty array for the name key (leave other axes like arch intact for build_and_push_operator); ensure the YAML remains valid after deleting the name keys.
🧹 Nitpick comments (1)
scripts/ci/lib.sh (1)
371-431: Extract the shared operator tag-publish policy.
push_operator_image_to_registryandpush_operator_image_manifest_listsnow duplicate the samelatest/major.minorpublication rules already implemented bypush_operator_imageandpush_operator_manifest_lists. Keeping four copies makes the next tagging-policy change easy to miss; please move that branching into a private helper and keep the public entrypoints thin.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/lib.sh` around lines 371 - 431, Both push_operator_image_to_registry and push_operator_image_manifest_lists duplicate the same "publish latest / major.minor" logic already used by push_operator_image and push_operator_manifest_lists; extract that branching into a single private helper (e.g., publish_operator_tag_policy or _publish_operator_tags) that takes the push_context, registry, tag and a push action (or parameters like arch/architectures) and performs the conditional tagging/pushing for "merge-to-master" (latest) and semver major.minor; then update push_operator_image_to_registry and push_operator_image_manifest_lists to call this helper after logging and registry_rw_login, removing the duplicated if [[ "$push_context" == "merge-to-master" ]] and semver-if blocks so the public entrypoints remain thin and reuse the central policy used by push_operator_image and push_operator_manifest_lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/build.yaml:
- Around line 75-76: The operator job matrices include an empty name axis which
breaks matrix expansion; remove the unused name: [] entries under the
"build_and_push_operator" and "push_operator_multiarch_manifests" matrix
definitions in .github/workflows/build.yaml so the matrix no longer contains an
empty array for the name key (leave other axes like arch intact for
build_and_push_operator); ensure the YAML remains valid after deleting the name
keys.
---
Nitpick comments:
In `@scripts/ci/lib.sh`:
- Around line 371-431: Both push_operator_image_to_registry and
push_operator_image_manifest_lists duplicate the same "publish latest /
major.minor" logic already used by push_operator_image and
push_operator_manifest_lists; extract that branching into a single private
helper (e.g., publish_operator_tag_policy or _publish_operator_tags) that takes
the push_context, registry, tag and a push action (or parameters like
arch/architectures) and performs the conditional tagging/pushing for
"merge-to-master" (latest) and semver major.minor; then update
push_operator_image_to_registry and push_operator_image_manifest_lists to call
this helper after logging and registry_rw_login, removing the duplicated if [[
"$push_context" == "merge-to-master" ]] and semver-if blocks so the public
entrypoints remain thin and reuse the central policy used by push_operator_image
and push_operator_manifest_lists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9463935b-785f-4628-b1d7-23096d045cd0
📒 Files selected for processing (2)
.github/workflows/build.yamlscripts/ci/lib.sh
Contributor
🚀 Build Images ReadyImages are ready for commit fbaa585. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-557-gfbaa5858ff |
|
@janisz: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem: The build workflow was building operator images separately for RHACS_BRANDING and STACKROX_BRANDING (4 builds total: 2 brandings × 2 architectures), even though the Docker layers are identical. The only difference between builds was the target registry
(quay.io/rhacs-eng vs quay.io/stackrox-io).
Investigation showed:
Solution: Build once per architecture, tag and push to both registries.
Changes:
Benefits:
Follows existing pattern from scanner-build.yaml which already uses registry-based matrix instead of branding matrix.
Description
change me!
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!