e2e: remove non-caib build and output logs#151
Merged
Conversation
📝 WalkthroughWalkthroughThe PR refactors end-to-end tests to replace real automotive image builds with a CAIB-based image build flow. It introduces setup for local registry, operator deployment, port-forwarding to Build API, health checks, service account/token management, and build verification within a unified test context. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
bennyz
force-pushed
the
e2e-improve
branch
2 times, most recently
from
02e2589 to
2c2aee6
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/e2e_test.go`:
- Around line 322-330: Replace the hard-coded localhost:8080 checks and kubectl
port-forward mapping by allocating a free ephemeral local port (use
net.Listen("tcp", "localhost:0") to get the OS-assigned port, read the
listener.Addr().(*net.TCPAddr).Port, then close the listener) and reuse that
port for both the health-check/dial and the port-forward command (build the
port-forward argument as "<localPort>:8080" when constructing portForwardCmd)
and for setting CAIB_SERVER/any health-check URLs; update the places using the
literal "8080" (the connection check, kubectl port-forward creation where
portForwardCmd is set, and subsequent CAIB_SERVER/health-check usage) to use the
dynamically chosen port variable.
- Around line 219-300: The BeforeAll block in test/e2e/e2e_test.go runs in the
same Ordered Describe and namespace as earlier tests, so change the setup to use
an isolated namespace or explicitly reset existing resources: create a fresh
namespace (or derive a new name variable) before the label/patch/deploy steps
and use it for subsequent kubectl commands (refer to the BeforeAll function and
namespace variable), or add explicit deletes for OperatorConfig "config",
deployment "ado-build-api", and Task "push-artifact-registry" (the same
resources touched by the patch and waitForPushTask closure) to ensure a clean
slate before calling waitForPushTask and waiting for deployments; ensure new
namespace is labeled with PSA labels and used in LoadImageToKindClusterWithName,
kubectl apply/patch/wait invocations so the waits validate the CAIB bootstrap
path from a fresh context.
- Around line 314-320: Replace the brittle bash|jq|kubectl replace pipeline in
the test with a robust, validated patch: either use kubectl patch (JSONPatch or
strategic merge) to inject "--plain-http --disable-path-validation" into the
task step script and set spec.steps[0].securityContext.runAsUser=0, or if you
must edit JSON, first fetch the task via kubectl get -o json, apply the edit in
Go (unmarshal, modify the spec.steps[0].script and securityContext), marshal and
kubectl apply the result, and then assert by running kubectl get task
push-artifact-registry -n <namespace> -o json and using ExpectWithOffset to
verify the script contains "--plain-http" and runAsUser==0 before continuing;
update the code around exec.Command, utils.Run and ExpectWithOffset to perform
these checked operations and remove the undocumented jq dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5307425d-ee4d-4eb3-919d-ac4ea617f094
📒 Files selected for processing (2)
test/e2e/e2e_suite_test.gotest/e2e/e2e_test.go
Comment on lines
+219
to
+300
| BeforeAll(func() { | ||
| registryHost = os.Getenv("REGISTRY_HOST") | ||
| if registryHost == "" { | ||
| Skip("REGISTRY_HOST not set; caib build tests require a local registry") | ||
| } | ||
| return nil | ||
| } | ||
| EventuallyWithOffset(1, verifyBuildStarted, 2*time.Minute, 5*time.Second).Should(Succeed()) | ||
|
|
||
| By("waiting for build to complete (this may take several minutes)") | ||
| verifyBuildCompleted := func() error { | ||
| cmd = exec.Command("kubectl", "get", "imagebuild", "e2e-real-build", | ||
| "-n", namespace, "-o", "jsonpath={.status.phase}") | ||
| output, err := utils.Run(cmd) | ||
| if err != nil { | ||
| return err | ||
| arch = os.Getenv("ARCH") | ||
| if arch == "" { | ||
| unameCmd := exec.Command("uname", "-m") | ||
| unameOutput, _ := utils.Run(unameCmd) | ||
| switch strings.TrimSpace(string(unameOutput)) { | ||
| case archARM64, aarch64: | ||
| arch = archARM64 | ||
| default: | ||
| arch = "amd64" | ||
| } | ||
| } | ||
| phase := string(output) | ||
| if phase == "Failed" { | ||
| // Get more details on failure | ||
| cmd = exec.Command("kubectl", "get", "imagebuild", "e2e-real-build", | ||
| "-n", namespace, "-o", "jsonpath={.status.message}") | ||
| msg, _ := utils.Run(cmd) | ||
| // Also get PipelineRun logs | ||
| cmd = exec.Command("kubectl", "get", "pipelineruns", "-n", namespace, | ||
| "-l", "automotive.sdv.cloud.redhat.com/imagebuild-name=e2e-real-build", | ||
| "-o", "jsonpath={.items[0].status.conditions[0].message}") | ||
| prMsg, _ := utils.Run(cmd) | ||
| Fail(fmt.Sprintf("Build failed: %s\nPipelineRun message: %s", string(msg), string(prMsg))) | ||
|
|
||
| By("ensuring namespace has privileged PSA labels") | ||
| cmd := exec.Command("kubectl", "label", "namespace", namespace, | ||
| "pod-security.kubernetes.io/enforce=privileged", "--overwrite") | ||
| _, err := utils.Run(cmd) | ||
| ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
| cmd = exec.Command("kubectl", "label", "namespace", namespace, | ||
| "pod-security.kubernetes.io/audit=privileged", "--overwrite") | ||
| _, _ = utils.Run(cmd) | ||
| cmd = exec.Command("kubectl", "label", "namespace", namespace, | ||
| "pod-security.kubernetes.io/warn=privileged", "--overwrite") | ||
| _, _ = utils.Run(cmd) | ||
|
|
||
| By("building the manager(Operator) image") | ||
| cmd = exec.Command("make", "docker-build", fmt.Sprintf("IMG=%s", projectimage)) | ||
| _, err = utils.Run(cmd) | ||
| ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
|
|
||
| By("loading the the manager(Operator) image on Kind") | ||
| err = utils.LoadImageToKindClusterWithName(projectimage) | ||
| ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
|
|
||
| By("installing CRDs") | ||
| cmd = exec.Command("make", "install") | ||
| _, err = utils.Run(cmd) | ||
| ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
|
|
||
| By("deploying the operator") | ||
| cmd = exec.Command("make", "deploy", fmt.Sprintf("IMG=%s", projectimage)) | ||
| _, err = utils.Run(cmd) | ||
| ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
|
|
||
| By("waiting for operator deployment to be available") | ||
| cmd = exec.Command("kubectl", "wait", "--for=condition=available", | ||
| "--timeout=10m", "deployment/ado-operator", "-n", namespace) | ||
| _, err = utils.Run(cmd) | ||
| ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
|
|
||
| By("waiting for Build API deployment to be available") | ||
| cmd = exec.Command("kubectl", "apply", "-f", | ||
| "config/samples/automotive_v1_operatorconfig.yaml") | ||
| _, err = utils.Run(cmd) | ||
| ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
|
|
||
| By("waiting for Build API deployment to be available") | ||
| cmd = exec.Command("kubectl", "wait", "--for=condition=available", | ||
| "--timeout=8m", "deployment/ado-build-api", "-n", namespace) | ||
| _, err = utils.Run(cmd) | ||
| ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
|
|
||
| By("patching OperatorConfig for Kind registry") | ||
| cmd = exec.Command("kubectl", "patch", "operatorconfig", "config", | ||
| "-n", namespace, "--type=merge", | ||
| "-p", fmt.Sprintf(`{"spec":{"osBuilds":{"clusterRegistryRoute":"%s:5000"}}}`, registryHost)) | ||
| _, err = utils.Run(cmd) | ||
| ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
|
|
||
| By("waiting for push-artifact-registry Task to be created") | ||
| waitForPushTask := func() error { | ||
| taskCmd := exec.Command("kubectl", "get", "task", "push-artifact-registry", "-n", namespace) | ||
| _, taskErr := utils.Run(taskCmd) | ||
| return taskErr | ||
| } | ||
| if phase != "Completed" { | ||
| return fmt.Errorf("build not completed yet, phase: %s", phase) | ||
| EventuallyWithOffset(1, waitForPushTask, 2*time.Minute, 5*time.Second).Should(Succeed(), | ||
| "push-artifact-registry Task was not created in time") |
There was a problem hiding this comment.
Isolate the CAIB setup from the earlier Operator spec.
This BeforeAll runs in the same Describe(..., Ordered) and namespace as the earlier It("should run successfully"), so OperatorConfig, ado-build-api, and push-artifact-registry can already exist before this setup starts. That makes the waits here capable of passing on leftover state instead of proving the CAIB bootstrap path works from a clean slate. Please give this context a fresh namespace or explicitly reset those resources before the waits.
🤖 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 219 - 300, The BeforeAll block in
test/e2e/e2e_test.go runs in the same Ordered Describe and namespace as earlier
tests, so change the setup to use an isolated namespace or explicitly reset
existing resources: create a fresh namespace (or derive a new name variable)
before the label/patch/deploy steps and use it for subsequent kubectl commands
(refer to the BeforeAll function and namespace variable), or add explicit
deletes for OperatorConfig "config", deployment "ado-build-api", and Task
"push-artifact-registry" (the same resources touched by the patch and
waitForPushTask closure) to ensure a clean slate before calling waitForPushTask
and waiting for deployments; ensure new namespace is labeled with PSA labels and
used in LoadImageToKindClusterWithName, kubectl apply/patch/wait invocations so
the waits validate the CAIB bootstrap path from a fresh context.
Comment on lines
+322
to
+330
| By("ensuring port 8080 is free before starting port-forward") | ||
| if conn, dialErr := net.DialTimeout("tcp", "localhost:8080", 500*time.Millisecond); dialErr == nil { | ||
| _ = conn.Close() | ||
| Fail("port 8080 is already in use; cannot set up port-forward to Build API") | ||
| } | ||
| return nil | ||
| } | ||
| // Allow up to 10 minutes for the build to complete | ||
| EventuallyWithOffset(1, verifyBuildCompleted, 10*time.Minute, 15*time.Second).Should(Succeed()) | ||
|
|
||
| By("verifying build status has expected fields") | ||
| cmd = exec.Command("kubectl", "get", "imagebuild", "e2e-real-build", | ||
| "-n", namespace, "-o", "jsonpath={.status.pipelineRunName}") | ||
| pipelineRunName, err := utils.Run(cmd) | ||
| ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
| ExpectWithOffset(1, string(pipelineRunName)).NotTo(BeEmpty(), "PipelineRunName should be set") | ||
|
|
||
| cmd = exec.Command("kubectl", "get", "imagebuild", "e2e-real-build", | ||
| "-n", namespace, "-o", "jsonpath={.status.message}") | ||
| message, err := utils.Run(cmd) | ||
| ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
| ExpectWithOffset(1, string(message)).To(ContainSubstring("completed"), "Message should indicate completion") | ||
|
|
||
| By("cleaning up real build resources") | ||
| cmd = exec.Command("kubectl", "delete", "imagebuild", "e2e-real-build", | ||
| "-n", namespace, "--ignore-not-found=true") | ||
| _, _ = utils.Run(cmd) | ||
| }) | ||
| }) | ||
|
|
||
| Context("caib image build", func() { | ||
| var portForwardCmd *exec.Cmd | ||
| var registryHost string | ||
| var arch string | ||
| var caibToken string | ||
| var projectimage = "automotive-dev-operator:test" | ||
| var caibBuildTimeout = 45 * time.Minute // max timeout for caib builds | ||
|
|
||
| BeforeAll(func() { | ||
| registryHost = os.Getenv("REGISTRY_HOST") | ||
| if registryHost == "" { | ||
| Skip("REGISTRY_HOST not set; caib build tests require a local registry") | ||
| } | ||
|
|
||
| arch = os.Getenv("ARCH") | ||
| if arch == "" { | ||
| unameCmd := exec.Command("uname", "-m") | ||
| unameOutput, _ := utils.Run(unameCmd) | ||
| switch strings.TrimSpace(string(unameOutput)) { | ||
| case archARM64, aarch64: | ||
| arch = archARM64 | ||
| default: | ||
| arch = "amd64" | ||
| By("setting up port-forward to Build API") | ||
| portForwardCmd = exec.Command("kubectl", "port-forward", | ||
| "-n", namespace, "svc/ado-build-api", "8080:8080") |
There was a problem hiding this comment.
Use a dynamic local port for the Build API tunnel.
Failing when localhost:8080 is busy makes this suite flaky on shared runners and developer machines. The test should allocate a free local port and reuse it for both the health check and CAIB_SERVER instead of depending on a globally fixed port.
Also applies to: 337-348, 383-384
🤖 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 322 - 330, Replace the hard-coded
localhost:8080 checks and kubectl port-forward mapping by allocating a free
ephemeral local port (use net.Listen("tcp", "localhost:0") to get the
OS-assigned port, read the listener.Addr().(*net.TCPAddr).Port, then close the
listener) and reuse that port for both the health-check/dial and the
port-forward command (build the port-forward argument as "<localPort>:8080" when
constructing portForwardCmd) and for setting CAIB_SERVER/any health-check URLs;
update the places using the literal "8080" (the connection check, kubectl
port-forward creation where portForwardCmd is set, and subsequent
CAIB_SERVER/health-check usage) to use the dynamically chosen port variable.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
changed the title
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test/e2e/e2e_test.go (3)
314-320:⚠️ Potential issue | 🟠 MajorAvoid the
bash | jq | kubectl replacetask rewrite.This still adds a
jqdependency to the e2e environment and relies on a string substitution of the task script, so a small upstream script change can turn the patch into a silent no-op. Prefer a structured patch in Go orkubectl patch, and at minimum assert that the stored task now contains--plain-httpandrunAsUser: 0before continuing.#!/bin/bash set -euo pipefail echo "=== Current e2e patch logic ===" sed -n '308,320p' test/e2e/e2e_test.go echo echo "=== Task/script references in repo ===" rg -n -C2 'push-artifact-registry|disable-path-validation|plain-http' . echo echo "=== jq references in repo/docs ===" rg -n -C2 '\bjq\b' .🤖 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 314 - 320, Replace the current exec.Command("bash", "-c", ...) jq-based rewrite of the Tekton Task "push-artifact-registry" with a structured patch using kubectl patch or the Go Kubernetes client to modify .spec.steps[0].script and .spec.steps[0].securityContext (referencing the cmd variable and the task name "push-artifact-registry" and namespace), then use utils.Run or the client to apply it and finally fetch the Task (same name/namespace) and Assert via ExpectWithOffset that the stored Task's step script contains "--plain-http" and the securityContext includes runAsUser: 0 before proceeding; remove jq/binary dependency and keep error checks identical to the existing ExpectWithOffset usage.
219-300:⚠️ Potential issue | 🟠 MajorRun the CAIB bootstrap in a fresh namespace.
This
BeforeAllstill reusesautomotive-dev-operator-systeminside the sameOrderedcontainer as Line 93-209, so the waits foroperatorconfig/config,deployment/ado-build-api, andtask/push-artifact-registrycan succeed on leftovers from the earlier test instead of proving the CAIB bootstrap path from scratch. Please give this context its own namespace or explicitly delete those resources before the waits.🤖 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 219 - 300, The BeforeAll setup reuses the existing namespace and can pass due to leftover resources; update the BeforeAll block to create and use a fresh namespace (or explicitly delete operatorconfig/config, deployment/ado-build-api, and task/push-artifact-registry) before proceeding: modify the BeforeAll function to either call a helper that creates a unique namespace and assigns it to the namespace variable (and labels it for privileged PSA) or run kubectl delete for the specific resources (operatorconfig/config, deployment/ado-build-api, task/push-artifact-registry) and wait for their removal before the patch/wait steps (references: BeforeAll, operatorconfig/config, deployment/ado-build-api, task/push-artifact-registry, waitForPushTask).
322-330:⚠️ Potential issue | 🟠 MajorUse an ephemeral local port for the Build API tunnel.
Hard-coding
localhost:8080still makes this suite flaky on shared runners and developer machines where that port is already taken. Reserve a free local port once and reuse it for the port-forward, the health probe, andCAIB_SERVER.Suggested fix
+ listener, err := net.Listen("tcp", "127.0.0.1:0") + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + localPort := listener.Addr().(*net.TCPAddr).Port + ExpectWithOffset(1, listener.Close()).To(Succeed()) + portForwardAddr := fmt.Sprintf("127.0.0.1:%d", localPort) - - By("ensuring port 8080 is free before starting port-forward") - if conn, dialErr := net.DialTimeout("tcp", "localhost:8080", 500*time.Millisecond); dialErr == nil { - _ = conn.Close() - Fail("port 8080 is already in use; cannot set up port-forward to Build API") - } By("setting up port-forward to Build API") portForwardCmd = exec.Command("kubectl", "port-forward", - "-n", namespace, "svc/ado-build-api", "8080:8080") + "-n", namespace, "svc/ado-build-api", fmt.Sprintf("%d:8080", localPort)) err = portForwardCmd.Start() ExpectWithOffset(1, err).NotTo(HaveOccurred()) By("waiting for Build API to respond on port-forward") httpClient := &http.Client{Timeout: 2 * time.Second} waitForBuildAPI := func() error { - resp, httpErr := httpClient.Get("http://localhost:8080/v1/healthz") + resp, httpErr := httpClient.Get(fmt.Sprintf("http://%s/v1/healthz", portForwardAddr)) if httpErr != nil { return httpErr } @@ - setErr = os.Setenv("CAIB_SERVER", "http://localhost:8080") + setErr = os.Setenv("CAIB_SERVER", fmt.Sprintf("http://%s", portForwardAddr)) ExpectWithOffset(1, setErr).NotTo(HaveOccurred())Also applies to: 337-348, 383-384
🤖 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 322 - 330, The test currently hard-codes localhost:8080 causing flakes; change to reserve an ephemeral local port by creating a listener on "localhost:0" (e.g., l, _ := net.Listen("tcp", "localhost:0")), extract the port from l.Addr().(*net.TCPAddr).Port, close the listener, then use that port in the port-forward command (portForwardCmd), the health probe dial/URL, and the CAIB_SERVER value instead of "8080"; update the net.DialTimeout check to use the reserved port string and adjust code paths that reference port 8080 (portForwardCmd, health probe helper, and any CAIB_SERVER assignment) to format "localhost:<ephemeralPort>" so the test consistently uses a free local port.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/e2e/e2e_test.go`:
- Around line 314-320: Replace the current exec.Command("bash", "-c", ...)
jq-based rewrite of the Tekton Task "push-artifact-registry" with a structured
patch using kubectl patch or the Go Kubernetes client to modify
.spec.steps[0].script and .spec.steps[0].securityContext (referencing the cmd
variable and the task name "push-artifact-registry" and namespace), then use
utils.Run or the client to apply it and finally fetch the Task (same
name/namespace) and Assert via ExpectWithOffset that the stored Task's step
script contains "--plain-http" and the securityContext includes runAsUser: 0
before proceeding; remove jq/binary dependency and keep error checks identical
to the existing ExpectWithOffset usage.
- Around line 219-300: The BeforeAll setup reuses the existing namespace and can
pass due to leftover resources; update the BeforeAll block to create and use a
fresh namespace (or explicitly delete operatorconfig/config,
deployment/ado-build-api, and task/push-artifact-registry) before proceeding:
modify the BeforeAll function to either call a helper that creates a unique
namespace and assigns it to the namespace variable (and labels it for privileged
PSA) or run kubectl delete for the specific resources (operatorconfig/config,
deployment/ado-build-api, task/push-artifact-registry) and wait for their
removal before the patch/wait steps (references: BeforeAll,
operatorconfig/config, deployment/ado-build-api, task/push-artifact-registry,
waitForPushTask).
- Around line 322-330: The test currently hard-codes localhost:8080 causing
flakes; change to reserve an ephemeral local port by creating a listener on
"localhost:0" (e.g., l, _ := net.Listen("tcp", "localhost:0")), extract the port
from l.Addr().(*net.TCPAddr).Port, close the listener, then use that port in the
port-forward command (portForwardCmd), the health probe dial/URL, and the
CAIB_SERVER value instead of "8080"; update the net.DialTimeout check to use the
reserved port string and adjust code paths that reference port 8080
(portForwardCmd, health probe helper, and any CAIB_SERVER assignment) to format
"localhost:<ephemeralPort>" so the test consistently uses a free local port.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8955d85c-fa68-41d9-969f-91d5100de51c
📒 Files selected for processing (2)
test/e2e/e2e_suite_test.gotest/e2e/e2e_test.go
bennyz
force-pushed
the
e2e-improve
branch
2 times, most recently
from
ac250da to
72035f5
Compare
too slow currently Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test/e2e/e2e_test.go (3)
307-319:⚠️ Potential issue | 🟠 MajorAvoid the
bash | jq | kubectl replacetask rewrite.This patch path is brittle and adds a hidden
jqdependency to the suite, and it never verifies that the task actually ended up with--plain-httpandrunAsUser: 0. Prefer a structuredkubectl patch/Go-side JSON mutation, then assert the mutated fields before continuing.🤖 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 307 - 319, Replace the brittle shell pipeline that rewrites the push-artifact-registry Task (the exec.Command invocations that run "bash -c" with jq and kubectl replace) with a structured mutation using "kubectl patch" (or perform the JSON patch in Go and call the Kubernetes API), targeting the Task named "push-artifact-registry" in the test's namespace; patch the step script to include "--plain-http --disable-path-validation" and set the step securityContext.runAsUser to 0, then use utils.Run or the Kubernetes client to apply the patch and add an explicit assertion (e.g., a kubectl get -o jsonpath check or a Go-side read of the Task) to verify the Task's spec.steps[0].script contains "--plain-http" and spec.steps[0].securityContext.runAsUser == 0 before proceeding.
321-329:⚠️ Potential issue | 🟠 MajorUse an ephemeral local port for the Build API tunnel.
Hard-coding
localhost:8080makes this spec flaky on shared runners and developer machines. Allocate a free local port once and reuse it for the port-forward mapping, the health check URL, andCAIB_SERVERinstead of failing whenever 8080 is busy.Also applies to: 333-347, 382-382
🤖 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 321 - 329, The test currently hard-codes localhost:8080 and fails if that port is busy; change to allocate an ephemeral local port by creating a temporary listener (net.Listen("tcp", "127.0.0.1:0")), read the assigned port from listener.Addr(), close the listener, and reuse that port for the connection check (net.DialTimeout), the kubectl port-forward command (set local mapping as fmt.Sprintf("%d:8080", localPort) when building portForwardCmd), and any health-check URL / CAIB_SERVER environment value used elsewhere; update uses of net.DialTimeout, portForwardCmd (exec.Command("kubectl","port-forward", ...)), and the CAIB_SERVER/health-check assignments accordingly (also apply identical changes in the other occurrences noted around the later blocks).
218-299:⚠️ Potential issue | 🟠 MajorRun the CAIB bootstrap from a clean state.
This
BeforeAllstill reuses the sameOrderedcontainer andautomotive-dev-operator-systemnamespace as the earlier operator smoke test. The waits here can therefore succeed on leftoverOperatorConfig,ado-build-api, andpush-artifact-registrystate instead of proving this CAIB path actually provisions them. Please give this context its own namespace or explicitly delete/reset those resources before starting.🤖 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 218 - 299, The BeforeAll is reusing the existing test namespace and resources so residual OperatorConfig/ado-build-api/push-artifact-registry can make waits pass; modify the BeforeAll (and any kubectl commands that use namespace) to either create and use a fresh namespace (e.g., generate a unique namespace variable like namespaceCAIB and kubectl create namespace before tests) or explicitly delete/reset the prior resources before proceeding (run kubectl delete --ignore-not-found for OperatorConfig named "config", deployment "ado-build-api", and task "push-artifact-registry" in the current namespace and wait for their removal). Update references to namespace in the BeforeAll setup (the BeforeAll block, all kubectl apply/patch/wait invocations, and the waitForPushTask closure) so they target the new namespace or the cleaned state.
🧹 Nitpick comments (1)
test/e2e/e2e_test.go (1)
436-443: Don't drop the CAIB logs on timeout.
utils.RunSafeonly returns the buffered command output after the process exits. If the deadline fires first, this branch fails immediately and never reads that result, so the most useful build logs are lost exactly on timeout failures. Cancel first and print the goroutine result, or stream stdout/stderr toGinkgoWriterwhile the build runs.🤖 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 436 - 443, The timeout branch currently drops CAIB build logs because utils.RunSafe buffers output until the goroutine exits; modify the select handling around containerCh and ctx.Done() so that on timeout you first cancel the build (call the cancel function used to create ctx) and then still drain containerCh to capture and write cr.output/cr.err to GinkgoWriter before failing; alternatively, change the build invocation to stream stdout/stderr into GinkgoWriter as it runs (replace utils.RunSafe buffering with a streaming variant) and ensure references to containerCh, ctx.Done(), caibBuildTimeout and the goroutine that pushes cr are updated so the logs are always emitted on timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/e2e/e2e_test.go`:
- Around line 307-319: Replace the brittle shell pipeline that rewrites the
push-artifact-registry Task (the exec.Command invocations that run "bash -c"
with jq and kubectl replace) with a structured mutation using "kubectl patch"
(or perform the JSON patch in Go and call the Kubernetes API), targeting the
Task named "push-artifact-registry" in the test's namespace; patch the step
script to include "--plain-http --disable-path-validation" and set the step
securityContext.runAsUser to 0, then use utils.Run or the Kubernetes client to
apply the patch and add an explicit assertion (e.g., a kubectl get -o jsonpath
check or a Go-side read of the Task) to verify the Task's spec.steps[0].script
contains "--plain-http" and spec.steps[0].securityContext.runAsUser == 0 before
proceeding.
- Around line 321-329: The test currently hard-codes localhost:8080 and fails if
that port is busy; change to allocate an ephemeral local port by creating a
temporary listener (net.Listen("tcp", "127.0.0.1:0")), read the assigned port
from listener.Addr(), close the listener, and reuse that port for the connection
check (net.DialTimeout), the kubectl port-forward command (set local mapping as
fmt.Sprintf("%d:8080", localPort) when building portForwardCmd), and any
health-check URL / CAIB_SERVER environment value used elsewhere; update uses of
net.DialTimeout, portForwardCmd (exec.Command("kubectl","port-forward", ...)),
and the CAIB_SERVER/health-check assignments accordingly (also apply identical
changes in the other occurrences noted around the later blocks).
- Around line 218-299: The BeforeAll is reusing the existing test namespace and
resources so residual OperatorConfig/ado-build-api/push-artifact-registry can
make waits pass; modify the BeforeAll (and any kubectl commands that use
namespace) to either create and use a fresh namespace (e.g., generate a unique
namespace variable like namespaceCAIB and kubectl create namespace before tests)
or explicitly delete/reset the prior resources before proceeding (run kubectl
delete --ignore-not-found for OperatorConfig named "config", deployment
"ado-build-api", and task "push-artifact-registry" in the current namespace and
wait for their removal). Update references to namespace in the BeforeAll setup
(the BeforeAll block, all kubectl apply/patch/wait invocations, and the
waitForPushTask closure) so they target the new namespace or the cleaned state.
---
Nitpick comments:
In `@test/e2e/e2e_test.go`:
- Around line 436-443: The timeout branch currently drops CAIB build logs
because utils.RunSafe buffers output until the goroutine exits; modify the
select handling around containerCh and ctx.Done() so that on timeout you first
cancel the build (call the cancel function used to create ctx) and then still
drain containerCh to capture and write cr.output/cr.err to GinkgoWriter before
failing; alternatively, change the build invocation to stream stdout/stderr into
GinkgoWriter as it runs (replace utils.RunSafe buffering with a streaming
variant) and ensure references to containerCh, ctx.Done(), caibBuildTimeout and
the goroutine that pushes cr are updated so the logs are always emitted on
timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a67cf8a-b96a-46c0-ba76-b904148fd2dd
📒 Files selected for processing (1)
test/e2e/e2e_test.go
bkhizgiy
approved these changes
Mar 11, 2026
maboras-rh
approved these changes
Mar 11, 2026
| EventuallyWithOffset(1, verifyBuildAPIDeployment, 3*time.Minute, 5*time.Second).Should(Succeed()) | ||
| }) | ||
|
|
||
| It("should build a real automotive image", func() { |
Contributor
There was a problem hiding this comment.
why are we dropping this test? Shouldn't we still test the scenario of building images via CRDs and controllers?
Contributor
Author
There was a problem hiding this comment.
caib image build already triggers this flow anyway, and it takes a non trivial amount so no point in keeping it
Contributor
|
lgtm - approved with short comment. |
10 tasks
10 tasks
14 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.
Remove old building test, switch one build to package mode and drop bootc test to remove flakiness and speed up the e2e
Summary by CodeRabbit