USHIFT-7412: Add RPM presubmit scenarios - #7175
Draft
agullon wants to merge 6 commits into
Draft
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@agullon: This pull request references USHIFT-7412 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
needs-rebase
2 tasks
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRPM scenarios are detected by path and routed through dedicated CI build logic. RPM jobs download RHEL 9.8 and 10.2 installer ISOs, build packages, and run four new presubmit scenarios. ChangesRPM CI workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Scenario
participant CI
participant S3
participant RPMBuilder
participant InstallerVM
Scenario->>CI: select RPM presubmit
CI->>S3: download RHEL installer ISOs
CI->>RPMBuilder: build RPMs
CI->>InstallerVM: create and configure VM
InstallerVM->>InstallerVM: run ordered RPM tests
CI->>InstallerVM: remove VM
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agullon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
|
@agullon: 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. |
agullon
marked this pull request as draft
August 11, 2026 09:17
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Add support for RPM-based presubmit CI that skips the image build phase
entirely. Instead of building ostree/bootc images, the RPM build path
downloads bare RHEL installer ISOs from the S3 build cache and installs
MicroShift via dnf from source-built RPMs.
Changes:
- Add scenarios-rpm type to get_scenario_type_from_path()
- Add RPM_BUILDS mode to ci_phase_iso_build.sh that downloads ISOs from
S3 and builds RPMs without composer or image builds
- Skip composer cleanup and mirror registry for RPM scenarios in
ci_phase_boot_and_test.sh
- Add 4 presubmit scenarios: el{98,102}-src@standard{1,2}.sh
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pre-commit.check-secrets: ENABLED
openshift-ci
Bot
removed
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@test/scenarios-rpm/presubmits/el102-src`@standard1.sh:
- Line 32: Update configure_fast_datapath_repo usage in
test/scenarios-rpm/presubmits/el102-src@standard1.sh lines 32-32 and
test/scenarios-rpm/presubmits/el102-src@standard2.sh lines 32-32 to use a RHEL
10-compatible Fast Datapath configuration, or skip the repository when
unsupported; do not allow the helper’s non-9 branch to configure the RHEL 9 CDN
path.
- Around line 1-3: Update the shell initialization in
test/scenarios-rpm/presubmits/el102-src@standard1.sh lines 1-3,
test/scenarios-rpm/presubmits/el102-src@standard2.sh lines 1-3,
test/scenarios-rpm/presubmits/el98-src@standard1.sh lines 1-3, and
test/scenarios-rpm/presubmits/el98-src@standard2.sh lines 1-3: use the
/usr/bin/bash shebang and add set -euo pipefail immediately afterward in each
scenario script.
- Around line 23-25: In test/scenarios-rpm/presubmits/el102-src@standard1.sh
lines 23-25, declare source_reponame and target_version separately from their
command substitutions so failures from basename and local_rpm_version propagate;
preserve source_repo_url and existing readonly behavior. Apply the same change
to test/scenarios-rpm/presubmits/el102-src@standard2.sh lines 23-25,
test/scenarios-rpm/presubmits/el98-src@standard1.sh lines 23-25, and
test/scenarios-rpm/presubmits/el98-src@standard2.sh lines 23-25, ensuring all
scripts pass shellcheck.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a2ccf907-00bf-4b73-acb9-ab429a4b1295
📒 Files selected for processing (7)
test/bin/ci_phase_boot_and_test.shtest/bin/ci_phase_iso_build.shtest/bin/common.shtest/scenarios-rpm/presubmits/el102-src@standard1.shtest/scenarios-rpm/presubmits/el102-src@standard2.shtest/scenarios-rpm/presubmits/el98-src@standard1.shtest/scenarios-rpm/presubmits/el98-src@standard2.sh
Comment on lines
+1
to
+3
| #!/bin/bash | ||
|
|
||
| # Sourced from scenario.sh and uses functions defined there. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the required shell initialization in every scenario.
test/scenarios-rpm/presubmits/el102-src@standard1.sh#L1-L3: use#!/usr/bin/bashand addset -euo pipefail.test/scenarios-rpm/presubmits/el102-src@standard2.sh#L1-L3: use#!/usr/bin/bashand addset -euo pipefail.test/scenarios-rpm/presubmits/el98-src@standard1.sh#L1-L3: use#!/usr/bin/bashand addset -euo pipefail.test/scenarios-rpm/presubmits/el98-src@standard2.sh#L1-L3: use#!/usr/bin/bashand addset -euo pipefail.
Proposed fix
-#!/bin/bash
+#!/usr/bin/bash
+
+set -euo pipefailAs per coding guidelines, Shell scripts must use the #!/usr/bin/bash shebang and set -euo pipefail.
📝 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.
Suggested change
| #!/bin/bash | |
| # Sourced from scenario.sh and uses functions defined there. | |
| #!/usr/bin/bash | |
| set -euo pipefail | |
| # Sourced from scenario.sh and uses functions defined there. |
📍 Affects 4 files
test/scenarios-rpm/presubmits/el102-src@standard1.sh#L1-L3(this comment)test/scenarios-rpm/presubmits/el102-src@standard2.sh#L1-L3test/scenarios-rpm/presubmits/el98-src@standard1.sh#L1-L3test/scenarios-rpm/presubmits/el98-src@standard2.sh#L1-L3
🤖 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 `@test/scenarios-rpm/presubmits/el102-src`@standard1.sh around lines 1 - 3,
Update the shell initialization in
test/scenarios-rpm/presubmits/el102-src@standard1.sh lines 1-3,
test/scenarios-rpm/presubmits/el102-src@standard2.sh lines 1-3,
test/scenarios-rpm/presubmits/el98-src@standard1.sh lines 1-3, and
test/scenarios-rpm/presubmits/el98-src@standard2.sh lines 1-3: use the
/usr/bin/bash shebang and add set -euo pipefail immediately afterward in each
scenario script.
Source: Coding guidelines
Comment on lines
+23
to
+25
| local -r source_reponame=$(basename "${LOCAL_REPO}") | ||
| local -r source_repo_url="${WEB_SERVER_URL}/rpm-repos/${source_reponame}" | ||
| local -r target_version=$(local_rpm_version) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not mask command failures with local.
local -r name=$(command) masks the command exit status. A failed RPM version lookup can continue with an empty TARGET_VERSION.
test/scenarios-rpm/presubmits/el102-src@standard1.sh#L23-L25: declare and assignsource_reponameandtarget_versionseparately.test/scenarios-rpm/presubmits/el102-src@standard2.sh#L23-L25: declare and assignsource_reponameandtarget_versionseparately.test/scenarios-rpm/presubmits/el98-src@standard1.sh#L23-L25: declare and assignsource_reponameandtarget_versionseparately.test/scenarios-rpm/presubmits/el98-src@standard2.sh#L23-L25: declare and assignsource_reponameandtarget_versionseparately.
Proposed fix
- local -r source_reponame=$(basename "${LOCAL_REPO}")
+ local source_reponame
+ source_reponame="$(basename -- "${LOCAL_REPO}")"
+ readonly source_reponame
local -r source_repo_url="${WEB_SERVER_URL}/rpm-repos/${source_reponame}"
- local -r target_version=$(local_rpm_version)
+ local target_version
+ target_version="$(local_rpm_version)"
+ readonly target_versionAs per coding guidelines, Shell scripts must pass shellcheck.
📝 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.
Suggested change
| local -r source_reponame=$(basename "${LOCAL_REPO}") | |
| local -r source_repo_url="${WEB_SERVER_URL}/rpm-repos/${source_reponame}" | |
| local -r target_version=$(local_rpm_version) | |
| local source_reponame | |
| source_reponame="$(basename -- "${LOCAL_REPO}")" | |
| readonly source_reponame | |
| local -r source_repo_url="${WEB_SERVER_URL}/rpm-repos/${source_reponame}" | |
| local target_version | |
| target_version="$(local_rpm_version)" | |
| readonly target_version |
📍 Affects 4 files
test/scenarios-rpm/presubmits/el102-src@standard1.sh#L23-L25(this comment)test/scenarios-rpm/presubmits/el102-src@standard2.sh#L23-L25test/scenarios-rpm/presubmits/el98-src@standard1.sh#L23-L25test/scenarios-rpm/presubmits/el98-src@standard2.sh#L23-L25
🤖 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 `@test/scenarios-rpm/presubmits/el102-src`@standard1.sh around lines 23 - 25,
In test/scenarios-rpm/presubmits/el102-src@standard1.sh lines 23-25, declare
source_reponame and target_version separately from their command substitutions
so failures from basename and local_rpm_version propagate; preserve
source_repo_url and existing readonly behavior. Apply the same change to
test/scenarios-rpm/presubmits/el102-src@standard2.sh lines 23-25,
test/scenarios-rpm/presubmits/el98-src@standard1.sh lines 23-25, and
test/scenarios-rpm/presubmits/el98-src@standard2.sh lines 23-25, ensuring all
scripts pass shellcheck.
Source: Coding guidelines
| configure_rhocp_repo "${RHOCP_MINOR_Y1}" 4 "${PREVIOUS_MINOR_VERSION}" | ||
| configure_rhocp_repo "${RHOCP_MINOR_Y1_BETA}" 4 "${PREVIOUS_MINOR_VERSION}" | ||
| run_command_on_vm host1 "sudo subscription-manager release --set 10.2" | ||
| configure_fast_datapath_repo |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Configure a RHEL 10 Fast Datapath repository.
On RHEL 10, configure_fast_datapath_repo takes its non-9 branch. That branch configures a RHEL 9 CDN path. Update the helper for RHEL 10, or skip this repository when it is unsupported.
test/scenarios-rpm/presubmits/el102-src@standard1.sh#L32-L32: use a RHEL 10-compatible Fast Datapath configuration.test/scenarios-rpm/presubmits/el102-src@standard2.sh#L32-L32: use a RHEL 10-compatible Fast Datapath configuration.
📍 Affects 2 files
test/scenarios-rpm/presubmits/el102-src@standard1.sh#L32-L32(this comment)test/scenarios-rpm/presubmits/el102-src@standard2.sh#L32-L32
🤖 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 `@test/scenarios-rpm/presubmits/el102-src`@standard1.sh at line 32, Update
configure_fast_datapath_repo usage in
test/scenarios-rpm/presubmits/el102-src@standard1.sh lines 32-32 and
test/scenarios-rpm/presubmits/el102-src@standard2.sh lines 32-32 to use a RHEL
10-compatible Fast Datapath configuration, or skip the repository when
unsupported; do not allow the helper’s non-9 branch to configure the RHEL 9 CDN
path.
The RPM build path skips manage_composer_config.sh which normally installs nginx as a dependency. Install it explicitly before starting the web server. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
The CI environment uses a custom AWS CLI binary downloaded by
fetch_tools.sh, not the system aws command. Use ${AWSCLI} and
ensure it's downloaded before the S3 copy operations.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pre-commit.check-secrets: ENABLED
The RPM path skips configure-composer.sh which installs podman (needed by make rpm-podman) and parallel (needed by build_rpms.sh). Install them explicitly alongside nginx. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
…ent) parallel comes from EPEL, not base RHEL repos. Install EPEL first. podman is already installed on the EC2 instance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/bin/ci_phase_iso_build.sh`:
- Line 215: Remove the unconditional “|| true” from the EPEL installation
command in the CI setup flow so failures stop the build. If idempotency is
required, explicitly handle only the already-installed case while propagating
all other installation errors before the subsequent parallel dependency setup.
- Line 215: Update test/bin/ci_phase_iso_build.sh to use the required
#!/usr/bin/bash shebang and quote the %{rhel} RPM macro in the dnf installation
command as rpm -E '%{rhel}'. Validate the script with shellcheck
--external-sources.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 553ec018-f114-4a11-8ca2-8fdf9b92deb7
📒 Files selected for processing (1)
test/bin/ci_phase_iso_build.sh
|
|
||
| # RPM builds skip composer entirely — install required packages and start the web server. | ||
| if ${RPM_BUILDS} ; then | ||
| $(dry_run) sudo dnf install -y "https://dl.fedoraproject.org/pub/epel/epel-release-latest-$(rpm -E %{rhel}).noarch.rpm" || true |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not suppress EPEL installation failures.
|| true allows the required EPEL setup to fail silently. If the repository installation fails, Line 216 may fail later because parallel depends on EPEL. Remove || true, or handle only the already-installed case explicitly.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 215-215: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 215-215: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
🤖 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 `@test/bin/ci_phase_iso_build.sh` at line 215, Remove the unconditional “||
true” from the EPEL installation command in the CI setup flow so failures stop
the build. If idempotency is required, explicitly handle only the
already-installed case while propagating all other installation errors before
the subsequent parallel dependency setup.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
shellcheck test/bin/ci_phase_iso_build.shRepository: openshift/microshift
Length of output: 1114
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
printf '%s\n' '--- file header ---'
sed -n '1,35p' test/bin/ci_phase_iso_build.sh
printf '%s\n' '--- target and nearby commands ---'
sed -n '205,232p' test/bin/ci_phase_iso_build.sh
printf '%s\n' '--- shellcheck target diagnostics ---'
shellcheck test/bin/ci_phase_iso_build.sh || trueRepository: openshift/microshift
Length of output: 3437
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
printf '%s\n' '--- EPEL and RPM build references ---'
rg -n -C 3 'epel|RPM_BUILDS|ci_phase_iso_build|dnf install.*nginx|dnf install.*parallel' \
--glob '!_output/**' .
printf '%s\n' '--- ShellCheck on an in-memory quoted variant ---'
python3 - <<'PY'
from pathlib import Path
import subprocess
path = Path("test/bin/ci_phase_iso_build.sh")
source = path.read_text()
old = "rpm -E %{rhel}"
new = "rpm -E '%{rhel}'"
if old not in source:
raise SystemExit("target text not found")
result = subprocess.run(
["shellcheck", "-s", "bash", "-"],
input=source.replace(old, new),
text=True,
capture_output=True,
)
print(result.stdout, end="")
print(result.stderr, end="")
print(f"shellcheck_exit={result.returncode}")
PYRepository: openshift/microshift
Length of output: 11518
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
printf '%s\n' '--- dry_run definition and call sites ---'
rg -n -C 8 'dry_run[[:space:]]*\(|function[[:space:]]+dry_run|dry_run=' test/bin/ci_phase_iso_build.sh test/bin
printf '%s\n' '--- source directive and common.sh header ---'
sed -n '220,230p' test/bin/ci_phase_iso_build.sh
sed -n '1,45p' test/bin/common.sh
printf '%s\n' '--- ShellCheck with external sources enabled ---'
shellcheck -x test/bin/ci_phase_iso_build.sh || trueRepository: openshift/microshift
Length of output: 11787
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
printf '%s\n' '--- dry_run definition and call sites ---'
rg -n -C 8 'dry_run[[:space:]]*\(|function[[:space:]]+dry_run|dry_run=' \
test/bin/ci_phase_iso_build.sh test/bin
printf '%s\n' '--- source directive and common.sh header ---'
sed -n '220,230p' test/bin/ci_phase_iso_build.sh
sed -n '1,45p' test/bin/common.sh
printf '%s\n' '--- ShellCheck with external sources enabled ---'
shellcheck -x test/bin/ci_phase_iso_build.sh || trueRepository: openshift/microshift
Length of output: 11787
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
printf '%s\n' '--- ShellCheck configuration and invocations ---'
rg -n -C 3 'shellcheck|SC1091|external-sources' \
--glob '!.git/**' \
--glob '!vendor/**' \
--glob '!deps/**' \
--glob '!etcd/**' \
.
printf '%s\n' '--- relevant tracked configuration files ---'
git ls-files '*shellcheck*' '.shellcheckrc' '.github/**' '.ci/**' | head -80Repository: openshift/microshift
Length of output: 50377
Use the required shebang and fix the RPM macro quoting.
- Change line 1 to
#!/usr/bin/bash. - Change
rpm -E %{rhel}torpm -E '%{rhel}'to remove SC1083. - Validate with
shellcheck --external-sources test/bin/ci_phase_iso_build.sh.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 215-215: This { is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
[warning] 215-215: This } is literal. Check expression (missing ;/\n?) or quote it.
(SC1083)
🤖 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 `@test/bin/ci_phase_iso_build.sh` at line 215, Update
test/bin/ci_phase_iso_build.sh to use the required #!/usr/bin/bash shebang and
quote the %{rhel} RPM macro in the dnf installation command as rpm -E '%{rhel}'.
Validate the script with shellcheck --external-sources.
Sources: Coding guidelines, Linters/SAST tools
scenario.sh check_dependencies() unconditionally starts the mirror registry if not running, which fails for RPM scenarios because no container images list exists. Skip the mirror registry check when the scenario type is rpm. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
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
2 participants
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.
Summary
scenarios-rpmtype to the CI framework (common.sh)ci_phase_iso_build.sh— downloads installer ISOs from S3 build cache and builds RPMs from source, skipping composer and image builds entirelyci_phase_boot_and_test.shtest/scenarios-rpm/presubmits/:el98-src@standard1.sh,el98-src@standard2.shel102-src@standard1.sh,el102-src@standard2.shEach scenario boots a bare RHEL VM from a cached installer ISO, installs MicroShift via
dnf installfrom source-built RPMs, runs a standard test suite, and removes MicroShift.Companion PR
e2e-aws-tests-rpmProw job)Test plan
/test e2e-aws-tests-rpmon this PR (after release PR merges)🤖 Generated with Claude Code
Summary by CodeRabbit