client: compare lease selectors#620
Conversation
when a lease is already is set in the env (JMP_LEASE), the requested selector will be ignored, even if it does not match the existing lease. Instead of silently using the env lease, warn and create a new lease Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughUpdates lease acquisition in request_async: if a lease name is provided, it fetches the current lease, compares selectors, logs a warning and clears the name if selectors differ, then creates a new lease; otherwise uses the existing lease. If no name is provided, it creates a new lease. Finally, it acquires the lease. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant LeaseClient as LeaseClient.request_async
participant LeaseAPI as Lease API
Caller->>LeaseClient: request_async(selector, name?)
alt name provided
LeaseClient->>LeaseAPI: fetch existing lease by name
LeaseAPI-->>LeaseClient: existing lease (selector_existing)
alt selector_existing != selector_requested
Note over LeaseClient: Log warning: selector mismatch
LeaseClient->>LeaseClient: clear name
LeaseClient->>LeaseAPI: _create(selector_requested)
LeaseAPI-->>LeaseClient: new lease
else selectors match
Note over LeaseClient: Use existing lease (no create)
end
else no name provided
LeaseClient->>LeaseAPI: _create(selector_requested)
LeaseAPI-->>LeaseClient: new lease
end
LeaseClient->>LeaseAPI: _acquire()
LeaseAPI-->>LeaseClient: acquired lease
LeaseClient-->>Caller: lease handle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (3)
packages/jumpstarter/jumpstarter/client/lease.py (3)
88-90: Avoid misleading log: log “using existing lease …” only after selectors match.Right now we log that we’re using the existing lease before we know if we’ll discard it. Move the debug log into the match branch.
- logger.debug("using existing lease via env or flag %s", self.name) - existing_lease = await self.get() - if existing_lease.selector != self.selector: + existing_lease = await self.get() + if existing_lease.selector != self.selector: logger.warning( "Existing lease from env or flag %s has selector '%s' but requested selector is '%s'. " "Creating a new lease instead", self.name, existing_lease.selector, self.selector, ) - self.name = None - await self._create() + await self._create() + else: + logger.debug("Using existing lease via env or flag %s", self.name)
98-99: Don’t null the name before creating the new lease.
_create()overwritesself.name. Dropping the intermediateNoneavoids a transient inconsistent state and preserves the old name if creation fails.
90-97: Selector comparison may be order/format sensitive.If selectors are logically equivalent but differ in textual order/spacing,
!=will falsely trigger a new lease. Consider normalizing/parsing selectors before comparison, or confirm the server returns a canonical form.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter/jumpstarter/client/lease.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter/jumpstarter/client/lease.py (1)
packages/jumpstarter/jumpstarter/common/metadata.py (1)
name(13-14)
⏰ 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). (8)
- GitHub Check: e2e
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
|
Successfully created backport PR for |
2 participants
when a lease is already is set in the env (JMP_LEASE), the requested selector will be ignored, even if it does not match the existing lease.
Instead of silently using the env lease, warn and create a new lease
Summary by CodeRabbit
Bug Fixes
Chores