feat: add crc auto setup#176
Merged
Merged
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:
📝 WalkthroughWalkthroughAdds a crc/ toolchain: scripts to prepare host and start CodeReady Containers (CRC), expose the OpenShift image registry, deploy the Automotive Dev Operator via OLM (ensuring Tekton), run staged sanity and optional end-to-end builds, and provide cross-platform cleanup/documentation. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/crc/01-prep-host-linux.sh`:
- Around line 269-282: The temporary sudoers grant (/etc/sudoers.d/crc-temp) can
persist if the su - "$CRC_USER" -c block fails; wrap creation and removal with a
trap to guarantee cleanup: after creating /etc/sudoers.d/crc-temp and chmod,
register a trap that removes /etc/sudoers.d/crc-temp (and disables the trap) on
EXIT or ERR, then run the su - "$CRC_USER" -c "..." block and finally ensure the
trap is cleared or a final rm -f /etc/sudoers.d/crc-temp is a no-op; modify the
section around the crc setup (the su - "$CRC_USER" -c invocation and the rm -f
/etc/sudoers.d/crc-temp) to use this trap-based cleanup so the NOPASSWD entry
cannot remain if crc setup fails.
- Around line 273-281: The su -c invocation builds a shell string with unquoted,
environment-controlled variables (CRC_MEMORY, CRC_CPUS, DISK_SIZE, CRC_PRESET,
TELEMETRY) which allows command injection; update the su - "$CRC_USER" -c call
to safely quote each variable inside the embedded heredoc/command string (or use
a here-doc or array exec form) so values are passed as literal strings (e.g.,
wrap ${CRC_MEMORY}, ${CRC_CPUS}, ${DISK_SIZE}, ${CRC_PRESET}, ${TELEMETRY} in
quotes inside the command that runs crc config set ...) and mirror the safe
quoting approach used in the macOS branch to eliminate injection risk while
keeping the same commands (crc config set and crc setup).
- Around line 306-319: The script currently sets CURRENT_DISK by calling
run_crc_cmd "crc config get disk-size", which returns only the configured value
and can be larger than the actual VM disk; instead, parse the JSON stored in
CRC_STATUS (from run_crc_cmd "crc status -o json") to extract the VM's actual
disk size (the "Disk Usage" or equivalent field showing actual/total) and assign
that numeric total to CURRENT_DISK, falling back to 0 if parsing fails; then
compare CURRENT_DISK to DISK_SIZE as before and remove the crc config get
disk-size call. Ensure you reference CRC_STATUS, CURRENT_DISK, DISK_SIZE, and
the run_crc_cmd "crc status -o json" invocation when making the change.
In `@hack/crc/02-deploy-operator.sh`:
- Around line 1-2: The script currently allows partial deployment success
because the safety directive `set -euo pipefail` is commented out and several
critical commands (notably oc wait, oc label, oc apply, oc patch) are not
guarded; uncomment or re-enable `set -euo pipefail` at the top (the existing
commented line after the shebang) and ensure each critical command (instances of
`oc wait`, `oc label`, `oc apply`, `oc patch`) is either chained with `|| fail`
or otherwise checked for non-zero exit (or wrap them with a helper that calls
the existing `fail` function on error) so failures abort the script instead of
falling through to the final success banner.
- Around line 84-97: The script currently masks failures by using "oc login ...
|| true" and falling back to ACTUAL_BYTES="0", which can falsely trigger crc
delete; change the auth and API calls (oc login and the oc get nodes JSONPath
used to set ACTUAL_BYTES) to check their exit statuses and fail loudly: remove
the "|| true" after oc login, capture any error from oc login and from the oc
get nodes call, log an explicit error (using warn/error helper) and exit
non-zero if either fails instead of defaulting ACTUAL_BYTES to "0"; only proceed
to the disk-size comparison and crc delete/recreate flow when oc login succeeds
and oc get nodes returned a valid numeric allocatable.ephemeral-storage value
parsed into ACTUAL_BYTES/ACTUAL_GB.
In `@hack/crc/03-validate.sh`:
- Around line 123-135: The sanity check currently greps for a generic
"Completed" which can match older images; change the check to look for the
specific submitted build name by using BUILD_NAME (extracted earlier) when
calling check_output for the "Build visible in caib image list" check — i.e.,
pass a pattern including BUILD_NAME (or "Completed.*$BUILD_NAME") as the
expected string so check_output runs "$CAIB_BIN image list --insecure" and
verifies the actual new build appears rather than any previously completed
entry.
- Around line 13-23: The current check() helper only verifies command exit
status, which lets selector-based oc get -l ... pass when no resources match;
add a new helper (e.g., check_non_empty) that takes a description and command,
captures the command output, verifies the command exits 0 and that the output is
non-empty (or has more than header lines as appropriate), and prints the same
PASS/FAIL messages while incrementing PASS/FAIL counters; then replace
selector-based invocations of check (the oc get -l ... calls) to use
check_non_empty instead so they fail when no matching resources are returned.
In `@hack/crc/crc-cleanup.sh`:
- Around line 98-110: The full-cleanup branch that runs when FULL==true
currently removes the CRC user, binary and /root/.crc but misses cleaning up
leftover sudoers files created by setup-crc.sh; update the FULL cleanup block to
check for and remove /etc/sudoers.d/crc-deploy and /etc/sudoers.d/crc-temp (or
any crc-* entries) after deleting the user and before finishing, using safe
checks (e.g., [[ -f "/etc/sudoers.d/crc-deploy" ]]) and rm -f to remove them and
call ok "… removed." to mirror existing messages; refer to the FULL variable and
CRC_USER in the cleanup logic when adding these removals.
- Around line 85-93: The temporary sudoers entry (/etc/sudoers.d/crc-temp) can
be left behind if the script exits early; add a trap immediately after creating
the file to ensure it is removed and the permission cleanup runs on EXIT and
common signals (INT, TERM, ERR) — modify the block around creation of crc-temp
and the su - "$CRC_USER" -c invocation to register a cleanup function (or inline
trap) that rm -f /etc/sudoers.d/crc-temp and unset the trap on completion; keep
the existing chmod 0440 and su usage, but ensure the trap runs even if su fails
so crc-temp is always removed.
In `@hack/crc/imagebuild.yaml`:
- Around line 29-34: Remove the hard-coded root password and insecure SSH
settings from the sample manifest: delete the root_password entry under auth and
avoid enabling sshd_config: PermitRootLogin and PasswordAuthentication in the
committed example; instead, leave SSH access disabled or document injecting a
generated SSH key or supplying a local password at build/runtime (do not embed
credentials in image). Update symbols affected: auth.root_password,
sshd_config.PermitRootLogin, and sshd_config.PasswordAuthentication.
In `@hack/crc/setup-crc.sh`:
- Around line 63-72: Make the sudoers cleanup unconditional by installing an
exit trap immediately after creating /etc/sudoers.d/crc-deploy so it always gets
removed even if the su - "$CRC_USER" -c block fails; add a trap that removes
/etc/sudoers.d/crc-deploy (and optionally clears the trap afterward) and ensure
the trap is set before running su - "$CRC_USER" -c "cd $DEPLOY_DIR; bash
hack/crc/02-deploy-operator.sh $PULL_SECRET_DEST" so the CRC_USER sudoers entry
cannot be left behind on error.
- Around line 66-70: The su - "$CRC_USER" -c "..." invocations embed unescaped
variables (e.g., $DEPLOY_DIR, $PULL_SECRET_DEST and the ~/.crc_env path)
allowing word-splitting and metacharacter injection; fix by constructing the
nested command with printf '%q' to escape each variable and then pass that
escaped command to su -c (e.g., build a cmd string using printf '%q' for
DEPLOY_DIR, PULL_SECRET_DEST and the crc env/script paths and call su -
"$CRC_USER" -c "$cmd"); apply the same change to both su -c occurrences so all
embedded variables are safely quoted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23ab5991-d659-416f-8acf-2c10405f8e98
📒 Files selected for processing (8)
Makefilehack/crc/01-prep-host-linux.shhack/crc/02-deploy-operator.shhack/crc/03-validate.shhack/crc/README.mdhack/crc/crc-cleanup.shhack/crc/imagebuild.yamlhack/crc/setup-crc.sh
💤 Files with no reviewable changes (1)
- Makefile
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
hack/crc/README.md (1)
271-279: Minor: Spelling inconsistency with "Virtualization".Line 278 uses "Virtualization" (American spelling) while other parts of the codebase may use "virtualisation" (British spelling). Consider using consistent spelling throughout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/README.md` around lines 271 - 279, In the "## Tested Platforms" section update the table header cell that currently reads "Virtualization" to the project's consistent spelling "Virtualisation"; specifically change the table row label under that section (the cell containing Virtualization) to Virtualisation so the README uses the same British spelling as the rest of the codebase.hack/crc/crc-cleanup.sh (1)
56-56: Minor: Tilde in output string won't expand.The
ok "~/.crc cache removed."message displays the literal~rather than the expanded path. This is cosmetic but could be clearer.Optional fix
- rm -rf ~/.crc && ok "~/.crc cache removed." + rm -rf "$HOME/.crc" && ok "$HOME/.crc cache removed."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/crc-cleanup.sh` at line 56, The output message uses a literal tilde so the path doesn't expand; update the ok call associated with the rm -rf ~/.crc command to log the expanded path (use $HOME or an expanded variable) instead of "~/.crc" so the message shows the actual directory; locate the rm -rf ~/.crc and ok "~/.crc cache removed." invocation and change the string passed to ok to include the expanded path (e.g., using $HOME/.crc or a variable like CRC_DIR) so the message prints the real path.hack/crc/01-prep-host.sh (1)
300-306: Potential command injection inrun_crc_cmdhelper.The
$1argument is interpolated directly into a shell string executed viasu -c. While current call sites (lines 308, 309, 317, 318, 325, 326) use hardcoded strings, future callers passing untrusted input could introduce injection vulnerabilities.Consider documenting this function as internal-only or validating that
$1contains no shell metacharacters:Suggested hardening
run_crc_cmd() { + # INTERNAL: Only pass trusted, hardcoded command strings if [[ "$OS" == "Linux" ]]; then su - "$CRC_USER" -c "source ~/.crc_env; $1" else eval "$1" fi }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/01-prep-host.sh` around lines 300 - 306, The run_crc_cmd helper currently interpolates its $1 directly into su -c which permits command injection; update run_crc_cmd to either enforce it is internal-only (documenting it) or validate/escape $1 before executing: implement a strict whitelist/regex check on $1 (no shell metacharacters like ;|&<>$`"'\n) and return non-zero if validation fails, or change the API to accept command + args separately and execute them without string interpolation (e.g., pass an argv-style command to su -c or use a safe exec mechanism); reference run_crc_cmd, CRC_USER and the ~/.crc_env usage when making the change so callers at the noted sites remain compatible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/crc/02-deploy-operator.sh`:
- Around line 56-59: The nested if-block that sets PULL_SECRET_PATH has
inconsistent indentation: align the inner if/else/fi blocks so the else and the
subsequent PULL_SECRET_PATH assignment are indented to match the inner if, and
move the inner fi to the same indentation level as that inner if (so the outer
fi remains at its own level); specifically adjust the block around the inner
"if" that assigns PULL_SECRET_PATH so its "else", the PULL_SECRET_PATH line, and
its closing "fi" are consistently indented and clearly nested under the outer
conditional.
In `@hack/crc/03-crc-operator-sanity.sh`:
- Around line 40-52: The check_non_empty function loses the invoked command's
exit code because `$?` after command substitution reflects the assignment's
success; capture the command's exit status into a local variable right after
running it (e.g., run output=$("$@" 2>/dev/null) then local rc=$?), and then
test `rc -eq 0` together with `-n "$output"` before deciding PASS/FAIL; update
references to PASS and FAIL accordingly and ensure all variables (output, rc)
are local and properly quoted.
In `@hack/crc/README.md`:
- Around line 93-94: The README currently claims the script will "Symlink `oc`
and `kubectl` to /usr/local/bin/", but 01-prep-host.sh actually downloads
kubectl directly into /usr/local/bin/kubectl and does not install or symlink oc
(it only references oc in the environment setup); update the README.md text to
reflect the real behavior: state that the script installs/downloads kubectl to
/usr/local/bin, remove the claim that oc is symlinked or installed, and add a
note that oc must be installed or symlinked by the user if needed (reference
01-prep-host.sh and the README section describing oc/kubectl).
---
Nitpick comments:
In `@hack/crc/01-prep-host.sh`:
- Around line 300-306: The run_crc_cmd helper currently interpolates its $1
directly into su -c which permits command injection; update run_crc_cmd to
either enforce it is internal-only (documenting it) or validate/escape $1 before
executing: implement a strict whitelist/regex check on $1 (no shell
metacharacters like ;|&<>$`"'\n) and return non-zero if validation fails, or
change the API to accept command + args separately and execute them without
string interpolation (e.g., pass an argv-style command to su -c or use a safe
exec mechanism); reference run_crc_cmd, CRC_USER and the ~/.crc_env usage when
making the change so callers at the noted sites remain compatible.
In `@hack/crc/crc-cleanup.sh`:
- Line 56: The output message uses a literal tilde so the path doesn't expand;
update the ok call associated with the rm -rf ~/.crc command to log the expanded
path (use $HOME or an expanded variable) instead of "~/.crc" so the message
shows the actual directory; locate the rm -rf ~/.crc and ok "~/.crc cache
removed." invocation and change the string passed to ok to include the expanded
path (e.g., using $HOME/.crc or a variable like CRC_DIR) so the message prints
the real path.
In `@hack/crc/README.md`:
- Around line 271-279: In the "## Tested Platforms" section update the table
header cell that currently reads "Virtualization" to the project's consistent
spelling "Virtualisation"; specifically change the table row label under that
section (the cell containing Virtualization) to Virtualisation so the README
uses the same British spelling as the rest of the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86176844-fbdc-4b99-8fd7-0d105f7d762a
📒 Files selected for processing (6)
hack/crc/01-prep-host.shhack/crc/02-deploy-operator.shhack/crc/03-crc-operator-sanity.shhack/crc/README.mdhack/crc/crc-cleanup.shhack/crc/setup-crc.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/crc/setup-crc.sh
maboras-rh
force-pushed
the
crc-auto-setup
branch
from
ec59909 to
0a3479a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
hack/crc/README.md (1)
93-94:⚠️ Potential issue | 🟡 MinorRemove the nonexistent
oc/kubectlsymlink step from this checklist.Step 10 still claims
01-prep-host.shsymlinks both binaries, but the surrounding prep flow only documents installingkubectl. Please rewrite this line so it matches the actual host-prep behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/README.md` around lines 93 - 94, Update the checklist line that mentions symlinking both `oc` and `kubectl` (Step 10) to reflect the actual behavior of the host preparation script `01-prep-host.sh`: remove the nonexistent `oc` symlink from the instruction and state only that `kubectl` is symlinked to `/usr/local/bin/` on Linux (or remove the symlink step entirely if the script no longer symlinks any binary), so the README matches `01-prep-host.sh`'s documented actions.hack/crc/03-crc-operator-sanity.sh (2)
40-52:⚠️ Potential issue | 🟠 Major
check_non_empty()can still PASS with zero matching resources.
oc get ... --no-headersprintsNo resources found ...and exits 0 when a selector matches nothing, so Lines 59, 64, 71, and 94 can still report PASS with no resources. Treat that sentinel output as failure instead of accepting any non-empty stdout.Suggested fix
check_non_empty() { local desc="$1" shift - local output - output=$("$@" 2>/dev/null) - if [[ $? -eq 0 && -n "$output" ]]; then + local output rc=0 + output=$("$@" 2>/dev/null) || rc=$? + if [[ $rc -eq 0 ]] && [[ -n "${output//[[:space:]]/}" ]] && [[ "$output" != No\ resources\ found* ]]; then echo -e " ${GREEN}PASS${NC} $desc" PASS=$((PASS + 1)) else echo -e " ${RED}FAIL${NC} $desc (no matching resources)" FAIL=$((FAIL + 1))Also applies to: 59-59, 64-64, 71-71, 94-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/03-crc-operator-sanity.sh` around lines 40 - 52, The check_non_empty() helper currently treats any non-empty stdout as success, but oc get ... --no-headers can return "No resources found" with exit 0; update check_non_empty() to treat that sentinel as failure by inspecting the captured output for the substring "No resources found" (and/or "Error from server" if present) and only count PASS when the command exit status is 0 AND output is non-empty AND does not contain those sentinel messages; reference the check_non_empty function and replace the current simple -n "$output" test with a conditional that rejects outputs matching "No resources found" (case-sensitive) before incrementing PASS, otherwise increment FAIL.
76-79:⚠️ Potential issue | 🟠 MajorAdd
-fflag to curl for proper Build API health check.Without
-f, curl exits with code 0 even on HTTP 4xx/5xx responses. Thecheckfunction treats exit code 0 as success, so a broken/healthzendpoint returning an error status will incorrectly pass the health check.Suggested fix
- check "Build API endpoint responds" curl -sk --max-time 10 "https://${BUILD_API_URL}/healthz" + check "Build API endpoint responds" curl -skf --max-time 10 "https://${BUILD_API_URL}/healthz"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/03-crc-operator-sanity.sh` around lines 76 - 79, The health check uses curl without the -f flag so HTTP 4xx/5xx responses still yield exit code 0; update the curl invocation in the BUILD_API_URL block (the line that calls check "Build API endpoint responds" curl -sk --max-time 10 "https://${BUILD_API_URL}/healthz") to include -f (and preserve -s/k/--max-time) so non-2xx/3xx responses cause curl to fail and the check function will correctly report failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/crc/02-deploy-operator.sh`:
- Around line 272-296: The script prints success and a CAIB_SERVER quick-start
even when BUILD_API_ROUTE is empty; update the post-deployment banner logic to
validate BUILD_API_ROUTE before showing Build API and CAIB_SERVER lines and
avoid a misleading success banner: check the BUILD_API_ROUTE variable (set by
BUILD_API_ROUTE=$(oc get route ...)) and if empty, either wait/retry or print a
clear warning and omit the "Build API" and "export CAIB_SERVER=..." lines and
the URL interpolation; also remove the hardcoded "--arch arm64" in the
quick-start and instead use the platform mapping variable used in
hack/crc/03-crc-operator-sanity.sh (or an $ARCH variable) so the sample command
reflects the detected platform. Ensure references to BUILD_API_ROUTE, the ok
"All components running." banner, the CAIB_SERVER export line, and the "--arch
arm64" token are updated accordingly.
- Around line 188-189: The current oc create namespace "$NAMESPACE" && ok ... ||
info ... treats any failure as "already exists"; change it to capture the create
command output/exit status and only map the "AlreadyExists" response to the info
path. Concretely, run oc create namespace "$NAMESPACE" and save its
stdout/stderr and exit code, call ok() on success, call info() only when the
captured output or error message contains "AlreadyExists", and for any other
non-zero exit code call an error handler (or exit non‑zero) so auth/API errors
are not misreported; reference the oc create namespace invocation, the
$NAMESPACE variable, and the ok/info functions when making the change.
---
Duplicate comments:
In `@hack/crc/03-crc-operator-sanity.sh`:
- Around line 40-52: The check_non_empty() helper currently treats any non-empty
stdout as success, but oc get ... --no-headers can return "No resources found"
with exit 0; update check_non_empty() to treat that sentinel as failure by
inspecting the captured output for the substring "No resources found" (and/or
"Error from server" if present) and only count PASS when the command exit status
is 0 AND output is non-empty AND does not contain those sentinel messages;
reference the check_non_empty function and replace the current simple -n
"$output" test with a conditional that rejects outputs matching "No resources
found" (case-sensitive) before incrementing PASS, otherwise increment FAIL.
- Around line 76-79: The health check uses curl without the -f flag so HTTP
4xx/5xx responses still yield exit code 0; update the curl invocation in the
BUILD_API_URL block (the line that calls check "Build API endpoint responds"
curl -sk --max-time 10 "https://${BUILD_API_URL}/healthz") to include -f (and
preserve -s/k/--max-time) so non-2xx/3xx responses cause curl to fail and the
check function will correctly report failure.
In `@hack/crc/README.md`:
- Around line 93-94: Update the checklist line that mentions symlinking both
`oc` and `kubectl` (Step 10) to reflect the actual behavior of the host
preparation script `01-prep-host.sh`: remove the nonexistent `oc` symlink from
the instruction and state only that `kubectl` is symlinked to `/usr/local/bin/`
on Linux (or remove the symlink step entirely if the script no longer symlinks
any binary), so the README matches `01-prep-host.sh`'s documented actions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 380ff416-4faa-48cd-9ca3-f71bcb0a3616
📒 Files selected for processing (6)
hack/crc/01-prep-host.shhack/crc/02-deploy-operator.shhack/crc/03-crc-operator-sanity.shhack/crc/README.mdhack/crc/crc-cleanup.shhack/crc/setup-crc.sh
✅ Files skipped from review due to trivial changes (2)
- hack/crc/setup-crc.sh
- hack/crc/01-prep-host.sh
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
hack/crc/02-deploy-operator.sh (2)
188-189:⚠️ Potential issue | 🟠 MajorNamespace creation still masks real failures as “already exists”.
Line 188/Line 189 use
cmd && ok || info, so auth/API errors are reported as “already exists” and deployment continues incorrectly.Suggested fix
-oc create namespace "$NAMESPACE" 2>/dev/null && ok "Namespace '$NAMESPACE' created." \ - || info "Namespace '$NAMESPACE' already exists." +if oc get namespace "$NAMESPACE" &>/dev/null; then + info "Namespace '$NAMESPACE' already exists." +else + oc create namespace "$NAMESPACE" || fail "Failed to create namespace '$NAMESPACE'." + ok "Namespace '$NAMESPACE' created." +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/02-deploy-operator.sh` around lines 188 - 189, The current use of the `oc create namespace "$NAMESPACE" && ok || info` pattern masks real failures (auth/API errors) as “already exists”; replace this with an explicit exit-code check around `oc create namespace "$NAMESPACE"` (or pre-check with `oc get namespace "$NAMESPACE"`), e.g. run the create, capture its exit status, then: if status==0 call ok("Namespace ... created"), elif the failure is “already exists” call info("Namespace ... already exists"), else call error/log and exit non-zero so real API/auth errors are not treated as success; reference the `NAMESPACE` variable and the `oc create namespace` invocation to locate the change.
272-296:⚠️ Potential issue | 🟠 MajorPost-deploy output is misleading when route is empty, and arch is hardcoded.
Line 295 always prints
CAIB_SERVER=https://${BUILD_API_ROUTE}and Line 296 hardcodes--arch arm64. On x86_64 or when route is empty, the quick-start is wrong.Suggested fix
BUILD_API_ROUTE=$(oc get route ado-build-api -n "$NAMESPACE" -o jsonpath='{.spec.host}' 2>/dev/null || echo "") ok "All components running." @@ -if [ -n "$BUILD_API_ROUTE" ]; then -echo " Build API: https://${BUILD_API_ROUTE}" -fi +if [ -n "$BUILD_API_ROUTE" ]; then + echo " Build API: https://${BUILD_API_ROUTE}" +fi @@ -echo " Quick start:" -echo " export CAIB_SERVER=https://${BUILD_API_ROUTE}" -echo " ./bin/caib image build <manifest.yml> --arch arm64 --push ${INTERNAL_REGISTRY}/${NAMESPACE}/my-image:latest --insecure" +case "$(uname -m)" in + x86_64) QUICKSTART_ARCH="amd64" ;; + aarch64|arm64) QUICKSTART_ARCH="arm64" ;; + *) QUICKSTART_ARCH="amd64" ;; +esac +echo " Quick start:" +if [ -n "$BUILD_API_ROUTE" ]; then + echo " export CAIB_SERVER=https://${BUILD_API_ROUTE}" +else + warn "Build API route is not available yet; skip CAIB_SERVER export." +fi +echo " ./bin/caib image build <manifest.yml> --arch ${QUICKSTART_ARCH} --push ${INTERNAL_REGISTRY}/${NAMESPACE}/my-image:latest --insecure"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/02-deploy-operator.sh` around lines 272 - 296, The post-deploy quick-start prints a CAIB_SERVER URL even when BUILD_API_ROUTE is empty and hardcodes "--arch arm64"; update the output logic around BUILD_API_ROUTE so the "export CAIB_SERVER=..." and any URL references are only printed when BUILD_API_ROUTE is non-empty (check the BUILD_API_ROUTE variable used above), and replace the hardcoded "--arch arm64" with a computed ARCH variable (detect via uname -m or map "aarch64"->"arm64" and "x86_64"->"amd64") and use that ${ARCH} in the example (also ensure INTERNAL_REGISTRY/${NAMESPACE}/... and other variables like get_kubeadmin_pass and IMG remain unchanged).hack/crc/03-crc-operator-sanity.sh (1)
40-51:⚠️ Potential issue | 🟠 Major
check_non_emptycan falsely pass on empty selector matches.On Line 44,
oc get ... -l ... --no-headersstill prints"No resources found ..."with exit code 0, so these checks can pass when nothing matches.Suggested fix
check_non_empty() { local desc="$1" shift local output - if output=$("$@" 2>/dev/null) && [[ -n "$output" ]]; then + if output=$("$@" 2>/dev/null) \ + && [[ -n "${output//[[:space:]]/}" ]] \ + && [[ "$output" != "No resources found"* ]]; then echo -e " ${GREEN}PASS${NC} $desc" PASS=$((PASS + 1)) else echo -e " ${RED}FAIL${NC} $desc (no matching resources)" FAIL=$((FAIL + 1)) fi }Also applies to: 58-58, 63-63, 70-70, 93-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/03-crc-operator-sanity.sh` around lines 40 - 51, The helper check_non_empty currently treats any stdout as success, but oc get prints "No resources found" with exit code 0; update check_non_empty (the function) to capture both stdout and stderr into a variable, then consider it a failure if the captured output is empty or contains the substring "No resources found" (or similar server-not-found text) before printing PASS/FAIL; apply the same change to the other call sites/duplicates referenced (the other uses of check_non_empty).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/crc/02-deploy-operator.sh`:
- Around line 93-99: The script fails because ACTUAL_BYTES may contain
Kubernetes ResourceQuantity suffixes (e.g., 20145724Ki) making bash arithmetic
in ACTUAL_GB invalid; update the code around the ACTUAL_BYTES/ACTUAL_GB logic to
first capture the raw value (ACTUAL_BYTES_RAW), parse its suffix (handle
Ki/Mi/Gi/Ti/Pi/Ei as 1024^n and K/M/G/T/P/E as 1000^n, and handle plain
integers), convert to a numeric byte count, then compute ACTUAL_GB=$((
BYTE_COUNT / 1073741824 )), preserving the existing fallback to DISK_SIZE when
empty or zero; reference ACTUAL_BYTES, ACTUAL_GB and DISK_SIZE when locating
where to add the parsing logic.
In `@hack/crc/03-crc-operator-sanity.sh`:
- Around line 98-157: The --sanity branch sets CAIB_SERVER from BUILD_API_URL
without validating it; update the block that exports CAIB_SERVER to check that
BUILD_API_URL is non-empty (when handling the --sanity flag) and if missing
print a clear error (use the same colored FAIL message), increment FAIL, and
skip the build steps (exit or continue) instead of exporting an empty
CAIB_SERVER; specifically modify the code around the export
CAIB_SERVER="https://${BUILD_API_URL}" to validate BUILD_API_URL first and guard
subsequent uses of CAIB_SERVER/caib image build (referencing BUILD_API_URL,
CAIB_SERVER, --sanity, and the image build submission logic).
- Line 77: The health-check invocation using curl in the check labeled "Build
API endpoint responds" currently uses curl -sk --max-time 10 which returns exit
code 0 for HTTP 4xx/5xx; update that curl invocation to include -f (e.g., curl
-f -sk --max-time 10 ...) so curl will exit non‑zero on HTTP error responses and
the check will properly fail on 4xx/5xx.
---
Duplicate comments:
In `@hack/crc/02-deploy-operator.sh`:
- Around line 188-189: The current use of the `oc create namespace "$NAMESPACE"
&& ok || info` pattern masks real failures (auth/API errors) as “already
exists”; replace this with an explicit exit-code check around `oc create
namespace "$NAMESPACE"` (or pre-check with `oc get namespace "$NAMESPACE"`),
e.g. run the create, capture its exit status, then: if status==0 call
ok("Namespace ... created"), elif the failure is “already exists” call
info("Namespace ... already exists"), else call error/log and exit non-zero so
real API/auth errors are not treated as success; reference the `NAMESPACE`
variable and the `oc create namespace` invocation to locate the change.
- Around line 272-296: The post-deploy quick-start prints a CAIB_SERVER URL even
when BUILD_API_ROUTE is empty and hardcodes "--arch arm64"; update the output
logic around BUILD_API_ROUTE so the "export CAIB_SERVER=..." and any URL
references are only printed when BUILD_API_ROUTE is non-empty (check the
BUILD_API_ROUTE variable used above), and replace the hardcoded "--arch arm64"
with a computed ARCH variable (detect via uname -m or map "aarch64"->"arm64" and
"x86_64"->"amd64") and use that ${ARCH} in the example (also ensure
INTERNAL_REGISTRY/${NAMESPACE}/... and other variables like get_kubeadmin_pass
and IMG remain unchanged).
In `@hack/crc/03-crc-operator-sanity.sh`:
- Around line 40-51: The helper check_non_empty currently treats any stdout as
success, but oc get prints "No resources found" with exit code 0; update
check_non_empty (the function) to capture both stdout and stderr into a
variable, then consider it a failure if the captured output is empty or contains
the substring "No resources found" (or similar server-not-found text) before
printing PASS/FAIL; apply the same change to the other call sites/duplicates
referenced (the other uses of check_non_empty).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d789ca8f-135e-42ba-a0ea-89a036a909f8
📒 Files selected for processing (5)
hack/crc/01-prep-host.shhack/crc/02-deploy-operator.shhack/crc/03-crc-operator-sanity.shhack/crc/README.mdhack/crc/crc-cleanup.sh
✅ Files skipped from review due to trivial changes (2)
- hack/crc/README.md
- hack/crc/crc-cleanup.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/crc/01-prep-host.sh
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/crc/02-deploy-operator.sh (1)
263-269: Minor: Loop variableiis unused.The static analysis correctly flags that
iis not used within the loop body. While this is harmless (the loop is correctly iterating 30 times), you can suppress the warning by using_as the loop variable if desired.Optional fix
-for i in {1..30}; do +for _ in {1..30}; do PHASE=$(oc get operatorconfig config -n "$NAMESPACE" -o jsonpath='{.status.phase}' 2>/dev/null || echo "") if [ "$PHASE" = "Ready" ]; then break fi sleep 5 done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/02-deploy-operator.sh` around lines 263 - 269, Change the for-loop to use a throwaway loop variable to avoid the unused-variable warning: replace the named iterator `i` in the loop that polls PHASE (the block that runs oc get operatorconfig ... and checks if PHASE equals "Ready") with `_` so the loop still runs 30 times but static analysis no longer flags an unused variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/crc/02-deploy-operator.sh`:
- Around line 263-269: Change the for-loop to use a throwaway loop variable to
avoid the unused-variable warning: replace the named iterator `i` in the loop
that polls PHASE (the block that runs oc get operatorconfig ... and checks if
PHASE equals "Ready") with `_` so the loop still runs 30 times but static
analysis no longer flags an unused variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e0a4809-efea-4766-9cbd-f89b5999cb68
📒 Files selected for processing (2)
hack/crc/02-deploy-operator.shhack/crc/03-crc-operator-sanity.sh
maboras-rh
force-pushed
the
crc-auto-setup
branch
3 times, most recently
from
88e2660 to
0af47f0
Compare
maboras-rh
requested review from
bennyz and
bkhizgiy
and removed request for
bennyz and
bkhizgiy
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hack/crc/02-deploy-operator.sh (2)
263-269: Unused loop variable is intentional (shellcheck false positive).The
ivariable controls the 30-iteration polling loop. This is a common pattern where the variable isn't referenced inside the loop body. You can optionally rename it to_or add a shellcheck directive to silence the warning:-for i in {1..30}; do +for _ in {1..30}; do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/02-deploy-operator.sh` around lines 263 - 269, The loop uses an unused iteration variable `i` in `for i in {1..30}; do` which triggers a shellcheck false positive; either rename `i` to `_` (e.g. `for _ in {1..30}; do`) to indicate intentional unused variable or add a shellcheck directive (e.g. `# shellcheck disable=SC2034`) immediately above the loop that contains the `PHASE=$(oc get operatorconfig config -n "$NAMESPACE" -o jsonpath='{.status.phase}' ...)` polling block so the linter ignores the unused-variable warning.
85-88: Consider definingget_kubeadmin_passearlier to avoid duplication.Lines 85-88 duplicate the password extraction logic that's later encapsulated in
get_kubeadmin_pass()(lines 133-140). Defining the function earlier would allow reuse at line 85:Optional refactor
+get_kubeadmin_pass() { + local pass="" + pass=$(crc console --credentials -o json 2>/dev/null | grep -o '"password":"[^"]*' | cut -d'"' -f4) + if [[ -z "$pass" ]]; then + pass=$(crc console --credentials 2>/dev/null | grep kubeadmin | sed "s/.*-p \([^ ]*\) .*/\1/" | head -1) + fi + echo "$pass" +} + if [[ -n "$CRC_RUNNING" ]]; then info "CRC is already running. Checking disk size..." eval "$(crc oc-env)" 2>/dev/null || true - KUBE_PASS_TMP=$(crc console --credentials -o json 2>/dev/null | grep -o '"password":"[^"]*' | cut -d'"' -f4) - if [[ -z "$KUBE_PASS_TMP" ]]; then - KUBE_PASS_TMP=$(crc console --credentials 2>/dev/null | grep kubeadmin | sed "s/.*-p \([^ ]*\) .*/\1/" | head -1) - fi + KUBE_PASS_TMP=$(get_kubeadmin_pass)Also applies to: 133-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/02-deploy-operator.sh` around lines 85 - 88, The code duplicates kubeadmin password extraction; move the get_kubeadmin_pass() function definition up above its first use and replace the duplicate block that sets KUBE_PASS_TMP at lines using crc console with a call to get_kubeadmin_pass; ensure get_kubeadmin_pass implements the same logic (try JSON creds then fallback to grep/sed extraction) and returns or echoes the password so KUBE_PASS_TMP can be assigned from it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/crc/02-deploy-operator.sh`:
- Around line 263-269: The loop uses an unused iteration variable `i` in `for i
in {1..30}; do` which triggers a shellcheck false positive; either rename `i` to
`_` (e.g. `for _ in {1..30}; do`) to indicate intentional unused variable or add
a shellcheck directive (e.g. `# shellcheck disable=SC2034`) immediately above
the loop that contains the `PHASE=$(oc get operatorconfig config -n "$NAMESPACE"
-o jsonpath='{.status.phase}' ...)` polling block so the linter ignores the
unused-variable warning.
- Around line 85-88: The code duplicates kubeadmin password extraction; move the
get_kubeadmin_pass() function definition up above its first use and replace the
duplicate block that sets KUBE_PASS_TMP at lines using crc console with a call
to get_kubeadmin_pass; ensure get_kubeadmin_pass implements the same logic (try
JSON creds then fallback to grep/sed extraction) and returns or echoes the
password so KUBE_PASS_TMP can be assigned from it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 067b0d6f-dbe7-4f17-82d2-0cbd2871675e
📒 Files selected for processing (8)
Makefilehack/crc/01-prep-host.shhack/crc/02-deploy-operator.shhack/crc/03-crc-operator-sanity.shhack/crc/README.mdhack/crc/crc-cleanup.shhack/crc/imagebuild.yamlhack/crc/setup-crc.sh
💤 Files with no reviewable changes (1)
- Makefile
✅ Files skipped from review due to trivial changes (4)
- hack/crc/imagebuild.yaml
- hack/crc/README.md
- hack/crc/crc-cleanup.sh
- hack/crc/01-prep-host.sh
maboras-rh
force-pushed
the
crc-auto-setup
branch
from
2a0e887 to
bed051a
Compare
maboras-rh
force-pushed
the
crc-auto-setup
branch
from
bed051a to
ab0b194
Compare
Contributor
Author
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
hack/crc/02-deploy-operator.sh (1)
262-270: Shellcheck SC2034: Loop variableiis unused.This is a common idiom for "repeat N times" polling loops and doesn't affect correctness. To silence the warning, you can prefix with underscore or use a different construct.
💡 Option to suppress warning
-for i in {1..30}; do +for _ in {1..30}; do PHASE=$(oc get operatorconfig config -n "$NAMESPACE" -o jsonpath='{.status.phase}' 2>/dev/null || echo "") if [ "$PHASE" = "Ready" ]; then break fi sleep 5 done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/02-deploy-operator.sh` around lines 262 - 270, The shellcheck warning SC2034 is due to the unused loop variable "i" in the polling loop that checks OperatorConfig status; update the for loop header to use an unused underscore (for _ in {1..30}; do) or another construct (e.g., a while loop) so the intent of repeating 30 times remains but the loop variable is not flagged; keep the body that sets PHASE via oc get and the final readiness check unchanged (references: the for loop, PHASE, and the fail check).hack/crc/03-crc-operator-sanity.sh (1)
121-124: Consider surfacingmakefailures for easier debugging.The
|| truesuppresses all build output and exit codes. Ifmake build-caibfails, the user sees only "caib binary not found" without knowing why. Preserving some diagnostic output would help troubleshooting.💡 Suggested improvement
if [[ ! -f "$CAIB_BIN" ]]; then echo -e " ${CYAN}INFO${NC} Building caib CLI..." - make -C "$REPO_DIR" build-caib &>/dev/null || true + if ! make -C "$REPO_DIR" build-caib 2>&1 | tail -5; then + echo -e " ${CYAN}INFO${NC} make build-caib did not succeed; will check if binary exists anyway." + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/03-crc-operator-sanity.sh` around lines 121 - 124, The build step for caib is currently silenced by "|| true", hiding failures; update the block that checks CAIB_BIN so the make invocation (make -C "$REPO_DIR" build-caib) is not suppressed—capture its stdout/stderr (or let it stream) and on failure print a clear diagnostic including the captured output and exit non‑zero so callers can see why the build failed; adjust the CAIB_BIN existence check to fail fast when the make command fails instead of silently continuing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/crc/crc-cleanup.sh`:
- Around line 49-57: The macOS full-cleanup branch (when FULL is true) only
removes /usr/local/bin/crc but not the documented ~/bin/crc; update the cleanup
block to also check for and remove $HOME/bin/crc. Inside the existing FULL block
(where brew uninstall and /usr/local/bin/crc removal occur), add an additional
conditional or elif that checks [[ -f "$HOME/bin/crc" ]] and removes it (e.g.,
rm -f "$HOME/bin/crc" && ok "Binary removed from $HOME/bin"), keeping the
existing brew and /usr/local/bin/crc checks and preserving the info/ok calls and
error handling style.
---
Nitpick comments:
In `@hack/crc/02-deploy-operator.sh`:
- Around line 262-270: The shellcheck warning SC2034 is due to the unused loop
variable "i" in the polling loop that checks OperatorConfig status; update the
for loop header to use an unused underscore (for _ in {1..30}; do) or another
construct (e.g., a while loop) so the intent of repeating 30 times remains but
the loop variable is not flagged; keep the body that sets PHASE via oc get and
the final readiness check unchanged (references: the for loop, PHASE, and the
fail check).
In `@hack/crc/03-crc-operator-sanity.sh`:
- Around line 121-124: The build step for caib is currently silenced by "||
true", hiding failures; update the block that checks CAIB_BIN so the make
invocation (make -C "$REPO_DIR" build-caib) is not suppressed—capture its
stdout/stderr (or let it stream) and on failure print a clear diagnostic
including the captured output and exit non‑zero so callers can see why the build
failed; adjust the CAIB_BIN existence check to fail fast when the make command
fails instead of silently continuing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8858743-9629-4585-a482-f8170ff57e10
📒 Files selected for processing (7)
hack/crc/01-prep-host.shhack/crc/02-deploy-operator.shhack/crc/03-crc-operator-sanity.shhack/crc/README.mdhack/crc/crc-cleanup.shhack/crc/imagebuild.yamlhack/crc/setup-crc.sh
✅ Files skipped from review due to trivial changes (2)
- hack/crc/imagebuild.yaml
- hack/crc/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/crc/setup-crc.sh
- hack/crc/01-prep-host.sh
bennyz
reviewed
Mar 23, 2026
bennyz
reviewed
Mar 23, 2026
bennyz
reviewed
Mar 23, 2026
bennyz
reviewed
Mar 23, 2026
bennyz
reviewed
Mar 23, 2026
bennyz
reviewed
Mar 23, 2026
bennyz
reviewed
Mar 23, 2026
bennyz
reviewed
Mar 23, 2026
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
hack/crc/04-expose-default-regustry.sh (1)
1-11: Filename contains a typo: "regustry" → "registry".The script filename
04-expose-default-regustry.shhas a spelling error. This affects discoverability and the error message in02-deploy-operator.sh(line 145) that references this filename.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/04-expose-default-regustry.sh` around lines 1 - 11, Rename the script file named "04-expose-default-regustry.sh" to the correctly spelled "04-expose-default-registry.sh" and update any code that references the old filename (e.g., the call or error message in the deploy script that mentions "04-expose-default-regustry.sh", such as the invocation in 02-deploy-operator.sh) to use "04-expose-default-registry.sh" so references match the new filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/crc/01-prep-host.sh`:
- Around line 224-227: The current check only tests for CRC_USER in /etc/subuid
so if a subuid exists but /etc/subgid is missing the script skips adding the
subgid; update the logic around CRC_USER so you independently verify and append
entries to both /etc/subuid and /etc/subgid (i.e., check grep -q "^${CRC_USER}:"
against each file separately and append "${CRC_USER}:100000:65536" to whichever
file is missing the entry) to ensure both mappings are populated even when one
already exists.
- Around line 239-243: The append to "$USER_HOME/.bashrc" may create the file as
root; ensure the target file exists and is owned by "${CRC_USER}" before
writing: check for the presence of "$USER_HOME/.bashrc" and if it does not exist
create it (e.g., touch or install) and chown it to "${CRC_USER}:${CRC_USER}"
prior to appending "source ~/.crc_env", then append the line and finally ensure
"$USER_HOME/.crc_env" is chowned to "${CRC_USER}:${CRC_USER}" as already
present; reference variables/functions to update: USER_HOME, CRC_USER, the
".bashrc" creation/append block and the existing chown of "$USER_HOME/.crc_env".
- Around line 306-339: The script currently only checks CRC_STATUS/"crcStatus"
via CRC_RUNNING before skipping startup; update the logic to also parse the
OpenShift status (e.g., extract an OPENSHIFT_STATUS from CRC_STATUS by matching
the "openshiftStatus" JSON field) and require both CRC_RUNNING and
OPENSHIFT_STATUS=="Running" (or the exact running value reported) before
treating the cluster as ready and skipping crc start; apply the same dual check
to CRC_STATUS_AFTER/CRC_RUNNING_AFTER so the script only skips starting when
both the VM and OpenShift are fully running (modify the conditional that uses
CRC_RUNNING and CRC_RUNNING_AFTER to include the new
OPENSHIFT_STATUS/OPENSHIFT_RUNNING checks).
In `@hack/crc/04-expose-default-regustry.sh`:
- Line 32: The PATH update is appending the oc file instead of its containing
directory; change the export that sets PATH (the line using PATH and
CRC_USER_HOME and .crc/bin/oc) to add the directory containing oc (i.e.,
.crc/bin) rather than the oc binary itself so PATH contains a directory, e.g.,
use ${CRC_USER_HOME}/.crc/bin when exporting PATH.
In `@hack/crc/crc_back/03-crc-operator-sanity.sh`:
- Around line 126-131: REPO_DIR is computed incorrectly because this script
lives in hack/crc/crc_back so SCRIPT_DIR/../.. points to hack/ not the repo
root; update the REPO_DIR assignment to climb one more level (use
SCRIPT_DIR/../../..) so CAIB_BIN and TEST_MANIFEST point at bin/caib and
test/config/test-manifest.aib.yml in the repository root, and verify the derived
variables CAIB_BIN, TEST_MANIFEST and TEST_IMAGE still use the corrected
REPO_DIR and existing NAMESPACE variable.
In `@hack/crc/crc_back/04-expose-default-regustry.sh`:
- Around line 72-75: Replace the fixed sleep and single oc get with a polling
loop that retries `oc get route default-route -n openshift-image-registry -o
jsonpath='{.spec.host}'` until a non-empty HOST is returned or a timeout
elapses; use the same HOST variable and keep the `[[ -n "$HOST" ]] || fail
"Could not resolve default registry route host."` check and `ok "Registry host:
$HOST"` call, but prevent premature exit under set -e by handling command
failures inside the loop and failing only after the timeout.
- Around line 80-87: Replace the fixed public tmp dir and broad oc extract with
a private temp dir and extraction limited to the cert only: create CERT_DIR
using mktemp -d instead of "/tmp/registry-certs", change the oc extract call in
the block that computes CERT_SECRET to include --keys=tls.crt so only tls.crt is
extracted, and add a trap to remove the temporary CERT_DIR on EXIT (and ensure
the subsequent existence check still looks for "$CERT_DIR/tls.crt").
- Around line 163-170: The podman login call currently passes the token via -p
which exposes OC_TOKEN in process argv; change both branches that call podman
login to use --password-stdin and pipe OC_TOKEN on stdin instead (for the su -
"$CRC_USER" -c branch wrap the piped input inside the su command so podman
receives the token via stdin as well), keeping the existing HOST and
--tls-verify="$TLS_VERIFY" args and preserving output redirection to >/dev/null;
also update the example text printed later to show using --password-stdin rather
than -p/--password.
In `@hack/crc/crc_back/crc-cleanup.sh`:
- Around line 76-98: The subshells that run crc stop, crc delete and crc cleanup
currently append "|| true" which masks failures from the outer "|| warn"
handlers; edit the three su - "$CRC_USER" -c "… crc stop/delete/cleanup
2>/dev/null || true" blocks to remove the trailing "|| true" so the subshell
exits non‑zero on real errors (keeping the stderr redirection and the
XDG_RUNTIME_DIR/DBUS_SESSION_BUS_ADDRESS exports intact), allowing the outer "||
warn" to observe and report failures for the crc stop/delete/cleanup commands.
- Line 74: Replace the unsafe eval-based home lookup that sets USER_HOME from
CRC_USER with a passwd-database lookup to avoid executing shell metacharacters;
specifically, stop using the construct that expands "~${CRC_USER}" and instead
obtain the user's home directory from the system user database (e.g., via getent
passwd or equivalent) and assign that to USER_HOME, and apply the same change
for the second occurrence that also derives USER_HOME from CRC_USER.
- Around line 119-120: The sed address /^${CRC_USER}:/d treats CRC_USER as a
regex and can match unintended names; update the two sed invocations in
crc-cleanup.sh that use CRC_USER so the username is matched literally (escape
regex metacharacters in ${CRC_USER} before embedding it into the sed address, or
switch to a literal match approach such as using sed -F/-f equivalent or an
awk/grep -F command that checks field 1 equals the username followed by a
colon). Specifically modify the lines that call sed with /^${CRC_USER}:/d to
either escape ${CRC_USER} (replace regex metacharacters with backslash-escaped
versions) or replace the sed invocation with an awk/grep -F check that deletes
only lines whose first field is exactly ${CRC_USER}.
In `@hack/crc/crc_back/setup-crc.sh`:
- Around line 17-18: The REPO_DIR calculation is wrong (SCRIPT_DIR is
hack/crc/crc_back so SCRIPT_DIR/../.. points to hack/), breaking downstream
paths; update the REPO_DIR assignment to go up three levels from SCRIPT_DIR (use
"$(cd "$SCRIPT_DIR/../../.." && pwd)") so REPO_DIR points to the repository root
and all later references that use REPO_DIR (e.g., where the script uses that
variable for paths on lines referencing scripts at 50,72,81,89,90,102,106)
resolve correctly.
In `@hack/crc/crc-cleanup.sh`:
- Around line 122-126: The cleanup loop uses "$HOME/bin/crc" which resolves to
root's home when run as root; change the path to "$USER_HOME/bin/crc" in the
for-loop that iterates over crc_path (the loop containing /usr/local/bin/crc
"$HOME/bin/crc" /usr/bin/crc) so it targets the intended user's home; also
ensure the USER_HOME variable is set/validated earlier (or fallback to a safe
default) before this loop to avoid empty or incorrect paths.
In `@hack/crc/setup-crc.sh`:
- Around line 32-44: The script currently clobbers any PULL_SECRET environment
variable by initializing PULL_SECRET=""; remove that initial reset so the script
will honor an exported PULL_SECRET, and keep the existing argument-parsing loop
(which already only assigns PULL_SECRET from argv when [[ -z "$PULL_SECRET" ]]);
ensure the rest of the checks (the [[ -n "$PULL_SECRET" ]] usage check and file
existence check) remain unchanged so both env and positional-arg fallbacks work.
---
Nitpick comments:
In `@hack/crc/04-expose-default-regustry.sh`:
- Around line 1-11: Rename the script file named "04-expose-default-regustry.sh"
to the correctly spelled "04-expose-default-registry.sh" and update any code
that references the old filename (e.g., the call or error message in the deploy
script that mentions "04-expose-default-regustry.sh", such as the invocation in
02-deploy-operator.sh) to use "04-expose-default-registry.sh" so references
match the new filename.
🪄 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: CHILL
Plan: Pro
Run ID: d636893e-66ea-44d9-9662-0764684b8021
📒 Files selected for processing (14)
hack/crc/01-prep-host.shhack/crc/02-deploy-operator.shhack/crc/03-crc-operator-sanity.shhack/crc/04-expose-default-regustry.shhack/crc/README.mdhack/crc/crc-cleanup.shhack/crc/crc_back/01-prep-host.shhack/crc/crc_back/02-deploy-operator.shhack/crc/crc_back/03-crc-operator-sanity.shhack/crc/crc_back/04-expose-default-regustry.shhack/crc/crc_back/README.mdhack/crc/crc_back/crc-cleanup.shhack/crc/crc_back/setup-crc.shhack/crc/setup-crc.sh
✅ Files skipped from review due to trivial changes (2)
- hack/crc/crc_back/README.md
- hack/crc/README.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
hack/crc/02-deploy-operator.sh (1)
186-194:⚠️ Potential issue | 🟡 MinorMissing validation of BUILD_API_ROUTE before printing success.
BUILD_API_ROUTEis fetched but not validated before printing "All components running." If the route query fails or returns empty, the success message is misleading. The conditional at lines 205-207 and 215-219 handle the display, but the "All components running" message at line 194 is unconditional.Consider moving the success message after validation or making it conditional:
Suggested fix
BUILD_API_ROUTE=$(oc get route ado-build-api -n "$NAMESPACE" -o jsonpath='{.spec.host}' 2>/dev/null || echo "") case "$(uname -m)" in x86_64) QUICKSTART_ARCH="amd64" ;; aarch64|arm64) QUICKSTART_ARCH="arm64" ;; *) QUICKSTART_ARCH="amd64" ;; esac -ok "All components running." +if [[ -n "$BUILD_API_ROUTE" ]]; then + ok "All components running." +else + warn "Operator deployed but Build API route not yet available." +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/02-deploy-operator.sh` around lines 186 - 194, The script prints ok "All components running." unconditionally while BUILD_API_ROUTE may be empty; update the logic so the success message is only printed after validating BUILD_API_ROUTE (the variable set by BUILD_API_ROUTE=$(oc get route ado-build-api -n "$NAMESPACE" -o jsonpath='{.spec.host}' 2>/dev/null || echo "")). Move or guard the ok "All components running." call so it runs only when BUILD_API_ROUTE is non-empty (or after the existing conditional display blocks that check and handle empty route), ensuring the message reflects actual readiness.
🧹 Nitpick comments (3)
hack/crc/04-expose-default-registry.sh (1)
179-179: Token interpolation may break if token contains special characters.
OC_TOKENis embedded within single quotes inside a double-quoted string passed torun_as_user, which useseval. If the token contains single quotes (unlikely but possible), the command will fail or behave unexpectedly.Consider using
printf %qto safely escape the token:Suggested fix
- run_as_user "podman login -u kubeadmin -p '$OC_TOKEN' '$HOST' --tls-verify='$TLS_VERIFY'" >/dev/null + ESCAPED_TOKEN=$(printf '%q' "$OC_TOKEN") + run_as_user "podman login -u kubeadmin -p $ESCAPED_TOKEN '$HOST' --tls-verify='$TLS_VERIFY'" >/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/04-expose-default-registry.sh` at line 179, The command passed to run_as_user embeds OC_TOKEN inside single quotes and is eval'd, which breaks if OC_TOKEN contains single quotes; before building the command escape the token using printf '%q' (e.g. safe_token=$(printf '%q' "$OC_TOKEN")) and use that escaped variable when constructing the string passed to run_as_user so the login invocation (the run_as_user call that runs podman login) is robust against special characters.hack/crc/README.md (1)
79-86: Script numbering in table may cause confusion.The table lists scripts in numeric order (01, 02, 03, 04), but the actual execution order in
setup-crc.shis 01 → 04 → 02 → 03 (as noted on line 52). Consider either renaming04-expose-default-registry.shto reflect the actual order (e.g.,02-expose-registry.sh) or adding a note clarifying the execution sequence differs from numbering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/README.md` around lines 79 - 86, The script numbering in the README table is misleading because setup-crc.sh runs scripts in order 01 → 04 → 02 → 03; update the README to clarify this by either renaming 04-expose-default-registry.sh to match execution order (e.g., 02-expose-default-registry.sh) and adjusting the table entries and any references, or add a short parenthetical note in the table row or above the table stating "Actual run order in setup-crc.sh: 01 → 04 → 02 → 03" so readers understand the discrepancy; ensure you update the table row for `04-expose-default-registry.sh` (or its new name) and any mention of setup-crc.sh execution to remain consistent.hack/crc/02-deploy-operator.sh (1)
209-211: Consider implications of printing credentials to stdout.The kubeadmin password is printed to stdout, which could end up in CI logs or terminal scrollback. For a local dev tool this is acceptable, but be aware that captured output may expose credentials.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/02-deploy-operator.sh` around lines 209 - 211, The script currently prints the kubeadmin password to stdout using the echo lines and get_kubeadmin_pass, which can leak credentials into CI logs; change this to a safer output method—either write the password to a file with restrictive permissions (create a file, chmod 600, write get_kubeadmin_pass output there and print only the file path) or send the password to stderr (so normal stdout logs don’t capture it) and/or mask it when printing; update the two echo lines that reference get_kubeadmin_pass accordingly and ensure any file-write approach uses secure permissions and clear descriptive context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/crc/01-prep-host.sh`:
- Around line 339-345: The CRC_CUSTOM_ARGS value is being embedded unquoted into
the su - "$CRC_USER" -c command (the line that runs crc start --pull-secret-file
... ${CRC_CUSTOM_ARGS}), allowing command injection; fix by not interpolating
raw user input into the command string — instead validate/sanitize
CRC_CUSTOM_ARGS against a strict whitelist of allowed flags/values (e.g., only
--cpus, --memory, --disk and numeric values) or parse it into a safe array
(CRC_ARGS) and invoke crc start with those arguments as separate, quoted
parameters (pass the array safely to the su invocation rather than concatenating
into the shell command string).
---
Duplicate comments:
In `@hack/crc/02-deploy-operator.sh`:
- Around line 186-194: The script prints ok "All components running."
unconditionally while BUILD_API_ROUTE may be empty; update the logic so the
success message is only printed after validating BUILD_API_ROUTE (the variable
set by BUILD_API_ROUTE=$(oc get route ado-build-api -n "$NAMESPACE" -o
jsonpath='{.spec.host}' 2>/dev/null || echo "")). Move or guard the ok "All
components running." call so it runs only when BUILD_API_ROUTE is non-empty (or
after the existing conditional display blocks that check and handle empty
route), ensuring the message reflects actual readiness.
---
Nitpick comments:
In `@hack/crc/02-deploy-operator.sh`:
- Around line 209-211: The script currently prints the kubeadmin password to
stdout using the echo lines and get_kubeadmin_pass, which can leak credentials
into CI logs; change this to a safer output method—either write the password to
a file with restrictive permissions (create a file, chmod 600, write
get_kubeadmin_pass output there and print only the file path) or send the
password to stderr (so normal stdout logs don’t capture it) and/or mask it when
printing; update the two echo lines that reference get_kubeadmin_pass
accordingly and ensure any file-write approach uses secure permissions and clear
descriptive context.
In `@hack/crc/04-expose-default-registry.sh`:
- Line 179: The command passed to run_as_user embeds OC_TOKEN inside single
quotes and is eval'd, which breaks if OC_TOKEN contains single quotes; before
building the command escape the token using printf '%q' (e.g.
safe_token=$(printf '%q' "$OC_TOKEN")) and use that escaped variable when
constructing the string passed to run_as_user so the login invocation (the
run_as_user call that runs podman login) is robust against special characters.
In `@hack/crc/README.md`:
- Around line 79-86: The script numbering in the README table is misleading
because setup-crc.sh runs scripts in order 01 → 04 → 02 → 03; update the README
to clarify this by either renaming 04-expose-default-registry.sh to match
execution order (e.g., 02-expose-default-registry.sh) and adjusting the table
entries and any references, or add a short parenthetical note in the table row
or above the table stating "Actual run order in setup-crc.sh: 01 → 04 → 02 → 03"
so readers understand the discrepancy; ensure you update the table row for
`04-expose-default-registry.sh` (or its new name) and any mention of
setup-crc.sh execution to remain consistent.
🪄 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: CHILL
Plan: Pro
Run ID: f510fd49-cb87-44be-b69d-a4f144148ffb
📒 Files selected for processing (7)
hack/crc/01-prep-host.shhack/crc/02-deploy-operator.shhack/crc/03-crc-operator-sanity.shhack/crc/04-expose-default-registry.shhack/crc/README.mdhack/crc/crc-cleanup.shhack/crc/setup-crc.sh
✅ Files skipped from review due to trivial changes (1)
- hack/crc/setup-crc.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/crc/03-crc-operator-sanity.sh
There was a problem hiding this comment.
♻️ Duplicate comments (1)
hack/crc/04-expose-default-registry.sh (1)
32-32:⚠️ Potential issue | 🟡 MinorIncorrect PATH modification — appending a file path instead of directory.
Line 32 appends
${CRC_USER_HOME}/.crc/bin/oc(a file) to PATH, but PATH expects directories. This will preventocfrom being found via PATH lookup.Suggested fix
-export PATH="${CRC_USER_HOME}/.crc/bin/oc:$PATH" +export PATH="${CRC_USER_HOME}/.crc/bin:$PATH"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/04-expose-default-registry.sh` at line 32, The PATH modification incorrectly appends the oc binary file path; change the appended entry to the containing directory so PATH contains a directory not a file — update the export of PATH that references ${CRC_USER_HOME}/.crc/bin/oc to use ${CRC_USER_HOME}/.crc/bin instead (preserving the existing PATH concatenation and colon placement) so shells can locate the oc executable.
🧹 Nitpick comments (3)
hack/crc/04-expose-default-registry.sh (3)
80-83: Consider polling for route availability instead of fixed sleep.The fixed
sleep 3may not be sufficient in slower environments. The relateddeploy-catalog.shuses a polling loop with timeout for the same route.Suggested polling approach
-sleep 3 -HOST="$(oc get route default-route -n openshift-image-registry -o jsonpath='{.spec.host}')" -[[ -n "$HOST" ]] || fail "Could not resolve default registry route host." +info "Waiting for default registry route..." +for i in {1..30}; do + HOST="$(oc get route default-route -n openshift-image-registry -o jsonpath='{.spec.host}' 2>/dev/null || echo "")" + [[ -n "$HOST" ]] && break + sleep 2 +done +[[ -n "$HOST" ]] || fail "Could not resolve default registry route host after 60s."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/04-expose-default-registry.sh` around lines 80 - 83, Replace the fixed sleep with a polling loop that queries the route until it produces a host or a timeout elapses: repeatedly run the same oc get route default-route -n openshift-image-registry -o jsonpath='{.spec.host}' (assigning to HOST) with a short sleep between attempts, stop when HOST is non-empty, and call fail "Could not resolve default registry route host." if the timeout is reached; ensure you continue to use the existing HOST variable and the ok "Registry host: $HOST" log on success.
55-61: Consider using array-based command execution instead of eval/su -c with string interpolation.The
run_as_userfunction usesevalandsu -cwith string concatenation. While all current callers pass controlled strings, this pattern can be fragile if future modifications introduce variables that aren't properly escaped.For a dev-only CRC setup script, this is acceptable, but be cautious if extending this function's usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/04-expose-default-registry.sh` around lines 55 - 61, The run_as_user function is using string interpolation with eval and su -c which is fragile; change run_as_user to accept the command as an array (use "$@" or a passed array) and invoke it without eval by calling the command directly for the non-root case and invoking it with a safe exec that preserves argument boundaries for the user case (e.g., use sudo -u "$CRC_USER" -- "$@" or su - "$CRC_USER" -s /bin/sh -c -- "$*" only after properly joining/escaping) instead of su - "$CRC_USER" -c "source ~/.crc_env 2>/dev/null; $1" and remove eval "$1"; update callers to pass the command and args as separate parameters to preserve safety.
193-194: Remove unusedREGISTRY_TLS_VERIFYfrom the deploy-catalog.sh invocation.The suggested instruction passes
REGISTRY_TLS_VERIFY=${TLS_VERIFY}todeploy-catalog.sh, but this variable is never referenced in that script. The script hardcodes--tls-verify=falsein its podman commands, making the environment variable parameter ineffective and potentially misleading to users.Either remove the variable from the suggested command or update
deploy-catalog.shto respect it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/04-expose-default-registry.sh` around lines 193 - 194, The example command exports REGISTRY_TLS_VERIFY=${TLS_VERIFY} but deploy-catalog.sh does not use that variable (it hardcodes --tls-verify=false), so either remove the environment variable from the suggested invocation in hack/crc/04-expose-default-registry.sh or modify deploy-catalog.sh to honor REGISTRY_TLS_VERIFY; specifically, update the echo lines to drop REGISTRY_TLS_VERIFY=${TLS_VERIFY} if you choose the first option, or update deploy-catalog.sh to read REGISTRY_TLS_VERIFY (or TLS_VERIFY) and conditionally set the podman --tls-verify flag accordingly if you choose the second option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@hack/crc/04-expose-default-registry.sh`:
- Line 32: The PATH modification incorrectly appends the oc binary file path;
change the appended entry to the containing directory so PATH contains a
directory not a file — update the export of PATH that references
${CRC_USER_HOME}/.crc/bin/oc to use ${CRC_USER_HOME}/.crc/bin instead
(preserving the existing PATH concatenation and colon placement) so shells can
locate the oc executable.
---
Nitpick comments:
In `@hack/crc/04-expose-default-registry.sh`:
- Around line 80-83: Replace the fixed sleep with a polling loop that queries
the route until it produces a host or a timeout elapses: repeatedly run the same
oc get route default-route -n openshift-image-registry -o
jsonpath='{.spec.host}' (assigning to HOST) with a short sleep between attempts,
stop when HOST is non-empty, and call fail "Could not resolve default registry
route host." if the timeout is reached; ensure you continue to use the existing
HOST variable and the ok "Registry host: $HOST" log on success.
- Around line 55-61: The run_as_user function is using string interpolation with
eval and su -c which is fragile; change run_as_user to accept the command as an
array (use "$@" or a passed array) and invoke it without eval by calling the
command directly for the non-root case and invoking it with a safe exec that
preserves argument boundaries for the user case (e.g., use sudo -u "$CRC_USER"
-- "$@" or su - "$CRC_USER" -s /bin/sh -c -- "$*" only after properly
joining/escaping) instead of su - "$CRC_USER" -c "source ~/.crc_env 2>/dev/null;
$1" and remove eval "$1"; update callers to pass the command and args as
separate parameters to preserve safety.
- Around line 193-194: The example command exports
REGISTRY_TLS_VERIFY=${TLS_VERIFY} but deploy-catalog.sh does not use that
variable (it hardcodes --tls-verify=false), so either remove the environment
variable from the suggested invocation in hack/crc/04-expose-default-registry.sh
or modify deploy-catalog.sh to honor REGISTRY_TLS_VERIFY; specifically, update
the echo lines to drop REGISTRY_TLS_VERIFY=${TLS_VERIFY} if you choose the first
option, or update deploy-catalog.sh to read REGISTRY_TLS_VERIFY (or TLS_VERIFY)
and conditionally set the podman --tls-verify flag accordingly if you choose the
second option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2392d88b-7490-43f1-9ef5-c24c1c290a1f
📒 Files selected for processing (2)
hack/crc/01-prep-host.shhack/crc/04-expose-default-registry.sh
✅ Files skipped from review due to trivial changes (1)
- hack/crc/01-prep-host.sh
Contributor
Author
maboras-rh
force-pushed
the
crc-auto-setup
branch
from
c669f15 to
a6aec06
Compare
bennyz
reviewed
Mar 30, 2026
bennyz
reviewed
Mar 30, 2026
bennyz
reviewed
Mar 30, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
hack/crc/02-deploy-operator.sh (1)
153-155:⚠️ Potential issue | 🟠 MajorDon’t suppress RBAC grant failures for the operator service account.
Line 154 unconditionally ignores errors (
|| true), so auth/API failures can be hidden and the script can continue with broken image-pull permissions.Suggested fix
info "Ensuring image pull permissions for operator service account..." -oc policy add-role-to-user system:image-puller system:serviceaccount:${NAMESPACE}:ado-operator -n "$NAMESPACE" 2>/dev/null || true +if oc policy add-role-to-user system:image-puller "system:serviceaccount:${NAMESPACE}:ado-operator" -n "$NAMESPACE" 2>/dev/null; then + ok "Image pull permissions ensured for ado-operator." +else + fail "Failed to grant image-puller role to serviceaccount ${NAMESPACE}/ado-operator." +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/02-deploy-operator.sh` around lines 153 - 155, The oc RBAC grant command that adds the system:image-puller role to the service account (oc policy add-role-to-user system:image-puller system:serviceaccount:${NAMESPACE}:ado-operator -n "$NAMESPACE") is currently silenced by "|| true"; remove this suppression and instead check the command's exit status, logging a clear error via info/error (including the command output and target serviceaccount ${NAMESPACE}:ado-operator) and exiting non‑zero if it fails so RBAC grant failures are not hidden.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/crc/setup-crc.sh`:
- Around line 22-26: The script currently only handles the root-on-Linux case
and leaves CRC_USER unset for non-root Linux runs causing set -u to crash;
update the startup logic so that on Linux when EUID != 0 you either initialize
CRC_USER from the current user (e.g., ${CRC_USER:-${SUDO_USER:-$USER}}) or print
a clear usage error and exit, and validate with id "$CRC_USER" before any set -u
or dereference; ensure the code paths that reference CRC_USER (the CRC_USER
variable, checks using EUID and id command) are adjusted so CRC_USER is always
defined or the script exits with a helpful message rather than crashing.
- Around line 73-75: The script currently grants CRC_USER passwordless full sudo
via the echo to /etc/sudoers.d/crc-deploy, chmod, and trap lines; since the
deploy script (02-deploy-operator.sh) does not use sudo, remove the lines that
create /etc/sudoers.d/crc-deploy (the echo, chmod 0440, and trap 'rm -f ...'
lines) so no NOPASSWD:ALL is granted, or if you expect a future specific sudo
need, replace the NOPASSWD:ALL entry with a minimal sudoers line listing only
the exact commands needed for CRC_USER.
---
Duplicate comments:
In `@hack/crc/02-deploy-operator.sh`:
- Around line 153-155: The oc RBAC grant command that adds the
system:image-puller role to the service account (oc policy add-role-to-user
system:image-puller system:serviceaccount:${NAMESPACE}:ado-operator -n
"$NAMESPACE") is currently silenced by "|| true"; remove this suppression and
instead check the command's exit status, logging a clear error via info/error
(including the command output and target serviceaccount
${NAMESPACE}:ado-operator) and exiting non‑zero if it fails so RBAC grant
failures are not hidden.
🪄 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: CHILL
Plan: Pro
Run ID: cf0e83da-c53a-4bbf-8c8b-dc92aa7bdc46
📒 Files selected for processing (7)
hack/crc/01-prep-host.shhack/crc/02-deploy-operator.shhack/crc/03-crc-operator-sanity.shhack/crc/04-expose-default-registry.shhack/crc/README.mdhack/crc/crc-cleanup.shhack/crc/setup-crc.sh
✅ Files skipped from review due to trivial changes (3)
- hack/crc/README.md
- hack/crc/04-expose-default-registry.sh
- hack/crc/crc-cleanup.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/crc/01-prep-host.sh
Contributor
|
check the latest coderabbit comments if they're relevant, generally looks ok to me |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/crc/setup-crc.sh (1)
90-100: Consider quoting$E2E_FLAGfor defensive consistency.While
E2E_FLAGis currently controlled (only""or"--sanity"), quoting it would maintain consistency with the other escaped variables and guard against future modifications.su - "$CRC_USER" -c " source ~/.crc_env 2>/dev/null || true eval \$(crc oc-env) cd $REPO_DIR_Q - bash hack/crc/03-crc-operator-sanity.sh $E2E_FLAG + bash hack/crc/03-crc-operator-sanity.sh ${E2E_FLAG:+\"$E2E_FLAG\"} " else cd "$REPO_DIR" - bash "$SCRIPT_DIR/03-crc-operator-sanity.sh" $E2E_FLAG + bash "$SCRIPT_DIR/03-crc-operator-sanity.sh" ${E2E_FLAG:+"$E2E_FLAG"} fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/crc/setup-crc.sh` around lines 90 - 100, Quote the $E2E_FLAG variable when invoking the sanity script to be defensive and consistent: update the command invocations that run "bash hack/crc/03-crc-operator-sanity.sh $E2E_FLAG" (inside the su - block where eval $(crc oc-env) and cd $REPO_DIR_Q are executed) and the else branch invocation "bash \"$SCRIPT_DIR/03-crc-operator-sanity.sh\" $E2E_FLAG" so that $E2E_FLAG is wrapped in quotes; leave other quoting/escaping unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/crc/setup-crc.sh`:
- Around line 90-100: Quote the $E2E_FLAG variable when invoking the sanity
script to be defensive and consistent: update the command invocations that run
"bash hack/crc/03-crc-operator-sanity.sh $E2E_FLAG" (inside the su - block where
eval $(crc oc-env) and cd $REPO_DIR_Q are executed) and the else branch
invocation "bash \"$SCRIPT_DIR/03-crc-operator-sanity.sh\" $E2E_FLAG" so that
$E2E_FLAG is wrapped in quotes; leave other quoting/escaping unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64b5b811-e7be-4f63-8220-7f3d953d0662
📒 Files selected for processing (3)
hack/crc/02-deploy-operator.shhack/crc/04-expose-default-registry.shhack/crc/setup-crc.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/crc/04-expose-default-registry.sh
Automated scripts to install OpenShift Local (CRC), deploy the operator, and validate the deployment on Linux (x86_64) and macOS (arm64). Scripts: - setup-crc.sh: all-in-one orchestrator with --sanity flag - 01-prep-host.sh: system prep, Go install, CRC setup and start - 02-deploy-operator.sh: Tekton, OLM catalog deploy, OperatorConfig - 03-crc-operator-sanity.sh: health checks and optional build test - 04-expose-default-registry.sh: external registry route for podman - crc-cleanup.sh: stop/delete VM, --full for complete removal Assisted-by: Claude-4.6 (Cursor AI) Signed-off-by: Mohamad Abo Ras <maboras@redhat.com>
maboras-rh
force-pushed
the
crc-auto-setup
branch
from
8a55164 to
d23da63
Compare
Contributor
|
I went through the changes, overall it looks good from my side. |
bkhizgiy
approved these changes
Mar 31, 2026
10 tasks
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
3 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
Automated scripts to install OpenShift Local (CRC), deploy the operator, and validate the deployment on Linux (x86_64) and macOS (arm64).
One command to go from bare metal to a running operator with validated sanity checks:
Linux:
sudo bash hack/crc/setup-crc.sh /path/to/pull-secret.txt --sanitymacOS:
bash hack/crc/setup-crc.sh /path/to/pull-secret.txt --sanityScripts
setup-crc.sh01-prep-host.shcrc setup,crc start02-deploy-operator.sh03-crc-operator-sanity.sh--sanity)crc-cleanup.sh--fullremoves binary, user, home directory, cacheBenchmarks
Key design decisions
Summary by CodeRabbit
New Features
Documentation