Skip to content

Conversation

@bennyz
Copy link
Member

@bennyz bennyz commented Dec 2, 2025

This could be useful for processes where the lease name across different components

needs supports in jumpstarter

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Lease creation now reliably uses a provided lease identifier when present; if none is supplied, the system generates a unique identifier automatically. This ensures consistent, properly formatted lease IDs and prevents ambiguous or missing identifiers during lease creation and subsequent management operations.

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

This could be useful for processes where the lease name across different components

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 2, 2025

📝 Walkthrough

Walkthrough

The CreateLease implementation now uses the provided LeaseId as the lease name when non-empty; otherwise it generates a UUIDv7 string. LeaseFromProtobuf is called with the final name string (no .String() conversion).

Changes

Cohort / File(s) Change Summary
Lease name assignment logic
internal/service/client/v1/client_service.go
Added conditional to use provided LeaseId as the lease name or generate a UUIDv7 when empty; LeaseFromProtobuf now receives the resolved name string directly (removed .String() usage)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I hopped in code with nimble delight,
Use your id if given, else seven will light.
Names now pass plainly, no .String() in sight,
A tiny change, cozy and bright. ✨

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 title 'support custom lease name request' directly aligns with the PR's main objective of adding support for custom lease names, and matches the primary change in the code where CreateLease now uses a provided LeaseId as the lease name.
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 custom-lease-name

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef11c56 and 34f8577.

📒 Files selected for processing (1)
  • internal/service/client/v1/client_service.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/service/client/v1/client_service.go
⏰ 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: e2e-tests-release-0-7
  • GitHub Check: e2e-tests (ubuntu-24.04)
  • GitHub Check: tests
  • GitHub Check: e2e-tests (ubuntu-24.04-arm)
  • GitHub Check: lint-go
  • GitHub Check: deploy-kind
  • GitHub Check: e2e-test-operator

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

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

@bennyz bennyz requested a review from mangelajo December 2, 2025 11:08
@bennyz bennyz marked this pull request as ready for review December 24, 2025 06:09
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
internal/service/client/v1/client_service.go (1)

177-185: Consider validating custom lease names upfront.

The implementation correctly allows custom lease names via req.LeaseId. However, Kubernetes resource names must follow DNS subdomain format (lowercase alphanumeric, '-', '.', max 253 chars). While Kubernetes will reject invalid names at s.Create(), validating upfront would provide clearer, earlier feedback to clients.

Example validation
 	// Use provided lease_id if specified, otherwise generate a UUIDv7
 	name := req.LeaseId
 	if name == "" {
 		id, err := uuid.NewV7()
 		if err != nil {
 			return nil, err
 		}
 		name = id.String()
+	} else {
+		// Validate custom lease name format
+		if err := validation.IsDNS1123Subdomain(name); len(err) > 0 {
+			return nil, fmt.Errorf("invalid lease name: %v", err)
+		}
 	}

Import required: "k8s.io/apimachinery/pkg/util/validation"

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 77ffdb7 and ef11c56.

📒 Files selected for processing (1)
  • internal/service/client/v1/client_service.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-12T13:58:24.065Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 115
File: internal/service/controller_service.go:740-775
Timestamp: 2025-03-12T13:58:24.065Z
Learning: Clients are allowed to see each other's leases in the Jumpstarter controller. The ListLeases function intentionally does not filter by client ownership.

Applied to files:

  • internal/service/client/v1/client_service.go
🧬 Code graph analysis (1)
internal/service/client/v1/client_service.go (2)
api/v1alpha1/lease_helpers.go (1)
  • LeaseFromProtobuf (76-115)
api/v1alpha1/lease_types.go (1)
  • Lease (80-86)
🔇 Additional comments (1)
internal/service/client/v1/client_service.go (1)

189-189: LGTM! Correct use of the name variable.

The name variable (whether custom or generated) is correctly passed to LeaseFromProtobuf to construct the lease object.

@bennyz bennyz merged commit 1cb7846 into main Dec 24, 2025
10 checks passed
@bennyz bennyz deleted the custom-lease-name branch December 24, 2025 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants