client: exclude None selector from match check#624
client: exclude None selector from match check#624mangelajo merged 2 commits intojumpstarter-dev:mainfrom
Conversation
if the selector is None, use the existing selector 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. |
WalkthroughIntroduces a conditional guard in Lease.request_async so selector comparison and mismatch handling only occur when a selector is explicitly provided; otherwise, an existing lease is reused. Logging about selector mismatches is now conditional on an explicitly provided selector. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Lease as Lease.request_async
participant Store as LeaseStore
Caller->>Lease: request_async(selector?)
Lease->>Store: get existing_lease
alt selector is provided (not None)
alt existing_lease exists and selector matches
Lease-->>Caller: reuse existing lease
else existing_lease exists and selector mismatches
Note over Lease: Log warning (selector mismatch)
Lease->>Store: create new lease
Lease-->>Caller: return new lease
end
else no selector provided (None)
alt existing_lease exists
Lease-->>Caller: reuse existing lease
else no existing_lease
Lease->>Store: create new lease
Lease-->>Caller: return new lease
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter/jumpstarter/client/lease.py (1)
100-102: Reject creating a lease when selector is None — raise LeaseError before CreateLeaseconfig.lease() allows selector=None and that None currently flows into Lease._create(), which calls ClientService.CreateLease(client_pb2.Lease(selector=selector)) — passing None will fail. Either validate/require a non-None selector at the callsite or guard in Lease.request_async before calling _create().
Suggested change (applies at packages/jumpstarter/jumpstarter/client/lease.py around lines 99–102):
- else: - await self._create() + else: + if self.selector is None: + raise LeaseError( + "selector must be provided to create a new lease when no existing lease is set" + ) + await self._create()Also reconcile the API mismatch: packages/jumpstarter/jumpstarter/config/client.py exposes lease(selector: str | None = None) but lease_async and Lease expect a non-None selector — either require selector upstream or convert/validate earlier.
🧹 Nitpick comments (3)
packages/jumpstarter/jumpstarter/client/lease.py (3)
33-33: Type should allow None for selector.Runtime now treats None as meaningful; the annotation still says str. Update to reflect API and avoid type-checker noise.
- selector: str + selector: str | None
79-86: Document None‑selector semantics in the docstring.Clarify behavior for callers to prevent misuse.
"""Request a lease, or verifies a lease which was already created. :return: lease :rtype: Lease :raises LeaseError: if lease is unsatisfiable :raises LeaseError: if lease is not pending :raises TimeoutError: if lease is not ready after timeout + Note: If selector is None and a lease name is provided, the existing lease's selector is kept. + If selector is None and no lease name is provided, creating a new lease is not allowed. """
87-102: Add tests for None‑selector flow.Cover:
- existing lease name set + selector=None → reuse, no warning, no create
- existing lease name set + selector=equal → reuse
- existing lease name set + selector=different → warning + new lease
- no existing lease name + selector=None → raises LeaseError (if you adopt the guard)
I can add async unit tests with a stubbed ClientService; say the word.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter/jumpstarter/client/lease.py(1 hunks)
⏰ 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). (11)
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (1)
packages/jumpstarter/jumpstarter/client/lease.py (1)
90-99: LGTM: mismatch check gated on explicit selector.This matches the PR intent and avoids spurious re-leasing when selector is None.
|
Successfully created backport PR for |
2 participants
if the selector is None, use the existing selector
Summary by CodeRabbit