fix: reconfigure local e2e suite for CRC OpenShift compatibility#243
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughSwitches local E2E tooling from Kind/Docker to CRC/OpenShift, updates CRC helper scripts to use CRC_API_URL and per-run cert staging, rewrites the e2e runner to bootstrap CRC and install Tekton via Operator Subscription, and adds OpenShift-aware test utilities and namespace cleanup logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Runner as hack/run-e2e-local.sh
participant CRC as CRC daemon
participant OC as oc CLI
participant Registry as OpenShift Registry
participant BuildAPI as Build API / CAIB
Dev->>Runner: invoke local e2e runner
Runner->>CRC: ensure crc running & source ~/.crc_env
Runner->>OC: run `crc oc-env` and `oc login` (kubeadmin via CRC_API_URL)
Runner->>OC: apply Tekton Subscription and wait for CSV Succeeded
Runner->>OC: detect node ARCH via `oc get nodes`
Runner->>Runner: PrepareOperatorImage() (build/tag/image)
Runner->>Registry: LoadImageToClusterWithName -> resolve registry route and push
Runner->>BuildAPI: run CAIB flows (health/auth checks) against Build API route
BuildAPI->>OC: trigger/monitor in-cluster builds
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
maboras-rh
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
test/utils/utils.go (3)
135-135: Nit: extract"podman"/"docker"to constants (goconst).Three
"podman"occurrences flagged bygoconstin the Lint Code check. Optional cleanup:+const ( + toolPodman = "podman" + toolDocker = "docker" +)and reuse in
resolveContainerTooland the twotool == "podman"branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/utils.go` at line 135, Extract the literal strings "podman" and "docker" into package-level constants (e.g., const podman = "podman", docker = "docker") and replace all direct string occurrences with those constants; update resolveContainerTool and every branch using tool == "podman" (and any tool == "docker") to use the new constants so the three flagged occurrences are consolidated and reused across the file.
202-219:splitImageNamedoes not handle digest references.
foo/bar@sha256:abc...will producetag = "abc..."andname = "foo/bar@sha256", andrepobecomes"bar@sha256". Not an issue for current e2e paths (tag-based refs), but worth documenting or guarding if this helper is reused. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/utils.go` around lines 202 - 219, splitImageName mis-parses digest references (foo/bar@sha256:abc...) as tags; update the function so it first checks for a digest marker by finding lastAt := strings.LastIndex(image, "@") and if lastAt >= 0 treat tag = image[lastAt+1:] and name = image[:lastAt]; otherwise fall back to the existing lastColon/lastSlash logic (use lastColon only when no '@' present and lastColon > lastSlash). Modify splitImageName to prefer the '@' branch so repo extraction (using lastSlash) works correctly for digest refs.
149-150: Avoid shelling out throughbash -cfor a single command.
oc create imagestream ...can be invoked directly withoutbash -c, avoiding any concern around shell-metacharacter interpolation innamespace/repo. While the inputs here are controlled, the direct form is simpler and safer:♻️ Proposed refactor
- _, _ = Run(exec.Command("bash", "-c", - "oc create imagestream "+repo+" -n "+namespace+" 2>/dev/null || true")) + _, _ = Run(exec.Command("oc", "create", "imagestream", repo, "-n", namespace))Ignoring the error already yields the "|| true" semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/utils.go` around lines 149 - 150, Replace the shell invocation that uses bash -c with a direct exec.Command call to avoid shell interpolation: change the Run(exec.Command("bash", "-c", "oc create imagestream "+repo+" -n "+namespace+" 2>/dev/null || true")) to call Run(exec.Command("oc", "create", "imagestream", repo, "-n", namespace)) and simply ignore its returned error (or explicitly discard stderr via the Cmd.Stderr set to ioutil.Discard) to preserve the "|| true" semantics; update the call site in test/utils/utils.go where Run and exec.Command are used.test/e2e/e2e_test.go (2)
356-361: Optional: setMinVersionon TLS configs used against cluster routes.
gosec/ast-grepflags the twotls.Config{InsecureSkipVerify: true}usages (lines 358 and 768) for missingMinVersion. Since these target a cluster-local Route in tests, the practical risk is near zero, but addingMinVersion: tls.VersionTLS12silences the lint noise and matches repository-wide crypto hygiene.-TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, +TLSClientConfig: &tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 356 - 361, Update the test TLS configs to include a minimum TLS version: in the http client creation that sets Transport: &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: true}}, add MinVersion: tls.VersionTLS12 to the tls.Config literal (and do the same for the other tls.Config usage flagged elsewhere in the test file). Locate the tls.Config initializers (e.g., the one assigned inside the httpClient variable and the second tls.Config occurrence) and set MinVersion to tls.VersionTLS12 to satisfy the linter and maintain crypto hygiene.
77-112: Duplicated namespace cleanup/finalizer-stripping logic across suites.The block at 77-112 (controller
BeforeAll) and 586-616 (OIDCBeforeAll) implement essentially the same "delete ns, strip finalizers from blocking resources, wait-for-gone" sequence. Extracting into a helper likecleanupNamespace(namespace)intest/utils(or a local file-scoped helper) would remove ~50 lines of duplication and make future maintenance (e.g., adding new finalizer-prone resources) a single-line change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 77 - 112, Extract the duplicated "delete ns, strip finalizers, wait for gone" sequence into a single helper function cleanupNamespace(namespace string) (e.g., in test/utils or a file-scoped helper) that runs the same steps currently implemented around waitForNamespaceGone: execute kubectl delete ns with --ignore-not-found and --timeout, iterate the resource list ("operatorconfig","taskruns","pipelineruns","persistentvolumeclaims") to patch metadata.finalizers to [], and perform the wait loop (repeating the strip logic while checking kubectl get ns) until the namespace is gone; then replace the duplicated blocks in both BeforeAlls with a single call to cleanupNamespace(namespace) and keep using utils.Run and Eventually as needed.hack/run-e2e-local.sh (1)
46-86:ensure_tektonCSV readiness label selector looks fragile.The label
operators.coreos.com/openshift-pipelines-operator-rh.openshift-operators=is an empty-value selector that depends on OLM having fully reconciled the subscription and labelled the CSV. On a fresh install,oc get csv ... -o jsonpath='{.items[0].status.phase}'may return empty for a long time before OLM creates the CSV, which is fine, but a transient kube-apiserver error in that loop swallows the error (|| true) and retries — also fine. Main risk: if the operator ever ships under a different CSV naming, the selector silently matches nothing forever until the 15-min cap. Consider falling back tooc get csv -n openshift-operators -l operators.coreos.com/openshift-pipelines-operator-rh.openshift-operators -o jsonpath='{.items[?(@.status.phase=="Succeeded")].metadata.name}'or polling by CSV name prefix. Optional hardening.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/run-e2e-local.sh` around lines 46 - 86, The CSV readiness check in ensure_tekton is brittle because it relies on an empty-value label selector; update the polling to be resilient by first attempting to find any CSV with status.phase == "Succeeded" using a jsonpath like {.items[?(@.status.phase=="Succeeded")].metadata.name} and, if that returns empty, fall back to polling CSVs by name prefix (e.g. names starting with "openshift-pipelines-operator-rh") or by listing all CSVs in the openshift-operators namespace and checking their .status.phase values; modify the loop in ensure_tekton to use this two-step approach and fail only after the timeout, ensuring oc errors are captured and handled (not silently swallowed) so the function reliably detects readiness even if labels or CSV naming change.
🤖 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/run-e2e-local.sh`:
- Line 4: The NAMESPACE variable declared in run-e2e-local.sh is unused
(Shellcheck SC2034); either remove the unused
NAMESPACE="automotive-dev-operator-system" line or make the script actually use
it by replacing hard-coded namespace occurrences with the variable (e.g.,
reference NAMESPACE where other scripts currently pass -n
automotive-dev-operator-system) or export NAMESPACE so sub-scripts can consume
it; update any invocation patterns in run-e2e-local.sh that call sub-scripts to
pass or inherit this variable to eliminate the unused-variable warning.
In `@test/e2e/e2e_test.go`:
- Around line 489-499: The test stores the OpenShift token directly into the
global env var REGISTRY_PASSWORD via the block in test/e2e/e2e_test.go (using
utils.Run and os.Setenv), which risks leaking it in downstream tool output;
instead, avoid putting the token in the global process env — pass it only to the
specific commands that need it (set command.Env on exec.Command) or set
REGISTRY_PASSWORD locally and immediately unset it (defer os.Unsetenv) after the
registry auth step, and ensure any test logging/sanitizers (where utils.Run
wraps command output) scrub or mask occurrences of the token; update the
e2e_test.go block to use per-command env injection or temporary setting+clearing
and verify utils.Run does not print the token.
In `@test/utils/utils.go`:
- Around line 134-140: The login call is leaking credentials because Run(cmd)
logs cmd.Args and errors; replace the current login invocation in
pushImageToOpenShiftRegistry to avoid passing the token as a CLI arg: build the
exec.Command for the login tool with the password-stdin flag (remove "-p", use
"--password-stdin" or podman's equivalent), set cmd.Stdin to the token (e.g.,
strings.NewReader(token)), and run cmd.Run() directly instead of Run() so the
token never appears in args/logs; also ensure any returned error or log message
does not include cmd.Args or the raw token (return a sanitized error message if
the login fails). Ensure tlsVerifyArgs() and username are still passed as flags
but never include the token in any logged string.
---
Nitpick comments:
In `@hack/run-e2e-local.sh`:
- Around line 46-86: The CSV readiness check in ensure_tekton is brittle because
it relies on an empty-value label selector; update the polling to be resilient
by first attempting to find any CSV with status.phase == "Succeeded" using a
jsonpath like {.items[?(@.status.phase=="Succeeded")].metadata.name} and, if
that returns empty, fall back to polling CSVs by name prefix (e.g. names
starting with "openshift-pipelines-operator-rh") or by listing all CSVs in the
openshift-operators namespace and checking their .status.phase values; modify
the loop in ensure_tekton to use this two-step approach and fail only after the
timeout, ensuring oc errors are captured and handled (not silently swallowed) so
the function reliably detects readiness even if labels or CSV naming change.
In `@test/e2e/e2e_test.go`:
- Around line 356-361: Update the test TLS configs to include a minimum TLS
version: in the http client creation that sets Transport:
&http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: true}}, add
MinVersion: tls.VersionTLS12 to the tls.Config literal (and do the same for the
other tls.Config usage flagged elsewhere in the test file). Locate the
tls.Config initializers (e.g., the one assigned inside the httpClient variable
and the second tls.Config occurrence) and set MinVersion to tls.VersionTLS12 to
satisfy the linter and maintain crypto hygiene.
- Around line 77-112: Extract the duplicated "delete ns, strip finalizers, wait
for gone" sequence into a single helper function cleanupNamespace(namespace
string) (e.g., in test/utils or a file-scoped helper) that runs the same steps
currently implemented around waitForNamespaceGone: execute kubectl delete ns
with --ignore-not-found and --timeout, iterate the resource list
("operatorconfig","taskruns","pipelineruns","persistentvolumeclaims") to patch
metadata.finalizers to [], and perform the wait loop (repeating the strip logic
while checking kubectl get ns) until the namespace is gone; then replace the
duplicated blocks in both BeforeAlls with a single call to
cleanupNamespace(namespace) and keep using utils.Run and Eventually as needed.
In `@test/utils/utils.go`:
- Line 135: Extract the literal strings "podman" and "docker" into package-level
constants (e.g., const podman = "podman", docker = "docker") and replace all
direct string occurrences with those constants; update resolveContainerTool and
every branch using tool == "podman" (and any tool == "docker") to use the new
constants so the three flagged occurrences are consolidated and reused across
the file.
- Around line 202-219: splitImageName mis-parses digest references
(foo/bar@sha256:abc...) as tags; update the function so it first checks for a
digest marker by finding lastAt := strings.LastIndex(image, "@") and if lastAt
>= 0 treat tag = image[lastAt+1:] and name = image[:lastAt]; otherwise fall back
to the existing lastColon/lastSlash logic (use lastColon only when no '@'
present and lastColon > lastSlash). Modify splitImageName to prefer the '@'
branch so repo extraction (using lastSlash) works correctly for digest refs.
- Around line 149-150: Replace the shell invocation that uses bash -c with a
direct exec.Command call to avoid shell interpolation: change the
Run(exec.Command("bash", "-c", "oc create imagestream "+repo+" -n "+namespace+"
2>/dev/null || true")) to call Run(exec.Command("oc", "create", "imagestream",
repo, "-n", namespace)) and simply ignore its returned error (or explicitly
discard stderr via the Cmd.Stderr set to ioutil.Discard) to preserve the "||
true" semantics; update the call site in test/utils/utils.go where Run and
exec.Command are used.
🪄 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: 45bbfa6b-4b12-49cd-bc7c-a75783d9d6d1
📒 Files selected for processing (5)
hack/crc/04-expose-default-registry.shhack/crc/README.mdhack/run-e2e-local.shtest/e2e/e2e_test.gotest/utils/utils.go
maboras-rh
force-pushed
the
localtest-infra
branch
from
add0dc0 to
f29e9ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/run-e2e-local.sh`:
- Around line 135-136: The shellcheck suppression currently disables SC1090 but
the reported warning for sourcing the dynamic file "$HOME/.crc_env" is SC1091;
update the directive on the sourcing line in hack/run-e2e-local.sh to suppress
the correct code (use disable=SC1091, or include both disable=SC1090,SC1091) so
the dynamic source line "source \"$HOME/.crc_env\"" no longer emits the
shellcheck warning.
- Around line 120-123: The sudo re-exec uses exec sudo -u "$SUDO_USER" -i bash
"$(realpath "$0")" "$@" which may drop the current working directory into the
target user's home and break later repo-relative paths like
hack/crc/04-expose-default-registry.sh and make test-e2e; update the re-exec so
it preserves the repository working directory (repo root) when switching
users—capture the current repo root (e.g., via pwd or realpath on the script or
repo root marker) and run the sudo -u "$SUDO_USER" invocation with a command
that first cds to that repo root before executing "$(realpath "$0")" with the
original "$@" so subsequent commands using repo-relative paths continue to work.
In `@test/utils/utils.go`:
- Around line 159-160: Replace the fragile shell-invocation that builds a "bash
-c" string with a direct exec.Command invocation and stop swallowing errors:
call Run(exec.Command("oc", "create", "imagestream", repo, "-n", namespace)) (or
capture the returned output/error from Run), remove the "2>/dev/null || true"
shell redirection, and handle or return any error so failures to create the
ImageStream are surfaced; update code that calls Run to check the error and fail
the test or log the detailed error if Run reports one.
🪄 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: dbddabdf-2b0c-4834-a02e-2fc956723fad
📒 Files selected for processing (3)
hack/run-e2e-local.shtest/e2e/e2e_test.gotest/utils/utils.go
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
maboras-rh
changed the title
|
@ambient-code please review |
![ambient-code[bot]](https://avatars.githubusercontent.com/u/231559327?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — PR #243: Reconfigure local e2e suite for CRC OpenShift compatibility
Overall: Well-structured PR. The migration from Kind to CRC/OpenShift for local e2e is a significant improvement — it makes local testing match closer to the real deployment target. The helper extraction (CleanupNamespace, PrepareOperatorImage, LoadImageToClusterWithName) reduces duplication nicely, and the Run()/RunSafe() env-merge fix is a subtle but important correctness improvement.
Highlights
- The OIDC test fix (switching from
kubectl applytokubectl patch --type=merge) is a great catch — the full apply would striposBuilds, triggering an unwanted reconcile. - Using
--password-stdinfor registry login inpushImageToOpenShiftRegistryis the right pattern — avoids leaking tokens in process args. - The
set_build_platformchange to read arch from the cluster (kubectl get nodes) instead ofuname -mis correct for CRC scenarios. - The
Eventually-based polling for OIDC config instead oftime.Sleep(10s)is more robust and faster when things work.
Issues
See inline comments for details. One critical item regarding the deleted pull-secret.txt.
maboras-rh
force-pushed
the
localtest-infra
branch
from
4a5c1e4 to
7502fac
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/utils/utils.go (1)
317-323:⚠️ Potential issue | 🟡 MinorAvoid GNU-only
xargs -rin namespace cleanup.On BSD/macOS hosts,
xargs -ris invalid; since the error is ignored, finalizers may never be stripped andCleanupNamespacecan time out. Prefer directkubectlcommands and skip empty names in Go.Suggested cleanup refactor
func stripNamespaceFinalizers(targetNamespace string) { for _, resource := range namespaceFinalizerResources { - stripCmd := exec.Command("bash", "-c", - `kubectl get `+resource+` -n `+targetNamespace+ - ` -o name 2>/dev/null | xargs -r -I{} kubectl patch {} -n `+targetNamespace+ - ` --type=merge -p '{"metadata":{"finalizers":[]}}' 2>/dev/null || true`) - _, _ = Run(stripCmd) + namesOutput, err := Run(exec.Command("kubectl", "get", resource, "-n", targetNamespace, "-o", "name")) + if err != nil { + continue + } + for _, name := range GetNonEmptyLines(string(namesOutput)) { + _, _ = Run(exec.Command( + "kubectl", "patch", name, + "-n", targetNamespace, + "--type=merge", + "-p", `{"metadata":{"finalizers":[]}}`, + )) + } } }#!/bin/bash # Verify whether GNU-only xargs flags remain in Go test helpers. rg -nP 'xargs\s+-r' --type=go -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/utils.go` around lines 317 - 323, stripNamespaceFinalizers currently builds a shell pipeline using GNU-only `xargs -r`, which fails on BSD/macOS and can leave finalizers unstripped; change it to call kubectl get -o name from Go, capture and split the output, skip empty lines, and for each resource name call kubectl patch via exec.Command to remove finalizers (using the same merge patch payload), handling/ignoring errors as before; update references in this function to use namespaceFinalizerResources and Run (or inline exec handling) so the logic is platform-agnostic and does not rely on shell-only xargs flags.
🤖 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/run-e2e-local.sh`:
- Around line 34-44: ensure_crc_login currently treats any valid oc session as
CRC; update it to verify the server is the CRC API endpoint
(https://api.crc.testing:6443) before returning or explicitly set/override
KUBECONFIG to the CRC kubeconfig. Specifically, in ensure_crc_login use `oc
whoami --show-server` and check the output equals https://api.crc.testing:6443
(or fail/continue to login if not), or export KUBECONFIG to the CRC kubeconfig
path prior to calling oc; keep the existing get_kubeadmin_pass and oc login
logic but ensure the server check or KUBECONFIG override prevents operating
against a non-CRC cluster.
- Around line 24-27: The current extraction of pass using jq can cause the
script to exit under set -euo pipefail if crc returns non-JSON; change the
assignment to make jq failures non-fatal, e.g. replace the pass assignment with
a safe subshell that swallows jq errors: pass="$(echo "$json" | jq -r
'.adminCredentials.password // empty' 2>/dev/null || true)". This ensures the
variables json and pass (and the crc console invocation) remain reachable even
when jq fails, and keeps the unique identifiers (json, pass, crc console, jq)
for easy location.
In `@test/e2e/e2e_test.go`:
- Around line 618-623: The HTTP probe currently creates an http.Client with a
custom Transport but no timeout, so requests (the client.Get call to apiURL +
"/v1/auth/config") can hang indefinitely; set the client's Timeout field (e.g.,
Timeout: 10*time.Second or same duration used by other route probes) on the
client struct and ensure time is imported so the request will fail fast instead
of blocking forever.
---
Duplicate comments:
In `@test/utils/utils.go`:
- Around line 317-323: stripNamespaceFinalizers currently builds a shell
pipeline using GNU-only `xargs -r`, which fails on BSD/macOS and can leave
finalizers unstripped; change it to call kubectl get -o name from Go, capture
and split the output, skip empty lines, and for each resource name call kubectl
patch via exec.Command to remove finalizers (using the same merge patch
payload), handling/ignoring errors as before; update references in this function
to use namespaceFinalizerResources and Run (or inline exec handling) so the
logic is platform-agnostic and does not rely on shell-only xargs flags.
🪄 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: 754fa843-51c4-4bf3-a610-f3f10cfc042e
📒 Files selected for processing (5)
hack/crc/04-expose-default-registry.shhack/crc/README.mdhack/run-e2e-local.shtest/e2e/e2e_test.gotest/utils/utils.go
✅ Files skipped from review due to trivial changes (1)
- hack/crc/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/crc/04-expose-default-registry.sh
|
@ambient-code please review |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/run-e2e-local.sh`:
- Around line 49-63: The function ensure_tekton currently returns as soon as
CRDs exist but must also verify the operator/controllers are healthy; update
ensure_tekton so after the CRD loop it additionally polls the OpenShift
Pipelines operator CSV (ensure its phase is "Succeeded") and/or checks the
Tekton controller deployment(s) (e.g., tekton-pipelines-controller or
pipelines-controller) have the desired replicas ready, with a retry loop and
timeout; if these health checks fail, treat as not ready (set ready=false or
wait) so the e2e suite only proceeds when reconciliation is available.
- Around line 34-47: The current ensure_crc_login function only checks for any
authenticated user; change it to verify the identity is the kubeadmin account
and, if not, perform an explicit kubeadmin login using get_kubeadmin_pass.
Specifically, in ensure_crc_login call oc whoami --show-user and ensure it
returns "kubeadmin"; if it does not, obtain the password from get_kubeadmin_pass
and run oc login -u kubeadmin -p "$kube_pass" https://api.crc.testing:6443
--insecure-skip-tls-verify, then re-check identity and fail with a clear message
if login still does not yield kubeadmin; keep references to ensure_crc_login,
get_kubeadmin_pass, oc whoami --show-user, and oc login so the change is easy to
locate.
In `@test/utils/utils.go`:
- Around line 121-128: The closure waitForNamespaceGone currently treats any
kubectl error as success (returns nil), which masks auth/context/API issues;
change it to only return nil when the error/command output clearly indicates the
namespace is NotFound (e.g., contains "NotFound" or "Error from server
(NotFound)"), otherwise return the original error so callers know the check
failed; call stripNamespaceFinalizers(targetNamespace) only when the namespace
still exists (i.e., when kubectl succeeded), and preserve/return the original
err from Run(cmd) for non-NotFound failures so timeouts and retries work
correctly.
🪄 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: 9cd07e60-a6a8-4eb9-b19e-6adcf68262f2
📒 Files selected for processing (3)
hack/run-e2e-local.shtest/e2e/e2e_test.gotest/utils/utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/e2e_test.go
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 `@hack/run-e2e-local.sh`:
- Line 130: The current sudo re-exec line uses exec "$@" which attempts to run
"$SCRIPT_PATH" directly and fails if that file isn't executable; update the
command so the handed-off process invokes the script via bash instead of
executing it directly—for the existing line that references SUDO_USER,
REPO_ROOT, and SCRIPT_PATH, replace the final exec "$@" with exec bash "$@"
(i.e., run the script through bash after the cd/shift handoff) so non-executable
script files will still run under sudo.
In `@test/e2e/e2e_test.go`:
- Around line 382-388: caibEnv is created as an isolated slice, causing
subprocesses to lose inherited environment; change caibEnv to start from
os.Environ() and then append/merge the CAIB-specific entries ("CAIB_TOKEN=...",
"CAIB_SERVER=...", and conditional "CAIB_INSECURE=true") so the subprocess
inherits PATH, HOME, KUBECONFIG, proxy and cert settings while the CAIB
variables override any existing keys. Ensure the merge logic for caibEnv (used
when calling NewCaibCommand and assigning to cmd.Env) replaces existing CAIB_*
entries if present rather than duplicating them.
🪄 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: 2efea175-b979-45e9-aaf3-245996ba940e
📒 Files selected for processing (6)
hack/crc/01-prep-host.shhack/crc/02-deploy-operator.shhack/crc/04-expose-default-registry.shhack/run-e2e-local.shtest/e2e/e2e_test.gotest/utils/utils.go
✅ Files skipped from review due to trivial changes (3)
- hack/crc/01-prep-host.sh
- hack/crc/02-deploy-operator.sh
- test/utils/utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/crc/04-expose-default-registry.sh
maboras-rh
force-pushed
the
localtest-infra
branch
3 times, most recently
from
5e21a11 to
1d5813d
Compare
maboras-rh
force-pushed
the
localtest-infra
branch
from
1d5813d to
68f1fe5
Compare
There was a problem hiding this comment.
Code Review -- PR #243: Reconfigure local e2e suite for CRC OpenShift compatibility
Overall: This is a well-executed migration. The previous review surfaced 14+ items and all have been addressed in subsequent commits. The code is substantially improved since the first review.
The CRC and Kind paths are cleanly separated -- CI continues to use Kind via the GitHub Actions workflow, while hack/run-e2e-local.sh now targets CRC exclusively. The test code uses IsOpenShiftCluster() to branch at the right points without coupling.
What looks good
CleanupNamespacewith finalizer stripping: The robust namespace cleanup logic with--ignore-not-found(addressing bennyz's suggestion) is a real reliability improvement. Stuck finalizers are a common cause of flaky e2e teardown.pushImageToOpenShiftRegistryusing--password-stdin: Properly avoids leaking tokens in process args and GinkgoWriter logs. Good follow-through on the previous review.caibEnvseeded fromos.Environ(): The fix to inherit the parent environment (PATH, HOME, KUBECONFIG, etc.) prevents broken subprocess execution.- CSV readiness check in
ensure_tekton: Checking the CSV phase rather than just CRD presence avoids races where CRDs exist but the controller is still rolling out. setEnvhelper: Clean deduplication logic that avoids pollutingos.Environ()globally. The in-place filtering pattern (filtered := caibEnv[:0]) is correct here because the write index never exceeds the read index.- OIDC test fix: Using
kubectl patch --type=mergeinstead ofkubectl applyto avoid strippingosBuildsis a subtle but important correctness fix.
Issues
See inline comments. No critical items -- two minor issues and two nits.
maboras-rh
force-pushed
the
localtest-infra
branch
2 times, most recently
from
b14635c to
c948b17
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/e2e_test.go (1)
252-257:⚠️ Potential issue | 🔴 CriticalRemove or clarify the
:5000port suffix on external registry routes.The external OpenShift registry route (
default-route-openshift-image-registry.<apps-domain>) is served on standard HTTPS (443), not port 5000. Appending:5000toregistryHostat lines 255 and 471 makesclusterRegistryRouteand thecaib --pushtarget invalid. Either remove the port suffix to use standard HTTPS, or use the internal cluster address (image-registry.openshift-image-registry.svc:5000) if the caib service is deployed in-cluster.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 252 - 257, The test incorrectly appends ":5000" to the external registry host (registryHost) when setting OperatorConfig.spec.osBuilds.clusterRegistryRoute and when forming the caib --push target; update the test to either remove the ":5000" suffix for external routes (use the HTTPS route as-is) or, if you intend to target the in-cluster registry, replace registryHost with the internal service address "image-registry.openshift-image-registry.svc:5000"; specifically modify the fmt.Sprintf call that sets clusterRegistryRoute and the place where the caib push target is constructed so they use the appropriate host form depending on deployment (no port for external HTTPS route or the internal svc:5000 for in-cluster).
🧹 Nitpick comments (5)
hack/run-e2e-local.sh (1)
159-159: 60soc waitmay be tight on slow CRC bring-ups.If CRC has just been started or resumed, the single node can take more than a minute to report
Ready(kubelet/CSR approvals, MCO catch-up). Withset -euo pipefailthis aborts the whole run. Consider bumping the timeout (e.g.,--timeout=10m) — the wait short-circuits as soon as the node is Ready, so a generous upper bound is essentially free.♻️ Suggested change
-oc wait --for=condition=Ready nodes --all --timeout=60s >/dev/null +oc wait --for=condition=Ready nodes --all --timeout=10m >/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/run-e2e-local.sh` at line 159, Increase the timeout on the oc wait call to avoid aborting runs on slow CRC bring-ups: update the command using oc wait --for=condition=Ready nodes --all --timeout=60s to a larger value (e.g., --timeout=10m) in hack/run-e2e-local.sh so the single-node readiness wait in that script (the oc wait invocation) has a generous upper bound but still short-circuits as soon as the node becomes Ready.hack/crc/04-expose-default-registry.sh (1)
90-96: Considermktemp -dinstead of a deterministic per-UID directory.The new path
${TMPDIR:-/tmp}/registry-certs-$(id -u)is predictable, and the stale-dir guard escalates withsudo rm -rfwhenever the directory exists and isn’t writable by the current user. On a shared host, another local user could pre-create that path (or a symlinked variant) before the script runs and trick the privileged removal into operating on a path they control. Usingmktemp -d -t registry-certs.XXXXXX(and trapping cleanup on EXIT) sidesteps both the staleness problem and the privileged removal entirely.♻️ Suggested refactor
-CERT_DIR="${TMPDIR:-/tmp}/registry-certs-$(id -u)" -# Clean up a stale dir left by a previous root run if we can't write to it. -if [[ -d "$CERT_DIR" ]] && [[ ! -w "$CERT_DIR" ]]; then - warn "Stale $CERT_DIR not writable (owned by another user). Removing via sudo..." - sudo rm -rf "$CERT_DIR" -fi -mkdir -p "$CERT_DIR" +CERT_DIR="$(mktemp -d -t registry-certs.XXXXXX)" +trap 'rm -rf "$CERT_DIR"' EXIT🤖 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 90 - 96, Replace the predictable CERT_DIR assignment and sudo-based stale-dir removal with a secure temporary directory created via mktemp -d (e.g., use mktemp -d -t registry-certs.XXXXXX) instead of "${TMPDIR:-/tmp}/registry-certs-$(id -u)"; remove the sudo rm -rf path and the writable check, and install a trap on EXIT to clean up the mktemp-created directory; update uses of CERT_DIR (the mkdir -p and any subsequent usage) to rely on the mktemp result and ensure the warn/error messages still reference CERT_DIR where applicable.test/utils/utils.go (1)
63-66: Behavior shift in env handling — confirm callers seedos.Environ()themselves.The new logic only copies
os.Environ()whencmd.Envis empty; otherwise the caller-supplied slice is used verbatim (plusGO111MODULE=on). This is fine for the current call sites (caibEnvis seeded fromos.Environ()intest/e2e/e2e_test.goline 377, andNewCaibCommandjust passes it through), but any future caller that pre-populatescmd.Envwith only a couple of vars will silently losePATH,HOME,KUBECONFIG, etc. Worth a short comment onRun/RunSafedocumenting that pre-setcmd.Envfully replaces the process environment.📝 Suggested doc tweak
// Run executes the provided command within this context. // Not goroutine-safe due to os.Chdir; use RunSafe for concurrent execution. +// If cmd.Env is non-empty, it fully replaces the process environment (callers +// must include any required vars such as PATH/HOME/KUBECONFIG themselves). func Run(cmd *exec.Cmd) ([]byte, error) {Also applies to: 83-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/utils.go` around lines 63 - 66, Update the documentation in Run and RunSafe to state that if cmd.Env is non-empty it is used verbatim (only "GO111MODULE=on" is appended) and therefore does not inherit the process environment (os.Environ()); callers must seed PATH, HOME, KUBECONFIG, etc. themselves if they pre-populate cmd.Env (refer to cmd.Env, Run, RunSafe and the GO111MODULE=on append logic).test/e2e/e2e_test.go (2)
459-471: Magic strings — extractmyorg/automotive-os-test1to constants.
"myorg"and"automotive-os-test1"appear in both this push reference and the OpenShiftoc create imagestreamcall (line 283). If they ever drift, the imagestream pre-creation and the push target will silently mismatch. Lifting them into the existingconst (...)block at the top of the file (next tostatusRunning) keeps them in sync.const ( artifactImageName = "automotive-os-test1" kindPushNamespace = "myorg" // ... )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 459 - 471, Extract the hardcoded values "myorg" and "automotive-os-test1" into constants (e.g., kindPushNamespace and artifactImageName) in the existing top-level const block next to statusRunning, then replace the literal usages: set pushNamespace's default to kindPushNamespace and use artifactImageName in the registry push fmt.Sprintf and in the OpenShift imagestream creation call so both places reference the same constants (look for pushNamespace, registryHost and containerBuildName to locate the push code and the imagestream creation near the earlier oc create call).
282-284: Inconsistent error handling on ImageStream pre-creation.
utils.pushImageToOpenShiftRegistry(utils.go lines 270-274) treats anything other thanAlreadyExistsas a hard failure, while this call site silently discards every error fromoc create imagestream. If the create fails for a reason other than the resource already existing (RBAC, API outage, etc.), the build will only fail much later inside the Tekton push step and the root cause won't be obvious in the test log. Worth aligning with the helper:♻️ Suggested alignment
- By("pre-creating artifact ImageStream on OpenShift") - cmd = exec.Command("oc", "create", "imagestream", "automotive-os-test1", "-n", namespace) - _, _ = utils.Run(cmd) + By("pre-creating artifact ImageStream on OpenShift") + createIS := exec.Command("oc", "create", "imagestream", "automotive-os-test1", "-n", namespace) + if out, err := utils.Run(createIS); err != nil && + !strings.Contains(string(out), "AlreadyExists") { + Fail(fmt.Sprintf("create ImageStream automotive-os-test1: %v: %s", err, out)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 282 - 284, The pre-creation of the ImageStream in the test currently ignores all errors from exec.Command("oc", "create", "imagestream", ...), causing unrelated failures later; change the call site in the test (the block creating the imagestream) to check the command result and handle errors the same way as utils.pushImageToOpenShiftRegistry: capture stderr/stdout from utils.Run, detect the "AlreadyExists" condition and ignore only that case, but fail the test (using the test harness' failure/assertion mechanism) for any other error (RBAC, API outage, etc.) so root causes show up immediately in the test logs. Ensure you reference the same helper behavior as utils.pushImageToOpenShiftRegistry when implementing the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/e2e/e2e_test.go`:
- Around line 252-257: The test incorrectly appends ":5000" to the external
registry host (registryHost) when setting
OperatorConfig.spec.osBuilds.clusterRegistryRoute and when forming the caib
--push target; update the test to either remove the ":5000" suffix for external
routes (use the HTTPS route as-is) or, if you intend to target the in-cluster
registry, replace registryHost with the internal service address
"image-registry.openshift-image-registry.svc:5000"; specifically modify the
fmt.Sprintf call that sets clusterRegistryRoute and the place where the caib
push target is constructed so they use the appropriate host form depending on
deployment (no port for external HTTPS route or the internal svc:5000 for
in-cluster).
---
Nitpick comments:
In `@hack/crc/04-expose-default-registry.sh`:
- Around line 90-96: Replace the predictable CERT_DIR assignment and sudo-based
stale-dir removal with a secure temporary directory created via mktemp -d (e.g.,
use mktemp -d -t registry-certs.XXXXXX) instead of
"${TMPDIR:-/tmp}/registry-certs-$(id -u)"; remove the sudo rm -rf path and the
writable check, and install a trap on EXIT to clean up the mktemp-created
directory; update uses of CERT_DIR (the mkdir -p and any subsequent usage) to
rely on the mktemp result and ensure the warn/error messages still reference
CERT_DIR where applicable.
In `@hack/run-e2e-local.sh`:
- Line 159: Increase the timeout on the oc wait call to avoid aborting runs on
slow CRC bring-ups: update the command using oc wait --for=condition=Ready nodes
--all --timeout=60s to a larger value (e.g., --timeout=10m) in
hack/run-e2e-local.sh so the single-node readiness wait in that script (the oc
wait invocation) has a generous upper bound but still short-circuits as soon as
the node becomes Ready.
In `@test/e2e/e2e_test.go`:
- Around line 459-471: Extract the hardcoded values "myorg" and
"automotive-os-test1" into constants (e.g., kindPushNamespace and
artifactImageName) in the existing top-level const block next to statusRunning,
then replace the literal usages: set pushNamespace's default to
kindPushNamespace and use artifactImageName in the registry push fmt.Sprintf and
in the OpenShift imagestream creation call so both places reference the same
constants (look for pushNamespace, registryHost and containerBuildName to locate
the push code and the imagestream creation near the earlier oc create call).
- Around line 282-284: The pre-creation of the ImageStream in the test currently
ignores all errors from exec.Command("oc", "create", "imagestream", ...),
causing unrelated failures later; change the call site in the test (the block
creating the imagestream) to check the command result and handle errors the same
way as utils.pushImageToOpenShiftRegistry: capture stderr/stdout from utils.Run,
detect the "AlreadyExists" condition and ignore only that case, but fail the
test (using the test harness' failure/assertion mechanism) for any other error
(RBAC, API outage, etc.) so root causes show up immediately in the test logs.
Ensure you reference the same helper behavior as
utils.pushImageToOpenShiftRegistry when implementing the check.
In `@test/utils/utils.go`:
- Around line 63-66: Update the documentation in Run and RunSafe to state that
if cmd.Env is non-empty it is used verbatim (only "GO111MODULE=on" is appended)
and therefore does not inherit the process environment (os.Environ()); callers
must seed PATH, HOME, KUBECONFIG, etc. themselves if they pre-populate cmd.Env
(refer to cmd.Env, Run, RunSafe and the GO111MODULE=on append logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8cdf6ec2-4c2a-425e-9b85-8c605b2f6b3e
📒 Files selected for processing (7)
hack/crc/01-prep-host.shhack/crc/02-deploy-operator.shhack/crc/04-expose-default-registry.shhack/crc/README.mdhack/run-e2e-local.shtest/e2e/e2e_test.gotest/utils/utils.go
✅ Files skipped from review due to trivial changes (2)
- hack/crc/02-deploy-operator.sh
- hack/crc/README.md
There was a problem hiding this comment.
🧹 Nitpick comments (7)
test/utils/utils.go (4)
54-68: Doc comment slightly misleading: env is not used strictly "as-is".The new comment says “If
cmd.Envis non-empty it is used as-is (callers must include PATH, HOME, etc.).” However, line 68 still unconditionally appendsGO111MODULE=on, so the caller-supplied env is always extended. Worth tweaking the wording so future readers don’t assume their env passes through untouched (and so they realize they shouldn’t set their ownGO111MODULEexpecting it to win — although later entries do override earlier ones inos/exec, so behavior is fine, the comment just lies a bit). Same comment applies toRunSafeat lines 81/89.Suggested wording
-// If cmd.Env is non-empty it is used as-is (callers must include PATH, HOME, etc.). +// If cmd.Env is non-empty it is used as the base environment (callers must include +// PATH, HOME, etc.); GO111MODULE=on is always appended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/utils.go` around lines 54 - 68, Update the doc comments for Run and RunSafe to accurately state that caller-supplied cmd.Env is not preserved strictly "as-is" but is augmented by the function (the function appends the current environment when cmd.Env is empty and always appends GO111MODULE=on), so callers should not assume their env will be untouched or that their GO111MODULE setting will prevail; reference the Run and RunSafe functions and adjust the wording to say the env will be extended/augmented rather than used "as-is."
178-182: Don’t silently dropjson.Marshalerror.
out, _ := json.Marshal(obj)discards a real error path. In practice the input was just unmarshaled so re-marshaling rarely fails, but caller-suppliedstepFields(themap[string]anyargument) can introduce non-marshalable values; in that caseoutis the empty byte slice andkubectl replace -f -will fail with a confusing parse error rather than the actual cause.Suggested fix
- out, _ := json.Marshal(obj) - rep := exec.Command("kubectl", "replace", "-f", "-") + out, err := json.Marshal(obj) + if err != nil { + return fmt.Errorf("task %s: marshal patched task: %w", taskName, err) + } + rep := exec.Command("kubectl", "replace", "-f", "-")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/utils.go` around lines 178 - 182, The code currently ignores the json.Marshal error (out, _ := json.Marshal(obj)), causing misleading kubectl parse errors; change to capture and handle the marshal error (e.g., out, err := json.Marshal(obj); if err != nil { return fmt.Errorf("marshal object for kubectl replace: %w", err) }) before building rep and calling Run, referencing the existing symbols json.Marshal, out, rep, and Run in test/utils/utils.go so the real serialization failure is returned to callers.
185-205: Add a lower-bound check forstepIndex.
getTektonTaskSteponly guards againststepIndex >= len(steps). A negativestepIndex(caller bug) would slip through and panic with an out-of-range index on line 200 (steps[stepIndex]). Cheap to harden:Suggested fix
- if stepIndex >= len(steps) { + if stepIndex < 0 || stepIndex >= len(steps) { return nil, fmt.Errorf("task %s: step %d out of range (%d steps)", taskName, stepIndex, len(steps)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/utils.go` around lines 185 - 205, getTektonTaskStep currently only checks stepIndex >= len(steps) and will panic for negative indices; add a lower-bound check (stepIndex < 0) early after validating steps and return a descriptive error (e.g. "task %s: step %d out of range (%d steps)") to match existing error style so callers get a proper error instead of an out-of-bounds panic. Reference: function getTektonTaskStep, the steps slice variable and the stepIndex parameter.
209-218:NewCaibCommandwith emptyenvproduces a subprocess with no environment.
cmd.Env = append([]string{}, env...)always setscmd.Envto a non-nil slice (even ifenvisnil/empty).os/execonly inherits the parent environment whencmd.Env == nil, so callingNewCaibCommand(ctx, nil, ...)would launchbin/caibwith noPATH,HOME,KUBECONFIG, etc. All current callers intest/e2e/e2e_test.gopass a fully-seededcaibEnv, so this isn’t a live bug — but it is a sharp edge. Consider falling back toos.Environ()whenenvis empty, or documenting that callers must always seed it.Suggested fix
- cmd.Env = append([]string{}, env...) - return cmd + if len(env) == 0 { + cmd.Env = append([]string{}, os.Environ()...) + } else { + cmd.Env = append([]string{}, env...) + } + return cmd🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/utils.go` around lines 209 - 218, NewCaibCommand currently always sets cmd.Env to a non-nil slice (cmd.Env = append([]string{}, env...)), which prevents os/exec from inheriting the parent environment when env is nil/empty; update NewCaibCommand so that if the provided env is nil or has length 0 it falls back to os.Environ() (so cmd.Env inherits the parent environment), otherwise copy the provided env into cmd.Env (e.g., append to a new slice) to avoid sharing the input slice; reference the NewCaibCommand function and the cmd.Env assignment when making the change.test/e2e/e2e_test.go (2)
58-66: Useutils.CleanupNamespacehere too for consistency / robustness.The OIDC suite's
AfterAll(line 616) and the controllerBeforeAll(line 51) both useutils.CleanupNamespace, which strips finalizers and polls until the namespace is gone. ThisAfterAllstill does a one-shotkubectl delete ns ... --timeout=60sand discards the error, so a namespace stuck inTerminating(e.g. due to leftover OperatorConfig or TaskRun finalizers) silently leaks. Since the next suite (OIDC Authentication) re-creates the same namespace, this can also race with namespace teardown —CleanupNamespacealready handles that wait.Suggested change
AfterAll(func() { By("deleting OperatorConfig resources") cmd := exec.Command("kubectl", "delete", "operatorconfig", "--all", "-n", namespace, "--timeout=30s") _, _ = utils.Run(cmd) By("removing manager namespace") - cmd = exec.Command("kubectl", "delete", "ns", namespace, "--timeout=60s") - _, _ = utils.Run(cmd) + utils.CleanupNamespace(namespace) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 58 - 66, Replace the one-shot kubectl delete calls in the AfterAll block with the existing cleanup helper: call utils.CleanupNamespace(namespace) (or the appropriate signature used elsewhere) instead of exec.Command("kubectl", "delete", "ns", ...) and remove the operatorconfig delete/no-op error discard; if needed, ensure OperatorConfig resources are deleted via utils.CleanupNamespace (or call utils.CleanupNamespace after removing OperatorConfig) so finalizers are stripped and the code polls until the namespace is fully removed—update the AfterAll that currently references exec.Command and utils.Run to use utils.CleanupNamespace and remove the discarded error handling.
282-287: Hardcoded ImageStream name should be derived fromartifactImageName.Line 286 hardcodes
"automotive-os-test1", but the same repo is referenced via the newartifactImageName = "automotive-os-test1:latest"const used at line 478. If the artifact name ever changes, these two will drift. Either split atektonArtifactRepo = "automotive-os-test1"constant (and reuse it inartifactImageName), or split at the use site:Example fix
+const tektonArtifactRepo = "automotive-os-test1" +const artifactImageName = tektonArtifactRepo + ":latest"- cmd = exec.Command("oc", "create", "imagestream", "automotive-os-test1", "-n", namespace) + cmd = exec.Command("oc", "create", "imagestream", tektonArtifactRepo, "-n", namespace)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 282 - 287, The ImageStream name is hardcoded ("automotive-os-test1") while the test already defines artifactImageName, causing drift; update the ImageStream creation to derive the repo name from artifactImageName (or introduce a tektonArtifactRepo constant and build artifactImageName from it) and use that variable in the exec.Command call that creates the imagestream (the code that sets cmd = exec.Command("oc", "create", "imagestream", ...)). Ensure you extract the repository portion (strip the :tag if present) so the same symbol (artifactImageName or new tektonArtifactRepo) is reused for both the ImageStream creation and the later artifact references.hack/crc/04-expose-default-registry.sh (1)
14-14: Optional: allowCRC_API_URLto be overridden via env.
CRC_API_URLis hardcoded with=instead of${CRC_API_URL:-...}. Other env-tunable knobs in this script (e.g.,REGISTRY_TLS_VERIFY,DISABLE_REDIRECT) follow the override pattern, andhack/run-e2e-local.shdefines its ownCRC_API_URL. Allowing override here keeps this script consistent and re-usable in non-default CRC setups.Suggested change
-CRC_API_URL="https://api.crc.testing:6443" +CRC_API_URL="${CRC_API_URL:-https://api.crc.testing:6443}"🤖 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 14, Replace the hardcoded CRC_API_URL assignment with a shell parameter expansion that allows an environment override: change the assignment of the CRC_API_URL variable (the line currently setting CRC_API_URL="https://api.crc.testing:6443") to use ${CRC_API_URL:-https://api.crc.testing:6443} so callers can override it via env; follow the same override pattern used by REGISTRY_TLS_VERIFY and DISABLE_REDIRECT to keep the script consistent with hack/run-e2e-local.sh.
🤖 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/04-expose-default-registry.sh`:
- Line 14: Replace the hardcoded CRC_API_URL assignment with a shell parameter
expansion that allows an environment override: change the assignment of the
CRC_API_URL variable (the line currently setting
CRC_API_URL="https://api.crc.testing:6443") to use
${CRC_API_URL:-https://api.crc.testing:6443} so callers can override it via env;
follow the same override pattern used by REGISTRY_TLS_VERIFY and
DISABLE_REDIRECT to keep the script consistent with hack/run-e2e-local.sh.
In `@test/e2e/e2e_test.go`:
- Around line 58-66: Replace the one-shot kubectl delete calls in the AfterAll
block with the existing cleanup helper: call utils.CleanupNamespace(namespace)
(or the appropriate signature used elsewhere) instead of exec.Command("kubectl",
"delete", "ns", ...) and remove the operatorconfig delete/no-op error discard;
if needed, ensure OperatorConfig resources are deleted via
utils.CleanupNamespace (or call utils.CleanupNamespace after removing
OperatorConfig) so finalizers are stripped and the code polls until the
namespace is fully removed—update the AfterAll that currently references
exec.Command and utils.Run to use utils.CleanupNamespace and remove the
discarded error handling.
- Around line 282-287: The ImageStream name is hardcoded ("automotive-os-test1")
while the test already defines artifactImageName, causing drift; update the
ImageStream creation to derive the repo name from artifactImageName (or
introduce a tektonArtifactRepo constant and build artifactImageName from it) and
use that variable in the exec.Command call that creates the imagestream (the
code that sets cmd = exec.Command("oc", "create", "imagestream", ...)). Ensure
you extract the repository portion (strip the :tag if present) so the same
symbol (artifactImageName or new tektonArtifactRepo) is reused for both the
ImageStream creation and the later artifact references.
In `@test/utils/utils.go`:
- Around line 54-68: Update the doc comments for Run and RunSafe to accurately
state that caller-supplied cmd.Env is not preserved strictly "as-is" but is
augmented by the function (the function appends the current environment when
cmd.Env is empty and always appends GO111MODULE=on), so callers should not
assume their env will be untouched or that their GO111MODULE setting will
prevail; reference the Run and RunSafe functions and adjust the wording to say
the env will be extended/augmented rather than used "as-is."
- Around line 178-182: The code currently ignores the json.Marshal error (out, _
:= json.Marshal(obj)), causing misleading kubectl parse errors; change to
capture and handle the marshal error (e.g., out, err := json.Marshal(obj); if
err != nil { return fmt.Errorf("marshal object for kubectl replace: %w", err) })
before building rep and calling Run, referencing the existing symbols
json.Marshal, out, rep, and Run in test/utils/utils.go so the real serialization
failure is returned to callers.
- Around line 185-205: getTektonTaskStep currently only checks stepIndex >=
len(steps) and will panic for negative indices; add a lower-bound check
(stepIndex < 0) early after validating steps and return a descriptive error
(e.g. "task %s: step %d out of range (%d steps)") to match existing error style
so callers get a proper error instead of an out-of-bounds panic. Reference:
function getTektonTaskStep, the steps slice variable and the stepIndex
parameter.
- Around line 209-218: NewCaibCommand currently always sets cmd.Env to a non-nil
slice (cmd.Env = append([]string{}, env...)), which prevents os/exec from
inheriting the parent environment when env is nil/empty; update NewCaibCommand
so that if the provided env is nil or has length 0 it falls back to os.Environ()
(so cmd.Env inherits the parent environment), otherwise copy the provided env
into cmd.Env (e.g., append to a new slice) to avoid sharing the input slice;
reference the NewCaibCommand function and the cmd.Env assignment when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27bda775-3820-4fd0-8d56-c806b7e46711
📒 Files selected for processing (3)
hack/crc/04-expose-default-registry.shtest/e2e/e2e_test.gotest/utils/utils.go
|
lgtm @bkhizgiy please look as well |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
test/e2e/e2e_test.go (1)
379-395: Optional: simplify per-test env handling.The
setEnvclosure uses an in-place filter (caibEnv[:0]aliases the backing array) and is correct, but it is non-obvious. SincecaibEnvis consumed byNewCaibCommandwhich fully overridescmd.Env, amap[string]stringkeyed by var name (rendered to a[]stringonce before eachNewCaibCommandcall, or via a small helper) reads more clearly and makes intent (override-or-insert) explicit.♻️ Sketch
- By("setting caib environment variables") - caibEnv = append([]string{}, os.Environ()...) - setEnv := func(key, value string) { - prefix := key + "=" - filtered := caibEnv[:0] - for _, entry := range caibEnv { - if !strings.HasPrefix(entry, prefix) { - filtered = append(filtered, entry) - } - } - caibEnv = append(filtered, prefix+value) - } - setEnv("CAIB_TOKEN", caibToken) - setEnv("CAIB_SERVER", caibServer) - if openShiftCluster { - setEnv("CAIB_INSECURE", "true") - } + By("setting caib environment variables") + overrides := map[string]string{ + "CAIB_TOKEN": caibToken, + "CAIB_SERVER": caibServer, + } + if openShiftCluster { + overrides["CAIB_INSECURE"] = "true" + }…then materialize
caibEnvonce after the registry credential block (filteringos.Environ()keys present inoverridesand appending the overrides).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 379 - 395, The test's setEnv closure mutates caibEnv in-place which is non-obvious; replace this with a simple map-based override approach: create a map[string]string (e.g., overrides) and set keys "CAIB_TOKEN", "CAIB_SERVER" and conditionally "CAIB_INSECURE" into it, then materialize caibEnv by starting from os.Environ(), filtering out any keys present in overrides, and appending "KEY=VALUE" strings before calling NewCaibCommand so intent (override-or-insert) is explicit and easier to read.test/utils/utils.go (3)
103-113: Confusing override semantics forOPENSHIFT_CLUSTER.Setting
OPENSHIFT_CLUSTER=false(or0/no) does not disable OpenShift detection — the env var is only honored when truthy, otherwise the function falls through toHasOpenShiftRouteCRD(). For an explicit override knob this is surprising; a reader might assume a falsy value forces non-OpenShift mode. Either tighten the doc to make the "truthy override only, otherwise probe" rule explicit, or usestrconv.ParseBooland treat any successfully parsed value as authoritative.♻️ Suggested fix
func IsOpenShiftCluster() bool { - v := strings.TrimSpace(os.Getenv("OPENSHIFT_CLUSTER")) - if strings.EqualFold(v, "true") || v == "1" { - return true - } - return HasOpenShiftRouteCRD() + if v := strings.TrimSpace(os.Getenv("OPENSHIFT_CLUSTER")); v != "" { + if b, err := strconv.ParseBool(v); err == nil { + return b + } + } + return HasOpenShiftRouteCRD() }(Requires
import "strconv".)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/utils.go` around lines 103 - 113, The IsOpenShiftCluster() helper currently only honors truthy OPENSHIFT_CLUSTER values and otherwise probes with HasOpenShiftRouteCRD(), which makes a falsy env var non-authoritative; change it to parse the env var with strconv.ParseBool and, if parsing succeeds, return that boolean (so explicit "false" disables OpenShift), otherwise fall back to HasOpenShiftRouteCRD(); update imports to include "strconv" and keep the function name IsOpenShiftCluster and the probe function HasOpenShiftRouteCRD referenced exactly as-is.
154-210: Document thatstepFieldsreplaces, not merges.
PatchTektonTaskStepoverwrites each top-level key instepFieldswholesale on the target step (e.g., passing{"securityContext": {"runAsUser": 0}}will drop any existingsecurityContext.runAsNonRoot/capabilitiesset by the operator). For the current Kind callsite this is fine since the generated step has nosecurityContext, but a brief godoc note on the function would prevent future foot-guns when callers want to merge into existing structured fields.📝 Suggested doc tweak
// PatchTektonTaskStep fetches a Tekton Task, applies script replacements and optional // field overrides on step[stepIndex], then runs kubectl replace. +// stepFields entries are assigned as-is to the step map, replacing any existing value +// at that top-level key (no recursive merge); pass a fully-formed sub-tree if you need +// to preserve sibling keys. func PatchTektonTaskStep(namespace, taskName string, stepIndex int, scriptReplacements map[string]string, stepFields map[string]any) error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/utils.go` around lines 154 - 210, Add a short godoc to PatchTektonTaskStep stating that the stepFields map overwrites top-level keys on the target step rather than merging nested objects (e.g., providing {"securityContext": {"runAsUser": 0}} will replace any existing securityContext rather than merge runAsNonRoot/capabilities); mention callers should merge structured fields themselves if they need preservation or perform a deep-merge before calling PatchTektonTaskStep. Include the note near the PatchTektonTaskStep declaration so it's visible to callers and reference that behavior applies to the step mutation performed after getTektonTaskStep returns the step map.
351-356: Add a comment explaining the intentional CRC-specific default, or consider flipping the default to match podman.The function defaults to
--tls-verify=false, which contradicts podman's default of--tls-verify=true. This intentional insecure default makes sense for CRC's self-signed certificates (documented inhack/crc/README.md), but risks silently disabling cert verification when the code is used against a non-CRC OpenShift cluster unless callers explicitly setREGISTRY_TLS_VERIFY=true. Either add a code comment documenting that this is CRC-specific behavior, or flip the default to require opt-out via env var for backward compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/utils.go` around lines 351 - 356, The tlsVerifyArgs() function currently returns "--tls-verify=false" by default which is CRC-specific; update the function tlsVerifyArgs() to either (A) add a clear code comment above tlsVerifyArgs() stating this intentional CRC-specific insecure default and referencing hack/crc/README.md so callers know why the default differs from podman, or (B) flip the default to "--tls-verify=true" and require callers to opt-out via REGISTRY_TLS_VERIFY=false; choose one approach and make the change consistently where tlsVerifyArgs() is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/e2e_test.go`:
- Around line 379-395: The test's setEnv closure mutates caibEnv in-place which
is non-obvious; replace this with a simple map-based override approach: create a
map[string]string (e.g., overrides) and set keys "CAIB_TOKEN", "CAIB_SERVER" and
conditionally "CAIB_INSECURE" into it, then materialize caibEnv by starting from
os.Environ(), filtering out any keys present in overrides, and appending
"KEY=VALUE" strings before calling NewCaibCommand so intent (override-or-insert)
is explicit and easier to read.
In `@test/utils/utils.go`:
- Around line 103-113: The IsOpenShiftCluster() helper currently only honors
truthy OPENSHIFT_CLUSTER values and otherwise probes with
HasOpenShiftRouteCRD(), which makes a falsy env var non-authoritative; change it
to parse the env var with strconv.ParseBool and, if parsing succeeds, return
that boolean (so explicit "false" disables OpenShift), otherwise fall back to
HasOpenShiftRouteCRD(); update imports to include "strconv" and keep the
function name IsOpenShiftCluster and the probe function HasOpenShiftRouteCRD
referenced exactly as-is.
- Around line 154-210: Add a short godoc to PatchTektonTaskStep stating that the
stepFields map overwrites top-level keys on the target step rather than merging
nested objects (e.g., providing {"securityContext": {"runAsUser": 0}} will
replace any existing securityContext rather than merge
runAsNonRoot/capabilities); mention callers should merge structured fields
themselves if they need preservation or perform a deep-merge before calling
PatchTektonTaskStep. Include the note near the PatchTektonTaskStep declaration
so it's visible to callers and reference that behavior applies to the step
mutation performed after getTektonTaskStep returns the step map.
- Around line 351-356: The tlsVerifyArgs() function currently returns
"--tls-verify=false" by default which is CRC-specific; update the function
tlsVerifyArgs() to either (A) add a clear code comment above tlsVerifyArgs()
stating this intentional CRC-specific insecure default and referencing
hack/crc/README.md so callers know why the default differs from podman, or (B)
flip the default to "--tls-verify=true" and require callers to opt-out via
REGISTRY_TLS_VERIFY=false; choose one approach and make the change consistently
where tlsVerifyArgs() is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3dda217-5a44-4cde-aad7-d00729cb9bb3
📒 Files selected for processing (3)
hack/crc/04-expose-default-registry.shtest/e2e/e2e_test.gotest/utils/utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/crc/04-expose-default-registry.sh
maboras-rh
force-pushed
the
localtest-infra
branch
3 times, most recently
from
fb9c2d3 to
4b51aa6
Compare
Leverage CRC OpenShift cluster into local E2e Test execution, instead of using kind cluster. while keeping E2E Tests workflow in Github Actions using kind cluster. Assisted-by: Claude-4.6 (Cursor AI) Signed-off-by: Mohamad Abo Ras <maboras@redhat.com> --------- Signed-off-by: Mohamad Abo Ras <maboras@redhat.com>
maboras-rh
force-pushed
the
localtest-infra
branch
from
4b51aa6 to
69c29b7
Compare
3 participants
Summary
Leverage CRC OpenShift cluster into local E2e Test execution, instead of using
kindcluster.while keeping
E2E Testsworkflow in Github Actions usingkindcluster.Related Issues
Relates to #176
Type of Change
Testing
make test)make lint)make manifests generate)Summary by CodeRabbit
Documentation
Chores
Tests