Open
Conversation
|
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:
WalkthroughAdds Makefile CI targets to build and optionally push an enclave tarball, a new build script that creates/validates/pushes Changes
Sequence DiagramsequenceDiagram
participant CI as CI System
participant Script as build_tarball.sh
participant FS as Filesystem
participant Podman as Podman
participant ORAS as ORAS
participant Quay as Quay Registry
CI->>Script: invoke (ACTION=build or build-push)
Script->>Script: determine TAG (TARBALL_TAG or git SHA or "dev")
Script->>FS: write .version (TAG)
Script->>FS: create /tmp/enclave.tar.gz (exclude .git, .github, scripts, Makefile.ci)
Script->>Script: validate size, required entries, excluded paths, per-dir file-counts
alt ACTION == build-push
Script->>Podman: podman login quay.io (QUAY_USER/QUAY_TOKEN)
Podman->>Quay: authenticate
Script->>ORAS: push tarball as OCI layer to quay.io/...:TAG
ORAS->>Quay: upload layer
Script->>FS: remove enclave.tar.gz
else ACTION == build
Script->>FS: leave enclave.tar.gz in place
end
Script->>FS: remove .version
Script->>CI: exit (success or error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
docs/PROW_MIGRATION.md (1)
19-41: Consider adding a language identifier to the fenced code block.The ASCII diagram code block lacks a language identifier. While not functionally required, adding
textorplaintextsatisfies markdown linters.📝 Optional fix
-``` +```text Prow Build Farm (build01)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/PROW_MIGRATION.md` around lines 19 - 41, The fenced ASCII diagram block (the triple-backtick block containing the "Prow Build Farm (build01)" diagram) lacks a language identifier; update the opening fence from ``` to ```text (or ```plaintext) so the block becomes a labeled code block—this satisfies markdown linters and preserves the diagram formatting while making the block explicit.scripts/ci/prow_setup.sh (1)
50-55: Consider adding a root privilege check.The script runs
dnf installandsystemctl enablewhich require root privileges. While OFCIR machines are accessed as root, a guard could improve error messages when run incorrectly.💡 Optional: Add root check
+# Verify running as root +if [[ $EUID -ne 0 ]]; then + echo "Error: This script must be run as root" + exit 1 +fi + # Install only missing packages MISSING=()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/prow_setup.sh` around lines 50 - 55, Add an early root-privilege guard to the script so users get a clear error before any dnf install or systemctl enable calls run; e.g., at the top of prow_setup.sh check EUID (if [[ $EUID -ne 0 ]]; then echo "This script must be run as root" >&2; exit 1; fi) and place it before any use of the MISSING array, the dnf install command, or any systemctl enable/invocations.scripts/ci/prow_cleanup.sh (1)
62-65: Consider consolidating the podman commands.The sushy-tools cleanup runs
podman ps -a | grep ...twice. This can be simplified withpodman stopfollowed bypodman rmin one pipeline, or usingpodman rm -fwhich stops and removes in one step.♻️ Simplified approach
# Stop sushy-tools containers - podman ps -a | grep sushy-tools | awk '{print $1}' | xargs -r podman stop 2>/dev/null || true - podman ps -a | grep sushy-tools | awk '{print $1}' | xargs -r podman rm 2>/dev/null || true + podman ps -aq --filter name=sushy-tools | xargs -r podman rm -f 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/prow_cleanup.sh` around lines 62 - 65, The two consecutive cleanup pipelines that call podman twice for sushy-tools (the lines invoking "podman ps -a | grep sushy-tools | awk '{print $1}' | xargs -r podman stop" and the subsequent similar rm pipeline) should be consolidated: replace them with a single pipeline that lists matching container IDs and invokes "podman rm -f" once (or use "podman rm -f" with the same xargs pattern) so containers are stopped and removed in one step; update the commands in prow_cleanup.sh where those podman pipelines appear to use the single pipeline with "podman rm -f" and preserve the "2>/dev/null || true" behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ci/build_tarball.sh`:
- Around line 64-72: The EXCLUDED_PATHS array used to validate tarball contents
is missing "scripts/" so the check in the loop that iterates over EXCLUDED_PATHS
won't catch if scripts/ is accidentally included; update the EXCLUDED_PATHS
declaration to include "scripts/" (alongside ".git/" ".github/" "Makefile.ci")
so the existing grep-based check in the for loop correctly fails if scripts/
appears in /tmp/tarball-contents.txt.
In `@scripts/ci/prow_cleanup.sh`:
- Around line 66-71: The pool cleanup loop using virsh pool-list currently uses
a strict regex `^(eci-|ecd-|nc-|nd-)([a-f0-9]{8}|[0-9]{8})(-lz|_pool)?$` which
can miss pools; change the grep pattern in the for loop (the line that pipes
into grep in the virsh pool-list loop) to the same looser prefix matching used
by the VM/network cleanup (e.g., grep -E "eci-|ecd-|nc-|nd-") so pools are
discovered consistently, or alternatively add a short comment in the virsh
pool-list loop explaining why the strict pattern is required if you must keep
it.
In `@scripts/ci/prow_e2e.sh`:
- Around line 108-113: The current prow_e2e.sh snippet reads /tmp/working_dir
into WORKING_DIR but doesn't validate it; update the block that runs make -f
Makefile.ci setup-working-dir and the subsequent conditional that sets
WORKING_DIR to explicitly check that /tmp/working_dir exists and contains a
non-empty path, export WORKING_DIR when valid, and exit with a non-zero status
and a clear error message (using echo or similar) if the file is missing or
empty so downstream steps that rely on the WORKING_DIR variable don't run with
it unset.
- Around line 115-116: The current call to ./scripts/setup/allocate_subnet.sh
allocate runs in a subprocess so exported variables are lost; instead run its
output through eval in the current shell so the exported environment variables
persist. Modify the invocation in prow_e2e.sh to capture and evaluate the
script's stdout (e.g., run allocate_subnet.sh and eval its output) so the
exported variables from allocate_subnet.sh become available to the rest of
prow_e2e.sh.
In `@scripts/ci/prow_infra_verify.sh`:
- Around line 70-71: The call to ./scripts/setup/allocate_subnet.sh in
prow_infra_verify.sh discards the exported environment variables; change the
invocation to capture and evaluate the script's stdout so ENCLAVE_SUBNET_ID,
ENCLAVE_BMC_NETWORK, and ENCLAVE_CLUSTER_NETWORK are set in this process.
Specifically, execute the allocate_subnet.sh output with eval (or source its
generated exports) instead of running it as a plain subprocess so the exported
variables produced by allocate_subnet.sh:51-57 are available to subsequent
provisioning steps.
- Around line 63-68: The script may leave WORKING_DIR unset if /tmp/working_dir
is missing; update the block that runs make -f Makefile.ci setup-working-dir and
reads /tmp/working_dir to either (a) set a safe fallback (e.g., create and
export a temporary working dir) or (b) validate and fail fast with a clear error
before calling allocate_subnet.sh; modify the logic around the WORKING_DIR
assignment/export so WORKING_DIR is always exported or the script exits with a
descriptive message, referencing the existing WORKING_DIR variable and the make
target setup-working-dir and ensuring compatibility with allocate_subnet.sh
which requires WORKING_DIR.
---
Nitpick comments:
In `@docs/PROW_MIGRATION.md`:
- Around line 19-41: The fenced ASCII diagram block (the triple-backtick block
containing the "Prow Build Farm (build01)" diagram) lacks a language identifier;
update the opening fence from ``` to ```text (or ```plaintext) so the block
becomes a labeled code block—this satisfies markdown linters and preserves the
diagram formatting while making the block explicit.
In `@scripts/ci/prow_cleanup.sh`:
- Around line 62-65: The two consecutive cleanup pipelines that call podman
twice for sushy-tools (the lines invoking "podman ps -a | grep sushy-tools | awk
'{print $1}' | xargs -r podman stop" and the subsequent similar rm pipeline)
should be consolidated: replace them with a single pipeline that lists matching
container IDs and invokes "podman rm -f" once (or use "podman rm -f" with the
same xargs pattern) so containers are stopped and removed in one step; update
the commands in prow_cleanup.sh where those podman pipelines appear to use the
single pipeline with "podman rm -f" and preserve the "2>/dev/null || true"
behavior.
In `@scripts/ci/prow_setup.sh`:
- Around line 50-55: Add an early root-privilege guard to the script so users
get a clear error before any dnf install or systemctl enable calls run; e.g., at
the top of prow_setup.sh check EUID (if [[ $EUID -ne 0 ]]; then echo "This
script must be run as root" >&2; exit 1; fi) and place it before any use of the
MISSING array, the dnf install command, or any systemctl enable/invocations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1311acce-9a9e-4b6a-a09c-34a3e53dab9f
📒 Files selected for processing (10)
Makefile.ciOWNERSdocs/OFCIR_SETUP.mddocs/PROW_MIGRATION.mdscripts/ci/build_tarball.shscripts/ci/prow_cleanup.shscripts/ci/prow_disconnected_dry_run.shscripts/ci/prow_e2e.shscripts/ci/prow_infra_verify.shscripts/ci/prow_setup.sh
Comment on lines
+66
to
+71
| # Clean up libvirt pools | ||
| for pool in $(virsh pool-list --all --name | grep -E "^(eci-|ecd-|nc-|nd-)([a-f0-9]{8}|[0-9]{8})(-lz|_pool)?$" || true); do | ||
| echo "Removing pool: $pool" | ||
| virsh pool-destroy "$pool" 2>/dev/null || true | ||
| virsh pool-undefine "$pool" 2>/dev/null || true | ||
| done |
There was a problem hiding this comment.
Pool cleanup pattern is stricter than VM/network patterns.
The pool regex ^(eci-|ecd-|nc-|nd-)([a-f0-9]{8}|[0-9]{8})(-lz|_pool)?$ requires exact format matching, while VM/network cleanup (lines 40, 47) uses loose prefix matching with grep -E "eci-|ecd-|...". This inconsistency could cause pools to be missed if their naming doesn't exactly match the strict pattern.
Consider aligning the patterns or documenting why pools need stricter matching.
🔧 Option: Use consistent loose matching for pools
# Clean up libvirt pools
- for pool in $(virsh pool-list --all --name | grep -E "^(eci-|ecd-|nc-|nd-)([a-f0-9]{8}|[0-9]{8})(-lz|_pool)?$" || true); do
+ for pool in $(virsh pool-list --all --name | grep -E "eci-|ecd-|enclave-ci-|enclave-test" || true); do
echo "Removing pool: $pool"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/prow_cleanup.sh` around lines 66 - 71, The pool cleanup loop using
virsh pool-list currently uses a strict regex
`^(eci-|ecd-|nc-|nd-)([a-f0-9]{8}|[0-9]{8})(-lz|_pool)?$` which can miss pools;
change the grep pattern in the for loop (the line that pipes into grep in the
virsh pool-list loop) to the same looser prefix matching used by the VM/network
cleanup (e.g., grep -E "eci-|ecd-|nc-|nd-") so pools are discovered
consistently, or alternatively add a short comment in the virsh pool-list loop
explaining why the strict pattern is required if you must keep it.
Comment on lines
+108
to
+113
| # Setup working directory | ||
| make -f Makefile.ci setup-working-dir | ||
| if [[ -f /tmp/working_dir ]]; then | ||
| WORKING_DIR=$(cat /tmp/working_dir) | ||
| export WORKING_DIR | ||
| fi |
There was a problem hiding this comment.
Conditional WORKING_DIR export may leave it unset.
Same issue as in prow_infra_verify.sh: if /tmp/working_dir doesn't exist, downstream scripts requiring WORKING_DIR will fail.
🛡️ Proposed fix: add validation
# Setup working directory
make -f Makefile.ci setup-working-dir
if [[ -f /tmp/working_dir ]]; then
WORKING_DIR=$(cat /tmp/working_dir)
export WORKING_DIR
+else
+ echo "ERROR: /tmp/working_dir not created by setup-working-dir"
+ exit 1
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/prow_e2e.sh` around lines 108 - 113, The current prow_e2e.sh
snippet reads /tmp/working_dir into WORKING_DIR but doesn't validate it; update
the block that runs make -f Makefile.ci setup-working-dir and the subsequent
conditional that sets WORKING_DIR to explicitly check that /tmp/working_dir
exists and contains a non-empty path, export WORKING_DIR when valid, and exit
with a non-zero status and a clear error message (using echo or similar) if the
file is missing or empty so downstream steps that rely on the WORKING_DIR
variable don't run with it unset.
Comment on lines
+115
to
+116
| # Allocate subnet | ||
| ./scripts/setup/allocate_subnet.sh allocate |
There was a problem hiding this comment.
Critical: Subnet allocation environment variables are not captured.
Same issue as in prow_infra_verify.sh: the exported variables from allocate_subnet.sh are lost because it runs in a subprocess.
🐛 Proposed fix: capture the output with eval
# Allocate subnet
-./scripts/setup/allocate_subnet.sh allocate
+eval "$(./scripts/setup/allocate_subnet.sh allocate)"📝 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
| # Allocate subnet | |
| ./scripts/setup/allocate_subnet.sh allocate | |
| # Allocate subnet | |
| eval "$(./scripts/setup/allocate_subnet.sh allocate)" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/prow_e2e.sh` around lines 115 - 116, The current call to
./scripts/setup/allocate_subnet.sh allocate runs in a subprocess so exported
variables are lost; instead run its output through eval in the current shell so
the exported environment variables persist. Modify the invocation in prow_e2e.sh
to capture and evaluate the script's stdout (e.g., run allocate_subnet.sh and
eval its output) so the exported variables from allocate_subnet.sh become
available to the rest of prow_e2e.sh.
Comment on lines
+63
to
+68
| # Setup working directory | ||
| make -f Makefile.ci setup-working-dir | ||
| if [[ -f /tmp/working_dir ]]; then | ||
| WORKING_DIR=$(cat /tmp/working_dir) | ||
| export WORKING_DIR | ||
| fi |
There was a problem hiding this comment.
Conditional WORKING_DIR export may leave it unset.
If /tmp/working_dir doesn't exist after setup-working-dir, WORKING_DIR remains unset. Per context snippet 1, allocate_subnet.sh requires WORKING_DIR to be set (it has :?WORKING_DIR environment variable is required). Consider adding a fallback or validation.
🛡️ Proposed fix: add fallback or validation
# Setup working directory
make -f Makefile.ci setup-working-dir
if [[ -f /tmp/working_dir ]]; then
WORKING_DIR=$(cat /tmp/working_dir)
export WORKING_DIR
+else
+ echo "ERROR: /tmp/working_dir not created by setup-working-dir"
+ exit 1
fi📝 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
| # Setup working directory | |
| make -f Makefile.ci setup-working-dir | |
| if [[ -f /tmp/working_dir ]]; then | |
| WORKING_DIR=$(cat /tmp/working_dir) | |
| export WORKING_DIR | |
| fi | |
| # Setup working directory | |
| make -f Makefile.ci setup-working-dir | |
| if [[ -f /tmp/working_dir ]]; then | |
| WORKING_DIR=$(cat /tmp/working_dir) | |
| export WORKING_DIR | |
| else | |
| echo "ERROR: /tmp/working_dir not created by setup-working-dir" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/prow_infra_verify.sh` around lines 63 - 68, The script may leave
WORKING_DIR unset if /tmp/working_dir is missing; update the block that runs
make -f Makefile.ci setup-working-dir and reads /tmp/working_dir to either (a)
set a safe fallback (e.g., create and export a temporary working dir) or (b)
validate and fail fast with a clear error before calling allocate_subnet.sh;
modify the logic around the WORKING_DIR assignment/export so WORKING_DIR is
always exported or the script exits with a descriptive message, referencing the
existing WORKING_DIR variable and the make target setup-working-dir and ensuring
compatibility with allocate_subnet.sh which requires WORKING_DIR.
Comment on lines
+70
to
+71
| # Allocate subnet | ||
| ./scripts/setup/allocate_subnet.sh allocate |
There was a problem hiding this comment.
Critical: Subnet allocation environment variables are not captured.
The allocate_subnet.sh script outputs environment variable exports to stdout (per scripts/setup/allocate_subnet.sh:51-57), but calling it as a subprocess discards them. The variables ENCLAVE_SUBNET_ID, ENCLAVE_BMC_NETWORK, and ENCLAVE_CLUSTER_NETWORK will not be set in this script, likely causing downstream infrastructure provisioning to fail or use incorrect network values.
🐛 Proposed fix: capture the output with eval
# Allocate subnet
-./scripts/setup/allocate_subnet.sh allocate
+eval "$(./scripts/setup/allocate_subnet.sh allocate)"📝 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
| # Allocate subnet | |
| ./scripts/setup/allocate_subnet.sh allocate | |
| # Allocate subnet | |
| eval "$(./scripts/setup/allocate_subnet.sh allocate)" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/prow_infra_verify.sh` around lines 70 - 71, The call to
./scripts/setup/allocate_subnet.sh in prow_infra_verify.sh discards the exported
environment variables; change the invocation to capture and evaluate the
script's stdout so ENCLAVE_SUBNET_ID, ENCLAVE_BMC_NETWORK, and
ENCLAVE_CLUSTER_NETWORK are set in this process. Specifically, execute the
allocate_subnet.sh output with eval (or source its generated exports) instead of
running it as a plain subprocess so the exported variables produced by
allocate_subnet.sh:51-57 are available to subsequent provisioning steps.
eliorerz
force-pushed
the
prow-ci-migration
branch
from
e87a9e5 to
ad3e8e4
Compare
eliorerz
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
scripts/ci/prow_e2e.sh (2)
110-113:⚠️ Potential issue | 🟡 MinorRequire non-empty
/tmp/working_dir.At Line 110, the script should error out if
/tmp/working_diris missing/empty to avoid running with unsetWORKING_DIR.🛠️ Proposed fix
make -f Makefile.ci setup-working-dir -if [[ -f /tmp/working_dir ]]; then - WORKING_DIR=$(cat /tmp/working_dir) - export WORKING_DIR -fi +if [[ -s /tmp/working_dir ]]; then + WORKING_DIR=$(cat /tmp/working_dir) + export WORKING_DIR +else + echo "ERROR: /tmp/working_dir missing or empty after setup-working-dir" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/prow_e2e.sh` around lines 110 - 113, The current block sets WORKING_DIR if /tmp/working_dir exists but doesn't ensure it's non-empty; change the logic to require a non-empty file and fail fast: test /tmp/working_dir is non-empty (e.g., -s) or read WORKING_DIR and immediately validate that WORKING_DIR is not empty, and if it is missing/empty print an error and exit 1 so the rest of prow_e2e.sh never runs with an unset WORKING_DIR.
116-116:⚠️ Potential issue | 🔴 CriticalEvaluate subnet exports in-process.
At Line 116, direct execution loses exported subnet variables from
allocate_subnet.sh; later targets won’t see them.🐛 Proposed fix
-./scripts/setup/allocate_subnet.sh allocate +eval "$(./scripts/setup/allocate_subnet.sh allocate)"#!/bin/bash # Verify caller invocation style and exporter behavior. rg -n 'allocate_subnet\.sh allocate' scripts/ci/prow_e2e.sh -C2 rg -n 'cat <<EOF|export ENCLAVE_SUBNET_ID|export ENCLAVE_BMC_NETWORK|export ENCLAVE_CLUSTER_NETWORK' scripts/setup/allocate_subnet.sh -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/prow_e2e.sh` at line 116, The line that runs "./scripts/setup/allocate_subnet.sh allocate" spawns a subshell so exported variables from allocate_subnet.sh (e.g. ENCLAVE_SUBNET_ID, ENCLAVE_BMC_NETWORK, ENCLAVE_CLUSTER_NETWORK) are lost; change the call in prow_e2e.sh to source the script in-process (use "source" or "." with the same arguments) so the export lines inside allocate_subnet.sh take effect in the caller and later targets can read those environment variables. Ensure the invocation preserves existing arguments ("allocate") and verify exported names in allocate_subnet.sh match the variables you rely on.scripts/ci/prow_infra_verify.sh (2)
65-68:⚠️ Potential issue | 🟡 MinorFail fast when
WORKING_DIRis missing or empty.At Line 65, the script silently continues if
/tmp/working_diris absent (or empty), which pushes failure to later steps.🛠️ Proposed fix
make -f Makefile.ci setup-working-dir -if [[ -f /tmp/working_dir ]]; then - WORKING_DIR=$(cat /tmp/working_dir) - export WORKING_DIR -fi +if [[ -s /tmp/working_dir ]]; then + WORKING_DIR=$(cat /tmp/working_dir) + export WORKING_DIR +else + echo "ERROR: /tmp/working_dir missing or empty after setup-working-dir" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/prow_infra_verify.sh` around lines 65 - 68, The script currently silently skips setting WORKING_DIR if /tmp/working_dir is missing or empty; update the block that reads /tmp/working_dir to verify the file exists and contains a non-empty path, and if not, print an error to stderr (include context like "WORKING_DIR missing or empty") and exit with a non-zero status; when valid, read into WORKING_DIR and export it as before (reference the WORKING_DIR variable and the /tmp/working_dir file in your change).
71-71:⚠️ Potential issue | 🔴 CriticalCapture subnet exports in the current shell.
At Line 71, running
allocate_subnet.shas a subprocess discards the exported subnet variables needed by later targets.🐛 Proposed fix
-./scripts/setup/allocate_subnet.sh allocate +eval "$(./scripts/setup/allocate_subnet.sh allocate)"#!/bin/bash # Verify invocation is currently subprocess-only and exporter prints env assignments. rg -n 'allocate_subnet\.sh allocate' scripts/ci/prow_infra_verify.sh -C2 rg -n 'WORKING_DIR environment variable is required|cat <<EOF|export ENCLAVE_SUBNET_ID|export ENCLAVE_BMC_NETWORK|export ENCLAVE_CLUSTER_NETWORK' scripts/setup/allocate_subnet.sh -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/prow_infra_verify.sh` at line 71, The current invocation in prow_infra_verify.sh runs ./scripts/setup/allocate_subnet.sh allocate as a subprocess so its exported environment variables (e.g., ENCLAVE_SUBNET_ID, ENCLAVE_BMC_NETWORK, ENCLAVE_CLUSTER_NETWORK) are lost; change the call so the script's stdout is evaluated in the current shell (i.e., capture and eval the exporter output of allocate_subnet.sh) so those exports are available to later targets instead of running it as a standalone subprocess.scripts/ci/build_tarball.sh (1)
65-72:⚠️ Potential issue | 🟡 MinorAdd
scripts/to excluded-path validation.Line 65 is missing
scripts/inEXCLUDED_PATHS, even thoughtarexcludesscriptsat Line 21. The validation should enforce that contract too.🛠️ Proposed fix
-EXCLUDED_PATHS=(".git/" ".github/" "Makefile.ci") +EXCLUDED_PATHS=(".git/" ".github/" "Makefile.ci" "scripts/")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/build_tarball.sh` around lines 65 - 72, The exclusion validation in build_tarball.sh uses the EXCLUDED_PATHS array but omits "scripts/", causing a mismatch with the tar exclusion earlier; update the EXCLUDED_PATHS variable in scripts/ci/build_tarball.sh to include "scripts/" (so the for-loop that checks EXCLUDED_PATHS will fail if scripts/ is present in /tmp/tarball-contents.txt), ensuring the validation matches the tar exclude logic that excludes scripts.
🧹 Nitpick comments (1)
scripts/ci/prow_setup.sh (1)
62-69: Ensure services are enabled, not only active.At Line 63, the check only verifies
is-active. A service can be active but not enabled, which breaks persistence expectations.♻️ Suggested adjustment
for svc in libvirtd firewalld NetworkManager; do - if ! systemctl is-active --quiet "$svc"; then + if ! systemctl is-enabled --quiet "$svc" || ! systemctl is-active --quiet "$svc"; then systemctl enable --now "$svc" echo " Started $svc" else echo " $svc is already running" fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/prow_setup.sh` around lines 62 - 69, The script currently checks services with systemctl is-active but doesn't verify they are enabled for boot persistence; update the loop that iterates over svc (libvirtd, firewalld, NetworkManager) to also check systemctl is-enabled --quiet "$svc" and if not enabled run systemctl enable "$svc" (or systemctl enable --now when starting) so services become persistent across reboots; modify the block that calls systemctl is-active --quiet "$svc" to conditionally run systemctl enable --now "$svc" when inactive, and if active but not enabled run systemctl enable "$svc" and log the action.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/PROW_MIGRATION.md`:
- Line 83: The job-flow test step lists two scripts but uses inconsistent paths;
update the entry that references `prow_disconnected_dry_run.sh` to use the same
`scripts/ci/` prefix as the other script so both read
`scripts/ci/prow_disconnected_dry_run.sh`, ensuring the `Test step - SSH` line
references `scripts/ci/prow_infra_verify.sh` and
`scripts/ci/prow_disconnected_dry_run.sh`.
- Around line 19-41: The fenced code block that begins with "Prow Build Farm
(build01)" is missing a language hint, causing markdownlint MD040; update the
opening fence (the triple backticks before that diagram) to include a language
tag (e.g., change ``` to ```text) so the block is explicitly marked as text.
In `@scripts/ci/prow_disconnected_dry_run.sh`:
- Around line 61-64: The script reads /tmp/working_dir into WORKING_DIR but
doesn't validate it; change the block around WORKING_DIR to first ensure the
file exists and is non-empty (e.g., test -s /tmp/working_dir), then read and
trim whitespace into WORKING_DIR, export it, and if the file is missing or the
resulting WORKING_DIR is empty print a clear error to stderr and exit with a
non-zero code; reference the WORKING_DIR variable and the existing file check
block so the check replaces the current if [[ -f /tmp/working_dir ]] { ... }
logic.
- Line 67: The allocate_subnet.sh call runs in a subprocess so its exported
variables are lost; change the invocation at the spot that currently runs
"./scripts/setup/allocate_subnet.sh allocate" so the caller captures and exports
the variables (e.g. have allocate_subnet.sh emit shell assignments and use eval
to source them, or have it write an env file and source that file) before
running "make -f Makefile.ci environment" / configure_devscripts.sh so
ENCLAVE_SUBNET_ID, ENCLAVE_BMC_NETWORK and ENCLAVE_CLUSTER_NETWORK are set in
the current environment.
---
Duplicate comments:
In `@scripts/ci/build_tarball.sh`:
- Around line 65-72: The exclusion validation in build_tarball.sh uses the
EXCLUDED_PATHS array but omits "scripts/", causing a mismatch with the tar
exclusion earlier; update the EXCLUDED_PATHS variable in
scripts/ci/build_tarball.sh to include "scripts/" (so the for-loop that checks
EXCLUDED_PATHS will fail if scripts/ is present in /tmp/tarball-contents.txt),
ensuring the validation matches the tar exclude logic that excludes scripts.
In `@scripts/ci/prow_e2e.sh`:
- Around line 110-113: The current block sets WORKING_DIR if /tmp/working_dir
exists but doesn't ensure it's non-empty; change the logic to require a
non-empty file and fail fast: test /tmp/working_dir is non-empty (e.g., -s) or
read WORKING_DIR and immediately validate that WORKING_DIR is not empty, and if
it is missing/empty print an error and exit 1 so the rest of prow_e2e.sh never
runs with an unset WORKING_DIR.
- Line 116: The line that runs "./scripts/setup/allocate_subnet.sh allocate"
spawns a subshell so exported variables from allocate_subnet.sh (e.g.
ENCLAVE_SUBNET_ID, ENCLAVE_BMC_NETWORK, ENCLAVE_CLUSTER_NETWORK) are lost;
change the call in prow_e2e.sh to source the script in-process (use "source" or
"." with the same arguments) so the export lines inside allocate_subnet.sh take
effect in the caller and later targets can read those environment variables.
Ensure the invocation preserves existing arguments ("allocate") and verify
exported names in allocate_subnet.sh match the variables you rely on.
In `@scripts/ci/prow_infra_verify.sh`:
- Around line 65-68: The script currently silently skips setting WORKING_DIR if
/tmp/working_dir is missing or empty; update the block that reads
/tmp/working_dir to verify the file exists and contains a non-empty path, and if
not, print an error to stderr (include context like "WORKING_DIR missing or
empty") and exit with a non-zero status; when valid, read into WORKING_DIR and
export it as before (reference the WORKING_DIR variable and the /tmp/working_dir
file in your change).
- Line 71: The current invocation in prow_infra_verify.sh runs
./scripts/setup/allocate_subnet.sh allocate as a subprocess so its exported
environment variables (e.g., ENCLAVE_SUBNET_ID, ENCLAVE_BMC_NETWORK,
ENCLAVE_CLUSTER_NETWORK) are lost; change the call so the script's stdout is
evaluated in the current shell (i.e., capture and eval the exporter output of
allocate_subnet.sh) so those exports are available to later targets instead of
running it as a standalone subprocess.
---
Nitpick comments:
In `@scripts/ci/prow_setup.sh`:
- Around line 62-69: The script currently checks services with systemctl
is-active but doesn't verify they are enabled for boot persistence; update the
loop that iterates over svc (libvirtd, firewalld, NetworkManager) to also check
systemctl is-enabled --quiet "$svc" and if not enabled run systemctl enable
"$svc" (or systemctl enable --now when starting) so services become persistent
across reboots; modify the block that calls systemctl is-active --quiet "$svc"
to conditionally run systemctl enable --now "$svc" when inactive, and if active
but not enabled run systemctl enable "$svc" and log the action.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 506be093-fb8e-4847-8a67-a250d557d182
📒 Files selected for processing (10)
Makefile.ciOWNERSdocs/OFCIR_SETUP.mddocs/PROW_MIGRATION.mdscripts/ci/build_tarball.shscripts/ci/prow_cleanup.shscripts/ci/prow_disconnected_dry_run.shscripts/ci/prow_e2e.shscripts/ci/prow_infra_verify.shscripts/ci/prow_setup.sh
✅ Files skipped from review due to trivial changes (2)
- OWNERS
- docs/OFCIR_SETUP.md
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile.ci
- scripts/ci/prow_cleanup.sh
Comment on lines
+19
to
+41
| ``` | ||
| Prow Build Farm (build01) | ||
| | | ||
| | ofcir-acquire → gets machine IP + SSH key | ||
| | | ||
| v | ||
| e2e-setup step (SSH into machine) | ||
| | | ||
| | SSH → clone repo, install deps, transfer pull secret | ||
| | | ||
| v | ||
| test step (SSH into machine) | ||
| | | ||
| | SSH → run scripts/ci/prow_*.sh on machine | ||
| | | ||
| v | ||
| e2e-teardown step (SSH into machine) | ||
| | | ||
| | SSH → collect artifacts, run cleanup | ||
| | | ||
| v | ||
| ofcir-release → return machine to pool | ||
| ``` |
There was a problem hiding this comment.
Add a language hint to the fenced code block.
Line 19 starts a fenced block without language, triggering markdownlint MD040.
🛠️ Proposed fix
-```
+```text
Prow Build Farm (build01)
|
| ofcir-acquire → gets machine IP + SSH key
|
@@
ofcir-release → return machine to pool</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/PROW_MIGRATION.md around lines 19 - 41, The fenced code block that
begins with "Prow Build Farm (build01)" is missing a language hint, causing
markdownlint MD040; update the opening fence (the triple backticks before that
diagram) to include a language tag (e.g., change totext) so the block is
explicitly marked as text.
</details>
<!-- fingerprinting:phantom:poseidon:hawk:e4254f19-5aab-4204-8c15-af8fd2edd39d -->
<!-- This is an auto-generated comment by CodeRabbit -->
| **Job flow:** | ||
| 1. `ofcir-acquire` - get a machine from the pool | ||
| 2. `e2e-setup` - SSH: clone repo, install deps, transfer pull secret | ||
| 3. Test step - SSH: run `scripts/ci/prow_infra_verify.sh` or `prow_disconnected_dry_run.sh` |
There was a problem hiding this comment.
Use a consistent script path in the job-flow step.
At Line 83, prow_disconnected_dry_run.sh is missing the scripts/ci/ prefix while the other script includes it.
🛠️ Proposed fix
-3. Test step - SSH: run `scripts/ci/prow_infra_verify.sh` or `prow_disconnected_dry_run.sh`
+3. Test step - SSH: run `scripts/ci/prow_infra_verify.sh` or `scripts/ci/prow_disconnected_dry_run.sh`📝 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
| 3. Test step - SSH: run `scripts/ci/prow_infra_verify.sh` or `prow_disconnected_dry_run.sh` | |
| 3. Test step - SSH: run `scripts/ci/prow_infra_verify.sh` or `scripts/ci/prow_disconnected_dry_run.sh` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/PROW_MIGRATION.md` at line 83, The job-flow test step lists two scripts
but uses inconsistent paths; update the entry that references
`prow_disconnected_dry_run.sh` to use the same `scripts/ci/` prefix as the other
script so both read `scripts/ci/prow_disconnected_dry_run.sh`, ensuring the
`Test step - SSH` line references `scripts/ci/prow_infra_verify.sh` and
`scripts/ci/prow_disconnected_dry_run.sh`.
Comment on lines
+61
to
+64
| if [[ -f /tmp/working_dir ]]; then | ||
| WORKING_DIR=$(cat /tmp/working_dir) | ||
| export WORKING_DIR | ||
| fi |
There was a problem hiding this comment.
Validate WORKING_DIR before continuing.
Line 61 should fail if /tmp/working_dir is missing or empty; otherwise downstream setup fails with less actionable errors.
🛠️ Proposed fix
make -f Makefile.ci setup-working-dir
-if [[ -f /tmp/working_dir ]]; then
- WORKING_DIR=$(cat /tmp/working_dir)
- export WORKING_DIR
-fi
+if [[ -s /tmp/working_dir ]]; then
+ WORKING_DIR=$(cat /tmp/working_dir)
+ export WORKING_DIR
+else
+ echo "ERROR: /tmp/working_dir missing or empty after setup-working-dir"
+ exit 1
+fi📝 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
| if [[ -f /tmp/working_dir ]]; then | |
| WORKING_DIR=$(cat /tmp/working_dir) | |
| export WORKING_DIR | |
| fi | |
| if [[ -s /tmp/working_dir ]]; then | |
| WORKING_DIR=$(cat /tmp/working_dir) | |
| export WORKING_DIR | |
| else | |
| echo "ERROR: /tmp/working_dir missing or empty after setup-working-dir" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/prow_disconnected_dry_run.sh` around lines 61 - 64, The script
reads /tmp/working_dir into WORKING_DIR but doesn't validate it; change the
block around WORKING_DIR to first ensure the file exists and is non-empty (e.g.,
test -s /tmp/working_dir), then read and trim whitespace into WORKING_DIR,
export it, and if the file is missing or the resulting WORKING_DIR is empty
print a clear error to stderr and exit with a non-zero code; reference the
WORKING_DIR variable and the existing file check block so the check replaces the
current if [[ -f /tmp/working_dir ]] { ... } logic.
| fi | ||
|
|
||
| # Allocate subnet | ||
| ./scripts/setup/allocate_subnet.sh allocate |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm caller uses subprocess and callee emits export statements.
rg -n 'allocate_subnet\.sh allocate' scripts/ci/prow_disconnected_dry_run.sh -C2
rg -n 'cat <<EOF|export ENCLAVE_SUBNET_ID|export ENCLAVE_BMC_NETWORK|export ENCLAVE_CLUSTER_NETWORK' scripts/setup/allocate_subnet.sh -C2Repository: rh-ecosystem-edge/enclave
Length of output: 501
🏁 Script executed:
cat -n scripts/ci/prow_disconnected_dry_run.sh | sed -n '60,100p'Repository: rh-ecosystem-edge/enclave
Length of output: 1031
🏁 Script executed:
# Check if these environment variables are referenced later in the script
rg 'ENCLAVE_SUBNET_ID|ENCLAVE_BMC_NETWORK|ENCLAVE_CLUSTER_NETWORK' scripts/ci/prow_disconnected_dry_run.shRepository: rh-ecosystem-edge/enclave
Length of output: 51
🏁 Script executed:
# Check the full allocate function in allocate_subnet.sh to understand its intended usage
cat -n scripts/setup/allocate_subnet.sh | head -30Repository: rh-ecosystem-edge/enclave
Length of output: 1296
🏁 Script executed:
rg 'ENCLAVE_SUBNET_ID|ENCLAVE_BMC_NETWORK|ENCLAVE_CLUSTER_NETWORK' Makefile.ci -A2 -B2Repository: rh-ecosystem-edge/enclave
Length of output: 51
🏁 Script executed:
# Check if these variables are passed to make or sourced from environment
grep -n "export" scripts/ci/prow_disconnected_dry_run.sh | head -20Repository: rh-ecosystem-edge/enclave
Length of output: 354
🏁 Script executed:
rg 'ENCLAVE_SUBNET_ID|ENCLAVE_BMC_NETWORK|ENCLAVE_CLUSTER_NETWORK' --type sh --type makefileRepository: rh-ecosystem-edge/enclave
Length of output: 101
🏁 Script executed:
# Check what the allocate function actually returns and if it's consumed
cat -n scripts/setup/allocate_subnet.sh | tail -20Repository: rh-ecosystem-edge/enclave
Length of output: 552
🏁 Script executed:
rg 'ENCLAVE_SUBNET_ID|ENCLAVE_BMC_NETWORK|ENCLAVE_CLUSTER_NETWORK' --type shRepository: rh-ecosystem-edge/enclave
Length of output: 1583
🏁 Script executed:
# Find the allocate_subnet function definition
sed -n '80,170p' scripts/setup/allocate_subnet.shRepository: rh-ecosystem-edge/enclave
Length of output: 2688
🏁 Script executed:
grep -n "environment:" Makefile.ci -A 10Repository: rh-ecosystem-edge/enclave
Length of output: 489
🏁 Script executed:
# Check if configure_devscripts.sh is called from make or scripts
rg 'configure_devscripts.sh' Makefile.ciRepository: rh-ecosystem-edge/enclave
Length of output: 110
Subnet environment variables are lost due to subprocess execution.
At line 67, allocate_subnet.sh allocate runs in a subprocess; the export statements it outputs to stdout are never captured. When line 70 then calls make -f Makefile.ci environment, which invokes configure_devscripts.sh, the required ENCLAVE_SUBNET_ID, ENCLAVE_BMC_NETWORK, and ENCLAVE_CLUSTER_NETWORK variables are not set in the environment, causing the network configuration to fail or use incorrect defaults.
🐛 Proposed fix
-./scripts/setup/allocate_subnet.sh allocate
+eval "$(./scripts/setup/allocate_subnet.sh allocate)"📝 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
| ./scripts/setup/allocate_subnet.sh allocate | |
| eval "$(./scripts/setup/allocate_subnet.sh allocate)" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/prow_disconnected_dry_run.sh` at line 67, The allocate_subnet.sh
call runs in a subprocess so its exported variables are lost; change the
invocation at the spot that currently runs "./scripts/setup/allocate_subnet.sh
allocate" so the caller captures and exports the variables (e.g. have
allocate_subnet.sh emit shell assignments and use eval to source them, or have
it write an env file and source that file) before running "make -f Makefile.ci
environment" / configure_devscripts.sh so ENCLAVE_SUBNET_ID, ENCLAVE_BMC_NETWORK
and ENCLAVE_CLUSTER_NETWORK are set in the current environment.
ad3e8e4 to
a2c29a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/ci/build_tarball.sh (1)
19-21: Add EXIT cleanup for.versionand temp content file.With
set -euo pipefail, failures before Line 91 leave.versionand/tmp/tarball-contents.txtbehind. Add a trap-based cleanup to make failures idempotent.Proposed refactor
echo "Building distribution tarball..." echo -n "$TAG" > .version +TARBALL_CONTENTS_FILE="$(mktemp /tmp/tarball-contents.XXXXXX)" +cleanup() { + rm -f .version "$TARBALL_CONTENTS_FILE" +} +trap cleanup EXIT tar --exclude='.git' --exclude='.gitignore' --exclude='.github' --exclude='scripts' \ @@ -tar -tzf "$TARBALL" > /tmp/tarball-contents.txt +tar -tzf "$TARBALL" > "$TARBALL_CONTENTS_FILE" @@ - if ! grep -q "^\./${file}$" /tmp/tarball-contents.txt; then + if ! grep -q "^\./${file}$" "$TARBALL_CONTENTS_FILE"; then @@ -rm -f .version +# handled by EXIT trapAlso applies to: 38-39, 91-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/build_tarball.sh` around lines 19 - 21, The script currently writes .version and uses /tmp/tarball-contents.txt but with set -euo pipefail failures leave those artifacts; add a trap handler (trap '... cleanup ...' EXIT) near the top of scripts/ci/build_tarball.sh that removes .version and /tmp/tarball-contents.txt (and any other temporary files created by this script) on exit, and ensure the cleanup handler is idempotent and does not error if files are missing; reference the TAG write to .version and any code that generates /tmp/tarball-contents.txt so the trap covers all temporary artifacts created before line 91.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ci/build_tarball.sh`:
- Around line 21-24: The tar invocation currently archives "." and can include a
previously generated archive, leading to recursion; modify the tar command that
uses the TARBALL variable (the line with tar --exclude=... -czvf "/tmp/$TARBALL"
.) to add an exclude for the target archive (e.g., --exclude="$TARBALL" or
--exclude='enclave.tar.gz') so the created file is not picked up, ensuring the
subsequent mv "/tmp/$TARBALL" . moves a clean archive.
---
Nitpick comments:
In `@scripts/ci/build_tarball.sh`:
- Around line 19-21: The script currently writes .version and uses
/tmp/tarball-contents.txt but with set -euo pipefail failures leave those
artifacts; add a trap handler (trap '... cleanup ...' EXIT) near the top of
scripts/ci/build_tarball.sh that removes .version and /tmp/tarball-contents.txt
(and any other temporary files created by this script) on exit, and ensure the
cleanup handler is idempotent and does not error if files are missing; reference
the TAG write to .version and any code that generates /tmp/tarball-contents.txt
so the trap covers all temporary artifacts created before line 91.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cd2ad49-659e-4459-bc5e-0a6834124de5
📒 Files selected for processing (3)
Makefile.ciOWNERSscripts/ci/build_tarball.sh
✅ Files skipped from review due to trivial changes (1)
- OWNERS
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile.ci
eliorerz
force-pushed
the
prow-ci-migration
branch
from
a2c29a8 to
333c9dd
Compare
eliorerz
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Makefile.ci (1)
245-254: Add tarball targets tohelpoutput for discoverability.Lines 248-253 expose public CI entrypoints, but they are not listed in the
helpsection, unlike other CI targets.Suggested Makefile diff
@@ `@echo` "CI Image targets:" `@echo` " make -f Makefile.ci build-ci-image - Build CI container image locally" `@echo` " make -f Makefile.ci push-ci-image - Push CI image to Quay.io" `@echo` " make -f Makefile.ci test-ci-image - Test CI image locally" `@echo` " make -f Makefile.ci build-push-ci-image - Build and push CI image" `@echo` "" + `@echo` "Tarball targets:" + `@echo` " make -f Makefile.ci build-tarball - Build and validate enclave.tar.gz locally" + `@echo` " make -f Makefile.ci build-push-tarball - Build, validate, and push enclave.tar.gz to Quay" + `@echo` ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile.ci` around lines 245 - 254, The Makefile.ci exposes CI entrypoints build-tarball and build-push-tarball but they are not listed in the help output; update the help target (or the function that prints help) to include brief descriptions for the build-tarball and build-push-tarball targets so they appear alongside the other CI targets and are discoverable in the Makefile.ci help text..github/workflows/Dockerfile.ci (1)
44-47: Consider potential side effects ofHOME=/tmp.Setting
HOME=/tmpis a common pattern for non-root containers, but it may affect other tools that write to$HOME(e.g., pip cache at~/.cache/pip, git config at~/.gitconfig). If you observe unexpected behavior from other tools, consider creating a dedicated writable directory instead:RUN mkdir -p /home/ci && chmod 777 /home/ci ENV HOME=/home/ciThat said,
/tmpworks fine for most CI workloads and the current approach is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/Dockerfile.ci around lines 44 - 47, The ENV block currently sets HOME=/tmp which can cause other tools to write into /tmp; instead create a dedicated writable home directory (e.g., /home/ci), ensure it has open permissions for the CI user, and change the HOME assignment to that directory; keep the existing ANSIBLE_COLLECTIONS_PATH and ANSIBLE_REMOTE_TMP env vars unchanged and make these changes in the same Dockerfile ENV/RUN sequence so HOME points to the new dedicated directory rather than /tmp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/Dockerfile.ci:
- Around line 44-47: The ENV block currently sets HOME=/tmp which can cause
other tools to write into /tmp; instead create a dedicated writable home
directory (e.g., /home/ci), ensure it has open permissions for the CI user, and
change the HOME assignment to that directory; keep the existing
ANSIBLE_COLLECTIONS_PATH and ANSIBLE_REMOTE_TMP env vars unchanged and make
these changes in the same Dockerfile ENV/RUN sequence so HOME points to the new
dedicated directory rather than /tmp.
In `@Makefile.ci`:
- Around line 245-254: The Makefile.ci exposes CI entrypoints build-tarball and
build-push-tarball but they are not listed in the help output; update the help
target (or the function that prints help) to include brief descriptions for the
build-tarball and build-push-tarball targets so they appear alongside the other
CI targets and are discoverable in the Makefile.ci help text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 696a88dd-bff0-4c52-9e1b-5ffe9091d7c5
📒 Files selected for processing (4)
.github/workflows/Dockerfile.ciMakefile.ciOWNERSscripts/ci/build_tarball.sh
✅ Files skipped from review due to trivial changes (1)
- OWNERS
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/ci/build_tarball.sh
carbonin
reviewed
Apr 9, 2026
|
|
||
| # Pre-install Ansible Galaxy collections | ||
| # Copy requirements.yml from repository root | ||
| COPY requirements.yml /tmp/requirements.yml |
Contributor
There was a problem hiding this comment.
requirements.yml doesn't exist. Is this related to this change specifically? I don't see it being renamed here so I guess this was just unused/broken before?
The change is obviously an improvement it's just a confusing diff.
Collaborator
Author
There was a problem hiding this comment.
The Dockerfile previously referenced requirements.yml which was removed in 5fb0919 and consolidated into ansible_collections.txt. The Dockerfile was never updated to match so the image build was already broken from that path. This PR fixes it to use ansible_collections.txt and also adds the HOME=/tmp + ANSIBLE_REMOTE_TMP env vars needed for non-root Prow containers.
Add CI scripts for OFCIR SSH-from-Pod pattern: - prow_setup.sh: machine setup (install deps, validate env) - prow_e2e.sh: E2E deployment (connected/disconnected) - prow_infra_verify.sh: infrastructure verification - prow_disconnected_dry_run.sh: mirror config validation - prow_cleanup.sh: periodic infrastructure cleanup Add OWNERS file for Prow review workflow. Add migration and OFCIR setup documentation.
eliorerz
force-pushed
the
prow-ci-migration
branch
from
333c9dd to
fa55172
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ci/build_tarball.sh`:
- Line 10: Validate the ACTION variable immediately after it's parsed (the
ACTION="${1:-build}" assignment) and fail fast on invalid values instead of
running build steps; add an explicit check (e.g., a case or allowed-list test)
right after that assignment to verify ACTION is one of the permitted values and
call exit 1 with a clear error message if not, and apply the same early
validation logic to the other ACTION-handling blocks referenced in the script
(the logic currently around the blocks corresponding to the 23-33 and 99-118
sections) so no build or validation steps run before the script rejects an
invalid ACTION.
- Around line 47-49: The grep check in the loop uses a regex with unescaped
metacharacters (grep -q "^\./${file}$") which causes false matches for names
like ".git"; change to fixed-string, whole-line matching by using grep with -F
and -x (or --fixed-strings and --line-regexp) against "./${file}" when checking
/tmp/tarball-contents.txt; apply the same change wherever the script uses the
regex pattern (e.g., the loop over REQUIRED_FILES and the similar check near
line 72) and keep references to the variables REQUIRED_FILES, file and the
target file /tmp/tarball-contents.txt so the checks match exact paths only.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0609001-c313-418d-be5f-1556fcaef7cd
📒 Files selected for processing (4)
.github/workflows/Dockerfile.ciMakefile.ciOWNERSscripts/ci/build_tarball.sh
✅ Files skipped from review due to trivial changes (1)
- OWNERS
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile.ci
- .github/workflows/Dockerfile.ci
|
|
||
| set -euo pipefail | ||
|
|
||
| ACTION="${1:-build}" |
There was a problem hiding this comment.
Fail fast on invalid ACTION before building artifacts.
Invalid values currently run build+validation first, then fail at the end. Validate ACTION immediately after parsing.
🛠️ Proposed fix
ACTION="${1:-build}"
+case "$ACTION" in
+ build|build-push) ;;
+ *)
+ echo "Unknown action: $ACTION"
+ echo "Usage: $0 build|build-push"
+ exit 1
+ ;;
+esac
@@
if [ "$ACTION" = "build-push" ]; then
@@
elif [ "$ACTION" = "build" ]; then
echo "Tarball built: $TARBALL"
-else
- echo "Unknown action: $ACTION"
- echo "Usage: $0 build|build-push"
- exit 1
fiAlso applies to: 23-33, 99-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/build_tarball.sh` at line 10, Validate the ACTION variable
immediately after it's parsed (the ACTION="${1:-build}" assignment) and fail
fast on invalid values instead of running build steps; add an explicit check
(e.g., a case or allowed-list test) right after that assignment to verify ACTION
is one of the permitted values and call exit 1 with a clear error message if
not, and apply the same early validation logic to the other ACTION-handling
blocks referenced in the script (the logic currently around the blocks
corresponding to the 23-33 and 99-118 sections) so no build or validation steps
run before the script rejects an invalid ACTION.
Comment on lines
+47
to
+49
| for file in "${REQUIRED_FILES[@]}"; do | ||
| if ! grep -q "^\./${file}$" /tmp/tarball-contents.txt; then | ||
| echo "Error: Required file '${file}' not found in tarball" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat >/tmp/grep-check.txt <<'EOF'
./agit/example.txt
./.github/workflows/x.yml
EOF
echo "Current regex check (should NOT match .git/, but does):"
if grep -q '^\./.git/' /tmp/grep-check.txt; then
echo "false-positive reproduced"
else
echo "no match"
fi
echo "Fixed string check:"
if grep -Fq './.git/' /tmp/grep-check.txt; then
echo "matched"
else
echo "correctly no match"
fiRepository: rh-ecosystem-edge/enclave
Length of output: 189
🏁 Script executed:
sed -n '40,80p' scripts/ci/build_tarball.shRepository: rh-ecosystem-edge/enclave
Length of output: 1316
🏁 Script executed:
# Also check the variable assignments for REQUIRED_FILES and EXCLUDED_PATHS
grep -n "REQUIRED_FILES\|EXCLUDED_PATHS" scripts/ci/build_tarball.shRepository: rh-ecosystem-edge/enclave
Length of output: 258
Use fixed-string matching for tarball path assertions.
Lines 48 and 72 interpolate filenames/paths into regex patterns without escaping. Entries like .version, .git/, and .github/ contain unescaped . metacharacters that match any character, causing false positives. The shell test demonstrates that grep -q "^\./.git/" incorrectly matches ./agit/example.txt.
🛠️ Proposed fix
for file in "${REQUIRED_FILES[@]}"; do
- if ! grep -q "^\./${file}$" /tmp/tarball-contents.txt; then
+ if ! grep -Fxq "./${file}" /tmp/tarball-contents.txt; then
echo "Error: Required file '${file}' not found in tarball"
head -20 /tmp/tarball-contents.txt
exit 1
@@
for path in "${EXCLUDED_PATHS[@]}"; do
- if grep -q "^\./${path}" /tmp/tarball-contents.txt; then
+ if grep -Fq "./${path}" /tmp/tarball-contents.txt; then
echo "Error: Excluded path '${path}' found in tarball"
exit 1
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/build_tarball.sh` around lines 47 - 49, The grep check in the loop
uses a regex with unescaped metacharacters (grep -q "^\./${file}$") which causes
false matches for names like ".git"; change to fixed-string, whole-line matching
by using grep with -F and -x (or --fixed-strings and --line-regexp) against
"./${file}" when checking /tmp/tarball-contents.txt; apply the same change
wherever the script uses the regex pattern (e.g., the loop over REQUIRED_FILES
and the similar check near line 72) and keep references to the variables
REQUIRED_FILES, file and the target file /tmp/tarball-contents.txt so the checks
match exact paths only.
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
Summary by CodeRabbit
Chores
Documentation