rename --lease to --lease-duration#171
Merged
Merged
Conversation
and have --lease allow users to provide their own lease Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds support for using an existing Jumpstarter lease by introducing LeaseName across API types, CLI options, server handlers, Tekton pipeline parameters, and flash scripts, with mutual-exclusivity checks against lease-duration and propagation of the lease name through build/flash flows. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI / User
participant API as Build API Server
participant Tekton as Tekton Pipeline
participant Task as Flash Task / Script
participant JS as Jumpstarter Service
CLI->>API: Submit BuildRequest (LeaseName or LeaseDuration)
API->>API: Validate mutual exclusivity (LeaseName XOR LeaseDuration)
alt LeaseName provided
API->>Tekton: Start pipeline with param lease-name
Tekton->>Task: Pass EXISTING_LEASE from param
Task->>JS: Use existing lease (no create)
JS-->>Task: Lease confirmed
else LeaseDuration provided
API->>Tekton: Start pipeline with flash-lease-name duration
Tekton->>Task: Create new lease
Task->>JS: Create lease with duration
JS-->>Task: Lease created
end
Task->>Task: Perform flash/build operations
alt existing lease used
Task->>JS: Leave lease active (no release)
else new lease created
Task->>JS: Release lease on completion
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📝 Coding Plan
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/buildapi/server.go (1)
1560-1578:⚠️ Potential issue | 🟠 MajorEnforce lease-name/lease-duration exclusivity in the build path too.
Line 1575 adds
FlashLeaseNamepropagation, butcreateBuildstill accepts bothFlashLeaseNameandFlashLeaseDurationsimultaneously. This creates ambiguous build input behavior and diverges from the flash endpoint validation.🛠️ Suggested fix
if req.FlashEnabled { if req.FlashClientConfig == "" { c.JSON(http.StatusBadRequest, gin.H{"error": "flash enabled but client config is required"}) return } + req.FlashLeaseName = strings.TrimSpace(req.FlashLeaseName) + req.FlashLeaseDuration = strings.TrimSpace(req.FlashLeaseDuration) + if req.FlashLeaseName != "" && req.FlashLeaseDuration != "" { + c.JSON(http.StatusBadRequest, gin.H{"error": "flashLeaseName and flashLeaseDuration are mutually exclusive"}) + return + } + if req.FlashLeaseName != "" { + req.FlashLeaseDuration = "" + } flashSecretName = req.Name + "-jumpstarter-client"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/buildapi/server.go` around lines 1560 - 1578, The build path currently allows both FlashLeaseName and FlashLeaseDuration to be set simultaneously (introduced when propagating FlashLeaseName), leading to ambiguous inputs; update the createBuild flow to validate exclusivity by checking req.FlashEnabled and then returning HTTP 400 if both req.FlashLeaseName and req.FlashLeaseDuration are non-zero/ non-empty, mirroring the flash endpoint validation—locate the createBuild handler (or the function that prepares flashSpec) and add a guard that rejects requests with both fields set and returns a clear error message like "flash lease-name and lease-duration are mutually exclusive".
🧹 Nitpick comments (1)
cmd/caib/image/image.go (1)
144-146: Consider deduplicating lease flag registration across commands.The same
--lease-duration/--leaseflag setup is repeated four times. Extracting a small helper will reduce drift risk and keep help text/behavior aligned.♻️ Suggested refactor
+func addLeaseFlags(cmd *cobra.Command, leaseDuration, leaseName *string) { + cmd.Flags().StringVar(leaseDuration, "lease-duration", "03:00:00", "device lease duration for flash (HH:MM:SS)") + cmd.Flags().StringVar(leaseName, "lease", "", "existing Jumpstarter lease name (mutually exclusive with --lease-duration)") +} - buildCmd.Flags().StringVar(opts.LeaseDuration, "lease-duration", "03:00:00", "device lease duration for flash (HH:MM:SS)") - buildCmd.Flags().StringVar(opts.LeaseName, "lease", "", "existing Jumpstarter lease name (mutually exclusive with --lease-duration)") + addLeaseFlags(buildCmd, opts.LeaseDuration, opts.LeaseName) - diskCmd.Flags().StringVar(opts.LeaseDuration, "lease-duration", "03:00:00", "device lease duration for flash (HH:MM:SS)") - diskCmd.Flags().StringVar(opts.LeaseName, "lease", "", "existing Jumpstarter lease name (mutually exclusive with --lease-duration)") + addLeaseFlags(diskCmd, opts.LeaseDuration, opts.LeaseName) - buildDevCmd.Flags().StringVar(opts.LeaseDuration, "lease-duration", "03:00:00", "device lease duration for flash (HH:MM:SS)") - buildDevCmd.Flags().StringVar(opts.LeaseName, "lease", "", "existing Jumpstarter lease name (mutually exclusive with --lease-duration)") + addLeaseFlags(buildDevCmd, opts.LeaseDuration, opts.LeaseName) - flashCmd.Flags().StringVar(opts.LeaseDuration, "lease-duration", "03:00:00", "device lease duration (HH:MM:SS)") - flashCmd.Flags().StringVar(opts.LeaseName, "lease", "", "existing Jumpstarter lease name (mutually exclusive with --lease-duration)") + addLeaseFlags(flashCmd, opts.LeaseDuration, opts.LeaseName)Also applies to: 202-204, 242-244, 268-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/image/image.go` around lines 144 - 146, Extract the repeated lease flag registration into a single helper (e.g., add a function registerLeaseFlags(cmd *cobra.Command, leaseDuration *string, leaseName *string)) and replace each duplicated block that calls buildCmd.Flags().StringVar(opts.LeaseDuration, "lease-duration", ...), buildCmd.Flags().StringVar(opts.LeaseName, "lease", ...) with a call to that helper; ensure the helper sets the same defaults and help text (including mutual-exclusion semantics) and update the four locations that currently register --lease-duration/--lease so they pass the appropriate command and opts fields (LeaseDuration, LeaseName).
🤖 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/buildcmd/build.go`:
- Around line 414-416: Add a mutual-exclusivity check before assigning
FlashLeaseDuration and FlashLeaseName: detect when both h.opts.LeaseDuration and
h.opts.LeaseName are non-nil (or set) and return an error, rejecting the request
(same logic as the validation in cmd/caib/flashcmd/flash.go around lines
103-105); only set req.FlashLeaseDuration = *h.opts.LeaseDuration or
req.FlashLeaseName = *h.opts.LeaseName when the other is not provided to ensure
they cannot both be included in the request.
In `@internal/buildapi/server.go`:
- Around line 2830-2834: Trim and normalize lease inputs before any validation
or use: call strings.TrimSpace on req.LeaseName and req.LeaseDuration
immediately after parsing the request (before the mutual-exclusivity check that
references req.LeaseName/req.LeaseDuration) and use those trimmed values when
you build the TaskRun parameters (the code path that constructs the TaskRun
param at/around the TaskRun creation). Ensure all checks and the value passed to
the TaskRun creation logic reference the trimmed variables so whitespace-only
strings are treated as empty.
---
Outside diff comments:
In `@internal/buildapi/server.go`:
- Around line 1560-1578: The build path currently allows both FlashLeaseName and
FlashLeaseDuration to be set simultaneously (introduced when propagating
FlashLeaseName), leading to ambiguous inputs; update the createBuild flow to
validate exclusivity by checking req.FlashEnabled and then returning HTTP 400 if
both req.FlashLeaseName and req.FlashLeaseDuration are non-zero/ non-empty,
mirroring the flash endpoint validation—locate the createBuild handler (or the
function that prepares flashSpec) and add a guard that rejects requests with
both fields set and returns a clear error message like "flash lease-name and
lease-duration are mutually exclusive".
---
Nitpick comments:
In `@cmd/caib/image/image.go`:
- Around line 144-146: Extract the repeated lease flag registration into a
single helper (e.g., add a function registerLeaseFlags(cmd *cobra.Command,
leaseDuration *string, leaseName *string)) and replace each duplicated block
that calls buildCmd.Flags().StringVar(opts.LeaseDuration, "lease-duration",
...), buildCmd.Flags().StringVar(opts.LeaseName, "lease", ...) with a call to
that helper; ensure the helper sets the same defaults and help text (including
mutual-exclusion semantics) and update the four locations that currently
register --lease-duration/--lease so they pass the appropriate command and opts
fields (LeaseDuration, LeaseName).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 529d15da-62c8-4895-b960-71c314d0f479
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (12)
api/v1alpha1/imagebuild_types.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/flashcmd/flash.gocmd/caib/image/image.gocmd/caib/main.gocmd/caib/runtime_wiring.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/scripts/flash_image.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/caib/buildcmd/build.go (1)
425-426:⚠️ Potential issue | 🔴 CriticalOnly one flash lease field should be sent in requests.
At Line 425/549/691 and Line 426/550/692, both
FlashLeaseDurationandFlashLeaseNameare always populated. This can still fail valid lease-name flows at downstream validation and breaks the new--leasebehavior.Proposed fix
- req.FlashLeaseDuration = *h.opts.LeaseDuration - req.FlashLeaseName = *h.opts.LeaseName + if *h.opts.LeaseName != "" { + req.FlashLeaseName = *h.opts.LeaseName + } else { + req.FlashLeaseDuration = *h.opts.LeaseDuration + }Apply this same conditional in all three flash-enabled blocks (RunBuild, RunDisk, RunBuildDev).
Also applies to: 549-550, 691-692
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/build.go` around lines 425 - 426, Both FlashLeaseDuration and FlashLeaseName are always being set on req which breaks downstream validation; change the three flash-enabled locations (inside RunBuild, RunDisk, RunBuildDev where req is populated) to set only one flash lease field: if opts.LeaseName is provided (non-nil/non-empty) set req.FlashLeaseName and do not set req.FlashLeaseDuration, otherwise if opts.LeaseDuration is provided set req.FlashLeaseDuration and do not set req.FlashLeaseName. Update the blocks that currently assign req.FlashLeaseDuration = *h.opts.LeaseDuration and req.FlashLeaseName = *h.opts.LeaseName to use this conditional logic so only one field is populated per request.
🧹 Nitpick comments (1)
cmd/caib/buildcmd/build_disk_test.go (1)
87-110: Add a success-path test for lease-only usage.This test covers the conflict path, but it does not assert the intended new behavior where only
--leaseis provided and execution proceeds. Please add a companion test to guard the PR objective and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/build_disk_test.go` around lines 87 - 110, Add a new unit test that verifies the success path when only --lease is provided (complementing TestRunDiskRejectsLeaseAndLeaseDuration). Create e.g. TestRunDiskAcceptsLeaseOnly that uses newTestDiskOpts(), sets *opts.LeaseName = "my-lease" and ensures you do NOT set the lease-duration flag, capture errors via opts.HandleError (like in the existing test), construct h := NewHandler(opts) and cmd := &cobra.Command{}, call h.RunDisk(cmd, []string{"quay.io/test/image:latest"}), and assert capturedErr == nil (fail the test if an error is returned) to ensure RunDisk accepts the lease-only case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 425-426: Both FlashLeaseDuration and FlashLeaseName are always
being set on req which breaks downstream validation; change the three
flash-enabled locations (inside RunBuild, RunDisk, RunBuildDev where req is
populated) to set only one flash lease field: if opts.LeaseName is provided
(non-nil/non-empty) set req.FlashLeaseName and do not set
req.FlashLeaseDuration, otherwise if opts.LeaseDuration is provided set
req.FlashLeaseDuration and do not set req.FlashLeaseName. Update the blocks that
currently assign req.FlashLeaseDuration = *h.opts.LeaseDuration and
req.FlashLeaseName = *h.opts.LeaseName to use this conditional logic so only one
field is populated per request.
---
Nitpick comments:
In `@cmd/caib/buildcmd/build_disk_test.go`:
- Around line 87-110: Add a new unit test that verifies the success path when
only --lease is provided (complementing
TestRunDiskRejectsLeaseAndLeaseDuration). Create e.g.
TestRunDiskAcceptsLeaseOnly that uses newTestDiskOpts(), sets *opts.LeaseName =
"my-lease" and ensures you do NOT set the lease-duration flag, capture errors
via opts.HandleError (like in the existing test), construct h :=
NewHandler(opts) and cmd := &cobra.Command{}, call h.RunDisk(cmd,
[]string{"quay.io/test/image:latest"}), and assert capturedErr == nil (fail the
test if an error is returned) to ensure RunDisk accepts the lease-only case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: decb35ed-f1f5-43f6-8735-93c2196e0cd3
📒 Files selected for processing (2)
cmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.go
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bkhizgiy
approved these changes
Mar 15, 2026
Merged
10 tasks
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.
and have --lease allow users to provide their own lease
Summary by CodeRabbit
New Features
Bug Fixes
Improvements