feat: s390x and arm64 fedora image publish#5464
Conversation
This change introduces a suffix to the built artifact to facilitate multi-architecture image testing. Without this modification, a race condition occurs when the jobs for s390x and x86_64 run in parallel. Signed-off-by: Nestor Acuna Blanco <nestor.acuna@ibm.com>
Uses a matrix approach in order to reduce code duplication. After the job runs, the artifacts should be published. Signed-off-by: Nestor Acuna Blanco <nestor.acuna@ibm.com>
The build.sh script primarily utilizes environment variables. To generate an image for a new architecture, these variables must be adjusted. This change relocates the generic environment variables to specific steps, facilitating the transition to multiarch building. Signed-off-by: Nestor Acuna Blanco <nestor.acuna@ibm.com>
This change enables the publication of the Fedora s390x and arm images, alongside the amd64 image in a single manifest. To achieve this, the following steps must be taken: 1. Install qemu-system-s390x. 2. Execute build.sh twice with different environment variables. After completing these steps, both all the images should be added to the manifest. Signed-off-by: Nestor Acuna Blanco <nestor.acuna@ibm.com>
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds Fedora 43 matrix-driven build inputs, updates both component-builder workflows to consume shared architecture data, and separates multi-arch manifest publishing from per-architecture image builds. Also increases the VM install wait timeout. ChangesFedora 43 Multi-Arch Build Pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Branch Management
Custom Commands
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
@coderabbitai review |
✅ Action performedReview finished.
|
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
nestoracunablanco
force-pushed
the
refactor/componentBuilderCommonCode
branch
from
de3e8c2 to
1ff2abc
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/component-builder-publish.yml:
- Line 151: The manifest creation step is using debug logging unnecessarily,
which is too noisy for CI and may expose registry details. Update the podman
manifest create invocation in the workflow to use Podman’s default log level by
removing the --log-level=debug flag, keeping the existing REMOTE_REPOSITORY,
remote_tag, and ARCH_IMAGES behavior unchanged.
- Around line 79-83: The Fedora base image fetch step currently downloads the
QCOW2 without integrity verification, so add a checksum/signature validation
step around the “Fetch base Fedora image” job in component-builder-publish.yml
before the image is used or published. Update the workflow to download the
matching Fedora CHECKSUM file for the same architecture, verify the downloaded
QCOW2 against it, and fail the job if the digest does not match; keep the
existing FEDORA_IMAGE and matrix.url_arch references so the validation is tied
to the exact artifact being fetched.
- Around line 107-111: The Quay login steps in the workflow currently pass
QUAY_TOKEN directly as a command-line argument in the podman login invocation,
which exposes the secret in process arguments. Update both logging steps to use
stdin-based authentication instead of the -p flag, and keep the secret only in
the environment so the existing Logging to quay.io and the other Quay login step
remain secure.
In @.github/workflows/component-builder.yml:
- Around line 55-72: Template-expanded matrix values are being used directly
inside shell scripts in the component-builder workflow, which can lead to unsafe
shell interpretation. Update the affected steps to pass values from
matrix.qemu_package, matrix.url_arch, and matrix.fedora_url through env on the
step, then reference those shell variables in the install and Fedora image
download commands with proper quoting. Use the existing workflow steps around
the apt-get install block and Fetch base Fedora image block as the place to make
the change.
- Around line 85-93: The Save container image as tarball step is expanding
matrix-derived CPU_ARCH directly inside the run script, which violates the
shell-variable guard used elsewhere. Update the podman save and echo commands in
the Save container image as tarball step to use the existing quoted shell
variables (local_repository, arch_tag, and a shell-held CPU_ARCH value if
needed) instead of interpolating env.CPU_ARCH inline, so the artifact/image
names are fully handled through shell variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1e8ab76-3059-4b1c-b4dc-2ea3e9a3cc93
📒 Files selected for processing (4)
.github/component-builder-config.json.github/workflows/component-builder-publish.yml.github/workflows/component-builder.ymlcontainers/fedora/build.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: guest-fedora (arm64, aarch64, qemu-system-aarch64, https://download.fedoraproject.org/pub/fedora/...
- GitHub Check: guest-fedora (s390x, s390x, qemu-system-s390x, https://download.fedoraproject.org/pub/fedora-seco...
- GitHub Check: guest-fedora (amd64, x86_64, qemu-system-x86, https://download.fedoraproject.org/pub/fedora/linux...
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
**: # AI Review and Development StandardsAssisted-by: Claude noreply@anthropic.com
Coding standards, conventions, and review guidelines for openshift-virtualization-tests.
These rules apply to ALL contributors and review tools — human and AI alike.
Strict Rules (MANDATORY)
Linter Suppressions PROHIBITED
- ❌ NEVER add
# noqa,# type: ignore,# pylint: disable- ❌ NEVER disable linter/mypy rules to work around issues
- ✅ FIX THE CODE - If linter complains, the code is wrong
- If you think a rule is wrong: ASK the user for explicit approval
Code Reuse (Search-First Development)
Before writing ANY new code:
- SEARCH codebase for existing implementations
- CHECK
utilities/for shared functions- CHECK
libs/for shared libraries- CHECK
tests/for shared fixtures and helper functions- CHECK
pyproject.tomldependencies — project packages (e.g.,pyhelper-utils,ocp-resources,openshift-python-wrapper) may already provide the functionality- VERIFY no similar logic exists elsewhere
- NEVER duplicate logic - extract to shared module
- REUSE existing code and patterns — only write new when nothing exists
External package examples:
- Shell commands — use
pyhelper_utils.shell.run_command, NEVER usesubprocess.rundirectly in test/utility code- OpenShift resources — use
ocp-resourcesclasses, NEVER construct raw YAML dictsPython Requirements
- Type hints MANDATORY - mypy strict mode in
libs/, all new public functions under utilities MUST be typed- Use
TYPE_CHECKINGfor type-only imports - wrap imports needed solely for type hints inif TYPE_CHECKING:to avoid runtime overhead and circular imports- Google-format docstrings REQUIRED - for all public functions with non-obvious return values OR side effects
- No defensive programming - fail-fast, don't hide bugs with fake defaults (see exceptions below)
- ALWAYS use
uv run-...
Files:
containers/fedora/build.sh
⚙️ CodeRabbit configuration file
**: ## PR Template Validation
Check the PR description for required sections from.github/pull_request_template.md.
Required sections (must be present, even if empty):
##### What this PR does / why we need it:— MUST be present AND have meaningful content.
Flag as HIGH if the section is missing, empty, whitespace-only, contains only HTML comments,
or contains only placeholder tokens such asTBD,TBA,N/A,-,—,none, or..##### Which issue(s) this PR fixes:— must be present (may be empty)##### Special notes for reviewer:— must be present (may be empty)##### jira-ticket:— must be present (may be empty)
If any required section is absent, orWhat this PR does / why we need it:has no content,
flag it as HIGH severity and ask the author to restore the missing template section(s).Approval Policy
You may approve the PR when ALL of the following are true:
- All your review comments have been addressed with either:
- a code/doc change that fixes the issue, or
- a substantive author response that justifies no code change.
Thread "resolved" state alone is not sufficient.
OR you had no review comments.- If you posted a test execution plan comment requesting tests, and the PR author replied
with a comment explaining why the requested tests are not needed or were already covered,
treat that as an acceptable response — do not block approval on the test plan alone.- The author's explanation must be reasonable and specific (not just "N/A" or "not needed").
Accept explanations like: "these tests were already run in CI", "this change is docs-only",
"the affected tests are quarantined", or "verified manually on cluster X".
Files:
containers/fedora/build.sh
🧠 Learnings (3)
📚 Learning: 2026-05-19T10:17:37.060Z
Learnt from: Anatw
Repo: RedHatQE/openshift-virtualization-tests PR: 4833
File: tests/network/localnet/migration_stuntime/libstuntime.py:13-13
Timestamp: 2026-05-19T10:17:37.060Z
Learning: In RedHatQE/openshift-virtualization-tests, when the PR template validation rule is triggered, avoid posting the PR template violation comment if the PR description already contains the required sections with meaningful content:
- `##### What this PR does / why we need it:`
- `##### Which issue(s) this PR fixes:`
- `##### Special notes for reviewer:`
- `##### jira-ticket:`
Because the current implementation can use a broad `**` path glob and re-run the check per diff context/file, reviewers/automation should verify the actual PR description content before flagging it as a violation or duplicating the comment.
Applied to files:
.github/component-builder-config.json.github/workflows/component-builder.yml.github/workflows/component-builder-publish.yml
📚 Learning: 2026-05-20T11:18:31.677Z
Learnt from: Anatw
Repo: RedHatQE/openshift-virtualization-tests PR: 4867
File: tests/network/libs/stuntime.py:1-1
Timestamp: 2026-05-20T11:18:31.677Z
Learning: In RedHatQE/openshift-virtualization-tests, make the PR template validation (HIGH-severity) heuristic more conservative: if the author explicitly rebuts the “missing template section” finding and confirms the required sections are present with meaningful content, treat the original finding as a false positive and do not re-raise it. Only raise the HIGH-severity flag when a required PR section header (e.g., "##### What this PR does / why we need it:") is clearly absent or its content can be verified as empty/placeholder-only.
Applied to files:
.github/workflows/component-builder.yml.github/workflows/component-builder-publish.yml
📚 Learning: 2026-06-03T15:08:46.871Z
Learnt from: geetikakay
Repo: RedHatQE/openshift-virtualization-tests PR: 5101
File: .github/workflows/request-coderabbit-test-instructions.yml:39-39
Timestamp: 2026-06-03T15:08:46.871Z
Learning: In RedHatQE/openshift-virtualization-tests, GitHub Actions workflow reviewers should follow the repo convention of allowing `uses:` references that are pinned to mutable version tags (e.g., `owner/actionv4`, `owner/actionv5`) and should NOT flag these as security issues. Full commit-SHA pinning for third-party actions is explicitly out of scope for this repository, so do not treat non-SHA `uses:` references as violations.
Applied to files:
.github/workflows/component-builder.yml.github/workflows/component-builder-publish.yml
🪛 zizmor (1.26.1)
.github/workflows/component-builder.yml
[warning] 58-58: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 70-70: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 71-71: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 27-27: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 51-51: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 17-17: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[info] 38-38: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[warning] 91-91: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 92-92: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 92-92: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 93-93: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 96-96: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
.github/workflows/component-builder-publish.yml
[warning] 70-70: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 82-82: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 83-83: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 35-35: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 62-62: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 20-20: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[info] 47-47: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[info] 122-122: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
🔇 Additional comments (4)
.github/component-builder-config.json (1)
1-26: LGTM!.github/workflows/component-builder-publish.yml (1)
2-5: LGTM!Also applies to: 17-78, 104-105, 113-120, 122-132, 139-150, 152-152
.github/workflows/component-builder.yml (1)
10-53: LGTM!Also applies to: 82-83, 95-99
containers/fedora/build.sh (1)
87-87: 🩺 Stability & Availability
--wait 60is already a 60-minute timeout. The Fedora install step is long-running, so this value looks intentional and doesn’t need changing.> Likely an incorrect or invalid review comment.
| - name: Logging to quay.io | ||
| run: podman login -u ${{ secrets.QUAY_USER }} -p ${{ secrets.QUAY_TOKEN }} quay.io | ||
| env: | ||
| QUAY_USER: ${{ secrets.QUAY_USER }} | ||
| QUAY_TOKEN: ${{ secrets.QUAY_TOKEN }} | ||
| run: podman login -u "$QUAY_USER" -p "$QUAY_TOKEN" quay.io |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant workflow sections with line numbers.
sed -n '90,150p' .github/workflows/component-builder-publish.yml | cat -n
# Find every podman login usage in the workflow.
rg -n "podman login|password-stdin|QUAY_TOKEN|QUAY_USER" .github/workflows/component-builder-publish.ymlRepository: RedHatQE/openshift-virtualization-tests
Length of output: 3301
HIGH: don’t pass QUAY_TOKEN on the command line.
podman login -p "$QUAY_TOKEN" puts the secret in process arguments, where it can leak through ps/job introspection. Use stdin auth instead on both Quay login steps.
Proposed fix
- run: podman login -u "$QUAY_USER" -p "$QUAY_TOKEN" quay.io
+ run: printf '%s' "$QUAY_TOKEN" | podman login -u "$QUAY_USER" --password-stdin quay.io📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Logging to quay.io | |
| run: podman login -u ${{ secrets.QUAY_USER }} -p ${{ secrets.QUAY_TOKEN }} quay.io | |
| env: | |
| QUAY_USER: ${{ secrets.QUAY_USER }} | |
| QUAY_TOKEN: ${{ secrets.QUAY_TOKEN }} | |
| run: podman login -u "$QUAY_USER" -p "$QUAY_TOKEN" quay.io | |
| - name: Logging to quay.io | |
| env: | |
| QUAY_USER: ${{ secrets.QUAY_USER }} | |
| QUAY_TOKEN: ${{ secrets.QUAY_TOKEN }} | |
| run: printf '%s' "$QUAY_TOKEN" | podman login -u "$QUAY_USER" --password-stdin quay.io |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/component-builder-publish.yml around lines 107 - 111, The
Quay login steps in the workflow currently pass QUAY_TOKEN directly as a
command-line argument in the podman login invocation, which exposes the secret
in process arguments. Update both logging steps to use stdin-based
authentication instead of the -p flag, and keep the secret only in the
environment so the existing Logging to quay.io and the other Quay login step
remain secure.
| ARCH_IMAGES="${ARCH_IMAGES} ${REMOTE_REPOSITORY}:${FEDORA_VERSION}-${arch}" | ||
| done | ||
| # Create and push the multi-arch manifest | ||
| podman manifest create --log-level=debug "${REMOTE_REPOSITORY}":"${remote_tag}" ${ARCH_IMAGES} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
MEDIUM: remove debug logging from manifest creation.
Debug output is too noisy for CI and can include registry/request details. Use Podman’s default log level here. As per coding guidelines, “NEVER use DEBUG level - if a log is needed, use INFO.”
Proposed fix
- podman manifest create --log-level=debug "${REMOTE_REPOSITORY}":"${remote_tag}" ${ARCH_IMAGES}
+ podman manifest create "${REMOTE_REPOSITORY}":"${remote_tag}" ${ARCH_IMAGES}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| podman manifest create --log-level=debug "${REMOTE_REPOSITORY}":"${remote_tag}" ${ARCH_IMAGES} | |
| podman manifest create "${REMOTE_REPOSITORY}":"${remote_tag}" ${ARCH_IMAGES} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/component-builder-publish.yml at line 151, The manifest
creation step is using debug logging unnecessarily, which is too noisy for CI
and may expose registry details. Update the podman manifest create invocation in
the workflow to use Podman’s default log level by removing the --log-level=debug
flag, keeping the existing REMOTE_REPOSITORY, remote_tag, and ARCH_IMAGES
behavior unchanged.
Source: Coding guidelines
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y \ | ||
| qemu-system-x86 \ | ||
| ${{ matrix.qemu_package }} \ | ||
| libvirt-daemon-system \ | ||
| virtinst cloud-image-utils \ | ||
| libguestfs-tools | ||
|
|
||
| - name: Tweak hosted runner to enable 'virt-sysprep' | ||
| # https://bugs.launchpad.net/ubuntu/+source/linux/+bug/759725 | ||
| run: sudo chmod 0644 /boot/vmlinuz* | ||
|
|
||
| - name: Fetch base Fedora image | ||
| working-directory: ./containers/fedora | ||
| run: wget -q "https://download.fedoraproject.org/pub/fedora/linux/releases/43/Cloud/x86_64/images/${{ env.FEDORA_IMAGE }}" | ||
| run: | | ||
| FEDORA_IMAGE="${FEDORA_IMAGE_BASE}.${{ matrix.url_arch }}.qcow2" | ||
| wget -q "${{ matrix.fedora_url }}/${FEDORA_IMAGE}" | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
HIGH: avoid template-expanded matrix values inside shell scripts.
${{ matrix.qemu_package }}, ${{ matrix.url_arch }}, and ${{ matrix.fedora_url }} are injected into the script before shell parsing, so a config change with shell metacharacters can execute unintended commands. Bind them through env and quote shell variables instead.
Proposed fix
- name: Install dependencies for VM build
+ env:
+ QEMU_PACKAGE: ${{ matrix.qemu_package }}
run: |
sudo apt-get update
sudo apt-get install -y \
- ${{ matrix.qemu_package }} \
+ "$QEMU_PACKAGE" \
libvirt-daemon-system \
virtinst cloud-image-utils \
libguestfs-tools
@@
- name: Fetch base Fedora image
working-directory: ./containers/fedora
+ env:
+ FEDORA_URL: ${{ matrix.fedora_url }}
+ URL_ARCH: ${{ matrix.url_arch }}
run: |
- FEDORA_IMAGE="${FEDORA_IMAGE_BASE}.${{ matrix.url_arch }}.qcow2"
- wget -q "${{ matrix.fedora_url }}/${FEDORA_IMAGE}"
+ fedora_image="${FEDORA_IMAGE_BASE}.${URL_ARCH}.qcow2"
+ wget -q "${FEDORA_URL}/${fedora_image}"As per coding guidelines, “FIX THE CODE - If linter complains, the code is wrong.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| sudo apt-get update | |
| sudo apt-get install -y \ | |
| qemu-system-x86 \ | |
| ${{ matrix.qemu_package }} \ | |
| libvirt-daemon-system \ | |
| virtinst cloud-image-utils \ | |
| libguestfs-tools | |
| - name: Tweak hosted runner to enable 'virt-sysprep' | |
| # https://bugs.launchpad.net/ubuntu/+source/linux/+bug/759725 | |
| run: sudo chmod 0644 /boot/vmlinuz* | |
| - name: Fetch base Fedora image | |
| working-directory: ./containers/fedora | |
| run: wget -q "https://download.fedoraproject.org/pub/fedora/linux/releases/43/Cloud/x86_64/images/${{ env.FEDORA_IMAGE }}" | |
| run: | | |
| FEDORA_IMAGE="${FEDORA_IMAGE_BASE}.${{ matrix.url_arch }}.qcow2" | |
| wget -q "${{ matrix.fedora_url }}/${FEDORA_IMAGE}" | |
| env: | |
| QEMU_PACKAGE: ${{ matrix.qemu_package }} | |
| run: | | |
| sudo apt-get update | |
| sudo apt-get install -y \ | |
| "$QEMU_PACKAGE" \ | |
| libvirt-daemon-system \ | |
| virtinst cloud-image-utils \ | |
| libguestfs-tools | |
| - name: Tweak hosted runner to enable 'virt-sysprep' | |
| # https://bugs.launchpad.net/ubuntu/+source/linux/+bug/759725 | |
| run: sudo chmod 0644 /boot/vmlinuz* | |
| - name: Fetch base Fedora image | |
| working-directory: ./containers/fedora | |
| env: | |
| FEDORA_URL: ${{ matrix.fedora_url }} | |
| URL_ARCH: ${{ matrix.url_arch }} | |
| run: | | |
| fedora_image="${FEDORA_IMAGE_BASE}.${URL_ARCH}.qcow2" | |
| wget -q "${FEDORA_URL}/${fedora_image}" |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 58-58: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 70-70: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 71-71: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/component-builder.yml around lines 55 - 72,
Template-expanded matrix values are being used directly inside shell scripts in
the component-builder workflow, which can lead to unsafe shell interpretation.
Update the affected steps to pass values from matrix.qemu_package,
matrix.url_arch, and matrix.fedora_url through env on the step, then reference
those shell variables in the install and Fedora image download commands with
proper quoting. Use the existing workflow steps around the apt-get install block
and Fetch base Fedora image block as the place to make the change.
Sources: Coding guidelines, Linters/SAST tools
|
Clean rebase detected — no code changes compared to previous head ( |
nestoracunablanco
force-pushed
the
refactor/componentBuilderCommonCode
branch
from
1ff2abc to
1e6904c
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/component-builder.yml (1)
67-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMEDIUM:
FEDORA_IMAGEfilename formula is computed twice.
${FEDORA_IMAGE_BASE}.<url_arch>.qcow2is built once locally in "Fetch base Fedora image" (line 70) and again independently as an env value in "Create VM" (line 82). Same formula, two places — a future edit to one (e.g. adding a checksum suffix) without the other will silently break the build by fetching one filename and referencing another.Compute it once (e.g. as a step output in "Fetch base Fedora image") and reuse it in "Create VM" instead of re-deriving it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/component-builder.yml around lines 67 - 82, The Fedora image filename is being derived twice with the same formula, once in the “Fetch base Fedora image” step and again in the “Create VM” step, which can drift over time. Update the workflow so the image name is computed once in the Fedora fetch step and passed forward as a shared value (for example via a step output or env var), then have the “Create VM” step reuse that value instead of rebuilding ${FEDORA_IMAGE_BASE}.${{ matrix.url_arch }}.qcow2 independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/component-builder-publish.yml:
- Around line 87-88: The checksum download in the workflow is using a hardcoded
CHECKSUM path and never uses the computed FEDORA_CHECKSUM value, so update the
Fedora publish step to fetch the configured per-architecture checksum file
derived from fedora_checksum_base and matrix.url_arch. Make sure the download
command references the constructed FEDORA_CHECKSUM name in the same
component-builder-publish workflow step so the later verification uses the
intended file.
---
Outside diff comments:
In @.github/workflows/component-builder.yml:
- Around line 67-82: The Fedora image filename is being derived twice with the
same formula, once in the “Fetch base Fedora image” step and again in the
“Create VM” step, which can drift over time. Update the workflow so the image
name is computed once in the Fedora fetch step and passed forward as a shared
value (for example via a step output or env var), then have the “Create VM” step
reuse that value instead of rebuilding ${FEDORA_IMAGE_BASE}.${{ matrix.url_arch
}}.qcow2 independently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e489a395-d42b-4e13-b1a3-1b2c31a771b5
📒 Files selected for processing (4)
.github/component-builder-config.json.github/workflows/component-builder-publish.yml.github/workflows/component-builder.ymlcontainers/fedora/build.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: build-container
- GitHub Check: tox
- GitHub Check: verify-bugs-are-open
- GitHub Check: security-committer-identity
- GitHub Check: security-suspicious-paths
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: guest-fedora (arm64, aarch64, qemu-system-aarch64, https://download.fedoraproject.org/pub/fedora/...
- GitHub Check: guest-fedora (amd64, x86_64, qemu-system-x86, https://download.fedoraproject.org/pub/fedora/linux...
- GitHub Check: guest-fedora (s390x, s390x, qemu-system-s390x, https://download.fedoraproject.org/pub/fedora-seco...
⚠️ CI failures not shown inline (1)
GitHub Check: can-be-merged: Check if can be merged
Conclusion: failure
Some check runs not started: tox, build-container, security-committer-identity, verified, cnv-smoke/validation, verify-bugs-are-open, security-suspicious-paths
PR has 3 unresolved review conversation(s):
- .github/workflows/component-builder-publish.yml:130 (https://github.com/RedHatQE/openshift-virtualization-tests/pull/5464#discussion_r3513322355)
- .github/workflows/component-builder-publish.yml:170 (https://github.com/RedHatQE/openshift-virtualization-tests/pull/5464#discussion_r3513322369)
- .github/workflows/component-builder.yml:72 (https://github.com/RedHatQE/openshift-virtualization-tests/pull/5464#discussion_r3513322375)
Missing approved from approvers: rnetser, vsibirsk, dshchedr, myakove
Missing lgtm from reviewers. Minimum 2 required, (0 given). Reviewers: rnetser, vsibirsk, dshchedr, RoniKishner, myakove.
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
**: # AI Review and Development StandardsAssisted-by: Claude noreply@anthropic.com
Coding standards, conventions, and review guidelines for openshift-virtualization-tests.
These rules apply to ALL contributors and review tools — human and AI alike.
Strict Rules (MANDATORY)
Linter Suppressions PROHIBITED
- ❌ NEVER add
# noqa,# type: ignore,# pylint: disable- ❌ NEVER disable linter/mypy rules to work around issues
- ✅ FIX THE CODE - If linter complains, the code is wrong
- If you think a rule is wrong: ASK the user for explicit approval
Code Reuse (Search-First Development)
Before writing ANY new code:
- SEARCH codebase for existing implementations
- CHECK
utilities/for shared functions- CHECK
libs/for shared libraries- CHECK
tests/for shared fixtures and helper functions- CHECK
pyproject.tomldependencies — project packages (e.g.,pyhelper-utils,ocp-resources,openshift-python-wrapper) may already provide the functionality- VERIFY no similar logic exists elsewhere
- NEVER duplicate logic - extract to shared module
- REUSE existing code and patterns — only write new when nothing exists
External package examples:
- Shell commands — use
pyhelper_utils.shell.run_command, NEVER usesubprocess.rundirectly in test/utility code- OpenShift resources — use
ocp-resourcesclasses, NEVER construct raw YAML dictsPython Requirements
- Type hints MANDATORY - mypy strict mode in
libs/, all new public functions under utilities MUST be typed- Use
TYPE_CHECKINGfor type-only imports - wrap imports needed solely for type hints inif TYPE_CHECKING:to avoid runtime overhead and circular imports- Google-format docstrings REQUIRED - for all public functions with non-obvious return values OR side effects
- No defensive programming - fail-fast, don't hide bugs with fake defaults (see exceptions below)
- ALWAYS use
uv run-...
Files:
containers/fedora/build.sh
⚙️ CodeRabbit configuration file
**: ## PR Template Validation
Check the PR description for required sections from.github/pull_request_template.md.
Required sections (must be present, even if empty):
##### What this PR does / why we need it:— MUST be present AND have meaningful content.
Flag as HIGH if the section is missing, empty, whitespace-only, contains only HTML comments,
or contains only placeholder tokens such asTBD,TBA,N/A,-,—,none, or..##### Which issue(s) this PR fixes:— must be present (may be empty)##### Special notes for reviewer:— must be present (may be empty)##### jira-ticket:— must be present (may be empty)
If any required section is absent, orWhat this PR does / why we need it:has no content,
flag it as HIGH severity and ask the author to restore the missing template section(s).Approval Policy
You may approve the PR when ALL of the following are true:
- All your review comments have been addressed with either:
- a code/doc change that fixes the issue, or
- a substantive author response that justifies no code change.
Thread "resolved" state alone is not sufficient.
OR you had no review comments.- If you posted a test execution plan comment requesting tests, and the PR author replied
with a comment explaining why the requested tests are not needed or were already covered,
treat that as an acceptable response — do not block approval on the test plan alone.- The author's explanation must be reasonable and specific (not just "N/A" or "not needed").
Accept explanations like: "these tests were already run in CI", "this change is docs-only",
"the affected tests are quarantined", or "verified manually on cluster X".
Files:
containers/fedora/build.sh
🧠 Learnings (3)
📚 Learning: 2026-05-19T10:17:37.060Z
Learnt from: Anatw
Repo: RedHatQE/openshift-virtualization-tests PR: 4833
File: tests/network/localnet/migration_stuntime/libstuntime.py:13-13
Timestamp: 2026-05-19T10:17:37.060Z
Learning: In RedHatQE/openshift-virtualization-tests, when the PR template validation rule is triggered, avoid posting the PR template violation comment if the PR description already contains the required sections with meaningful content:
- `##### What this PR does / why we need it:`
- `##### Which issue(s) this PR fixes:`
- `##### Special notes for reviewer:`
- `##### jira-ticket:`
Because the current implementation can use a broad `**` path glob and re-run the check per diff context/file, reviewers/automation should verify the actual PR description content before flagging it as a violation or duplicating the comment.
Applied to files:
.github/component-builder-config.json.github/workflows/component-builder.yml.github/workflows/component-builder-publish.yml
📚 Learning: 2026-05-20T11:18:31.677Z
Learnt from: Anatw
Repo: RedHatQE/openshift-virtualization-tests PR: 4867
File: tests/network/libs/stuntime.py:1-1
Timestamp: 2026-05-20T11:18:31.677Z
Learning: In RedHatQE/openshift-virtualization-tests, make the PR template validation (HIGH-severity) heuristic more conservative: if the author explicitly rebuts the “missing template section” finding and confirms the required sections are present with meaningful content, treat the original finding as a false positive and do not re-raise it. Only raise the HIGH-severity flag when a required PR section header (e.g., "##### What this PR does / why we need it:") is clearly absent or its content can be verified as empty/placeholder-only.
Applied to files:
.github/workflows/component-builder.yml.github/workflows/component-builder-publish.yml
📚 Learning: 2026-06-03T15:08:46.871Z
Learnt from: geetikakay
Repo: RedHatQE/openshift-virtualization-tests PR: 5101
File: .github/workflows/request-coderabbit-test-instructions.yml:39-39
Timestamp: 2026-06-03T15:08:46.871Z
Learning: In RedHatQE/openshift-virtualization-tests, GitHub Actions workflow reviewers should follow the repo convention of allowing `uses:` references that are pinned to mutable version tags (e.g., `owner/actionv4`, `owner/actionv5`) and should NOT flag these as security issues. Full commit-SHA pinning for third-party actions is explicitly out of scope for this repository, so do not treat non-SHA `uses:` references as violations.
Applied to files:
.github/workflows/component-builder.yml.github/workflows/component-builder-publish.yml
🪛 GitHub Check: can-be-merged
.github/workflows/component-builder.yml
[error] 72-72: PR has an unresolved review conversation at line 72: #5464 (comment).
.github/workflows/component-builder-publish.yml
[error] 130-130: PR has an unresolved review conversation at line 130: #5464 (comment).
[error] 170-170: PR has an unresolved review conversation at line 170: #5464 (comment).
🪛 GitHub Check: security-suspicious-paths
.github/workflows/component-builder.yml
[warning] 1-1: Suspicious path detection: PR modifies security-sensitive CI/CD workflow file under configured prefix '.github/workflows/'. Please review carefully for supply-chain risks.
.github/workflows/component-builder-publish.yml
[warning] 1-1: Suspicious path detection: PR modifies security-sensitive CI/CD workflow file under configured prefix '.github/workflows/'. Please review carefully for supply-chain risks.
🪛 zizmor (1.26.1)
.github/workflows/component-builder.yml
[error] 27-27: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 17-17: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[info] 38-38: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
.github/workflows/component-builder-publish.yml
[error] 36-36: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[warning] 87-87: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 88-88: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 93-93: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🔇 Additional comments (7)
.github/workflows/component-builder.yml (4)
55-61: CRITICAL (recurring):matrix.qemu_packagestill interpolated raw intorun:.This is the same template-injection pattern already flagged in a prior review round —
${{ matrix.qemu_package }}is expanded before the shell parses the script, so a change to the config JSON containing shell metacharacters could execute arbitrary code on the runner. The fix (bind throughenv:, reference as a quoted shell variable) still hasn't landed here.🔒 Proposed fix
- name: Install dependencies for VM build + env: + QEMU_PACKAGE: ${{ matrix.qemu_package }} run: | sudo apt-get update sudo apt-get install -y \ - ${{ matrix.qemu_package }} \ + "$QEMU_PACKAGE" \ libvirt-daemon-system \ virtinst cloud-image-utils \ libguestfs-tools
67-71: CRITICAL (recurring):matrix.url_arch/matrix.fedora_urlstill injected directly intorun:.Same template-injection concern raised previously —
${{ matrix.url_arch }}and${{ matrix.fedora_url }}are expanded before the wget script runs. Route throughenv:with quoted shell vars.🔒 Proposed fix
- name: Fetch base Fedora image working-directory: ./containers/fedora + env: + FEDORA_URL: ${{ matrix.fedora_url }} + URL_ARCH: ${{ matrix.url_arch }} run: | - FEDORA_IMAGE="${FEDORA_IMAGE_BASE}.${{ matrix.url_arch }}.qcow2" - wget -q "${{ matrix.fedora_url }}/${FEDORA_IMAGE}" + fedora_image="${FEDORA_IMAGE_BASE}.${URL_ARCH}.qcow2" + wget -q "${FEDORA_URL}/${fedora_image}"
85-93: CRITICAL (recurring, appears regressed):env.REMOTE_REPOSITORY/env.CPU_ARCHstill raw-interpolated intorun:.This exact block was previously flagged and marked "Addressed in commits dd085d6 to 1e6904c," but the current code still expands
${{ env.REMOTE_REPOSITORY }}and${{ env.CPU_ARCH }}directly inside thepodman tag/podman save/echocommands (lines 91-93) instead of via already-defined shell variables (local_repository,arch_tag). Please confirm whether the fix was reverted, or re-apply it.🔒 Proposed fix
run: | mkdir -p artifacts - podman tag "${local_repository}":"${arch_tag}" "${{ env.REMOTE_REPOSITORY }}":"${arch_tag}" - podman save -o artifacts/fedora-image-${{ env.CPU_ARCH }}.tar "${{ env.REMOTE_REPOSITORY }}":"${arch_tag}" - echo "Saved image to artifacts/fedora-image-${{ env.CPU_ARCH }}.tar" + artifact_path="artifacts/fedora-image-${CPU_ARCH}.tar" + podman tag "${local_repository}:${arch_tag}" "${REMOTE_REPOSITORY}:${arch_tag}" + podman save -o "${artifact_path}" "${REMOTE_REPOSITORY}:${arch_tag}" + echo "Saved image to ${artifact_path}"
17-46: LGTM!.github/workflows/component-builder-publish.yml (1)
27-47: LGTM!Also applies to: 57-59, 90-102, 160-160
.github/component-builder-config.json (1)
1-27: LGTM!containers/fedora/build.sh (1)
87-87: LGTM!
| FEDORA_CHECKSUM="${FEDORA_CHECKSUM_BASE}.-${{ matrix.url_arch }}-CHECKSUM" | ||
| wget -q "${{ matrix.fedora_url }}/CHECKSUM" -O CHECKSUM |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
HIGH: fetch the configured checksum file, not hardcoded CHECKSUM.
Line 87 builds an unused, malformed name (.-${{ matrix.url_arch }}), then Line 88 ignores it and downloads ${{ matrix.fedora_url }}/CHECKSUM. The config provides fedora_checksum_base specifically to construct per-arch files like Fedora-Cloud-43-1.6-x86_64-CHECKSUM; using the hardcoded path can fail before verification.
Proposed fix
- FEDORA_CHECKSUM="${FEDORA_CHECKSUM_BASE}.-${{ matrix.url_arch }}-CHECKSUM"
- wget -q "${{ matrix.fedora_url }}/CHECKSUM" -O CHECKSUM
+ FEDORA_CHECKSUM="${FEDORA_CHECKSUM_BASE}-${{ matrix.url_arch }}-CHECKSUM"
+ wget -q "${{ matrix.fedora_url }}/${FEDORA_CHECKSUM}" -O CHECKSUM📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FEDORA_CHECKSUM="${FEDORA_CHECKSUM_BASE}.-${{ matrix.url_arch }}-CHECKSUM" | |
| wget -q "${{ matrix.fedora_url }}/CHECKSUM" -O CHECKSUM | |
| FEDORA_CHECKSUM="${FEDORA_CHECKSUM_BASE}-${{ matrix.url_arch }}-CHECKSUM" | |
| wget -q "${{ matrix.fedora_url }}/${FEDORA_CHECKSUM}" -O CHECKSUM |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 87-87: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 88-88: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/component-builder-publish.yml around lines 87 - 88, The
checksum download in the workflow is using a hardcoded CHECKSUM path and never
uses the computed FEDORA_CHECKSUM value, so update the Fedora publish step to
fetch the configured per-architecture checksum file derived from
fedora_checksum_base and matrix.url_arch. Make sure the download command
references the constructed FEDORA_CHECKSUM name in the same
component-builder-publish workflow step so the later verification uses the
intended file.
Move hardcoded Fedora version, image base, and architecture matrix out of both workflow files and into a single source of truth. Both component-builder and component-builder-publish now read their matrix, and variables from its config file via a dedicate 'prepare' job step. Assisted-by: IBM Bob Signed-off-by: Nestor Acuna Blanco <nestor.acuna@ibm.com>
Signed-off-by: Nestor Acuna Blanco <nestor.acuna@ibm.com>
nestoracunablanco
force-pushed
the
refactor/componentBuilderCommonCode
branch
from
1e6904c to
1c6aa60
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
6 participants
Commit 1:
fix: architecture suffix for the fedora-container-image artifact
This change introduces a suffix to the built artifact to facilitate
multi-architecture image testing. Without this modification, a race
condition occurs when the jobs for s390x and x86_64 run in parallel.
Commit 2:
feat: build s390x and arm64 images in the component builder workflow
Uses a matrix approach in order to reduce code duplication.
After the job runs, the artifacts should be published.
Commit 3:
Refactor: preparation work for Fedora image multiarch publish
The build.sh script primarily utilizes environment variables. To
generate an image for a new architecture, these variables must be
adjusted. This change relocates the generic environment variables
to specific steps, facilitating the transition to multiarch building.
Commit 4:
feat: component Builder Publish s390x and arm64 enablement
This change enables the publication of the Fedora s390x and arm images,
alongside the amd64 image in a single manifest. To achieve this,
the following steps must be taken:
After completing these steps, both all the images should be added
to the manifest.
Commit 5:
refactor: extract fedora image builder config into a shared file
Move hardcoded Fedora version, image base, and architecture
matrix out of both workflow files and into a single source of
truth.
Both component-builder and component-builder-publish now read
their matrix, and variables from its config file via a
dedicate 'prepare' job step.
jira-ticket:
NONE
Summary by CodeRabbit
New Features
Bug Fixes
Refactor