caib: add progress bar#125
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a server-side build progress API and schemas, client GetBuildProgress, pod-annotation progress emission in task scripts, CLI progress-bar rendering integrated into the build-wait flow, RBAC to permit pod patching, and pipeline/task wiring changes (remove prepare-builder, add rebuild-builder). Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI / User
participant API as API Server
participant K8s as Kubernetes (Tekton / Pods)
participant Scripts as Build Scripts
CLI->>API: POST /v1/builds/{name} (start build)
API->>K8s: Create PipelineRun / TaskRuns
K8s->>Scripts: Execute tasks (build scripts)
Scripts->>K8s: emit_progress -> PATCH pod annotation
loop Poll (when not following logs)
CLI->>API: GET /v1/builds/{name}/progress
API->>K8s: List pods / read annotations
K8s-->>API: Pod annotations with progress markers
API->>API: Aggregate -> BuildProgress
API-->>CLI: BuildProgress {Phase, Step}
CLI->>CLI: Render progressBar (TTY or plain)
end
K8s-->>API: PipelineRun completes
CLI->>API: GET /v1/builds/{name} (final status)
API-->>CLI: Final build result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/buildapi/progress.go (1)
106-116: Consider usingdeferfor stream closure to prevent resource leaks.If
scanner.Scan()or any future code betweenStream()andClose()were to panic, the stream would leak. Wrapping the per-container log reading in a closure withdefer stream.Close()is more idiomatic Go and safer.♻️ Suggested refactor
- stream, err := cs.CoreV1().Pods(namespace).GetLogs(pod.Name, logOpts).Stream(ctx) - if err != nil { - continue - } - scanner := bufio.NewScanner(stream) - for scanner.Scan() { - if step, ok := parseProgressMarker(scanner.Text()); ok { - latest = step + func() { + stream, err := cs.CoreV1().Pods(namespace).GetLogs(pod.Name, logOpts).Stream(ctx) + if err != nil { + return } - } - _ = stream.Close() + defer stream.Close() + scanner := bufio.NewScanner(stream) + for scanner.Scan() { + if step, ok := parseProgressMarker(scanner.Text()); ok { + latest = step + } + } + }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/buildapi/progress.go` around lines 106 - 116, The per-pod log stream opened with cs.CoreV1().Pods(...).GetLogs(...).Stream(ctx) is closed explicitly with _ = stream.Close(), which can leak if scanner.Scan() panics or returns early; wrap the code that reads the stream in an inline closure (e.g., anonymous func() { ... }()) immediately after successfully obtaining stream, call defer stream.Close() at the top of that closure, and move the bufio.NewScanner(stream)/for scanner.Scan() logic inside it (also handle scanner.Err() if present) and remove the explicit _ = stream.Close() call; this applies to the code that uses parseProgressMarker and updates latest.internal/common/tasks/scripts/build_image.sh (2)
321-333: Heartbeat-while-wait pattern is repeated four times — consider extracting a helper.The same background-heartbeat + wait + kill pattern appears for the bootc build (lines 322–333), image/package build (lines 409–420), disk build (lines 449–460), and builder preparation (lines 233–244). A helper function like
run_with_heartbeat <pid> <stage> <step> <total>would reduce duplication and lower the risk of copy-paste mistakes.♻️ Example helper
# Run a heartbeat loop while a background process is alive. # Usage: run_with_heartbeat <pid> <stage> <step> <total> run_with_heartbeat() { local pid="$1" stage="$2" step="$3" total="$4" while kill -0 "$pid" 2>/dev/null; do emit_progress "$stage" "$step" "$total" sleep 30 done & local hb_pid=$! wait "$pid" local rc=$? kill "$hb_pid" 2>/dev/null; wait "$hb_pid" 2>/dev/null || true return "$rc" }Then each call site simplifies to:
aib --verbose build ... & AIB_PID=$! run_with_heartbeat "$AIB_PID" "Building image" "$STEP_BUILD" "$PROGRESS_TOTAL" || exit $?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/build_image.sh` around lines 321 - 333, Extract the repeated background-heartbeat + wait + kill logic into a helper function named run_with_heartbeat that accepts the background PID, stage message, step variable and total variable (e.g., run_with_heartbeat <pid> <stage> <step> <total>); implement it to run the emit_progress loop while kill -0 "$pid" succeeds (sleep 30), spawn that loop in background, wait for the target pid, capture its exit code, kill and wait the heartbeat pid, and return the captured exit code. Replace each duplicated block that currently uses patterns with variables like AIB_PID, BOOTC_PID, IMAGE_PID, BUILDER_PREPARE_PID (or similarly named PIDs) with a call to run_with_heartbeat "$PID" "Building image"/"Booting build"/... "$STEP_BUILD" "$PROGRESS_TOTAL" and propagate non-zero returns with exit $?. Ensure the helper uses local variables for pid/hb_pid/rc and that callers use the helper's return code to decide exit.
3-8:emit_progressis duplicated across four scripts, but centralizing it incommon.shwon't work for all of them.The function is defined in
build_image.sh,build_builder.sh,flash_image.sh, andpush_artifact.sh. However,common.shis only prepended tobuild_image.shandbuild_builder.shat embed time (as stated in its header and confirmed by theinit()function). Thepush_artifact.shandflash_image.shscripts are embedded independently and do not includecommon.sh.Additionally,
push_artifact.shomits thelocalkeyword in itsemit_progressfunction (stage="$1"; done="$2"; total="$3"), which causes those variables to leak into the calling scope. The other three scripts correctly uselocal.To address this, either:
- Prepend
common.shto all four scripts in the embedding logic and moveemit_progresstocommon.sh, or- Accept the duplication as necessary given the current architecture and add the
localkeyword topush_artifact.shfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/build_image.sh` around lines 3 - 8, The emit_progress function is duplicated across build_image.sh, build_builder.sh, flash_image.sh, and push_artifact.sh and push_artifact.sh's implementation leaks variables (missing local); either prepend common.sh into the embedding process for all four scripts and move emit_progress into common.sh (remove duplicates from build_image.sh, build_builder.sh, flash_image.sh, push_artifact.sh), or keep duplication but update the emit_progress in push_artifact.sh to declare its parameters with local (e.g., local stage="$1" local done="$2" local total="$3") so variables do not leak; locate the function by name emit_progress to apply the 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 `@cmd/caib/main.go`:
- Around line 1847-1853: Clamp the progress values before building the bar:
ensure filled is computed as an int bounded between 0 and barWidth (e.g., filled
= max(0, min(barWidth, barWidth*step.Done/step.Total))) and compute remaining =
barWidth - filled (guaranteed non-negative) before calling strings.Repeat so
strings.Repeat never receives a negative count; update the block that sets
barWidth, filled, and bar (referencing variables barWidth, filled, step.Done,
step.Total and the strings.Repeat calls) to perform this clamping.
---
Nitpick comments:
In `@internal/buildapi/progress.go`:
- Around line 106-116: The per-pod log stream opened with
cs.CoreV1().Pods(...).GetLogs(...).Stream(ctx) is closed explicitly with _ =
stream.Close(), which can leak if scanner.Scan() panics or returns early; wrap
the code that reads the stream in an inline closure (e.g., anonymous func() {
... }()) immediately after successfully obtaining stream, call defer
stream.Close() at the top of that closure, and move the
bufio.NewScanner(stream)/for scanner.Scan() logic inside it (also handle
scanner.Err() if present) and remove the explicit _ = stream.Close() call; this
applies to the code that uses parseProgressMarker and updates latest.
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 321-333: Extract the repeated background-heartbeat + wait + kill
logic into a helper function named run_with_heartbeat that accepts the
background PID, stage message, step variable and total variable (e.g.,
run_with_heartbeat <pid> <stage> <step> <total>); implement it to run the
emit_progress loop while kill -0 "$pid" succeeds (sleep 30), spawn that loop in
background, wait for the target pid, capture its exit code, kill and wait the
heartbeat pid, and return the captured exit code. Replace each duplicated block
that currently uses patterns with variables like AIB_PID, BOOTC_PID, IMAGE_PID,
BUILDER_PREPARE_PID (or similarly named PIDs) with a call to run_with_heartbeat
"$PID" "Building image"/"Booting build"/... "$STEP_BUILD" "$PROGRESS_TOTAL" and
propagate non-zero returns with exit $?. Ensure the helper uses local variables
for pid/hb_pid/rc and that callers use the helper's return code to decide exit.
- Around line 3-8: The emit_progress function is duplicated across
build_image.sh, build_builder.sh, flash_image.sh, and push_artifact.sh and
push_artifact.sh's implementation leaks variables (missing local); either
prepend common.sh into the embedding process for all four scripts and move
emit_progress into common.sh (remove duplicates from build_image.sh,
build_builder.sh, flash_image.sh, push_artifact.sh), or keep duplication but
update the emit_progress in push_artifact.sh to declare its parameters with
local (e.g., local stage="$1" local done="$2" local total="$3") so variables do
not leak; locate the function by name emit_progress to apply the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/caib/main.gointernal/buildapi/client/client.gointernal/buildapi/openapi.yamlinternal/buildapi/progress.gointernal/buildapi/server.gointernal/common/tasks/scripts/build_builder.shinternal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/flash_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.go
bkhizgiy
reviewed
Feb 23, 2026
| if err != nil { | ||
| continue | ||
| } | ||
| scanner := bufio.NewScanner(stream) |
Contributor
There was a problem hiding this comment.
IIRC bufio.Scanner has a line limit, if a log line is longer we will get an error and we might miss later the progress markers.
maybe we should increase the scanner buffer and check scanner.Err() so progress doesn’t randomly stall.
| Container: container.Name, | ||
| TailLines: &tailLines, | ||
| } | ||
| stream, err := cs.CoreV1().Pods(namespace).GetLogs(pod.Name, logOpts).Stream(ctx) |
Contributor
There was a problem hiding this comment.
we’re opening log streams for every step ,since /progress is likely polled often, wouldn't this add noticeable latency under load?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/common/tasks/scripts/build_builder.sh (1)
3-13:emit_progressis duplicated verbatim across four scripts.This function is copy-pasted in
build_builder.sh,build_image.sh,flash_image.sh, andpush_artifact.sh. If the annotation key or the curl logic ever changes, all four copies must be updated in lockstep.Consider extracting it into a shared helper (e.g., a file sourced at embed time, similar to how
common.shis prepended tobuild_builder.shandbuild_image.sh). For scripts that don't usecommon.shtoday, you could either extend the prepend mechanism or source a small standalone snippet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/build_builder.sh` around lines 3 - 13, The emit_progress function is duplicated across multiple scripts; extract it into a single shared helper (e.g., common_emit_progress.sh or add to common.sh) and update each script that currently defines emit_progress to instead source that helper at the top and call emit_progress as before; ensure the helper exposes the same function name (emit_progress) and has the same behavior (uses KUBERNETES_SERVICE_* env, HOSTNAME and serviceaccount files), and remove the duplicate function definitions from build_builder.sh, build_image.sh, flash_image.sh and push_artifact.sh so future changes are made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 469-471: The "Compressing artifacts" progress line is emitted
unconditionally even when no compression step is included (e.g.,
BUILD_DISK_IMAGE=false and BUILD_MODE=bootc), causing a misleading stage label;
wrap the emit_progress call so it only runs when the compression step is
actually counted (mirror the same condition used to add the compression step to
PROGRESS_TOTAL), i.e., check the build flags/values (BUILD_DISK_IMAGE and/or
BUILD_MODE) before calling emit_progress "Compressing artifacts" with
STEP_FINALIZE and PROGRESS_TOTAL so the label is only emitted when compression
will occur.
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 4-14: The script relies on the Bash-only HOSTNAME variable inside
emit_progress causing portability issues; fix emit_progress by explicitly
setting a safe pod name variable (e.g., resolve HOSTNAME at script start or
inside emit_progress using hostname command) and use that variable in the PATCH
URL instead of raw ${HOSTNAME}; update the top of the script or the
emit_progress function to assign HOSTNAME="$(hostname)" (or similar) so the
value is defined under /bin/sh before it’s referenced.
---
Nitpick comments:
In `@internal/common/tasks/scripts/build_builder.sh`:
- Around line 3-13: The emit_progress function is duplicated across multiple
scripts; extract it into a single shared helper (e.g., common_emit_progress.sh
or add to common.sh) and update each script that currently defines emit_progress
to instead source that helper at the top and call emit_progress as before;
ensure the helper exposes the same function name (emit_progress) and has the
same behavior (uses KUBERNETES_SERVICE_* env, HOSTNAME and serviceaccount
files), and remove the duplicate function definitions from build_builder.sh,
build_image.sh, flash_image.sh and push_artifact.sh so future changes are made
in one place.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.claude/skills/test-bootc-build/SKILL.mdcmd/caib/main.goconfig/rbac/kustomization.yamlconfig/rbac/pipeline_pod_role.yamlconfig/rbac/pipeline_pod_rolebinding.yamlinternal/buildapi/client/client.gointernal/buildapi/openapi.yamlinternal/buildapi/progress.gointernal/buildapi/server.gointernal/common/tasks/scripts/build_builder.shinternal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/flash_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.gointernal/controller/operatorconfig/controller.go
💤 Files with no reviewable changes (1)
- internal/controller/operatorconfig/controller.go
✅ Files skipped from review due to trivial changes (1)
- internal/controller/imagebuild/controller.go
Comment on lines
+4
to
+14
| # Format: stage|done|total (pipe-delimited, stored as a pod annotation) | ||
| emit_progress() { | ||
| _ep_stage="$1"; _ep_done="$2"; _ep_total="$3" | ||
| curl -s --cacert /var/run/secrets/kubernetes.io/serviceaccount/ca.crt \ | ||
| -X PATCH \ | ||
| -H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" \ | ||
| -H "Content-Type: application/merge-patch+json" \ | ||
| "https://${KUBERNETES_SERVICE_HOST}:${KUBERNETES_SERVICE_PORT}/api/v1/namespaces/$(cat /var/run/secrets/kubernetes.io/serviceaccount/namespace)/pods/${HOSTNAME}" \ | ||
| -d "{\"metadata\":{\"annotations\":{\"automotive.sdv.cloud.redhat.com/progress\":\"${_ep_stage}|${_ep_done}|${_ep_total}\"}}}" \ | ||
| > /dev/null 2>&1 || true | ||
| } |
There was a problem hiding this comment.
HOSTNAME is undefined in POSIX sh (SC3028).
The shebang is #!/bin/sh but HOSTNAME is a Bash-specific variable, not guaranteed by POSIX. In Kubernetes pods, HOSTNAME is typically set as an environment variable by the container runtime, so this will likely work in practice, but it's fragile.
Consider explicitly setting it at the top of the script or using $(hostname):
Suggested fix
emit_progress() {
_ep_stage="$1"; _ep_done="$2"; _ep_total="$3"
+ _ep_hostname="${HOSTNAME:-$(hostname)}"
curl -s --cacert /var/run/secrets/kubernetes.io/serviceaccount/ca.crt \
-X PATCH \
-H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" \
-H "Content-Type: application/merge-patch+json" \
- "https://${KUBERNETES_SERVICE_HOST}:${KUBERNETES_SERVICE_PORT}/api/v1/namespaces/$(cat /var/run/secrets/kubernetes.io/serviceaccount/namespace)/pods/${HOSTNAME}" \
+ "https://${KUBERNETES_SERVICE_HOST}:${KUBERNETES_SERVICE_PORT}/api/v1/namespaces/$(cat /var/run/secrets/kubernetes.io/serviceaccount/namespace)/pods/${_ep_hostname}" \
-d "{\"metadata\":{\"annotations\":{\"automotive.sdv.cloud.redhat.com/progress\":\"${_ep_stage}|${_ep_done}|${_ep_total}\"}}}" \
> /dev/null 2>&1 || true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Format: stage|done|total (pipe-delimited, stored as a pod annotation) | |
| emit_progress() { | |
| _ep_stage="$1"; _ep_done="$2"; _ep_total="$3" | |
| curl -s --cacert /var/run/secrets/kubernetes.io/serviceaccount/ca.crt \ | |
| -X PATCH \ | |
| -H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" \ | |
| -H "Content-Type: application/merge-patch+json" \ | |
| "https://${KUBERNETES_SERVICE_HOST}:${KUBERNETES_SERVICE_PORT}/api/v1/namespaces/$(cat /var/run/secrets/kubernetes.io/serviceaccount/namespace)/pods/${HOSTNAME}" \ | |
| -d "{\"metadata\":{\"annotations\":{\"automotive.sdv.cloud.redhat.com/progress\":\"${_ep_stage}|${_ep_done}|${_ep_total}\"}}}" \ | |
| > /dev/null 2>&1 || true | |
| } | |
| # Format: stage|done|total (pipe-delimited, stored as a pod annotation) | |
| emit_progress() { | |
| _ep_stage="$1"; _ep_done="$2"; _ep_total="$3" | |
| _ep_hostname="${HOSTNAME:-$(hostname)}" | |
| curl -s --cacert /var/run/secrets/kubernetes.io/serviceaccount/ca.crt \ | |
| -X PATCH \ | |
| -H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" \ | |
| -H "Content-Type: application/merge-patch+json" \ | |
| "https://${KUBERNETES_SERVICE_HOST}:${KUBERNETES_SERVICE_PORT}/api/v1/namespaces/$(cat /var/run/secrets/kubernetes.io/serviceaccount/namespace)/pods/${_ep_hostname}" \ | |
| -d "{\"metadata\":{\"annotations\":{\"automotive.sdv.cloud.redhat.com/progress\":\"${_ep_stage}|${_ep_done}|${_ep_total}\"}}}" \ | |
| > /dev/null 2>&1 || true | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 11-11: In POSIX sh, HOSTNAME is undefined.
(SC3028)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/tasks/scripts/push_artifact.sh` around lines 4 - 14, The
script relies on the Bash-only HOSTNAME variable inside emit_progress causing
portability issues; fix emit_progress by explicitly setting a safe pod name
variable (e.g., resolve HOSTNAME at script start or inside emit_progress using
hostname command) and use that variable in the PATCH URL instead of raw
${HOSTNAME}; update the top of the script or the emit_progress function to
assign HOSTNAME="$(hostname)" (or similar) so the value is defined under /bin/sh
before it’s referenced.
There was a problem hiding this comment.
@bennyz, you're absolutely right! The hostname of a Container is the name of the Pod in which the Container is running, and Kubernetes automatically sets the HOSTNAME environment variable in pod containers. Since this script is specifically designed to run within Kubernetes pods (as evident from the service account token usage and Kubernetes API calls), the HOSTNAME variable will always be available.
The shellcheck warning is technically correct from a pure POSIX portability perspective, but it doesn't apply in your Kubernetes context. Thanks for the clarification!
✏️ Learnings added
Learnt from: bennyz
Repo: centos-automotive-suite/automotive-dev-operator PR: 125
File: internal/common/tasks/scripts/push_artifact.sh:4-14
Timestamp: 2026-02-24T12:35:48.161Z
Learning: In the centos-automotive-suite/automotive-dev-operator repository, shell scripts that run within Kubernetes pods (such as those in internal/common/tasks/scripts/) can safely rely on the HOSTNAME environment variable being set by Kubernetes, even when using #!/bin/sh. The HOSTNAME variable is automatically provided by Kubernetes and equals the pod name. Shellcheck warnings about HOSTNAME being undefined in POSIX sh (SC3028) can be ignored for scripts that are exclusively executed in Kubernetes pod containers.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: bennyz
Repo: centos-automotive-suite/automotive-dev-operator PR: 123
File: internal/common/tasks/scripts/common.sh:23-30
Timestamp: 2026-02-20T14:04:45.140Z
Learning: Guideline for Bash scripts: when reading values with IFS= read -r into a Bash array and expanding with "${ARRAY[@]}", shell metacharacters in values are not interpreted if used in a quoted array expansion, so injection risk from content is mitigated by word-splitting and expansion being avoided. In reviews, focus on the format of assignments (KEY=VALUE) rather than restricting allowed characters, unless the value will be used in an unquoted context or via eval. This pattern applies broadly to Bash scripts in the repo; ensure such assignment formatting is followed and that values are safely handled when not within quoted contexts.
Since both have a lot of overlap, it makes sense to consolidate them and reduce the pod start overhead Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: gpt-5.3
to avoid redownloading RPMs use --cache --build-dir for build-builder Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: gpt-5.3
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 `@config/rbac/kustomization.yaml`:
- Around line 32-33: Change the pipeline_pod_role.yaml to define a
namespace-scoped Role instead of a ClusterRole by setting kind: Role and adding
metadata.namespace: system, keeping the same rules and metadata.name; then
update pipeline_pod_rolebinding.yaml to point roleRef.kind: Role (instead of
ClusterRole) so the RoleBinding in the system namespace correctly references the
namespaced Role. Ensure the Role name used in metadata.name matches the
roleRef.name in the RoleBinding.
In `@internal/buildapi/progress.go`:
- Around line 84-97: parseProgressAnnotation currently accepts negative integer
values which can corrupt aggregate progress; update the function
(parseProgressAnnotation) to validate the parsed integers and reject any
negative values by returning nil,false when done < 0 or total < 0 (and
optionally when total == 0 if you want to avoid divide-by-zero downstream). Keep
the existing error paths for parse failures and only return a BuildStep{Stage:
parts[0], Done: done, Total: total} when both integers are non-negative (and
total > 0 if chosen).
In `@internal/common/tasks/scripts/common.sh`:
- Around line 5-16: The emit_progress function can hang because curl has no
timeouts; update the curl invocation inside emit_progress to include connection
and total operation time limits (e.g., add --connect-timeout and --max-time
flags) so the PATCH call will fail fast on DNS/TCP/TLS stalls or long responses;
modify the curl command in emit_progress to pass appropriate timeout values that
match your cluster SLA and keep the existing error-suppression behavior (the
trailing || true).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.claude/skills/test-bootc-build/SKILL.mdcmd/caib/main.goconfig/rbac/kustomization.yamlconfig/rbac/pipeline_pod_role.yamlconfig/rbac/pipeline_pod_rolebinding.yamlinternal/buildapi/client/client.gointernal/buildapi/openapi.yamlinternal/buildapi/progress.gointernal/buildapi/server.gointernal/common/tasks/scripts.gointernal/common/tasks/scripts/build_builder.shinternal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/common.shinternal/common/tasks/scripts/flash_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.gointernal/controller/operatorconfig/controller.go
💤 Files with no reviewable changes (1)
- internal/controller/operatorconfig/controller.go
🚧 Files skipped from review as they are similar to previous changes (4)
- config/rbac/pipeline_pod_rolebinding.yaml
- internal/controller/imagebuild/controller.go
- internal/buildapi/server.go
- config/rbac/pipeline_pod_role.yaml
Comment on lines
+32
to
+33
| - pipeline_pod_role.yaml | ||
| - pipeline_pod_rolebinding.yaml |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate and inspect the RBAC files mentioned in the review
echo "=== Searching for pipeline_pod_role.yaml ==="
fd -e yaml 'pipeline_pod_role' -exec cat {} \;
echo ""
echo "=== Searching for pipeline_pod_rolebinding.yaml ==="
fd -e yaml 'pipeline_pod_rolebinding' -exec cat {} \;Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 581
🏁 Script executed:
#!/bin/bash
# Locate and inspect the RBAC files with corrected syntax
echo "=== Searching for pipeline_pod_role.yaml ==="
find . -name "pipeline_pod_role.yaml" -type f | head -20
echo ""
echo "=== Searching for pipeline_pod_rolebinding.yaml ==="
find . -name "pipeline_pod_rolebinding.yaml" -type f | head -20
echo ""
echo "=== Listing files in config/rbac directory ==="
ls -la config/rbac/ 2>/dev/null || echo "Directory not found"Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 1764
🏁 Script executed:
#!/bin/bash
# Read the actual content of both RBAC files
echo "=== Content of pipeline_pod_role.yaml ==="
cat config/rbac/pipeline_pod_role.yaml
echo ""
echo "=== Content of pipeline_pod_rolebinding.yaml ==="
cat config/rbac/pipeline_pod_rolebinding.yamlRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 636
Use a namespace-scoped Role instead of ClusterRole for pod patching.
The pipeline_pod_role.yaml file currently defines a ClusterRole, but since it's bound via a RoleBinding scoped to the system namespace, a namespace-scoped Role is more appropriate. While the effective permissions are limited to the namespace, the ClusterRole object exists cluster-wide and can be rebound elsewhere by any cluster-admin, unnecessarily broadening the attack surface. This violates the principle of least privilege.
Change pipeline_pod_role.yaml to use kind: Role and add the namespace: system metadata field, then update the roleRef.kind in pipeline_pod_rolebinding.yaml from ClusterRole to Role.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/rbac/kustomization.yaml` around lines 32 - 33, Change the
pipeline_pod_role.yaml to define a namespace-scoped Role instead of a
ClusterRole by setting kind: Role and adding metadata.namespace: system, keeping
the same rules and metadata.name; then update pipeline_pod_rolebinding.yaml to
point roleRef.kind: Role (instead of ClusterRole) so the RoleBinding in the
system namespace correctly references the namespaced Role. Ensure the Role name
used in metadata.name matches the roleRef.name in the RoleBinding.
use annotations to report the progress of a build Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: gpt-5.3
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/buildapi/progress.go (1)
82-97:⚠️ Potential issue | 🟡 MinorReject negative progress values from pod annotations.
Negative
done/totalcan flow into pipeline totals and produce misleading progress. Guard during parsing.🛡️ Suggested guard
done, err := strconv.Atoi(parts[1]) -if err != nil { +if err != nil || done < 0 { return nil, false } total, err := strconv.Atoi(parts[2]) -if err != nil { +if err != nil || total < 0 { return nil, false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/buildapi/progress.go` around lines 82 - 97, The parseProgressAnnotation function currently accepts negative numeric values; update it to validate parsed integers and reject negatives by returning nil,false when either done or total is negative (i.e., after strconv.Atoi for parts[1] and parts[2] check if done < 0 || total < 0 and return nil,false). Keep using the same function and BuildStep struct; no other behavior changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/rbac/pipeline_pod_role.yaml`:
- Around line 1-8: The Role resource "pipeline-pod-annotator" is
namespace-scoped but has no metadata.namespace, so the RoleBinding in namespace
"system" will not resolve; update the Role manifest (resource named
pipeline-pod-annotator) to include metadata.namespace: system (or confirm
kustomize injection) so the Role and the RoleBinding target the same namespace
and pod patching works.
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 222-225: The script prints user-supplied CUSTOM_DEFS_ARGS
(possible secrets) to logs; remove or redact those values in the echo/logging
statements. Specifically, in the build step guarded by BUILDER_CACHED (the block
with the echo "Running: aib build-builder ... ${CUSTOM_DEFS_ARGS[*]}
$LOCAL_BUILDER_IMAGE"), stop including CUSTOM_DEFS_ARGS in the echoed
command—either omit that array entirely or replace it with a safe placeholder
like "<REDACTED_DEFS>"—but keep the actual aib invocation using
"${CUSTOM_DEFS_ARGS[@]}". Ensure no other echo/log lines expose
CUSTOM_DEFS_ARGS.
---
Duplicate comments:
In `@internal/buildapi/progress.go`:
- Around line 82-97: The parseProgressAnnotation function currently accepts
negative numeric values; update it to validate parsed integers and reject
negatives by returning nil,false when either done or total is negative (i.e.,
after strconv.Atoi for parts[1] and parts[2] check if done < 0 || total < 0 and
return nil,false). Keep using the same function and BuildStep struct; no other
behavior changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmd/caib/main.goconfig/rbac/kustomization.yamlconfig/rbac/pipeline_pod_role.yamlconfig/rbac/pipeline_pod_rolebinding.yamlinternal/buildapi/client/client.gointernal/buildapi/openapi.yamlinternal/buildapi/progress.gointernal/buildapi/server.gointernal/common/tasks/scripts.gointernal/common/tasks/scripts/build_builder.shinternal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/common.shinternal/common/tasks/scripts/flash_image.shinternal/common/tasks/scripts/push_artifact.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/buildapi/client/client.go
- internal/common/tasks/scripts/common.sh
- config/rbac/kustomization.yaml
- internal/common/tasks/scripts.go
- internal/buildapi/openapi.yaml
Comment on lines
+222
to
+225
| if [ "$BUILDER_CACHED" = "false" ]; then | ||
| echo "Builder image not found, building..." | ||
| echo "Running: aib build-builder --distro $(params.distro) --build-dir /_build --cache /_build/dnf-cache ${CUSTOM_DEFS_ARGS[*]} $LOCAL_BUILDER_IMAGE" | ||
| aib --verbose build-builder --build-dir /_build --cache /_build/dnf-cache --distro "$(params.distro)" "${CUSTOM_DEFS_ARGS[@]}" "$LOCAL_BUILDER_IMAGE" |
There was a problem hiding this comment.
Avoid logging custom definition values (may include secrets).
CUSTOM_DEFS_ARGS is sourced from user-provided definitions and can include credentials. Echoing the full array writes those values into task logs.
🔒 Suggested redaction
- echo "Running: aib build-builder --distro $(params.distro) --build-dir /_build --cache /_build/dnf-cache ${CUSTOM_DEFS_ARGS[*]} $LOCAL_BUILDER_IMAGE"
+ if [ "${`#CUSTOM_DEFS_ARGS`[@]}" -gt 0 ]; then
+ echo "Running: aib build-builder --distro $(params.distro) --build-dir /_build --cache /_build/dnf-cache (custom defs: ${`#CUSTOM_DEFS_ARGS`[@]} provided) $LOCAL_BUILDER_IMAGE"
+ else
+ echo "Running: aib build-builder --distro $(params.distro) --build-dir /_build --cache /_build/dnf-cache $LOCAL_BUILDER_IMAGE"
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ "$BUILDER_CACHED" = "false" ]; then | |
| echo "Builder image not found, building..." | |
| echo "Running: aib build-builder --distro $(params.distro) --build-dir /_build --cache /_build/dnf-cache ${CUSTOM_DEFS_ARGS[*]} $LOCAL_BUILDER_IMAGE" | |
| aib --verbose build-builder --build-dir /_build --cache /_build/dnf-cache --distro "$(params.distro)" "${CUSTOM_DEFS_ARGS[@]}" "$LOCAL_BUILDER_IMAGE" | |
| if [ "$BUILDER_CACHED" = "false" ]; then | |
| echo "Builder image not found, building..." | |
| if [ "${`#CUSTOM_DEFS_ARGS`[@]}" -gt 0 ]; then | |
| echo "Running: aib build-builder --distro $(params.distro) --build-dir /_build --cache /_build/dnf-cache (custom defs: ${`#CUSTOM_DEFS_ARGS`[@]} provided) $LOCAL_BUILDER_IMAGE" | |
| else | |
| echo "Running: aib build-builder --distro $(params.distro) --build-dir /_build --cache /_build/dnf-cache $LOCAL_BUILDER_IMAGE" | |
| fi | |
| aib --verbose build-builder --build-dir /_build --cache /_build/dnf-cache --distro "$(params.distro)" "${CUSTOM_DEFS_ARGS[@]}" "$LOCAL_BUILDER_IMAGE" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/tasks/scripts/build_image.sh` around lines 222 - 225, The
script prints user-supplied CUSTOM_DEFS_ARGS (possible secrets) to logs; remove
or redact those values in the echo/logging statements. Specifically, in the
build step guarded by BUILDER_CACHED (the block with the echo "Running: aib
build-builder ... ${CUSTOM_DEFS_ARGS[*]} $LOCAL_BUILDER_IMAGE"), stop including
CUSTOM_DEFS_ARGS in the echoed command—either omit that array entirely or
replace it with a safe placeholder like "<REDACTED_DEFS>"—but keep the actual
aib invocation using "${CUSTOM_DEFS_ARGS[@]}". Ensure no other echo/log lines
expose CUSTOM_DEFS_ARGS.
bkhizgiy
approved these changes
Feb 24, 2026
Merged
This was referenced Apr 5, 2026
This was referenced Apr 13, 2026
Merged
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Changes
Permissions