skip duration validation for lease transfer

[bennyz]

needed for jumpstarter-dev/jumpstarter#765

Summary by CodeRabbit

• Bug Fixes

  • Improved lease update validation to prevent modifying begin time after a lease has already started.

• Performance

  • Optimized lease update operations to skip unnecessary parsing and validation when updating non-time fields.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

UpdateLease now conditionally parses and applies time-related fields (BeginTime, Duration, EndTime) only when explicitly provided in the request, deferring related validations and recalculations. Non-time field updates skip unnecessary time parsing logic.

Changes

Cohort / File(s)	Summary
Lease time-field conditional handling internal/service/client/v1/client_service.go	UpdateLease refactored to parse, validate, and apply time fields only when provided; short-circuits unnecessary time-field processing when updating non-time attributes; BeginTime validation against current lease state added as guard condition

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• support future leases, calculate effective begin/end/duration lease times #168: Modifies the same UpdateLease function with ReconcileLeaseTimeFields to conditionally handle BeginTime, EndTime, and Duration fields.

Suggested reviewers

• mangelajo

Poem

🐰 With conditional whiskers and time-field care,
UpdateLease now skips what isn't there,
When leases need parsing, validation's in place,
But non-time updates hop fast through the race!
Optimized hops for your lease rebinding,
Smart little logic—no wasted rewinding! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'skip duration validation for lease transfer' is partially related to the changeset. The summary shows the change conditionally handles time-field parsing and validation, but the title focuses only on 'duration validation' and 'lease transfer', which is narrower than the actual scope of changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch skip-duration-validation

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1cb7846 and a9925e4.

📒 Files selected for processing (1)

• internal/service/client/v1/client_service.go

🧰 Additional context used

🧬 Code graph analysis (1)

internal/service/client/v1/client_service.go (2)

api/v1alpha1/lease_types.go (1)

• Lease (80-86)

api/v1alpha1/lease_helpers.go (2)

• LeaseFromProtobuf (172-211)
• ReconcileLeaseTimeFields (54-75)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: lint-go
• GitHub Check: e2e-tests-release-0-7
• GitHub Check: e2e-tests (ubuntu-24.04-arm)
• GitHub Check: e2e-tests (ubuntu-24.04)
• GitHub Check: tests
• GitHub Check: e2e-test-operator
• GitHub Check: deploy-kind

🔇 Additional comments (3)

internal/service/client/v1/client_service.go (3)

237-253: LGTM! Conditional field updates are correctly implemented.

The selective update logic properly:

• Restricts BeginTime changes after a lease starts (line 239-243)
• Preserves existing values when fields aren't provided in the request
• Validates BeginTime consistency only when explicitly updated

This approach correctly enables lease transfers without touching time fields, which aligns with the PR objectives.

---

279-284: LGTM! Conditional reconciliation achieves the PR's goal.

The conditional call to ReconcileLeaseTimeFields is the key change that allows lease transfers to bypass duration validation when no time fields are updated. When time fields are provided, the validation ensures consistency of the final state. This is a clean implementation of the feature described in the PR title.

---

226-235: Clarify the API contract for partial time field updates in UpdateLease.

The conditional parsing enables the PR goal (skipping time validation during lease transfers), but creates a constraint: if a user updates a single time field (e.g., only Duration), the final ReconcileLeaseTimeFields call at line 281 will fail if the existing lease already has all three time fields set.

Example:

• Existing lease: BeginTime=T1, Duration=1h, EndTime=T1+1h
• Update request: Duration=2h only
• After line 248: jlease.Spec has BeginTime=T1, Duration=2h, EndTime=T1+1h
• Line 281 reconciliation: calculates Duration = EndTime - BeginTime = 1h, conflicts with Duration=2h → error

This means users must provide at least two consistent time fields (e.g., Duration + EndTime, or BeginTime + Duration) to update any existing time field. Either document this API constraint or modify the implementation to validate partial updates before merging with existing state (e.g., use the UpdateMask field that already exists in UpdateLeaseRequest).

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}