basic jumpstarter integration#42
Conversation
📝 WalkthroughWalkthroughAdds Jumpstarter "flash" support: new Flash API types/CRD fields and status, server endpoints and client methods, controller and Tekton Task/TaskRun support with embedded flash script, CLI flash command and log streaming with lease extraction, OperatorConfig Jumpstarter image option, and vendor/module updates. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI User
participant caib as caib (CLI)
participant API as Build API Server
participant Tekton as Tekton API
participant Jumpstarter as Jumpstarter Exporter
CLI->>caib: flash --client <base64-config> --lease <dur> <image-ref>
caib->>API: POST /v1/flash (FlashRequest)
API->>API: create Jumpstarter client Secret
API->>Tekton: Create TaskRun (flash-image) with secret workspace
Tekton->>Jumpstarter: run flash_image.sh (create lease, execute flash)
Jumpstarter-->>Tekton: emit logs (lease ID, progress)
Tekton-->>API: TaskRun status & logs
caib->>API: GET /v1/flash/{name}/logs (stream)
API-->>caib: streamed logs (lease ID extracted)
API->>API: update ImageBuild status (Flashing → Completed/Failed)
sequenceDiagram
participant User as ImageBuild Creator
participant Controller as ImageBuild Controller
participant Tekton as Tekton Pipeline API
participant PVC as VolumeClaimTemplate
participant FlashTask as flash-image Task
User->>Controller: create ImageBuild (flash enabled)
Controller->>Controller: resolve OperatorConfig, flash target mapping
Controller->>Tekton: create PipelineRun (PVC template, flash params, jumpstarter-client workspace)
Tekton->>Tekton: run build → push → flash-image (when conditions met)
FlashTask->>PVC: mount jumpstarter-client secret
FlashTask->>FlashTask: run flash_image.sh (lease, exec)
Tekton-->>Controller: TaskRun completion
Controller->>Controller: handleFlashingState(), extract lease ID, update status
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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
Comment |
bennyz
force-pushed
the
jumpstarter-flash
branch
from
e66373b to
c537277
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/samples/automotive_v1_image_http.yaml (1)
46-74: Inconsistent metadata after distro change.The
distrofield was changed to"autosd"but the description (Line 52), tags (Line 54), and labels (Line 72) still reference "CentOS Stream 9" / "cs9". Update these fields to maintain consistency:Suggested fix
spec: distro: "autosd" target: "edge-commit" architecture: "x86_64" exportFormat: "image" mode: "image" version: "3.1.0" - description: "CentOS Stream 9 Edge Commit stored in Container Registry" + description: "AutoSD Edge Commit stored in Container Registry" tags: - - "centos-stream-9" + - "autosd" - "edge" - "commit" location: type: "registry" registry: - url: "quay.io/automotive/cs9-edge-commit:3.1.0-x86_64" + url: "quay.io/automotive/autosd-edge-commit:3.1.0-x86_64" digest: "sha256:abcdef123456789012345678901234567890123456789012345678901234abcd" secretRef: "registry-credentials" size: compressedBytes: 536870912 # 512MB compressed uncompressedBytes: 3221225472 # 3GB uncompressed virtualBytes: 10737418240 # 10GB virtual metadata: createdBy: "ci-pipeline" - sourceImageBuild: "cs9-edge-commit-build-123" + sourceImageBuild: "autosd-edge-commit-build-123" buildDate: "2025-01-12T08:20:15Z" labels: - "automotive.io/distro": "cs9" + "automotive.io/distro": "autosd" "automotive.io/arch": "x86_64" "automotive.io/type": "edge-commit"
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Around line 590-599: When flash is enabled (flashAfterBuild &&
jumpstarterClient != ""), validate that exportOCI (the --push flag) is true
before setting req.FlashEnabled and reading the client config; if exportOCI is
false, call handleError with a clear error (e.g., "flash requires
--push/exportOCI to be set") and abort. Update the conditional in main.go around
flashAfterBuild/jumpstarterClient to check exportOCI or explicitly add an if
that checks exportOCI and calls handleError when missing, then proceed to
os.ReadFile(jumpstarterClient), base64 encode into req.FlashClientConfig, set
req.FlashEnabled and req.FlashLeaseDuration as before.
- Around line 865-874: The build-dev flow is missing the same flash
configuration validation as the build command: when flashAfterBuild is true and
jumpstarterClient is provided you must read the file, handle read errors,
base64-encode the bytes and set req.FlashEnabled, req.FlashClientConfig and
req.FlashLeaseDuration; update the build-dev handling code (where you construct
the request for the build-dev command—look for the buildDev handler or function
that builds req) to include the same block used in main.go (checking
flashAfterBuild && jumpstarterClient != "", os.ReadFile -> handleError on
failure, base64.StdEncoding.EncodeToString -> assign to req.FlashClientConfig,
set req.FlashEnabled = true and req.FlashLeaseDuration = leaseDuration).
- Around line 483-492: When flashAfterBuild is true the code sets
req.FlashEnabled without validating that a disk image will be available; add a
guard that checks the image export flag (exportOCI / the --push-disk flag
variable) or whatever variable indicates a produced disk image before enabling
flash. In the block that handles flashAfterBuild and jumpstarterClient, if
neither exportOCI is true nor a valid image reference/path is present, call
handleError with a clear error (e.g., "cannot enable --flash without exporting a
disk image (--push-disk)") and do not set req.FlashEnabled/req.FlashImageRef;
otherwise proceed to read jumpstarterClient and set req.FlashEnabled,
req.FlashClientConfig and req.FlashLeaseDuration as before.
- Around line 1176-1188: The lease ID capture logic in the block that sets
state.leaseID (when checking for "jmp shell --lease " or "Lease acquired: ") may
include trailing flags/text; instead, after splitting by the marker, further
split the resulting substring by whitespace (or use strings.Fields) and assign
only the first token to state.leaseID so you don't capture extra arguments or
text.
- Around line 1481-1492: The parseLeaseDuration function currently swallows
fmt.Sscanf errors and can return a zero duration for inputs like "abc:def:ghi";
update parseLeaseDuration to validate parsing by checking the error return
values (or using strconv.Atoi on each parts[i]) and if any parse fails or values
are out of range, return the safe default (time.Hour) instead of zero; ensure
the function still returns a proper time.Duration and adjust callers such as
waitForFlashCompletion to rely on the non-zero default lease duration.
In `@config/samples/automotive_v1_operatorconfig.yaml`:
- Around line 27-30: The sample currently enables nodeSelector (nodeSelector:
aib: "true") which can make the manifest fail on clusters without that label;
either comment out the nodeSelector block to keep it as a usage example or add
an explicit inline comment above it stating that this requires nodes labeled
`aib: "true"` (e.g., "# OPTIONAL: requires nodes labeled aib: \"true\" —
uncomment only if your cluster has such nodes"), so update the `nodeSelector`
block accordingly.
In `@internal/buildapi/server.go`:
- Around line 1696-1698: The auto-generated name using req.Name =
fmt.Sprintf("flash-%s", time.Now().Format("20060102-150405")) can collide within
the same second; update the req.Name assignment to include a unique suffix
(e.g., append a short UUID or UnixNano timestamp) so names are globally unique —
change the code that sets req.Name (the fmt.Sprintf call) to include a
uuid.New().String() or time.Now().UnixNano() suffix and add the appropriate
import for the UUID package if used.
- Around line 1843-1853: Add a defensive check that taskRun.UID is not empty
before assigning it to createdSecret.OwnerReferences: after the TaskRun
creation/response (where taskRun is populated) verify taskRun.UID != "" and if
it is empty return or log an error and skip setting the OwnerReference (or
fail-fast), instead of proceeding to set createdSecret.OwnerReferences and
calling clientset.CoreV1().Secrets(namespace).Update; this ensures the
OwnerReference uses a valid UID and avoids relying on the Update error to
surface missing UID.
In `@internal/common/tasks/scripts.go`:
- Around line 28-31: The build fails because the go:embed directive references a
missing file; add a new shell script at scripts/flash_image.sh so the embedded
variable FlashImageScript in internal/common/tasks/scripts.go resolves;
implement the expected flash logic or a minimal compatible stub consistent with
the other scripts (e.g., accept the same CLI args/ENV used by tasks.go), ensure
the file is committed to the repository so go:embed finds it, and verify
tasks.go (which references FlashImageScript) still works with the new script.
In `@internal/controller/imagebuild/controller.go`:
- Around line 974-1006: The workspace binding is created unconditionally with
clientConfigSecretRef which may be empty; add the same validation used in
createBuildTaskRun to check clientConfigSecretRef != "" before appending the
"jumpstarter-client" workspace: if empty, do not add the workspace (or return an
error) so GenerateFlashTask/TaskRun creation does not include a WorkspaceBinding
with an empty SecretName; update the code around clientConfigSecretRef,
workspaces, and TaskRun creation to guard against an empty secret reference.
🧹 Nitpick comments (2)
internal/common/tasks/tasks.go (1)
1166-1167: Pin the Jumpstarter image to a specific version instead of using:latest.Using
:latestcan lead to non-reproducible builds and unexpected behavior when the upstream image updates. Specify a stable version tag (e.g.,0.5.0) for production deployments to ensure consistent behavior across runs.Suggested change:
-const JumpstarterImage = "quay.io/jumpstarter-dev/jumpstarter:latest" +const JumpstarterImage = "quay.io/jumpstarter-dev/jumpstarter:0.5.0"internal/controller/imagebuild/controller.go (1)
323-323: Remove unnecessary blank identifier assignment.
buildConfigis used later at lines 564-566 forRuntimeClassName, so this blank identifier assignment is unnecessary and misleading.♻️ Suggested fix
- _ = buildConfig // buildConfig used for RuntimeClassName if needed
| if req.Name == "" { | ||
| req.Name = fmt.Sprintf("flash-%s", time.Now().Format("20060102-150405")) | ||
| } |
There was a problem hiding this comment.
Potential name collision with timestamp-based auto-generation.
The auto-generated name using time.Now().Format("20060102-150405") has second-level precision. If two flash requests arrive within the same second, they'll have identical names causing a conflict.
🔧 Suggested fix using UUID suffix
// Auto-generate name if not provided
if req.Name == "" {
- req.Name = fmt.Sprintf("flash-%s", time.Now().Format("20060102-150405"))
+ req.Name = fmt.Sprintf("flash-%s-%s", time.Now().Format("20060102-150405"), uuid.New().String()[:8])
}🤖 Prompt for AI Agents
In `@internal/buildapi/server.go` around lines 1696 - 1698, The auto-generated
name using req.Name = fmt.Sprintf("flash-%s",
time.Now().Format("20060102-150405")) can collide within the same second; update
the req.Name assignment to include a unique suffix (e.g., append a short UUID or
UnixNano timestamp) so names are globally unique — change the code that sets
req.Name (the fmt.Sprintf call) to include a uuid.New().String() or
time.Now().UnixNano() suffix and add the appropriate import for the UUID package
if used.
| createdSecret.OwnerReferences = []metav1.OwnerReference{ | ||
| { | ||
| APIVersion: "tekton.dev/v1", | ||
| Kind: "TaskRun", | ||
| Name: taskRun.Name, | ||
| UID: taskRun.UID, | ||
| }, | ||
| } | ||
| if _, err := clientset.CoreV1().Secrets(namespace).Update(ctx, createdSecret, metav1.UpdateOptions{}); err != nil { | ||
| log.Printf("WARNING: failed to set owner reference on secret %s: %v", secretName, err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and examine context around lines 1843-1853
cat -n internal/buildapi/server.go | sed -n '1820,1860p'Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 1888
🏁 Script executed:
# Search for how k8sClient.Create is used in this file and nearby context
rg -n "k8sClient\.Create|taskRun\s*=" --type go internal/buildapi/server.go -B 3 -A 3Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 1223
🏁 Script executed:
# Look for the broader context of how taskRun is created and used
rg -n "k8sClient" --type go internal/buildapi/server.go | head -20Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 1680
🏁 Script executed:
# Check the imports to confirm controller-runtime usage
head -50 internal/buildapi/server.go | rg -n "^import|sigs.k8s.io"Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 149
🏁 Script executed:
# Check the go.mod to see what version of controller-runtime is used
grep -i "controller-runtime" go.modRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 130
🏁 Script executed:
# Check for go version requirement in the codebase
cat go.mod | head -5Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 187
🏁 Script executed:
# Search for how UID is handled after Create in this codebase
rg -n "\.UID\s*=" --type go internal/buildapi/server.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Look for any other places where UID is set after Create
rg -n "UID" --type go internal/buildapi/server.go -B 2 -A 2Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 206
🏁 Script executed:
# Check if there are any tests or examples in the codebase showing Create behavior
find . -name "*test.go" -type f | head -5Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 294
🏁 Script executed:
# Check server_test.go for tests related to TaskRun creation and UID handling
grep -n "k8sClient.Create\|taskRun\|UID" internal/buildapi/server_test.go | head -30Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Check integration tests
grep -n "taskRun\|UID\|OwnerReference" internal/buildapi/integration_test.go -A 3 -B 3 | head -50Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Look at the full context around the owner reference setting to understand the flow better
sed -n '1800,1870p' internal/buildapi/server.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 2614
🏁 Script executed:
# Check if there are any other uses of UID after Create that might show the pattern
rg -n "\.Create.*\)" --type go internal/buildapi/server.go -A 5 | grep -E "UID|\.Create"Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 818
🏁 Script executed:
# Look for how taskRun is used after Create to see if UID should be populated
sed -n '1835,1865p' internal/buildapi/server.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 1155
🏁 Script executed:
# Search for any documentation or comments about UID handling
rg -n "UID.*populated|populate.*UID|Create.*UID" --type go .Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 3237
🏁 Script executed:
# Check how the k8sClient is configured - is it a cached client or direct client?
sed -n '280,295p' internal/buildapi/server.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 615
🏁 Script executed:
# Look for imports related to controller-runtime caching
rg -n "cache\|cached" --type go internal/buildapi/server.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Check the getClientFromRequest function to understand client setup
rg -n "func getClientFromRequest" --type go -A 20 internal/buildapi/server.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 806
🏁 Script executed:
# Search for any test cases or code that verifies UID after Create
rg -n "\.Create.*taskRun\|taskRun.*UID" --type go -A 10 -B 5 internal/buildapi/Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Check if there are any patterns in the codebase of fetching the object after Create
rg -n "k8sClient.Get.*Name.*req.Name" --type go -A 3 internal/buildapi/server.go | head -30Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 370
🏁 Script executed:
# Check documentation or examples in vendor about Create behavior
find vendor/sigs.k8s.io/controller-runtime -name "*.go" -path "*/client/*" | head -3 | xargs grep -l "func.*Create" 2>/dev/null | head -1Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Check the vendor directory for controller-runtime client implementation
find vendor/sigs.k8s.io/controller-runtime -path "*/client/*" -name "*.go" -type f | head -5Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 434
🏁 Script executed:
# Look at the actual client interface in controller-runtime to understand Create behavior
grep -r "func.*Create" vendor/sigs.k8s.io/controller-runtime/pkg/client/ 2>/dev/null | head -10Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 1846
🏁 Script executed:
# Search for any error handling or validation of UID in similar patterns in this codebase
rg -n "if.*UID.*==|UID.*empty|UID.*nil" --type go .Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 12308
🏁 Script executed:
# Look at the actual Create implementation in controller-runtime
sed -n '1,150p' vendor/sigs.k8s.io/controller-runtime/pkg/client/client.go | grep -A 30 "func (c \*client) Create"Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Check if there's documentation or examples about UID population after Create
grep -r "UID" vendor/sigs.k8s.io/controller-runtime/pkg/client/client.go -B 5 -A 5 | head -50Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Check how the API server response is handled - look for JSON unmarshaling
rg -n "json\|Unmarshal" vendor/sigs.k8s.io/controller-runtime/pkg/client/client.go | head -10Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Read the client.go file to understand Create implementation
wc -l vendor/sigs.k8s.io/controller-runtime/pkg/client/client.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 153
🏁 Script executed:
# Search for the Create function implementation
grep -n "func (c \*client) Create" vendor/sigs.k8s.io/controller-runtime/pkg/client/client.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 181
🏁 Script executed:
# Get the Create function implementation
sed -n '140,200p' vendor/sigs.k8s.io/controller-runtime/pkg/client/client.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 1743
🏁 Script executed:
# Get the actual Create function implementation (starting at line 272)
sed -n '272,320p' vendor/sigs.k8s.io/controller-runtime/pkg/client/client.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 1759
🏁 Script executed:
# Search for what happens after Create - look for any response handling or UID setting
sed -n '272,350p' vendor/sigs.k8s.io/controller-runtime/pkg/client/client.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 2920
🏁 Script executed:
# Find the typedClient.Create implementation
rg -n "func.*typedClient.*Create" vendor/sigs.k8s.io/controller-runtime/pkg/client/client_rest_resources.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Look at client_rest_resources.go for the Create implementation
grep -n "func.*Create" vendor/sigs.k8s.io/controller-runtime/pkg/client/client_rest_resources.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Get the typedClient Create implementation
sed -n '1,50p' vendor/sigs.k8s.io/controller-runtime/pkg/client/client_rest_resources.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 1578
🏁 Script executed:
# Search for typedClient definition and its Create method
grep -n "type typedClient" vendor/sigs.k8s.io/controller-runtime/pkg/client/client_rest_resources.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Look for Create method in the typedClient struct
grep -n "func.*Create" vendor/sigs.k8s.io/controller-runtime/pkg/client/ -r | grep typedClientRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 476
🏁 Script executed:
# Search in all client files for Create implementation
wc -l vendor/sigs.k8s.io/controller-runtime/pkg/client/client_rest_resources.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 168
🏁 Script executed:
# Get the typedClient.Create implementation
sed -n '34,100p' vendor/sigs.k8s.io/controller-runtime/pkg/client/typed_client.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 1754
🏁 Script executed:
# Look at more of the Create implementation to understand UID handling
sed -n '34,120p' vendor/sigs.k8s.io/controller-runtime/pkg/client/typed_client.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 2285
🏁 Script executed:
# Check if there's any error handling around the Create that might bypass the Into call
sed -n '1835,1860p' internal/buildapi/server.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 1067
🏁 Script executed:
# Verify if taskRun.UID could be empty after Create based on typical Kubernetes API behavior
rg -n "if.*taskRun\.UID\|taskRun\.UID.*nil\|taskRun\.UID.*==" --type go internal/buildapi/server.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Check if there are any test cases showing this pattern
grep -n "taskRun\|\.UID" internal/buildapi/server_test.go -B 2 -A 2 | head -30Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
Add validation that TaskRun UID is populated before setting owner reference.
While controller-runtime's Create() method populates the UID via the API server response (using .Into(obj)), the code should explicitly validate that taskRun.UID is not empty before using it in the owner reference. Currently, if UID were somehow absent, the invalid owner reference would fail silently until the Update call attempts to validate it (at line 1851). Adding a check like if taskRun.UID == "" immediately after the Create would be more defensive and fail-fast on unexpected API responses.
🤖 Prompt for AI Agents
In `@internal/buildapi/server.go` around lines 1843 - 1853, Add a defensive check
that taskRun.UID is not empty before assigning it to
createdSecret.OwnerReferences: after the TaskRun creation/response (where
taskRun is populated) verify taskRun.UID != "" and if it is empty return or log
an error and skip setting the OwnerReference (or fail-fast), instead of
proceeding to set createdSecret.OwnerReferences and calling
clientset.CoreV1().Secrets(namespace).Update; this ensures the OwnerReference
uses a valid UID and avoids relying on the Update error to surface missing UID.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yaml (1)
250-257: Update phase description to reflect new enum values.The description still lists only Building/Completed/Failed, but the enum now includes Pending/Uploading/Pushing/Flashing.
✏️ Suggested update
- description: Phase represents the current phase of the build (Building, - Completed, Failed) + description: Phase represents the current phase of the build (Pending, + Uploading, Building, Pushing, Flashing, Completed, Failed)
🤖 Fix all issues with AI agents
In `@internal/buildapi/server.go`:
- Around line 1835-1878: The controller-runtime client used by
getClientFromRequest must register Tekton types so TaskRun operations succeed;
update getClientFromRequest (the place that builds the controller-runtime
client/scheme) to call tektonv1.AddToScheme on the client scheme (or otherwise
register the tektonv1 types) before creating the client so
k8sClient.Create/List/Get for tektonv1.TaskRun won't error with “no kind is
registered”; ensure this change covers all client constructions referenced by
listFlash and the other TaskRun CRUD call sites.
- Around line 986-998: When req.FlashEnabled is true but req.FlashClientConfig
is empty the code silently skips creating flashSpec/secret; change the logic to
validate this and return a 400 Bad Request instead of continuing: detect
(req.FlashEnabled && req.FlashClientConfig == "") and call
c.JSON(http.StatusBadRequest, ...) / return, before attempting to compute
flashSecretName or call createFlashClientSecret; apply the same validation
pattern to the other flash handling blocks that construct
flashSpec/flashSecretName and call createFlashClientSecret (the blocks using
variables flashSpec, flashSecretName and automotivev1alpha1.FlashSpec).
In `@internal/common/tasks/scripts/flash_image.sh`:
- Around line 17-22: The default FLASH_CMD is incorrect and uses "j" and a
broken expansion; replace the brittle multi-step substitutions by setting
FLASH_CMD to a correct default that embeds the shell variable directly and
remove the later replacement lines: set FLASH_CMD="${FLASH_CMD:-jmp storage
flash ${IMAGE_REF}}", delete the subsequent FLASH_CMD replacement/trim lines,
and if you must support legacy placeholders handle them with a single parameter
expansion or a simple sed substitution that replaces "{image_uri}" or
"${image_uri}" with "${IMAGE_REF}" when FLASH_CMD was provided by the user.
In `@internal/common/tasks/tasks.go`:
- Around line 1215-1221: The workspace "jumpstarter-client" is declared
Optional: true but a task step unconditionally sets the JMP_CLIENT_CONFIG env
var to "/workspace/jumpstarter-client/client.yaml" so the step will fail if the
workspace is not bound; make the workspace required by removing Optional: true
from the Workspaces entry for "jumpstarter-client" in the Workspaces:
[]tektonv1.WorkspaceDeclaration block (or alternatively guard the step that sets
JMP_CLIENT_CONFIG to only run when the workspace is present), and ensure the
step that references JMP_CLIENT_CONFIG (the flash/client step) expects the
config to exist.
♻️ Duplicate comments (6)
cmd/caib/main.go (5)
483-492: Missing validation:--flashwithout a disk image to flash.When
flashAfterBuildis enabled, the code doesn't validate that there's actually a disk image to flash. For bootc builds, this requiresexportOCI(--push-disk) to be set, otherwise the controller receives an emptyflash-image-ref.🐛 Proposed fix
// Add flash configuration if enabled if flashAfterBuild && jumpstarterClient != "" { + if exportOCI == "" { + handleError(fmt.Errorf("--flash requires --push-disk to specify the disk image OCI reference")) + } clientConfigBytes, err := os.ReadFile(jumpstarterClient)
590-599: Same validation issue for disk command.Consistent with the build command, this should validate that
exportOCI(--push) is set when flash is enabled.🐛 Proposed fix
// Add flash configuration if enabled if flashAfterBuild && jumpstarterClient != "" { + if exportOCI == "" { + handleError(fmt.Errorf("--flash requires --push to specify the disk image OCI reference")) + } clientConfigBytes, err := os.ReadFile(jumpstarterClient)
865-874: Same validation issue for build-dev command.🐛 Proposed fix
// Add flash configuration if enabled if flashAfterBuild && jumpstarterClient != "" { + if exportOCI == "" { + handleError(fmt.Errorf("--flash requires --push to specify the disk image OCI reference")) + } clientConfigBytes, err := os.ReadFile(jumpstarterClient)
1176-1188: Lease ID capture may include trailing text.The current string splitting approach captures everything after the marker, which could include additional flags or text. Consider extracting only the first token.
🔧 Proposed fix
// Capture lease ID from flash logs // Format: "jmp shell --lease <lease-id>" or "Lease acquired: <lease-id>" if strings.Contains(line, "jmp shell --lease ") { parts := strings.Split(line, "jmp shell --lease ") if len(parts) > 1 { - state.leaseID = strings.TrimSpace(parts[1]) + leaseAndRest := strings.TrimSpace(parts[1]) + if fields := strings.Fields(leaseAndRest); len(fields) > 0 { + state.leaseID = fields[0] + } } } else if strings.Contains(line, "Lease acquired: ") { parts := strings.Split(line, "Lease acquired: ") if len(parts) > 1 { - state.leaseID = strings.TrimSpace(parts[1]) + leaseAndRest := strings.TrimSpace(parts[1]) + if fields := strings.Fields(leaseAndRest); len(fields) > 0 { + state.leaseID = fields[0] + } } }
1481-1492:parseLeaseDurationsilently returns zero duration on invalid input.The function suppresses
fmt.Sscanferrors. Invalid input like"abc:def:ghi"would parse as zeros, returning a0duration. This could cause an immediate timeout inwaitForFlashCompletion.🔧 Proposed fix with validation
// parseLeaseDuration converts HH:MM:SS format to time.Duration -func parseLeaseDuration(duration string) time.Duration { +func parseLeaseDuration(duration string) (time.Duration, error) { parts := strings.Split(duration, ":") if len(parts) != 3 { - return time.Hour // Default 1 hour + return 0, fmt.Errorf("invalid lease duration format %q, expected HH:MM:SS", duration) } var hours, mins, secs int - _, _ = fmt.Sscanf(parts[0], "%d", &hours) - _, _ = fmt.Sscanf(parts[1], "%d", &mins) - _, _ = fmt.Sscanf(parts[2], "%d", &secs) - return time.Duration(hours)*time.Hour + time.Duration(mins)*time.Minute + time.Duration(secs)*time.Second + if _, err := fmt.Sscanf(parts[0], "%d", &hours); err != nil { + return 0, fmt.Errorf("invalid hours in lease duration: %w", err) + } + if _, err := fmt.Sscanf(parts[1], "%d", &mins); err != nil { + return 0, fmt.Errorf("invalid minutes in lease duration: %w", err) + } + if _, err := fmt.Sscanf(parts[2], "%d", &secs); err != nil { + return 0, fmt.Errorf("invalid seconds in lease duration: %w", err) + } + return time.Duration(hours)*time.Hour + time.Duration(mins)*time.Minute + time.Duration(secs)*time.Second, nil }Then update the caller at line 1548:
- timeoutDuration := parseLeaseDuration(leaseDuration) + 10*time.Minute + timeoutDuration, err := parseLeaseDuration(leaseDuration) + if err != nil { + handleError(fmt.Errorf("invalid lease duration: %w", err)) + } + timeoutDuration += 10 * time.Minuteinternal/controller/imagebuild/controller.go (1)
999-1006: Missing validation for emptyclientConfigSecretRef.Unlike the pipeline workspace binding (lines 531-542) which validates
clientSecretRef != ""before adding the workspace, this function creates a workspace binding with potentially emptySecretName. This would create an invalid TaskRun.🐛 Proposed fix
leaseDuration := imageBuild.Spec.GetFlashLeaseDuration() clientConfigSecretRef := imageBuild.Spec.GetFlashClientConfigSecretRef() + if clientConfigSecretRef == "" { + return fmt.Errorf("flash client config secret reference is required") + } flashTask := tasks.GenerateFlashTask(OperatorNamespace)
🧹 Nitpick comments (2)
hack/deploy-catalog.sh (1)
210-217: Broaden internal-registry rewrites to match the intent.The comment says “Replace ALL image references,” but the current
sedonly matches two exact patterns. If new images or tag variants are added later, they’ll still point at the external route. Consider replacing the namespace prefix instead of a single image.♻️ Suggested adjustment
-OPERATOR_IMG_INTERNAL="image-registry.openshift-image-registry.svc:5000/${NAMESPACE}/automotive-dev-operator:${OPERATOR_TAG}" -sed -i.bak "s|${REGISTRY}/${NAMESPACE}/automotive-dev-operator:${OPERATOR_TAG}|${OPERATOR_IMG_INTERNAL}|g" bundle/manifests/automotive-dev-operator.clusterserviceversion.yaml +INTERNAL_PREFIX="image-registry.openshift-image-registry.svc:5000/${NAMESPACE}/" +OPERATOR_IMG_INTERNAL="${INTERNAL_PREFIX}automotive-dev-operator:${OPERATOR_TAG}" +sed -i.bak "s|${REGISTRY}/${NAMESPACE}/|${INTERNAL_PREFIX}|g" bundle/manifests/automotive-dev-operator.clusterserviceversion.yamlinternal/common/tasks/tasks.go (1)
1166-1167: Pin the Jumpstarter image to a specific stable version instead of:latest.Using
:latestcan lead to unpredictable behavior in production as the image may change without warning. The current latest stable version is 0.7.3 (released Nov 27, 2025).♻️ Suggested change
-const JumpstarterImage = "quay.io/jumpstarter-dev/jumpstarter:latest" +const JumpstarterImage = "quay.io/jumpstarter-dev/jumpstarter:0.7.3"
| Workspaces: []tektonv1.WorkspaceDeclaration{ | ||
| { | ||
| Name: "jumpstarter-client", | ||
| Description: "Workspace containing the Jumpstarter client config (client.yaml)", | ||
| MountPath: "/workspace/jumpstarter-client", | ||
| Optional: true, | ||
| }, |
There was a problem hiding this comment.
Workspace marked optional but task step depends on it.
The workspace is declared as Optional: true, but the step sets JMP_CLIENT_CONFIG environment variable unconditionally pointing to the workspace path. If the workspace is not bound, the client config file won't exist and the flash will fail with a confusing error.
Since flash requires the client config, consider making this workspace required (remove Optional: true).
🔧 Suggested fix
Workspaces: []tektonv1.WorkspaceDeclaration{
{
Name: "jumpstarter-client",
Description: "Workspace containing the Jumpstarter client config (client.yaml)",
MountPath: "/workspace/jumpstarter-client",
- Optional: 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.
| Workspaces: []tektonv1.WorkspaceDeclaration{ | |
| { | |
| Name: "jumpstarter-client", | |
| Description: "Workspace containing the Jumpstarter client config (client.yaml)", | |
| MountPath: "/workspace/jumpstarter-client", | |
| Optional: true, | |
| }, | |
| Workspaces: []tektonv1.WorkspaceDeclaration{ | |
| { | |
| Name: "jumpstarter-client", | |
| Description: "Workspace containing the Jumpstarter client config (client.yaml)", | |
| MountPath: "/workspace/jumpstarter-client", | |
| }, |
🤖 Prompt for AI Agents
In `@internal/common/tasks/tasks.go` around lines 1215 - 1221, The workspace
"jumpstarter-client" is declared Optional: true but a task step unconditionally
sets the JMP_CLIENT_CONFIG env var to
"/workspace/jumpstarter-client/client.yaml" so the step will fail if the
workspace is not bound; make the workspace required by removing Optional: true
from the Workspaces entry for "jumpstarter-client" in the Workspaces:
[]tektonv1.WorkspaceDeclaration block (or alternatively guard the step that sets
JMP_CLIENT_CONFIG to only run when the workspace is present), and ensure the
step that references JMP_CLIENT_CONFIG (the flash/client step) expects the
config to exist.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/common/tasks/tasks.go`:
- Around line 1171-1172: The JumpstarterImage constant currently uses the
non-deterministic ":latest" tag; update the constant JumpstarterImage to
reference a specific, published version (e.g.,
"quay.io/jumpstarter-dev/jumpstarter:0.7.3") so builds are reproducible and
deterministic—modify the const JumpstarterImage declaration accordingly to use
the chosen version tag.
🧹 Nitpick comments (4)
internal/common/tasks/scripts/flash_image.sh (1)
57-66: Add success message after flash completion.The script logs the start of the flash operation and errors, but doesn't log successful completion. Adding a success message improves observability in Tekton logs.
✨ Suggested improvement
if ! jmp shell --client-config "${JMP_CLIENT_CONFIG}" --lease "${LEASE_NAME}" -- ${FLASH_CMD}; then echo "" echo "ERROR: Flash command failed" exit 1 fi FLASH_SUCCESS=true + +echo "" +echo "=== Flash operation completed successfully ===" +echo "Lease preserved: ${LEASE_NAME}"internal/common/tasks/tasks.go (1)
600-604: Consider adding a When condition to the lease-id pipeline result to match the flash-image task's skip condition.The
lease-idresult references$(tasks.flash-image.results.lease-id). When the flash-image task is skipped due to its When condition, Tekton does not emit this result (it is simply omitted fromstatus.pipelineResults). While the controller already guards withIsFlashEnabled()before callingextractLeaseID, the pipeline definition would be cleaner if the result itself were also guarded to explicitly indicate it only applies when flash is enabled. This makes the pipeline's intent clearer and avoids the result being conditionally present in the output.cmd/caib/main.go (2)
483-499: Consider extracting shared flash‑config setup.
The validation + client‑config encoding block is duplicated inrunBuild,runDisk, andrunBuildDev; a small helper would keep behavior consistent and reduce drift.Also applies to: 597-613, 884-900
82-88: Use separate variables for flash-specific flags to improve code clarity.While the shared
targetandwaitForBuildvariables work correctly today (Cobra's flag system maintains per-command defaults), reusing them across different commands creates unnecessary risk and makes the code harder to maintain. InrunFlash(lines 1541–1588), using dedicatedflashTargetandflashWaitvariables prevents potential confusion if these globals are modified elsewhere, and makes the flash command's requirements explicit. The suggested refactoring improves readability without changing behavior.Suggested refactoring
- flashAfterBuild bool - jumpstarterClient string - flashName string - exporterSelector string - leaseDuration string + flashAfterBuild bool + jumpstarterClient string + flashName string + exporterSelector string + leaseDuration string + flashTarget string + flashWait bool- flashCmd.Flags().StringVarP(&target, "target", "t", "", "target platform for exporter lookup") + flashCmd.Flags().StringVarP(&flashTarget, "target", "t", "", "target platform for exporter lookup")- flashCmd.Flags().BoolVarP(&waitForBuild, "wait", "w", true, "wait for flash to complete") + flashCmd.Flags().BoolVarP(&flashWait, "wait", "w", true, "wait for flash to complete")- if target == "" && exporterSelector == "" { + if flashTarget == "" && exporterSelector == "" { handleError(fmt.Errorf("either --target or --exporter is required")) }- Target: target, + Target: flashTarget,- if waitForBuild || followLogs { + if flashWait || followLogs { waitForFlashCompletion(ctx, api, resp.Name) }
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/imagebuild/controller.go (1)
455-554: Validate flash image ref and client secret before creating the PipelineRun.With flash enabled,
flash-image-refcan be empty and the client config secret can be missing, which will cause the flash task to fail at runtime. Enforce both upfront (parity with legacy flash path).🔧 Suggested fix
- // Add flash params if flash is enabled - var flashExporterSelector, flashCmd string + // Add flash params if flash is enabled + var flashExporterSelector, flashCmd, flashImageRef, flashClientSecret string if imageBuild.Spec.IsFlashEnabled() { target := imageBuild.Spec.GetTarget() if operatorConfig.Spec.Jumpstarter != nil { if mapping, ok := operatorConfig.Spec.Jumpstarter.TargetMappings[target]; ok { flashExporterSelector = mapping.Selector flashCmd = mapping.FlashCmd } } if flashExporterSelector == "" { // Flash was requested but no target mapping exists - fail with clear error return fmt.Errorf("flash enabled but no Jumpstarter target mapping found for target %q; "+ "configure OperatorConfig.spec.jumpstarter.targetMappings[%q] with selector and flashCmd", target, target) } + flashImageRef = imageBuild.Spec.GetExportOCI() + if flashImageRef == "" { + return fmt.Errorf("flash enabled but export.disk.oci is empty") + } + flashClientSecret = imageBuild.Spec.GetFlashClientConfigSecretRef() + if flashClientSecret == "" { + return fmt.Errorf("flash enabled but flash client config secret ref is required") + } params = append(params, tektonv1.Param{ Name: "flash-enabled", Value: tektonv1.ParamValue{Type: tektonv1.ParamTypeString, StringVal: "true"}, }, tektonv1.Param{ Name: "flash-image-ref", - Value: tektonv1.ParamValue{Type: tektonv1.ParamTypeString, StringVal: imageBuild.Spec.GetExportOCI()}, + Value: tektonv1.ParamValue{Type: tektonv1.ParamTypeString, StringVal: flashImageRef}, }, tektonv1.Param{ Name: "flash-exporter-selector", Value: tektonv1.ParamValue{Type: tektonv1.ParamTypeString, StringVal: flashExporterSelector}, }, @@ - // Add jumpstarter-client workspace if flash is enabled - if imageBuild.Spec.IsFlashEnabled() && flashExporterSelector != "" { - clientSecretRef := imageBuild.Spec.GetFlashClientConfigSecretRef() - if clientSecretRef != "" { - pipelineWorkspaces = append(pipelineWorkspaces, tektonv1.WorkspaceBinding{ - Name: "jumpstarter-client", - Secret: &corev1.SecretVolumeSource{ - SecretName: clientSecretRef, - }, - }) - } - } + // Add jumpstarter-client workspace if flash is enabled + if imageBuild.Spec.IsFlashEnabled() && flashExporterSelector != "" { + pipelineWorkspaces = append(pipelineWorkspaces, tektonv1.WorkspaceBinding{ + Name: "jumpstarter-client", + Secret: &corev1.SecretVolumeSource{ + SecretName: flashClientSecret, + }, + }) + }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controller/imagebuild/controller.go`:
- Around line 543-554: The pipeline path silently skips adding the
"jumpstarter-client" workspace when
imageBuild.Spec.GetFlashClientConfigSecretRef() returns empty while still
setting the "flash-enabled=true" parameter; make this consistent with
createFlashTaskRun by validating in the createBuildTaskRun/pipeline assembly
path: when imageBuild.Spec.IsFlashEnabled() && flashExporterSelector != ""
ensure clientSecretRef := imageBuild.Spec.GetFlashClientConfigSecretRef() is
non-empty and return an error (or fail fast) if missing instead of omitting the
workspace; update the logic that builds pipelineWorkspaces and the flash-enabled
param to only set flash-enabled when the required clientSecretRef is present or
else surface the validation error.
🧹 Nitpick comments (3)
internal/buildapi/server.go (1)
1881-1881: Consider logging the cleanup error for observability.When TaskRun creation fails, the secret deletion error is silently ignored. While the cleanup is best-effort, logging the failure would aid debugging orphaned resources.
🔧 Suggested improvement
if err := k8sClient.Create(ctx, taskRun); err != nil { // Clean up the secret if TaskRun creation fails - _ = clientset.CoreV1().Secrets(namespace).Delete(ctx, secretName, metav1.DeleteOptions{}) + if delErr := clientset.CoreV1().Secrets(namespace).Delete(ctx, secretName, metav1.DeleteOptions{}); delErr != nil { + log.Printf("WARNING: failed to clean up secret %s after TaskRun creation failed: %v", secretName, delErr) + } c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("failed to create flash TaskRun: %v", err)}) return }internal/controller/imagebuild/controller.go (2)
79-84: Misleading "Legacy phase" comments.The comments describe Pushing and Flashing as "Legacy phases" that are "now part of the pipeline," but
handlePushingState(lines 842-851) still actively transitions builds to the "Flashing" phase when flash is enabled. This creates confusion about the actual execution flow.Consider clarifying the comments to indicate these phases are still used in certain code paths (e.g., when the build was started before flash was integrated into the pipeline, or when using the legacy TaskRun-based flash approach).
328-328: Remove unnecessary blank identifier assignment.
buildConfigis used at lines 576-578 forRuntimeClassName, so this blank assignment is unnecessary and creates confusion about whether the variable is actually used.♻️ Proposed fix
- _ = buildConfig // buildConfig used for RuntimeClassName if needed + // buildConfig is used below for RuntimeClassName if needed
- add --client --flash to kickoff flashing with jumpstarter Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> fix brace issue Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> add nodeselector to e2e node Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> add lease id to imagebuild status Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> add configurable jumpstarter image Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> add validations Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> make sure flash is enabled when adding secret Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
jumpstarter-flash
branch
from
02ee001 to
37dea08
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yaml (1)
253-263: Update the phase description to mention “Flashing”.
The enum now includesFlashing, but the description still lists only Building/Completed/Failed. This can confuse users reading the CRD.
🤖 Fix all issues with AI agents
In `@internal/buildapi/server.go`:
- Around line 1879-1883: The current error handler for k8sClient.Create(ctx,
taskRun) unconditionally deletes the secret
(clientset.CoreV1().Secrets(...).Delete) which can remove a secret needed by an
existing TaskRun when Create returns an AlreadyExists error; change the handler
to detect apierrors.IsAlreadyExists(err) (importing
k8s.io/apimachinery/pkg/api/errors as apierrors) and skip deleting the secret
when the error is an AlreadyExists, but continue to delete the secret for other
failure cases and still return the same HTTP error response; reference
k8sClient.Create, secretName and the Delete call when making this conditional
change.
In `@internal/buildapi/types.go`:
- Around line 175-179: The struct field LeaseDuration's comment currently states
a default of "01:00:00" but the server/CLI use "03:00:00"; update the comment on
the LeaseDuration field in internal/buildapi/types.go (the LeaseDuration string
`json:"leaseDuration,omitempty"` declaration) to reflect the correct default
"03:00:00" (or alternatively remove the hardcoded default text and reference the
canonical default source) so docs match runtime behavior.
In `@internal/controller/imagebuild/controller.go`:
- Around line 455-477: When flash is enabled (imageBuild.Spec.IsFlashEnabled()),
validate that imageBuild.Spec.GetExportOCI() returns a non-empty OCI ref before
appending the "flash-image-ref" Param; if it's empty, return a clear error
(similar to the existing mapping error) indicating flash requires a disk export
OCI. Update the flash setup block that computes flashExporterSelector/flashCmd
and appends to params so it checks GetExportOCI(), returns an error when
missing, and only appends the "flash-image-ref" Param when the OCI ref is
present.
🧹 Nitpick comments (3)
api/v1alpha1/operatorconfig_types.go (1)
57-78: Pin the default Jumpstarter image to a specific version instead of:latest.Using a floating tag makes builds non-deterministic and can introduce unexpected behavior. The Jumpstarter repository has stable release tags available (e.g.,
release-0.7,0.7.4-13-gde656df2) or you can pin to a specific image digest. Update both the constant and the kubebuilder default annotation to stay in sync.api/v1alpha1/imagebuild_types.go (1)
61-68: Add CRD validation for lease duration format.Line 66 documents HH:MM:SS but currently allows any string; a simple pattern guard will prevent invalid values from reaching the controller.
♻️ Suggested CRD validation
// LeaseDuration is the duration for the device lease in HH:MM:SS format +// +kubebuilder:validation:Pattern=`^\d{2}:\d{2}:\d{2}$` // +kubebuilder:default="03:00:00" LeaseDuration string `json:"leaseDuration,omitempty"`internal/controller/imagebuild/controller.go (1)
302-303: Remove the redundant_ = buildConfig.Line 328 is no longer needed because
buildConfigis used later in the function.♻️ Suggested cleanup
- _ = buildConfig // buildConfig used for RuntimeClassName if neededAlso applies to: 328-328
| if err := k8sClient.Create(ctx, taskRun); err != nil { | ||
| // Clean up the secret if TaskRun creation fails | ||
| _ = clientset.CoreV1().Secrets(namespace).Delete(ctx, secretName, metav1.DeleteOptions{}) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("failed to create flash TaskRun: %v", err)}) | ||
| return |
There was a problem hiding this comment.
Avoid deleting the secret when TaskRun already exists.
If k8sClient.Create fails with AlreadyExists, deleting the newly created secret can break an existing flash job that still expects it.
🐛 Safer failure handling
if err := k8sClient.Create(ctx, taskRun); err != nil {
+ if k8serrors.IsAlreadyExists(err) {
+ c.JSON(http.StatusConflict, gin.H{"error": fmt.Sprintf("flash %s already exists", req.Name)})
+ return
+ }
// Clean up the secret if TaskRun creation fails
_ = clientset.CoreV1().Secrets(namespace).Delete(ctx, secretName, metav1.DeleteOptions{})
c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("failed to create flash TaskRun: %v", err)})
return
}🤖 Prompt for AI Agents
In `@internal/buildapi/server.go` around lines 1879 - 1883, The current error
handler for k8sClient.Create(ctx, taskRun) unconditionally deletes the secret
(clientset.CoreV1().Secrets(...).Delete) which can remove a secret needed by an
existing TaskRun when Create returns an AlreadyExists error; change the handler
to detect apierrors.IsAlreadyExists(err) (importing
k8s.io/apimachinery/pkg/api/errors as apierrors) and skip deleting the secret
when the error is an AlreadyExists, but continue to delete the secret for other
failure cases and still return the same HTTP error response; reference
k8sClient.Create, secretName and the Delete call when making this conditional
change.
| // Add flash params if flash is enabled | ||
| var flashExporterSelector, flashCmd string | ||
| if imageBuild.Spec.IsFlashEnabled() { | ||
| target := imageBuild.Spec.GetTarget() | ||
| if operatorConfig.Spec.Jumpstarter != nil { | ||
| if mapping, ok := operatorConfig.Spec.Jumpstarter.TargetMappings[target]; ok { | ||
| flashExporterSelector = mapping.Selector | ||
| flashCmd = mapping.FlashCmd | ||
| } | ||
| } | ||
| if flashExporterSelector == "" { | ||
| return fmt.Errorf("flash enabled but no Jumpstarter target mapping found for target %q; "+ | ||
| "configure OperatorConfig.spec.jumpstarter.targetMappings[%q] with selector and flashCmd", target, target) | ||
| } | ||
| params = append(params, | ||
| tektonv1.Param{ | ||
| Name: "flash-enabled", | ||
| Value: tektonv1.ParamValue{Type: tektonv1.ParamTypeString, StringVal: "true"}, | ||
| }, | ||
| tektonv1.Param{ | ||
| Name: "flash-image-ref", | ||
| Value: tektonv1.ParamValue{Type: tektonv1.ParamTypeString, StringVal: imageBuild.Spec.GetExportOCI()}, | ||
| }, |
There was a problem hiding this comment.
Validate that a disk export OCI ref exists when flash is enabled.
Line 476 uses GetExportOCI() without checking it’s set. If an API client enables flash without exporting a disk image, the pipeline will fail later with an empty flash-image-ref.
🐛 Suggested validation
if imageBuild.Spec.IsFlashEnabled() {
target := imageBuild.Spec.GetTarget()
if operatorConfig.Spec.Jumpstarter != nil {
if mapping, ok := operatorConfig.Spec.Jumpstarter.TargetMappings[target]; ok {
flashExporterSelector = mapping.Selector
flashCmd = mapping.FlashCmd
}
}
if flashExporterSelector == "" {
return fmt.Errorf("flash enabled but no Jumpstarter target mapping found for target %q; "+
"configure OperatorConfig.spec.jumpstarter.targetMappings[%q] with selector and flashCmd", target, target)
}
+ imageRef := imageBuild.Spec.GetExportOCI()
+ if imageRef == "" {
+ return fmt.Errorf("flash enabled but export OCI is empty; set export.disk.oci")
+ }
params = append(params,
tektonv1.Param{
Name: "flash-enabled",
Value: tektonv1.ParamValue{Type: tektonv1.ParamTypeString, StringVal: "true"},
},
tektonv1.Param{
Name: "flash-image-ref",
- Value: tektonv1.ParamValue{Type: tektonv1.ParamTypeString, StringVal: imageBuild.Spec.GetExportOCI()},
+ Value: tektonv1.ParamValue{Type: tektonv1.ParamTypeString, StringVal: imageRef},
},🤖 Prompt for AI Agents
In `@internal/controller/imagebuild/controller.go` around lines 455 - 477, When
flash is enabled (imageBuild.Spec.IsFlashEnabled()), validate that
imageBuild.Spec.GetExportOCI() returns a non-empty OCI ref before appending the
"flash-image-ref" Param; if it's empty, return a clear error (similar to the
existing mapping error) indicating flash requires a disk export OCI. Update the
flash setup block that computes flashExporterSelector/flashCmd and appends to
params so it checks GetExportOCI(), returns an error when missing, and only
appends the "flash-image-ref" Param when the OCI ref is present.
1 participant
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.