Use Lease related methods from client service (phase 1)#342
Conversation
WalkthroughThis pull request reorganizes import statements and refactors the protocol buffer definitions and gRPC service implementations into a new namespace. It introduces new files for the client v1 APIs—including message definitions and a gRPC service for lease and exporter operations—and removes the outdated files under the previous namespace. Additionally, the gRPC client now supports comprehensive lease management with extended identifier parsing, new Lease data models, and corresponding asynchronous methods. Several deprecated lease management methods and tests have been removed, and a small configuration update has been made in the project file. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as ClientConfigV1Alpha1
participant ClientSvc as ClientService (gRPC Client)
participant Stub as gRPC Stub
participant Server as gRPC Server (Lease Provider)
Config->>ClientSvc: CreateLease(selector, duration)
ClientSvc->>Stub: CreateLease(request)
Stub->>Server: Process CreateLease request
Server-->>Stub: Lease Created Response
Stub-->>ClientSvc: Return Lease Info
ClientSvc-->>Config: Lease object returned
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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 (5)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
24-26: Consider unifying parse functions or keep them separate for clarity.These three functions differ only by the
kindargument. If you prefer a single function with an argument or enum for different resource types, it would reduce repetition. If readability is paramount, retaining them as separate functions is also fine.Also applies to: 28-30, 32-34
47-57: Well-structuredLeasemodel, consider adding tests.Your approach to storing lease data and providing from/to Protobuf methods is solid. For increased confidence, add unit tests covering edge cases (e.g., empty selectors, zero durations) and confirm your assumptions about namespace, name, etc.
Also applies to: 59-65, 67-85
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py (1)
1-143: Address unused imports and line length issues in generated code.Ruff flags multiple unused imports (e.g.,
google.api.annotations_pb2, etc.) and long lines. If you wish to suppress these warnings, remove unneeded references from the proto definitions or configure your style checks to ignore generated files.🧰 Tools
🪛 Ruff (0.8.2)
25-25: Module level import not at top of file
(E402)
25-25:
google.api.annotations_pb2imported but unusedRemove unused import:
google.api.annotations_pb2(F401)
26-26: Module level import not at top of file
(E402)
26-26:
google.api.client_pb2imported but unusedRemove unused import:
google.api.client_pb2(F401)
27-27: Module level import not at top of file
(E402)
27-27:
google.api.field_behavior_pb2imported but unusedRemove unused import:
google.api.field_behavior_pb2(F401)
28-28: Module level import not at top of file
(E402)
28-28:
google.api.resource_pb2imported but unusedRemove unused import:
google.api.resource_pb2(F401)
29-29: Module level import not at top of file
(E402)
29-29:
google.protobuf.duration_pb2imported but unusedRemove unused import:
google.protobuf.duration_pb2(F401)
30-30: Module level import not at top of file
(E402)
30-30:
google.protobuf.empty_pb2imported but unusedRemove unused import:
google.protobuf.empty_pb2(F401)
31-31: Module level import not at top of file
(E402)
31-31:
google.protobuf.field_mask_pb2imported but unusedRemove unused import:
google.protobuf.field_mask_pb2(F401)
32-32: Module level import not at top of file
(E402)
32-32:
google.protobuf.timestamp_pb2imported but unusedRemove unused import:
google.protobuf.timestamp_pb2(F401)
33-33: Module level import not at top of file
(E402)
33-33:
...v1.kubernetes_pb2imported but unusedRemove unused import:
...v1.kubernetes_pb2(F401)
36-36: Line too long (6048 > 120)
(E501)
43-43: Line too long (265 > 120)
(E501)
49-49: Line too long (159 > 120)
(E501)
65-65: Line too long (121 > 120)
(E501)
69-69: Line too long (142 > 120)
(E501)
71-71: Line too long (130 > 120)
(E501)
73-73: Line too long (136 > 120)
(E501)
81-81: Line too long (124 > 120)
(E501)
83-83: Line too long (130 > 120)
(E501)
91-91: Line too long (131 > 120)
(E501)
101-101: Line too long (127 > 120)
(E501)
103-103: Line too long (159 > 120)
(E501)
105-105: Line too long (163 > 120)
(E501)
107-107: Line too long (154 > 120)
(E501)
109-109: Line too long (158 > 120)
(E501)
111-111: Line too long (181 > 120)
(E501)
113-113: Line too long (182 > 120)
(E501)
115-115: Line too long (154 > 120)
(E501)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.py (2)
1-337: Add documentation comments to the protocol buffer fileThis auto-generated gRPC file correctly defines the ClientService with methods for exporter and lease management. However, there are multiple "Missing associated documentation comment in .proto file" notices throughout the code (lines 10, 59, 65, 71, etc.).
Since this is a generated file that shouldn't be modified directly, you should add proper documentation comments to the original .proto file that defines the ClientService. This would improve the generated code's documentation and make the API more self-explanatory for developers.
143-143: Consider implementing validation in future servicer implementationsLine 143 adds the server handlers, but note that the placeholder implementations in ClientServiceServicer don't perform any validation. When implementing the actual servicer, make sure to add proper input validation, error handling, and authentication checks for each method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/e2e.yaml(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/__init__.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2.py(0 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2_grpc.py(0 hunks)packages/jumpstarter/conftest.py(0 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(4 hunks)packages/jumpstarter/jumpstarter/client/lease.py(5 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)packages/jumpstarter/jumpstarter/config/exporter_test.py(0 hunks)packages/jumpstarter/jumpstarter/listener_test.py(0 hunks)
💤 Files with no reviewable changes (5)
- packages/jumpstarter/conftest.py
- packages/jumpstarter/jumpstarter/config/exporter_test.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2_grpc.py
- packages/jumpstarter/jumpstarter/listener_test.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2.py
🧰 Additional context used
🪛 GitHub Check: typos
packages/jumpstarter/jumpstarter/client/grpc.py
[warning] 58-58:
"ser" should be "set".
🪛 GitHub Actions: Spell Check
packages/jumpstarter/jumpstarter/client/grpc.py
[warning] 58-58: "ser" should be "set".
[error] 1-1: error: ser should be set
🪛 Ruff (0.8.2)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py
25-25: Module level import not at top of file
(E402)
25-25: google.api.annotations_pb2 imported but unused
Remove unused import: google.api.annotations_pb2
(F401)
26-26: Module level import not at top of file
(E402)
26-26: google.api.client_pb2 imported but unused
Remove unused import: google.api.client_pb2
(F401)
27-27: Module level import not at top of file
(E402)
27-27: google.api.field_behavior_pb2 imported but unused
Remove unused import: google.api.field_behavior_pb2
(F401)
28-28: Module level import not at top of file
(E402)
28-28: google.api.resource_pb2 imported but unused
Remove unused import: google.api.resource_pb2
(F401)
29-29: Module level import not at top of file
(E402)
29-29: google.protobuf.duration_pb2 imported but unused
Remove unused import: google.protobuf.duration_pb2
(F401)
30-30: Module level import not at top of file
(E402)
30-30: google.protobuf.empty_pb2 imported but unused
Remove unused import: google.protobuf.empty_pb2
(F401)
31-31: Module level import not at top of file
(E402)
31-31: google.protobuf.field_mask_pb2 imported but unused
Remove unused import: google.protobuf.field_mask_pb2
(F401)
32-32: Module level import not at top of file
(E402)
32-32: google.protobuf.timestamp_pb2 imported but unused
Remove unused import: google.protobuf.timestamp_pb2
(F401)
33-33: Module level import not at top of file
(E402)
33-33: ...v1.kubernetes_pb2 imported but unused
Remove unused import: ...v1.kubernetes_pb2
(F401)
36-36: Line too long (6048 > 120)
(E501)
43-43: Line too long (265 > 120)
(E501)
49-49: Line too long (159 > 120)
(E501)
65-65: Line too long (121 > 120)
(E501)
69-69: Line too long (142 > 120)
(E501)
71-71: Line too long (130 > 120)
(E501)
73-73: Line too long (136 > 120)
(E501)
81-81: Line too long (124 > 120)
(E501)
83-83: Line too long (130 > 120)
(E501)
91-91: Line too long (131 > 120)
(E501)
101-101: Line too long (127 > 120)
(E501)
103-103: Line too long (159 > 120)
(E501)
105-105: Line too long (163 > 120)
(E501)
107-107: Line too long (154 > 120)
(E501)
109-109: Line too long (158 > 120)
(E501)
111-111: Line too long (181 > 120)
(E501)
113-113: Line too long (182 > 120)
(E501)
115-115: Line too long (154 > 120)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (20)
.github/workflows/e2e.yaml (1)
21-21: Verify correct E2E branch reference.By switching to
client-svc-leasehere, you’re pointing the E2E test workflow to a new branch for the controller. Please confirm that theclient-svc-leasebranch exists and is stable, so that the workflow can run successfully.packages/jumpstarter-protocol/jumpstarter_protocol/__init__.py (1)
1-4: Reorganized imports look consistent with the new client structure.This change properly aligns the package imports with the updated
client_pb2andclient_pb2_grpcmodules. No issues found here.packages/jumpstarter/jumpstarter/client/lease.py (4)
10-10: Ensure compatibility with newClientServiceandnamespaceusage.
- You’ve replaced the controller-based logic (
ControllerServiceStub) withClientService. Please verify thatClientServiceincludes all functionality formerly provided by the controller, especially for lease management.- The new
namespace: strattribute in theLeasedataclass enforces namespace usage. If an empty namespace is not valid upstream, consider providing a default or raising an error when none is supplied.Also applies to: 15-15, 19-19, 35-35, 48-48
52-53: Confirm server-side handling oftimedelta.Here, the lease creation call passes a native Python
timedeltaobject. Verify that the server or client code properly convertstimedeltainto the corresponding protobuf duration format (if required). Otherwise, this might cause a runtime type mismatch.Also applies to: 55-55, 58-61, 64-64
101-104: Check error handling for missing or invalid lease retrieval.While wrapped in
translate_grpc_exceptions(), ensure that a non-existent or revoked lease triggers an appropriate exception. If additional error states exist (e.g., forbidden namespace), consider handling them as well.
138-141: Finalize lease deletion logic.Deleting the lease within
__aexit__is good for resource cleanup. Be mindful of potential race conditions if the lease has already been invalidated or expired before the delete call. Confirm that the gRPC stub or service’s default error handling gracefully handles such cases.packages/jumpstarter/jumpstarter/client/grpc.py (8)
13-21: No issues found inparse_identifier.
58-58: Potential spelling fix forser_json_timedelta.The pipeline flagged “ser” as a possible spelling error. If it’s a custom config name, ignore this. Otherwise, consider renaming to something like
serialize_json_timedeltaorset_json_timedelta.🧰 Tools
🪛 GitHub Check: typos
[warning] 58-58:
"ser" should be "set".🪛 GitHub Actions: Spell Check
[warning] 58-58: "ser" should be "set".
110-126:LeaseListappears consistent and robust.This design matches the pattern used in
ExporterList, maintaining congruency across data structures.
162-169:GetLeasemethod logic looks solid.Retrieves the lease by full resource name. This aligns with the gRPC patterns for other resources.
170-187:ListLeasesmethod mirrors your exporters logic.Handling pagination and filtering similarly ensures consistency and reduces the maintenance burden.
188-208:CreateLeasemethod aligns with new lease concepts.The code properly uses durations, parent resource paths, and returns a
Leaseobject. Nice implementation.
209-232:UpdateLeasemethod uses field masks effectively.Your approach ensures selective updates for the
durationfield. If more fields need updating, remember to extend the mask.
233-238:DeleteLeasemethod is straightforward and correct.Implements the standard pattern for resource deletion via gRPC.
packages/jumpstarter/jumpstarter/config/client.py (4)
120-120: Namespace reference for lease creation.Passing
namespace=self.metadata.namespaceensures the lease is scoped appropriately. Good move.
132-135: Updatedlist_leases_asyncto invokeClientService.Delegating listing to the new lease functionality is consistent with your refactoring.
138-140:release_lease_asyncmethod.Calls
DeleteLeaseon the client service, fully transitioning away from the prior controller logic. Nicely done.
158-158: Verifymetadata.namespaceavailability.If
metadata.namespacecan beNone, this call may fail. Ensure it’s always set at runtime or handle the missing case to avoid exceptions.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.py (2)
8-53: LGTM: ClientServiceStub implementation looks goodThe ClientServiceStub properly defines all necessary methods for lease management (GetLease, ListLeases, CreateLease, UpdateLease, DeleteLease) and exporter operations (GetExporter, ListExporters). The method configurations for serialization and deserialization are correctly set up.
145-336: Note: ClientService uses experimental gRPC APIThe ClientService class is marked as part of an EXPERIMENTAL API (line 145). Be aware that this API might change in future gRPC releases. Consider documenting this in your client usage documentation to warn developers about potential breaking changes in the future.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pyproject.toml (1)
70-72: Extend Typos Recognition with Custom WordThe addition of the
[tool.typos.default.extend-words]section withser = "ser"correctly extends the typo-checking tool's vocabulary to treat "ser" as valid. Please verify that this is the intended configuration and consider whether any additional related terms should be added for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyproject.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
NickCao
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
13-13: Consider clarifying error messages or adding docstrings
While the logic seems correct, more descriptive error messages or docstrings could improve debugging.Also applies to: 19-20
47-90: Add docstrings or usage examples
TheLeasemodel is well-defined. Adding docstrings or usage examples could help future maintainers understand the intent of each field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter/jumpstarter/client/grpc.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(5 hunks)packages/jumpstarter/jumpstarter/config/client.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter/jumpstarter/client/lease.py
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (18)
packages/jumpstarter/jumpstarter/client/grpc.py (12)
4-4: No concerns with these import additions
These appear necessary for the new lease operations.Also applies to: 7-7, 9-9, 10-10
24-33: Helper functions usage
These wrapper functions for each identifier type are nicely organized and consistent.
94-94: Filtering outnext_page_token
Excludingnext_page_tokenfrom the model representation is a good design choice if it’s an internal field.
110-126: Consistent pattern withLeaseList
This approach mirrorsExporterList, promoting code familiarity and maintainability.
131-131: Namespace property inClientService
Having an explicitnamespacefield ensures clarity and consistency for all gRPC requests.
140-140: String formatting for exporter names
Usingself.namespacein the constructed resource path aligns with the new refactor.
154-154: Parent path usage
Providing the namespace in theparentfield matches the updated resource structure.
162-168:GetLeasemethod
Implementation is straightforward and consistent with existing gRPC call patterns.
170-186:ListLeasesmethod
Uses the same pattern as exporter listing for consistency across resource operations.
187-206:CreateLeasemethod
ConvertingtimedeltatoDurationis a reliable approach.
207-229:UpdateLeaseand field mask
Utilizing field masks is a good way to handle partial updates in gRPC.
230-235:DeleteLeasemethod
Cleanly removes a lease resource, no issues found.packages/jumpstarter/jumpstarter/config/client.py (6)
94-96:get_exporter_asyncrefactor
Passingnamespace=self.metadata.namespaceensures correct resource scoping.
104-106:list_exporters_asyncrefactor
Uses the same approach for scoping with namespace, maintaining consistency.
118-118: Consistent namespace usage
Addingnamespace=self.metadata.namespacealigns Lease creation with the new model.
130-130: New approach to listing leases
Switching from a controller stub toClientServicecreates uniform gRPC usage.Also applies to: 132-133
136-136: Lease deletion logic
Delegating lease deletion toClientServiceunifies the code path for lease operations.Also applies to: 138-138
156-156: Namespace injection inlease_async
Ensuring the Lease has the correct namespace fosters consistency with other resources.
NickCao
force-pushed
the
client-svc-phase1
branch
from
87eae9a to
e05b146
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
24-33: Consider consolidating parse functions.
These thin wrappers each pass a differentkindtoparse_identifier. While it’s clear and readable, you might consider consolidating them into a single parse function or use partial application if these are invoked frequently.
47-55: Add docstrings or inline comments for clarity.
Documenting usage and constraints ofnamespace,selector,client, etc., might help future developers quickly reference how these fields map to your system.
207-229: Extend update mask usage if new fields are updated later.
Currently, your update mask only includes"duration". This is correct for now, but remember to add new fields (e.g.,selector) if you plan to update them in the future.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py (2)
25-33: Remove unused imports or ignore them in generated code.
Static analysis identifies multiple imports (google.api.annotations_pb2,google.api.client_pb2, etc.) that appear unused. Generated code often includes them automatically. If they’re truly unnecessary, remove or suppress lint warnings for this file to maintain clarity.🧰 Tools
🪛 Ruff (0.8.2)
25-25: Module level import not at top of file
(E402)
25-25:
google.api.annotations_pb2imported but unusedRemove unused import:
google.api.annotations_pb2(F401)
26-26: Module level import not at top of file
(E402)
26-26:
google.api.client_pb2imported but unusedRemove unused import:
google.api.client_pb2(F401)
27-27: Module level import not at top of file
(E402)
27-27:
google.api.field_behavior_pb2imported but unusedRemove unused import:
google.api.field_behavior_pb2(F401)
28-28: Module level import not at top of file
(E402)
28-28:
google.api.resource_pb2imported but unusedRemove unused import:
google.api.resource_pb2(F401)
29-29: Module level import not at top of file
(E402)
29-29:
google.protobuf.duration_pb2imported but unusedRemove unused import:
google.protobuf.duration_pb2(F401)
30-30: Module level import not at top of file
(E402)
30-30:
google.protobuf.empty_pb2imported but unusedRemove unused import:
google.protobuf.empty_pb2(F401)
31-31: Module level import not at top of file
(E402)
31-31:
google.protobuf.field_mask_pb2imported but unusedRemove unused import:
google.protobuf.field_mask_pb2(F401)
32-32: Module level import not at top of file
(E402)
32-32:
google.protobuf.timestamp_pb2imported but unusedRemove unused import:
google.protobuf.timestamp_pb2(F401)
33-33: Module level import not at top of file
(E402)
33-33:
...v1.kubernetes_pb2imported but unusedRemove unused import:
...v1.kubernetes_pb2(F401)
36-36: Consider excluding generated files from strict line-length checks.
Line 36 is extremely long due to the serialized descriptor. Typically, ignoring line-length and layout checks for protobuf-generated files is preferred, as manual edits will be overwritten.🧰 Tools
🪛 Ruff (0.8.2)
36-36: Line too long (6048 > 120)
(E501)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.py (1)
5-5: Unused imports in generated code.
google.protobuf.empty_pb2is indeed used, but confirm that all other imports are necessary. If not, remove or ignore them in lint configurations to avoid warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/e2e.yaml(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/__init__.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2.py(0 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2_grpc.py(0 hunks)packages/jumpstarter/conftest.py(0 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(5 hunks)packages/jumpstarter/jumpstarter/config/client.py(4 hunks)packages/jumpstarter/jumpstarter/config/exporter_test.py(0 hunks)packages/jumpstarter/jumpstarter/listener_test.py(0 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (5)
- packages/jumpstarter/jumpstarter/listener_test.py
- packages/jumpstarter/jumpstarter/config/exporter_test.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2_grpc.py
- packages/jumpstarter/conftest.py
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/e2e.yaml
- packages/jumpstarter-protocol/jumpstarter_protocol/init.py
- packages/jumpstarter/jumpstarter/client/lease.py
- packages/jumpstarter/jumpstarter/config/client.py
- pyproject.toml
🧰 Additional context used
🪛 Ruff (0.8.2)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py
25-25: Module level import not at top of file
(E402)
25-25: google.api.annotations_pb2 imported but unused
Remove unused import: google.api.annotations_pb2
(F401)
26-26: Module level import not at top of file
(E402)
26-26: google.api.client_pb2 imported but unused
Remove unused import: google.api.client_pb2
(F401)
27-27: Module level import not at top of file
(E402)
27-27: google.api.field_behavior_pb2 imported but unused
Remove unused import: google.api.field_behavior_pb2
(F401)
28-28: Module level import not at top of file
(E402)
28-28: google.api.resource_pb2 imported but unused
Remove unused import: google.api.resource_pb2
(F401)
29-29: Module level import not at top of file
(E402)
29-29: google.protobuf.duration_pb2 imported but unused
Remove unused import: google.protobuf.duration_pb2
(F401)
30-30: Module level import not at top of file
(E402)
30-30: google.protobuf.empty_pb2 imported but unused
Remove unused import: google.protobuf.empty_pb2
(F401)
31-31: Module level import not at top of file
(E402)
31-31: google.protobuf.field_mask_pb2 imported but unused
Remove unused import: google.protobuf.field_mask_pb2
(F401)
32-32: Module level import not at top of file
(E402)
32-32: google.protobuf.timestamp_pb2 imported but unused
Remove unused import: google.protobuf.timestamp_pb2
(F401)
33-33: Module level import not at top of file
(E402)
33-33: ...v1.kubernetes_pb2 imported but unused
Remove unused import: ...v1.kubernetes_pb2
(F401)
36-36: Line too long (6048 > 120)
(E501)
43-43: Line too long (265 > 120)
(E501)
49-49: Line too long (159 > 120)
(E501)
65-65: Line too long (121 > 120)
(E501)
69-69: Line too long (142 > 120)
(E501)
71-71: Line too long (130 > 120)
(E501)
73-73: Line too long (136 > 120)
(E501)
81-81: Line too long (124 > 120)
(E501)
83-83: Line too long (130 > 120)
(E501)
91-91: Line too long (131 > 120)
(E501)
101-101: Line too long (127 > 120)
(E501)
103-103: Line too long (159 > 120)
(E501)
105-105: Line too long (163 > 120)
(E501)
107-107: Line too long (154 > 120)
(E501)
109-109: Line too long (158 > 120)
(E501)
111-111: Line too long (181 > 120)
(E501)
113-113: Line too long (182 > 120)
(E501)
115-115: Line too long (154 > 120)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
🔇 Additional comments (4)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
13-21: Validate broader usage ofparse_identifier.
This helper function is concise and efficient. Just ensure future code—or other internal methods—handle the raisedValueErrorappropriately to prevent unhandled exceptions in your application flow.
66-83: Validate missing exporter references.
Whendata.exporteris empty, you default to an empty string. Confirm that your usage downstream handles an emptyexporterstring without error. Otherwise, consider raising or logging a warning for clarity.
110-126: Check partial responses inLeaseList.
If the server returns incomplete or partial lease data, ensure your client handles it gracefully. Right now, everything is mapped, but confirm that any field-level missing data doesn’t cause undesired exceptions.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.py (1)
1-337: Avoid manual edits in gRPC-generated files.
All changes here appear to be generated code. Manual modifications might be lost during regeneration. If needed, apply custom logic in a separate extension or partial class.
| pytestmark = pytest.mark.anyio | ||
|
|
||
|
|
||
| async def test_exporter_serve(mock_controller): |
There was a problem hiding this comment.
It also requires a working mock controller, which we don't have anymore. And it's already covered by e2e anyway.
| pytestmark = pytest.mark.anyio | ||
|
|
||
|
|
||
| @pytest.mark.xfail(raises=Exception) |
NickCao
force-pushed
the
client-svc-phase1
branch
from
e05b146 to
cfe2876
Compare
NickCao
force-pushed
the
client-svc-phase1
branch
from
cfe2876 to
b799c40
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
13-20: Consider adding checks for all path segments inparse_identifier.
Currently, the code validates only the first and third segments. You might also want to ensure the second and fourth segments meet any additional criteria if necessary, for more robust error handling.
162-168: Consider implementing error handling inGetLease.
Currently, any gRPC or server-side errors are unhandled. You may want to catch exceptions here to provide clearer logs or fallback logic.
170-185:ListLeasesparallelsListExporterswell.
As withGetLease, consider whether additional timeouts, retries, or exception handling might be needed.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py (1)
25-35: Address unused imports in generated code.
Static analysis flags these imports as unused. Since this file is auto-generated, usually you wouldn’t modify it directly. Instead, consider either adjusting your proto definitions or configuring your linter to skip auto-generated files.🧰 Tools
🪛 Ruff (0.8.2)
25-25: Module level import not at top of file
(E402)
25-25:
google.api.annotations_pb2imported but unusedRemove unused import:
google.api.annotations_pb2(F401)
26-26: Module level import not at top of file
(E402)
26-26:
google.api.client_pb2imported but unusedRemove unused import:
google.api.client_pb2(F401)
27-27: Module level import not at top of file
(E402)
27-27:
google.api.field_behavior_pb2imported but unusedRemove unused import:
google.api.field_behavior_pb2(F401)
28-28: Module level import not at top of file
(E402)
28-28:
google.api.resource_pb2imported but unusedRemove unused import:
google.api.resource_pb2(F401)
29-29: Module level import not at top of file
(E402)
29-29:
google.protobuf.duration_pb2imported but unusedRemove unused import:
google.protobuf.duration_pb2(F401)
30-30: Module level import not at top of file
(E402)
30-30:
google.protobuf.empty_pb2imported but unusedRemove unused import:
google.protobuf.empty_pb2(F401)
31-31: Module level import not at top of file
(E402)
31-31:
google.protobuf.field_mask_pb2imported but unusedRemove unused import:
google.protobuf.field_mask_pb2(F401)
32-32: Module level import not at top of file
(E402)
32-32:
google.protobuf.timestamp_pb2imported but unusedRemove unused import:
google.protobuf.timestamp_pb2(F401)
33-33: Module level import not at top of file
(E402)
33-33:
...v1.kubernetes_pb2imported but unusedRemove unused import:
...v1.kubernetes_pb2(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/jumpstarter-protocol/jumpstarter_protocol/__init__.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2.py(0 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2_grpc.py(0 hunks)packages/jumpstarter/conftest.py(0 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(5 hunks)packages/jumpstarter/jumpstarter/config/client.py(4 hunks)packages/jumpstarter/jumpstarter/config/exporter_test.py(0 hunks)packages/jumpstarter/jumpstarter/listener_test.py(0 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (5)
- packages/jumpstarter/jumpstarter/config/exporter_test.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2.py
- packages/jumpstarter/conftest.py
- packages/jumpstarter/jumpstarter/listener_test.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2_grpc.py
🚧 Files skipped from review as they are similar to previous changes (4)
- pyproject.toml
- packages/jumpstarter-protocol/jumpstarter_protocol/init.py
- packages/jumpstarter/jumpstarter/client/lease.py
- packages/jumpstarter/jumpstarter/config/client.py
🧰 Additional context used
🪛 Ruff (0.8.2)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py
25-25: Module level import not at top of file
(E402)
25-25: google.api.annotations_pb2 imported but unused
Remove unused import: google.api.annotations_pb2
(F401)
26-26: Module level import not at top of file
(E402)
26-26: google.api.client_pb2 imported but unused
Remove unused import: google.api.client_pb2
(F401)
27-27: Module level import not at top of file
(E402)
27-27: google.api.field_behavior_pb2 imported but unused
Remove unused import: google.api.field_behavior_pb2
(F401)
28-28: Module level import not at top of file
(E402)
28-28: google.api.resource_pb2 imported but unused
Remove unused import: google.api.resource_pb2
(F401)
29-29: Module level import not at top of file
(E402)
29-29: google.protobuf.duration_pb2 imported but unused
Remove unused import: google.protobuf.duration_pb2
(F401)
30-30: Module level import not at top of file
(E402)
30-30: google.protobuf.empty_pb2 imported but unused
Remove unused import: google.protobuf.empty_pb2
(F401)
31-31: Module level import not at top of file
(E402)
31-31: google.protobuf.field_mask_pb2 imported but unused
Remove unused import: google.protobuf.field_mask_pb2
(F401)
32-32: Module level import not at top of file
(E402)
32-32: google.protobuf.timestamp_pb2 imported but unused
Remove unused import: google.protobuf.timestamp_pb2
(F401)
33-33: Module level import not at top of file
(E402)
33-33: ...v1.kubernetes_pb2 imported but unused
Remove unused import: ...v1.kubernetes_pb2
(F401)
36-36: Line too long (6048 > 120)
(E501)
43-43: Line too long (265 > 120)
(E501)
49-49: Line too long (159 > 120)
(E501)
65-65: Line too long (121 > 120)
(E501)
69-69: Line too long (142 > 120)
(E501)
71-71: Line too long (130 > 120)
(E501)
73-73: Line too long (136 > 120)
(E501)
81-81: Line too long (124 > 120)
(E501)
83-83: Line too long (130 > 120)
(E501)
91-91: Line too long (131 > 120)
(E501)
101-101: Line too long (127 > 120)
(E501)
103-103: Line too long (159 > 120)
(E501)
105-105: Line too long (163 > 120)
(E501)
107-107: Line too long (154 > 120)
(E501)
109-109: Line too long (158 > 120)
(E501)
111-111: Line too long (181 > 120)
(E501)
113-113: Line too long (182 > 120)
(E501)
115-115: Line too long (154 > 120)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
🔇 Additional comments (14)
packages/jumpstarter/jumpstarter/client/grpc.py (12)
4-4: Imports look correct.
These newly added imports fortimedelta, protobuf modules, and Pydantic are appropriate. No immediate issues.Also applies to: 7-7, 9-9, 10-10
24-33: DRY approach to identifier parsing is solid.
Reusingparse_identifierfor clients, exporters, and leases ensures consistency. No issues found.
47-63:Leasemodel structure is well designed.
Allowing arbitrary types and custom serialization of conditions provides flexibility. Implementation looks correct.
66-89: Factory and dump methods forLeaseare consistent.
Parsing from protobuf, handling empty exporter fields, and providing JSON/YAML dumps appear well-structured with Pydantic.
94-94: No issues with newly introducednext_page_token.
Excluding it by default is a standard pattern for pagination tokens. Code is fine.
110-125:LeaseListmirrorsExporterListappropriately.
Parsing from protobuf and dumping to JSON/YAML are implemented consistently.
131-131: Namespace field inClientService.
Introducing a required namespace at initialization for building resource names is a reasonable design choice.
137-140:GetExporteruses the correct resource path.
The f-string approach to build the exporter name is straightforward and consistent.
154-154:ListExportersparent reference is correct.
No issues found with the usage ofnamespaces/{}
187-205:CreateLeaseimplementation follows established patterns.
TranslatingtimedeltatoDurationis correct. No issues found.
207-228:UpdateLeaseusage ofFieldMaskis appropriate.
This approach precisely updates thedurationfield. Looks good.
230-235:DeleteLeasemethod is straightforward.
Returning an empty response aligns with the proto definition.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py (1)
1-143: File is auto-generated by protoc.
Direct modifications to this file can be lost on regeneration. Looks fine as-is, aside from the linter warnings for unused imports.🧰 Tools
🪛 Ruff (0.8.2)
25-25: Module level import not at top of file
(E402)
25-25:
google.api.annotations_pb2imported but unusedRemove unused import:
google.api.annotations_pb2(F401)
26-26: Module level import not at top of file
(E402)
26-26:
google.api.client_pb2imported but unusedRemove unused import:
google.api.client_pb2(F401)
27-27: Module level import not at top of file
(E402)
27-27:
google.api.field_behavior_pb2imported but unusedRemove unused import:
google.api.field_behavior_pb2(F401)
28-28: Module level import not at top of file
(E402)
28-28:
google.api.resource_pb2imported but unusedRemove unused import:
google.api.resource_pb2(F401)
29-29: Module level import not at top of file
(E402)
29-29:
google.protobuf.duration_pb2imported but unusedRemove unused import:
google.protobuf.duration_pb2(F401)
30-30: Module level import not at top of file
(E402)
30-30:
google.protobuf.empty_pb2imported but unusedRemove unused import:
google.protobuf.empty_pb2(F401)
31-31: Module level import not at top of file
(E402)
31-31:
google.protobuf.field_mask_pb2imported but unusedRemove unused import:
google.protobuf.field_mask_pb2(F401)
32-32: Module level import not at top of file
(E402)
32-32:
google.protobuf.timestamp_pb2imported but unusedRemove unused import:
google.protobuf.timestamp_pb2(F401)
33-33: Module level import not at top of file
(E402)
33-33:
...v1.kubernetes_pb2imported but unusedRemove unused import:
...v1.kubernetes_pb2(F401)
36-36: Line too long (6048 > 120)
(E501)
43-43: Line too long (265 > 120)
(E501)
49-49: Line too long (159 > 120)
(E501)
65-65: Line too long (121 > 120)
(E501)
69-69: Line too long (142 > 120)
(E501)
71-71: Line too long (130 > 120)
(E501)
73-73: Line too long (136 > 120)
(E501)
81-81: Line too long (124 > 120)
(E501)
83-83: Line too long (130 > 120)
(E501)
91-91: Line too long (131 > 120)
(E501)
101-101: Line too long (127 > 120)
(E501)
103-103: Line too long (159 > 120)
(E501)
105-105: Line too long (163 > 120)
(E501)
107-107: Line too long (154 > 120)
(E501)
109-109: Line too long (158 > 120)
(E501)
111-111: Line too long (181 > 120)
(E501)
113-113: Line too long (182 > 120)
(E501)
115-115: Line too long (154 > 120)
(E501)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.py (1)
1-337: Generated gRPC stubs and servicer definition are valid.
All service methods follow the expected unary-unary pattern. Implementation placeholders raiseNotImplementedError, which is standard practice for future server-side logic.
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Chores