Custom driver descriptions #690
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
I guess it needs jumpstarter-dev/jumpstarter-protocol#28 first |
WalkthroughReplaces many Click decorators with server-aware Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as DriverClickGroup
participant Client as DriverClient
participant Server as Exporter
User->>CLI: request help / invoke command
CLI->>Client: read `description` for group help
CLI->>Client: read `methods_description[method]` for command help
Server-->>Client: GetReport returns (description, methods_description)
CLI-->>User: display help (server-provided or fallback)
User->>CLI: run command
CLI->>Client: execute command / call driver
Client->>Server: RPCs as needed
Server-->>Client: responses
sequenceDiagram
autonumber
participant ControllerClient
participant gRPCServer
participant ExporterClient
ControllerClient->>gRPCServer: ReportStatus(request)
gRPCServer-->>ControllerClient: ReportStatus(response)
ExporterClient->>gRPCServer: GetStatus(request)
gRPCServer-->>ExporterClient: GetStatus(response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-06T15:44:51.089ZApplied to files:
⏰ 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). (10)
🔇 Additional comments (4)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
37-52: Reintroduce parent-group injection when wiring child CLIsThe new unconditional
v.cli()call breaks clients whose CLI still requires the parent group. For example,StorageMuxClient.cli(self, base)(seepackages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py, Line 790) now raisesTypeError: missing 1 required positional argument: 'base'. Previously we detected that signature and passed the composite group in, so CLI registration succeeded. Please restore that behaviour so existing drivers continue to function.A possible fix:
@@ -import logging -from dataclasses import dataclass +import inspect +import logging +from dataclasses import dataclass @@ - for k, v in self.children.items(): - if hasattr(v, "cli"): - base.add_command(v.cli(), k) + for k, v in self.children.items(): + if not hasattr(v, "cli"): + continue + + cli_callable = v.cli + sig = inspect.signature(cli_callable) + if "base" in sig.parameters or "click_group" in sig.parameters: + command = cli_callable(base) + else: + command = cli_callable() + + base.add_command(command, k)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py(2 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(1 hunks)packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py(2 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(1 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(1 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py(1 hunks)packages/jumpstarter-driver-shell/README.md(5 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(2 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py(7 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(1 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py(1 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py(5 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py(2 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py(8 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.py(1 hunks)packages/jumpstarter/jumpstarter/client/base.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/driver/base.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/session.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (4)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
cli(42-56)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (2)
Shell(12-222)client(46-47)packages/jumpstarter/jumpstarter/common/utils.py (1)
serve(31-39)packages/jumpstarter/jumpstarter/client/base.py (1)
call(39-49)
packages/jumpstarter/jumpstarter/client/client.py (1)
packages/jumpstarter/jumpstarter/driver/base.py (2)
client(98-101)report(195-213)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
call(39-49)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (2)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
cli(36-53)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
cli(23-35)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (1)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)
cli(34-55)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (6)
base(417-418)base(552-554)cli(408-522)cli(550-590)cli(717-741)cli(791-793)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (2)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
cli(36-53)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)
cli(34-55)
🪛 GitHub Actions: documentation
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py
[error] 34-34: ImportError: cannot import name 'common_pb2' from 'jumpstarter_protocol.jumpstarter.v1'.
🪛 GitHub Actions: Run E2E Tests
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py
[error] 34-34: ImportError: cannot import name 'common_pb2' from 'jumpstarter_protocol.jumpstarter.v1'
🪛 GitHub Actions: Run Tests
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py
[error] 34-34: ImportError: cannot import name 'common_pb2' from 'jumpstarter_protocol.jumpstarter.v1'
🔇 Additional comments (4)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (1)
26-26: LGTM! Test updates correctly reflect the CLI refactor.All test invocations have been consistently updated to use
client.cli()directly and invoke commands without the "tmt" subcommand prefix. This accurately reflects the change from a group-based CLI to a standalone command pattern inclient.py.Also applies to: 30-30, 37-37, 48-48, 53-53, 61-61, 69-69, 80-80, 85-85, 96-96, 99-99, 430-430, 435-435
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)
34-55: Composite CLI integration confirmed
CompositeClient.cliusesbase.add_command(v.cli(), k), so havingTMTClient.cli()return a standalone command integrates correctly.packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
23-24: LGTM – verify packaging configuration and documentationThe switch from
@click.groupto@click.commandaligns SSH with the standalone-driver pattern but is a breaking change. Confirm:
- project.entry-points in
pyproject.tomlcorrectly registers thesshcommand (e.g. underjumpstarter.drivers)- README usage examples and any shell/completion scripts reflect the standalone
j sshinvocation- Any integration or end-to-end tests exercise the new command signature
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (1)
105-105: Confirmself.descriptionis initialized in the driver base class. The CLI group’s help usesself.description or "RideSX storage operations", so ensuredescriptionis set (e.g., inFlasherClient,CompositeClient, or a shared base) to avoid runtime errors.
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py
Show resolved
Hide resolved
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py
Show resolved
Hide resolved
michalskrivanek
force-pushed
the
custom-descriptions
branch
2 times, most recently
from
9c3f11b to
6aa10b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (1)
24-43: Consider more specific type hints for the dict format.The helper methods correctly handle both string and dict formats with appropriate fallbacks. However, the dict structure could be more precisely typed using a TypedDict for better type safety and IDE support.
Consider defining a TypedDict for the method configuration:
from typing import TypedDict class MethodConfig(TypedDict, total=False): command: str description: str timeout: intThen update the type hint:
-methods: dict[str, str | dict[str, str | int]] +methods: dict[str, str | MethodConfig]This would provide better type checking and autocomplete for the configuration format.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (2)
100-107: Test scope narrower than docstring suggests.The docstring states "Test that CLI methods can be executed" but the test only verifies that the 'echo' command exists in the CLI and has the correct name. It doesn't actually execute the command. Consider either:
- Updating the docstring to reflect the actual scope: "Test that CLI commands are properly registered"
- Or expanding the test to actually execute a CLI command
155-255: Consider adding tests for per-method timeout functionality.The new tests provide excellent coverage of the description and format handling. However, the driver supports per-method
timeoutconfiguration in the dict format (as seen in driver.py lines 11-221), but this feature is not tested.Consider adding a test like:
def test_per_method_timeout(): """Test that methods can have custom timeout values""" shell = Shell( timeout=10, # Global default methods={ "quick": { "command": "echo 'fast'", "timeout": 1 }, "slow": { "command": "sleep 5 && echo 'done'", "timeout": 10 }, } ) with serve(shell) as client: # Verify the timeout is used correctly # (implementation depends on how you want to verify timing) returncode = client.quick() assert returncode == 0This would ensure timeout configuration works correctly and prevent regressions in this critical shell execution feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(1 hunks)packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py(2 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(2 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(1 hunks)packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py(1 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(1 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py(1 hunks)packages/jumpstarter-driver-shell/README.md(5 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(2 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py(7 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py(2 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py(8 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.py(1 hunks)packages/jumpstarter/jumpstarter/client/base.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/driver/base.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/session.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/session_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py
- packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
- packages/jumpstarter/jumpstarter/driver/base.py
- packages/jumpstarter/jumpstarter/client/client.py
- packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
- packages/jumpstarter/jumpstarter/client/base.py
- packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T07:32:04.548Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#589
File: packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py:0-0
Timestamp: 2025-09-02T07:32:04.548Z
Learning: *_pb2.py files in the jumpstarter codebase are auto-generated from gRPC/protobuf definitions and should not be manually modified. Issues with these files should be addressed at the protobuf generation tooling level or runtime environment setup.
Applied to files:
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.py
🧬 Code graph analysis (4)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (7)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
base(46-47)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
base(754-755)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py (1)
base(24-25)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
base(39-40)packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py (1)
base(64-65)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (1)
base(40-41)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
base(45-46)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (4)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
cli(42-56)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (2)
Shell(12-222)client(46-47)packages/jumpstarter/jumpstarter/common/utils.py (1)
serve(31-39)packages/jumpstarter/jumpstarter/client/base.py (1)
call(39-49)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
call(39-49)
packages/jumpstarter/jumpstarter/exporter/session_test.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (3)
Driver(56-273)client(98-101)report(195-213)packages/jumpstarter/jumpstarter/exporter/session.py (2)
Session(31-142)GetReport(101-111)
⏰ 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). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
🔇 Additional comments (27)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.py (1)
27-27: LGTM – protobuf-generated updates.These changes reflect standard protobuf code generation (DESCRIPTOR handling moved from
_globalsto direct reference). Based on learnings,*_pb2.pyfiles are auto-generated and modifications should occur at the protobuf tooling level.Also applies to: 33-33
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.py (1)
27-27: LGTM – protobuf-generated updates.Consistent with other pb2 files in this PR, the DESCRIPTOR initialization and options handling have been updated by the protobuf compiler.
Also applies to: 33-33
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py (1)
30-30: LGTM – protobuf regeneration with common.proto dependency.The new
common_pb2import and expanded DESCRIPTOR reflect the addition of common.proto to the protobuf surface. Past review comments indicatingImportErrorfor this module have been marked as addressed.Also applies to: 33-33, 39-122
packages/jumpstarter/jumpstarter/exporter/session.py (1)
103-103: LGTM – formatting improvement.The additional blank line improves readability with no functional change.
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.py (1)
1-4: LGTM – gRPC-generated boilerplate.New file providing gRPC service stubs for common.proto. This is standard protobuf compiler output.
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.py (1)
1-38: LGTM – new protobuf module for common definitions.This file introduces the common proto surface (ExporterStatus and LogSource enums) that is now referenced throughout the protocol modules. Standard protobuf compiler output.
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py (1)
34-34: LGTM – protobuf regeneration with Exporter.status field.The addition of
common_pb2import and the newstatusfield on the Exporter message reflect the expanded protocol surface. PastImportErrorconcerns for this module have been resolved.Also applies to: 37-37, 43-145
packages/jumpstarter/jumpstarter/exporter/session_test.py (1)
1-141: LGTM – comprehensive test coverage for driver descriptions.The test suite thoroughly validates the new description feature across multiple scenarios:
- Presence/absence of descriptions in GetReport responses
- Client reception of descriptions
- CLI help text fallback behavior
- Multiple drivers with distinct descriptions
- Empty description handling
Well-structured tests that align with the PR's objective of propagating driver descriptions through reports and into clients/CLI.
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py (2)
1-1: Generated code follows correct patterns.This file is auto-generated from protocol buffer definitions. The new RPCs (
ReportStatusonControllerServiceandGetStatusonExporterService) are correctly wired following the established patterns for unary-unary calls. All client stubs, servicer implementations, handler registrations, and experimental static methods are consistent with existing code.
613-617: Verify proto definition and add documentation for GetStatus. Unable to locate the.protofile definingGetStatus; please confirm its location and add a descriptive RPC comment there.packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py (1)
63-65: LGTM! Consistent CLI help text enhancement.The addition of help text to the Click group decorator improves the user experience by providing descriptive help output. The pattern of using
self.description or "probe-rs client"allows for dynamic descriptions while providing a sensible default.packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (1)
39-41: LGTM! Consistent CLI help text enhancement.This change follows the same pattern as other drivers in the PR, improving CLI help output with dynamic descriptions.
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
20-22: LGTM! Consistent CLI help text enhancement.The addition of help text follows the established pattern across drivers in this PR.
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
37-47: LGTM! Consistent CLI help text enhancement.The help text addition is consistent with the pattern across other drivers in this PR.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (2)
416-418: LGTM! Consistent CLI help text enhancement.The help text addition follows the established pattern across drivers in this PR.
551-553: LGTM! Consistent CLI help text enhancement.This second instance in the file also follows the same pattern for the flasher interface.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (6)
15-19: LGTM! Well-documented dual-format support.The updated docstring clearly explains the two supported formats for method configuration. The type hint correctly reflects that methods can be either strings or dicts.
24-29: Note the default "echo Hello" command.When using the dict format without specifying a
commandkey, the method defaults to"echo Hello". While this is documented in the README, it might be unexpected behavior. Consider whether this is the desired default or if raising an error for missing commands would be clearer.Is the default
"echo Hello"command intentional for methods without an explicit command? This is mentioned in the README but could surprise users who provide only a description.
55-58: LGTM! Well-designed public API.The exported
get_method_descriptionmethod provides a clean public API for clients to retrieve method descriptions at runtime, enabling dynamic CLI help text generation.
70-76: LGTM! Clean integration of per-method configuration.The call_method correctly uses the helper methods to resolve command, timeout, and logs the timeout value for debugging.
153-186: LGTM! Correct timeout parameter handling.The timeout parameter is properly documented and handled with a clear fallback to the global timeout when None. The implementation correctly prioritizes per-method timeout over the global default.
189-204: LGTM! Proper timeout enforcement.The timeout check uses the resolved timeout value, and the graceful termination sequence (SIGTERM followed by SIGKILL if needed) is correctly implemented. The TimeoutExpired exception properly includes the resolved timeout value.
packages/jumpstarter-driver-shell/README.md (2)
14-84: LGTM! Comprehensive and clear documentation.The documentation thoroughly explains both configuration formats with excellent examples. The Configuration Parameters table is well-structured and covers all options including the per-method settings. The note about mixing formats is particularly helpful.
118-166: LGTM! Helpful CLI usage examples.The CLI usage section clearly demonstrates the help output differences between the two formats and includes a practical mixed-format example. This will help users understand the feature's capabilities.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (2)
44-46: LGTM! Consistent CLI help text enhancement.The help text addition follows the established pattern across drivers in this PR.
76-90: LGTM! Well-implemented dynamic CLI registration with backward compatibility.The dynamic command registration correctly:
- Fetches per-method descriptions at runtime with graceful fallback for older servers (try/except)
- Applies decorators in the correct order (argument → option → command)
- Passes the description as help text to the command decorator
- Maintains proper Click context settings for argument handling
The backward compatibility handling ensures this works with both old and new server versions.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (1)
109-153: LGTM! Clean test structure for CLI validation.Both
test_cli_includes_all_methodsandtest_cli_exit_codesare well-implemented:
- Proper use of the
serve()context manager- Clear assertions with helpful error messages
- Good coverage of CLI command registration and return code propagation
michalskrivanek
force-pushed
the
custom-descriptions
branch
from
6aa10b4 to
c447e70
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
76-90: Prefer specific exception handling for better diagnostics.The broad
Exceptioncatch at lines 77-80 could silently hide unexpected errors beyond version compatibility issues. Consider:
- Catching specific exceptions (e.g.,
AttributeError,NotImplementedError) that indicate missing method support- Adding a debug log when falling back to default description
- Distinguishing between version compatibility and other potential errors
Apply this diff to improve error handling:
- # Try to get custom description, fall back to default for older than 0.7 servers - try: - description = self.call("get_method_description", method_name) - except Exception: - description = f"Execute the {method_name} shell method" + # Try to get custom description, fall back to default for servers without this capability + try: + description = self.call("get_method_description", method_name) + except (AttributeError, NotImplementedError): + # Server doesn't support per-method descriptions (e.g., < 0.7) + description = f"Execute the {method_name} shell method" + except Exception as e: + # Log unexpected errors but continue with fallback + self.logger.debug(f"Failed to get description for {method_name}: {e}") + description = f"Execute the {method_name} shell method"packages/jumpstarter/jumpstarter/driver/base.py (1)
95-107: Consider more specific exception handling.The broad
except Exceptioncould hide legitimate errors during development. While the current defensive approach handles edge cases (properties, descriptors), you might consider catching specific exceptions likeAttributeErroror at minimum logging ignored exceptions at debug level.Example refinement:
def _collect_methods_description(self): """Collect help texts from @export and @exportstream decorated methods""" for name in dir(self): if name.startswith('_'): continue try: method = getattr(self, name) if callable(method) and hasattr(method, MARKER_HELP): help_text = getattr(method, MARKER_HELP) self.methods_description[name] = help_text - except Exception: - continue + except (AttributeError, TypeError) as e: + self.logger.debug(f"Skipping {name}: {e}") + continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(1 hunks)packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py(2 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(2 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(1 hunks)packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py(1 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(1 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py(1 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(2 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py(2 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py(8 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.py(1 hunks)packages/jumpstarter/jumpstarter/client/base.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/driver/base.py(4 hunks)packages/jumpstarter/jumpstarter/driver/decorators.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/session.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/session_test.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter/jumpstarter/exporter/session.py
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py
- packages/jumpstarter/jumpstarter/client/base.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.py
- packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py
- packages/jumpstarter/jumpstarter/exporter/session_test.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.py
- packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
- packages/jumpstarter/jumpstarter/client/client.py
🧰 Additional context used
🧬 Code graph analysis (3)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (7)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
base(46-47)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
base(754-755)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py (1)
base(24-25)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
base(39-40)packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py (1)
base(64-65)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (1)
base(40-41)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
base(45-46)
packages/jumpstarter/jumpstarter/driver/base.py (1)
packages/jumpstarter/jumpstarter/common/metadata.py (1)
name(13-14)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
call(42-52)
⏰ 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). (1)
- GitHub Check: e2e
🔇 Additional comments (16)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py (2)
29-33: LGTM! ReportStatus RPC correctly generated.The new
ReportStatusRPC is properly wired across all service components (stub, servicer, registration, and experimental API). The documentation comment clearly describes its purpose for exporters to report status to the controller.Also applies to: 97-103, 179-183, 290-315
570-574: Add documentation comment in the proto file.The new
GetStatusRPC is mechanically correct and properly wired. However, Line 614 indicates that the documentation comment is missing in the source.protofile.Please add a documentation comment for the
GetStatusRPC in the corresponding.protofile (likely in the related PR jumpstarter-dev/jumpstarter-protocol#28) so future regeneration includes proper documentation.Also applies to: 613-617, 647-651, 800-825
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (1)
105-105: LGTM! CLI help text enhancement.The addition of dynamic help text to the storage CLI group improves user experience by exposing driver descriptions.
packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py (1)
37-37: LGTM! CLI help text enhancements for both GPIO clients.The additions of dynamic help text to both DigitalOutputClient and DigitalInputClient CLI groups improve user experience with clear, context-specific descriptions.
Also applies to: 81-81
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
20-20: LGTM! CLI help text enhancement.The addition of dynamic help text improves the SNMP CLI group's discoverability.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
753-753: LGTM! CLI help text enhancement.The addition of dynamic help text to the flasher CLI group aligns with the broader pattern of exposing driver descriptions.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (2)
416-418: LGTM! CLI help text enhancement for OpendalClient.The addition of dynamic help text and removal of the inline docstring in favor of decorator-provided help improves consistency.
551-551: LGTM! CLI help text enhancement for FlasherClientInterface.The addition of dynamic help text aligns with the broader pattern of exposing driver descriptions across flasher interfaces.
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
37-37: LGTM! CLI help text enhancement for composite device.The addition of dynamic help text improves the composite CLI group's discoverability while maintaining the existing child CLI wiring.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
44-44: LGTM! CLI help text enhancement.The addition of dynamic help text to the shell CLI group aligns with the broader pattern across drivers.
packages/jumpstarter/jumpstarter/driver/base.py (3)
76-80: LGTM!The new fields are well-defined with clear documentation. The
init=Falseonmethods_descriptionis appropriate since it's populated automatically.
92-93: LGTM!The placement and sequencing of the method collection call is appropriate.
233-234: LGTM!The fields are correctly passed to the report. The defensive
orpatterns are harmless.packages/jumpstarter/jumpstarter/driver/decorators.py (3)
8-8: LGTM!The new marker constant follows the established pattern and is correctly typed.
11-23: LGTM!The decorator correctly implements the factory pattern to support both
@exportand@export(help="...")usage. The help text is appropriately stored only when provided, and existing marker logic is preserved.
26-33: LGTM!The
exportstreamdecorator follows the same pattern asexport, maintaining consistency across the decorator API.
michalskrivanek
force-pushed
the
custom-descriptions
branch
from
c447e70 to
44b824b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter/jumpstarter/driver/decorators.py (1)
86-86: Consider modernizing the super() call.The old-style
super(DriverClickGroup, self)syntax can be simplified tosuper()in Python 3.Apply this diff:
- return super(DriverClickGroup, self).command(*args, **kwargs)(f) + return super().command(*args, **kwargs)(f)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter/jumpstarter/client/base.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/driver/base.py(4 hunks)packages/jumpstarter/jumpstarter/driver/decorators.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter/jumpstarter/driver/base.py
- packages/jumpstarter/jumpstarter/client/client.py
⏰ 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). (1)
- GitHub Check: e2e
🔇 Additional comments (4)
packages/jumpstarter/jumpstarter/client/base.py (1)
36-40: LGTM! Clean metadata additions.The new fields are properly typed, use correct dataclass patterns for mutable defaults, and have clear docstrings explaining their purpose and usage.
packages/jumpstarter/jumpstarter/driver/decorators.py (3)
2-10: LGTM! Import additions support new functionality.The new imports (Any, Callable, Final, click) and MARKER_HELP constant follow existing patterns and are necessary for the help text feature.
13-25: LGTM! Correct decorator pattern for optional arguments.The implementation properly supports both
@exportand@export(help="...")usage patterns. The logic correctly identifies function types and attaches help metadata only when provided.
28-35: LGTM! Consistent with export decorator pattern.The exportstream decorator correctly implements the same optional-argument pattern as export, maintaining consistency across the codebase.
michalskrivanek
force-pushed
the
custom-descriptions
branch
from
44b824b to
a39d7a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
29-42: Address type safety concern from previous review.The
clientparameter is typed asAny, but the implementation assumes it hasdescription(line 39) andmethods_description(line 76) attributes. A previous review comment suggested adding type safety via a Protocol or runtime validation to prevent AttributeError at runtime.
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/driver/base.py (2)
95-107: Consider more specific exception handling.The method correctly collects help text from decorated methods. However, the broad
except Exceptionat line 106 might hide unexpected errors during introspection.Consider catching only expected exceptions:
try: method = getattr(self, name) if callable(method) and hasattr(method, MARKER_HELP): help_text = getattr(method, MARKER_HELP) self.methods_description[name] = help_text - except Exception: + except (AttributeError, TypeError): continueThis makes it clearer what errors are expected during introspection while still being resilient.
233-234: Remove redundant fallback for methods_description.Line 234 uses
or {}butmethods_descriptionis already adictwithdefault_factory=dict(line 79), so it will never beNoneor falsy unless explicitly cleared.Apply this diff:
description=self.description or None, - methods_description=self.methods_description or {}, + methods_description=self.methods_description,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter/jumpstarter/client/base.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/client/decorators.py(1 hunks)packages/jumpstarter/jumpstarter/driver/base.py(4 hunks)packages/jumpstarter/jumpstarter/driver/decorators.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter/jumpstarter/client/client.py
🧰 Additional context used
🧬 Code graph analysis (3)
packages/jumpstarter/jumpstarter/driver/decorators.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
decorator(68-81)
packages/jumpstarter/jumpstarter/driver/base.py (1)
packages/jumpstarter/jumpstarter/common/metadata.py (1)
name(13-14)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
packages/jumpstarter/jumpstarter/driver/decorators.py (2)
decorator(17-26)decorator(32-36)
⏰ 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). (4)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (10)
packages/jumpstarter/jumpstarter/client/decorators.py (3)
1-9: LGTM!The module docstring and imports are appropriate for a Click-based CLI helper.
44-57: LGTM!The
add_optionmethod provides a clean fluent interface for adding Click options to the group. The implementation correctly appends options toself.paramsand returnsselffor chaining.
59-82: Verify help text resolution behavior.The help text resolution logic is well-structured with clear priority ordering. However, line 76 accesses
self.client.methods_descriptionwithout checking if the attribute exists, which ties into the type safety concern raised earlier.Additionally, verify that the empty string fallback (line 79) provides the desired user experience in the CLI when no help text is available.
packages/jumpstarter/jumpstarter/driver/base.py (3)
24-30: LGTM!The addition of
MARKER_HELPto imports is necessary for the_collect_methods_descriptionmethod.
76-80: LGTM!The new
descriptionandmethods_descriptionfields are correctly typed and documented. Usinginit=Falseformethods_descriptionis appropriate since it's populated during__post_init__.
92-93: LGTM!Calling
_collect_methods_description()in__post_init__is the correct place to populate method-level metadata after initialization.packages/jumpstarter/jumpstarter/driver/decorators.py (3)
1-12: LGTM!The updated imports and new
MARKER_HELPconstant follow the existing pattern and support the new help text functionality.
15-27: LGTM!The
exportdecorator has been correctly converted to a decorator factory pattern, supporting both@exportand@export(help="...")usage. The implementation properly setsMARKER_HELPwhen help text is provided.
30-37: LGTM!The
exportstreamdecorator follows the same correct decorator factory pattern asexport, maintaining consistency across the codebase.packages/jumpstarter/jumpstarter/client/base.py (1)
36-40: Client initialization populates new attributes
descriptionandmethods_descriptionare assigned fromreportviagetattr(...), confirming they’re populated as intended.
michalskrivanek
force-pushed
the
custom-descriptions
branch
from
a39d7a0 to
e06ca8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/driver/base.py (1)
95-107: Overly broad exception handling masks errors.The bare
except Exception:at line 106 silently suppresses all exceptions during method help collection. While this prevents collection failures from breaking driver initialization, it also hides legitimate errors such as AttributeError from broken properties or unexpected runtime issues.Consider logging suppressed exceptions or narrowing the exception types:
def _collect_methods_description(self): """Collect help texts from @export and @exportstream decorated methods""" for name in dir(self): if name.startswith('_'): continue try: method = getattr(self, name) if callable(method) and hasattr(method, MARKER_HELP): help_text = getattr(method, MARKER_HELP) self.methods_description[name] = help_text - except Exception: - continue + except (AttributeError, TypeError) as e: + self.logger.debug(f"Skipping {name} during help collection: {e}") + continuepackages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
732-732: Consider using consistent decorator style.Lines 717, 722, and 727 use
@base.command()(with parentheses), but this line uses@base.command(without parentheses). While both forms are functionally equivalent, using a consistent style throughout the method improves readability.Consider this change for consistency:
- @base.command + @base.command() @click.argument("file") def write_local_file(file):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py(2 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(2 hunks)packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py(3 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(4 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(2 hunks)packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py(2 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(2 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py(2 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(3 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(2 hunks)packages/jumpstarter/jumpstarter/client/base.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/client/decorators.py(1 hunks)packages/jumpstarter/jumpstarter/driver/base.py(4 hunks)packages/jumpstarter/jumpstarter/driver/decorators.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/session_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/jumpstarter/jumpstarter/client/base.py
- packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py
- packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py
- packages/jumpstarter/jumpstarter/exporter/session_test.py
- packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
- packages/jumpstarter/jumpstarter/client/decorators.py
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py
- packages/jumpstarter/jumpstarter/client/client.py
🧰 Additional context used
🧬 Code graph analysis (7)
packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (2)
client(79-80)client(228-229)packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(10-83)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(10-83)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(10-83)
packages/jumpstarter/jumpstarter/driver/decorators.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
decorator(68-81)
packages/jumpstarter/jumpstarter/driver/base.py (1)
packages/jumpstarter/jumpstarter/common/metadata.py (1)
name(13-14)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(10-83)
packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py (2)
packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/driver.py (2)
client(100-101)client(163-164)packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(10-83)
⏰ 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). (5)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (18)
packages/jumpstarter/jumpstarter/driver/decorators.py (3)
12-12: LGTM! Clean constant addition.The new
MARKER_HELPconstant follows the existing naming convention and serves as a clear marker for attaching help text to exported methods.
15-27: Well-implemented decorator factory pattern.The
exportdecorator now supports both direct usage (@export) and parameterized usage (@export(help="...")). The implementation correctly handles both cases and conditionally attaches theMARKER_HELPattribute only when help text is provided.
30-37: Consistent implementation with export decorator.The
exportstreamdecorator follows the same decorator factory pattern asexport, maintaining consistency across the codebase.packages/jumpstarter/jumpstarter/driver/base.py (4)
26-26: LGTM! Import aligns with usage.The
MARKER_HELPimport is used correctly in the_collect_methods_descriptionmethod below.
76-80: LGTM! Clean field additions.The new
descriptionandmethods_descriptionfields are well-documented and appropriately typed. Usinginit=Falseformethods_descriptionis correct since it's populated during__post_init__.
92-93: LGTM! Appropriate placement in post_init.Collecting method descriptions during initialization ensures the
methods_descriptiondictionary is populated before the driver is used, which aligns with its usage in thereportmethod.
233-234: LGTM! Defensive defaults for optional fields.Using
or Nonefordescriptionandor {}formethods_descriptionensures sensible defaults are included in the report even when these fields are not populated.packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (2)
10-11: LGTM: Import correctly added.The import of
DriverClickGroupis properly placed and follows the project's import conventions.
106-106: Verifydescriptionandmethods_descriptionon the client at runtime
- Confirm that the merged
jumpstarter-protocolPR #28 adds these attributes to the client class.- Bump the protocol dependency version in your manifest and reinstall dependencies to prevent
AttributeErrorinDriverClickGroup.packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py (1)
8-8: No action needed—methods_descriptionis always a dict.
DriverClientdefinesmethods_descriptionwith adefault_factory=dictand usesgetattr(..., {}), soDriverClickGroupwon’t error.packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py (2)
15-15: LGTM!Import addition is correct and necessary for the
DriverClickGroupusage below.
24-24: Verify that command docstrings are intentionally no longer used as help text.The refactor to
DriverClickGroupchanges the CLI help text behavior. By design,DriverClickGroupprioritizes server-providedmethods_description, then explicithelp=parameters, then defaults to empty string—docstrings are not used as a fallback. This means the existing command docstrings (e.g., lines 30-34 forforward_tcp, 54-59 forforward_unix, 73-75 foraddress) will no longer appear in CLI help unless the server providesmethods_descriptionfor these commands.Given the PR comment indicating this depends on jumpstarter-protocol PR #28, ensure that PR is merged first so server-provided descriptions are available when this change lands, otherwise users will see empty help text for commands.
Based on the PR comment about the protocol dependency, verify that the jumpstarter-protocol PR #28 is merged before this PR to ensure server descriptions are available.
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (2)
9-9: LGTM!Import addition is correct and necessary for the
DriverClickGroupusage below.
40-40: Verify that command docstrings are intentionally no longer used as help text.Similar to the network driver, this refactor changes CLI help text behavior. The
start_consolecommand's docstring (line 44) will no longer appear in CLI help unless the server providesmethods_description['start_console']. Ensure the protocol changes are in place so server-provided descriptions are available.Confirm that the jumpstarter-protocol PR #28 (mentioned in the PR comments) is merged first to ensure server descriptions are available when this lands.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (4)
22-22: LGTM!The import of
DriverClickGroupis correct and necessary for enabling server-provided CLI descriptions.
417-417: LGTM!The migration to
DriverClickGroupis correct. This enables the CLI to use server-provided descriptions while providing "Opendal Storage" as a fallback help text.
550-550: LGTM!The migration to
DriverClickGroupis correct. This enables server-provided descriptions with "Generic flasher interface" as the fallback.
715-715: LGTM!The migration to
DriverClickGroupis correct. This enables server-provided descriptions with "Storage operations" as the fallback.
michalskrivanek
force-pushed
the
custom-descriptions
branch
from
e06ca8e to
b73a97c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter/jumpstarter/exporter/session_test.py (1)
143-151: Clarify test name to match implementation.The test name
test_description_override_in_exporter_configsuggests it tests overriding a default description via exporter config. However, it only tests that a driver instantiated with a description works correctly. To truly test config override, you would need to:
- Show a driver with a default description
- Override it via exporter config
- Verify the override took effect
Consider either:
- Renaming the test to
test_description_passed_via_driver_instantiation- Or updating the test to actually demonstrate config override:
def test_description_override_in_exporter_config(): - """Test that description in exporter config overrides default""" - # Create a driver with a custom description - custom_driver = SimpleDriver(description="Custom override description") + """Test that description in exporter config overrides driver default""" + from jumpstarter.config.exporter import ExporterConfigV1Alpha1DriverInstance + + # Create config that overrides the description + config = ExporterConfigV1Alpha1DriverInstance.from_str(""" + type: jumpstarter.exporter.session_test.SimpleDriver + description: "Config override description" + """) + + custom_driver = config.instantiate() with serve(custom_driver) as client: - # Client should receive the custom description - assert client.description == "Custom override description" + # Client should receive the config-overridden description + assert client.description == "Config override description"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py(2 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(2 hunks)packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py(3 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(4 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(2 hunks)packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py(2 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(2 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py(2 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(3 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(2 hunks)packages/jumpstarter/jumpstarter/client/base.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/client/decorators.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(2 hunks)packages/jumpstarter/jumpstarter/driver/base.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/session_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py
- packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
- packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py
- packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py
- packages/jumpstarter/jumpstarter/client/base.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py
- packages/jumpstarter/jumpstarter/client/client.py
- packages/jumpstarter/jumpstarter/client/decorators.py
🧰 Additional context used
🧬 Code graph analysis (5)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (2)
DriverClickGroup(10-83)add_option(44-57)
packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(10-83)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(10-83)
packages/jumpstarter/jumpstarter/exporter/session_test.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (3)
Driver(56-277)client(101-104)report(198-217)packages/jumpstarter/jumpstarter/exporter/session.py (1)
Session(31-141)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(10-83)
⏰ 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). (5)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
🔇 Additional comments (23)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (4)
22-22: LGTM!Import added to support the new DriverClickGroup usage throughout the file.
417-417: LGTM!The programmatic group creation using DriverClickGroup is correct. The implementation will use server-provided descriptions when available and fall back to the specified help text.
550-550: LGTM!The DriverClickGroup instantiation follows the same pattern. Since concrete implementations extend DriverClient, the necessary attributes for server-provided descriptions will be available at runtime.
715-715: LGTM! Verify protocol dependency before merging.The DriverClickGroup usage is consistent with the other changes. The optional
baseparameter is correctly handled for command composition scenarios.As noted in the PR comments, ensure that jumpstarter-dev/jumpstarter-protocol#28 is merged first. The graceful fallback design makes this change backward compatible, but the protocol changes should be in place for the full functionality to work as intended.
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (4)
9-9: LGTM! Import addition is correct.The import of
DriverClickGroupaligns with the migration pattern used across other drivers in this PR.
40-40: LGTM! DriverClickGroup initialization is correct.The help text "Serial port client" will be used as a fallback if
self.descriptionis not available on the client instance. This follows the documented behavior of DriverClickGroup where the priority is: 1)client.descriptionfrom server, 2)help=parameter.
42-47: Verify help text for thestart_consolecommand.The function's docstring will not be used as help text by
DriverClickGroup.command(). According to the help text priority order (1.methods_descriptionfrom server, 2.help=parameter, 3. empty string), the command will have empty help text unless the server provides it inmethods_description['start_console'].If the server does not provide the description, consider explicitly adding the help text:
- @base.command() + @base.command(help="Start serial port console") def start_console(): - """Start serial port console"""Based on the DriverClickGroup implementation in
packages/jumpstarter/jumpstarter/client/decorators.py.
49-49: LGTM! Returning the group object is correct.This allows the CLI to be properly exposed and follows the pattern used across other drivers in this PR.
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (4)
8-8: LGTM! Import addition is correct.The import of
DriverClickGroupfollows the consistent migration pattern across all drivers in this PR.
38-45: LGTM! DriverClickGroup usage and option configuration are correct.The migration properly:
- Instantiates
DriverClickGroupwith fallback help text "Generic composite device"- Uses the chainable
add_optionmethod to attach the--log-leveloption- Preserves all existing option parameters (type, help, expose_value, callback)
The help text will prioritize
self.descriptionfrom the server if available, otherwise fall back to the provided help text.
47-49: LGTM! Child command attachment is correct.The iteration over children and dynamic command attachment preserves the existing behavior while working correctly with the new
DriverClickGroupstructure.
51-51: LGTM! Returning the group object is correct.This allows the CLI to be properly composed and exposed, following the pattern established across other drivers in this PR.
packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py (2)
9-9: LGTM!The import is correct and necessary for the DriverClickGroup usage below.
64-64: LGTM: DriverClickGroup usage is correct and protocol dependency is resolved
The refactor aligns with our established pattern—server‐provided descriptions will be wired up with a fallback tohelp="probe-rs client". The referenced protocol PR #28 has been merged.packages/jumpstarter/jumpstarter/config/exporter.py (1)
31-32: LGTM! Fields properly defined.The new optional fields
descriptionandmethods_descriptionare well-typed with appropriate defaults, maintaining backward compatibility.packages/jumpstarter/jumpstarter/driver/base.py (2)
75-79: LGTM! Well-documented fields.The new fields are properly typed with clear docstrings explaining their purpose. The use of
field(default_factory=dict)formethods_descriptioncorrectly avoids mutable default argument issues.
215-216: Proto fields present and normalization consistent. jumpstarter-protocol PR #28 addsoptional string description = 4andmap<string, string> methods_description = 5toDriverInstanceReport; the normalization inbase.pyaligns with these definitions and test expectations.packages/jumpstarter/jumpstarter/exporter/session_test.py (6)
9-23: LGTM! Minimal test doubles.The
SimpleDriverandCompositeDriver_test doubles provide minimal implementations sufficient for testing the description functionality without unnecessary complexity.
25-68: LGTM! Thorough test coverage.The test correctly verifies that:
- Drivers with descriptions have them in GetReport
- Drivers without descriptions have the field unset or empty
- The report structure includes both drivers
The use of
reports_by_uuidfor lookup is clear and efficient.
119-141: LGTM! Edge case handling verified.The test correctly verifies that empty strings are normalized and not included in the report, which aligns with the
self.description or Nonelogic in theDriver.report()method.
203-220: LGTM! Config-based method descriptions tested.The test verifies that
methods_descriptioncan be set via server configuration and is properly stored on the driver instance.
222-254: LGTM! GetReport integration verified.The test confirms that
methods_descriptionis properly included in the GetReport response and accessible via the proto message.
277-312: LGTM! Override priority tested.The test simulates the DriverClickGroup behavior and verifies the priority: server methods_description > client help parameter > empty string. This documents the expected behavior clearly.
michalskrivanek
force-pushed
the
custom-descriptions
branch
from
b73a97c to
56190f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/jumpstarter/jumpstarter/config/exporter.py (1)
51-56: Past review concern about backward compatibility still applies.As flagged in the previous review, passing
**self.root.configalong with the new description fields can raiseTypeErrorif the config contains keys that the dataclass__init__doesn't recognize. The previous review comment provides detailed mitigation strategies (filtering config keys or updating Driver.init to accept **kwargs).
🧹 Nitpick comments (3)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
75-79: Narrow the exception handling scope.The broad
except Exception:on Line 78 catches all exceptions, which could hide programming errors or connectivity issues unrelated to version compatibility. Consider catching more specific exceptions (e.g.,grpc.RpcError,AttributeError).Apply this diff to narrow the exception scope:
- try: - description = self.call("get_method_description", method_name) - except Exception: - description = f"Execute the {method_name} shell method" + try: + description = self.call("get_method_description", method_name) + except (grpc.RpcError, AttributeError): + description = f"Execute the {method_name} shell method"packages/jumpstarter/jumpstarter/exporter/session_test.py (2)
51-52: Consider using pytest-asyncio fixtures instead of asyncio.run().The tests directly call
asyncio.run(session.GetReport(...))on Lines 52, 133, and 242. Consider using pytest-asyncio fixtures (@pytest.mark.asyncioandasync def test_...) for more idiomatic async test patterns and better integration with pytest.Example refactor for this test:
+import pytest + +@pytest.mark.asyncio -def test_get_report_includes_descriptions(): +async def test_get_report_includes_descriptions(): """Test that GetReport includes descriptions for drivers that have them""" # ... setup code ... - import asyncio - response = asyncio.run(session.GetReport(empty_pb2.Empty(), None)) + response = await session.GetReport(empty_pb2.Empty(), None)
294-297: Clarify or remove the always-false condition.Line 294 checks
elif "help" in {}:, which will always evaluate toFalsesince it's checking an empty dict literal. This appears to be simulating the absence of a client-provided help parameter. Consider either removing this branch or adding a comment explaining its purpose.Apply this diff to clarify the intent:
# Method with server override method_name = "on" if method_name in client.methods_description: help_text = client.methods_description[method_name] - elif "help" in {}: # Simulate client's help= parameter - help_text = {}["help"] + # (no client help= parameter provided) else: help_text = ""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py(2 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(2 hunks)packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py(3 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(4 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(2 hunks)packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py(2 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(2 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py(2 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(3 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(2 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(2 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py(2 hunks)packages/jumpstarter/jumpstarter/client/base.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/client/decorators.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(2 hunks)packages/jumpstarter/jumpstarter/driver/base.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/session_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/jumpstarter/jumpstarter/client/base.py
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py
- packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py
- packages/jumpstarter/jumpstarter/client/client.py
- packages/jumpstarter/jumpstarter/driver/base.py
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
- packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
🧰 Additional context used
🧬 Code graph analysis (10)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (2)
packages/jumpstarter/jumpstarter/client/decorators.py (2)
DriverClickGroup(35-110)command(84-110)packages/jumpstarter/jumpstarter/client/base.py (1)
call(42-52)
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(35-110)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (2)
DriverClickGroup(35-110)add_option(69-82)
packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py (2)
packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/driver.py (2)
client(100-101)client(163-164)packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(35-110)
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (2)
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py (1)
client(117-118)packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(35-110)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(35-36)packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_command(10-32)
packages/jumpstarter/jumpstarter/exporter/session_test.py (3)
packages/jumpstarter/jumpstarter/driver/base.py (3)
Driver(56-277)client(101-104)report(198-217)packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
session(101-121)packages/jumpstarter/jumpstarter/exporter/session.py (1)
Session(31-141)
packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (2)
client(79-80)client(228-229)packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(35-110)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
DriverClickGroup(35-110)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (2)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver.py (1)
client(24-25)packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_command(10-32)
⏰ 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: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (13)
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
8-8: Clean refactoring to DriverClickGroup pattern.The migration from decorator-based Click group to programmatic
DriverClickGroupinstantiation is well-executed. This enables server-provided driver descriptions while maintaining backward compatibility through the help text fallback.Also applies to: 39-39
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)
10-10: Correct migration to driver_click_command decorator.The single-command driver pattern is correctly implemented using
driver_click_command, preserving the context settings while enabling server-provided descriptions.Also applies to: 37-41
packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py (1)
15-15: Consistent DriverClickGroup refactoring.The migration follows the established pattern correctly, maintaining the same help text and command structure while enabling future server-provided descriptions.
Also applies to: 24-24
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
13-13: Proper single-command driver migration.The decorator change correctly implements the pattern for single-command drivers, maintaining context settings and enabling extensibility for server-provided descriptions.
Also applies to: 25-29
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (1)
9-9: Well-executed refactoring for serial driver CLI.The migration to
DriverClickGroupis clean and maintains the existing command structure. The addition of an explicit return statement improves the API.Also applies to: 40-49
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
8-8: Excellent use of DriverClickGroup with options.The refactoring demonstrates proper usage of the
add_optionmethod for programmatically adding options to the group. The composite pattern of adding child CLIs is preserved correctly.Also applies to: 38-49
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
5-6: Correct inheritance pattern with DriverClickGroup.The SNMP client correctly reuses the parent
PowerClientCLI commands while wrapping them in its ownDriverClickGroup. The pattern is clean and maintains compatibility.Also applies to: 21-26
packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py (1)
8-8: Consistent refactoring across both GPIO client classes.Both
DigitalOutputClientandDigitalInputClientare correctly migrated to useDriverClickGroup, with appropriate help texts distinguishing their purposes. The inheritance pattern for reusing parent commands is properly maintained.Also applies to: 38-58, 80-106
packages/jumpstarter/jumpstarter/config/exporter.py (1)
31-32: LGTM! New fields align with Driver base class.The addition of
descriptionandmethods_descriptionfields properly matches the Driver base class definition shown in the relevant code snippets.packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (2)
45-45: LGTM! Proper use of DriverClickGroup.The switch from static
@click.groupdecorator to dynamicDriverClickGroupenables server-provided descriptions to populate CLI help text.
75-79: get_method_description implementation verified
Found in packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py and decorated with @export.packages/jumpstarter/jumpstarter/exporter/session_test.py (1)
1-312: Excellent test coverage for description features.The test suite comprehensively covers:
- GetReport inclusion of descriptions and methods_description
- Client initialization from server data
- CLI help text fallback behavior
- Composite structure handling
- Empty description handling
- Config override precedence
packages/jumpstarter/jumpstarter/client/decorators.py (1)
1-110: Well-designed CLI helper utilities.The module provides clean abstractions for building driver CLIs with server-provided descriptions:
driver_click_command: Simple decorator for single-command drivers with automatic description fallbackDriverClickGroup: Full-featured group with method-level description support and chainable option API- Clear priority order: server descriptions → client help= → empty string
- Good documentation with usage examples
michalskrivanek
force-pushed
the
custom-descriptions
branch
from
56190f8 to
0ad1f51
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/jumpstarter/jumpstarter/client/decorators.py (2)
36-42: Document the description override priority.The logic extracts the docstring if
helpis not provided (lines 36-38), but then unconditionally overrides it withclient.descriptionif present (lines 41-42). This means server-provided descriptions always win, even if a developer explicitly passeshelp=or provides a docstring. While this may be intentional, consider documenting this priority explicitly in the docstring or allowing opt-out.Consider adding a note to the docstring:
:param client: DriverClient instance (provides description and methods_description) :param kwargs: Keyword arguments passed to DriverClickGroup :return: Decorator that creates a DriverClickGroup + + Note: Server-provided description (client.description) takes precedence over + both explicit help= kwargs and function docstrings. """
69-70: Consider same override clarity for single-command decorator.Same as in
driver_click_group, server description unconditionally overrides any explicitly passedhelp=kwarg. This behavior should be consistent and documented.Add a similar note to the docstring here as well.
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
38-49: Inconsistent usage pattern compared to other drivers.Unlike other driver clients that use
@driver_click_group(self)as a decorator on a function, this code creates the group programmatically and chainsadd_option. While functionally correct, this inconsistency may confuse future maintainers.Consider aligning with the decorator pattern used in other files:
def cli(self): @driver_click_group(self) def base(): """Generic composite device""" pass - base.add_option( + base.add_option( "--log-level", type=click.Choice(["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"]), help="Set the log level", expose_value=False, callback=_opt_log_level_callback, )However, the current approach is valid if you need the chainable
add_optionbefore adding commands.packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (2)
78-92: Avoid per-method RPC for descriptions; rely on DriverClickGroup overrideDriverClickGroup.command already overrides help from client.methods_description. The extra get_method_description per method adds N+1 RPCs and risks inconsistencies. Use a local fallback help and let the group override when available.
Also, explicit name=method_name bypasses Click’s underscore-to-hyphen normalization. If hyphenated commands are desired (as elsewhere), normalize here.
Apply:
- # Try to get custom description, fall back to default for older than 0.7 servers - try: - description = self.call("get_method_description", method_name) - except Exception: - description = f"Execute the {method_name} shell method" + # Fallback description; DriverClickGroup.command will override via methods_description when provided by server + default_help = f"Execute the {method_name} shell method" @@ - method_command = group.command( - name=method_name, - help=description, + method_command = group.command( + name=method_name, # consider: method_name.replace('_', '-') + help=default_help, context_settings={"ignore_unknown_options": True, "allow_interspersed_args": False}, )(method_command)This keeps compatibility with older servers without extra RPCs and defers overrides to the shared mechanism. See jumpstarter/client/decorators.py:command for the override behavior. [Based on relevant code snippets]
Please confirm whether command names should be hyphenated; if yes, replace name=method_name with name=method_name.replace('_', '-').
62-71: Parse --env via Click callback to simplify handler and validate earlyMove KEY=VALUE parsing into an option callback. This reduces in-function branching and provides earlier validation.
def _add_method_command(self, group, method_name): """Add a Click command for a specific shell method""" - def method_command(args, env): - # Parse environment variables - env_dict = {} - for env_var in env: - if '=' in env_var: - key, value = env_var.split('=', 1) - env_dict[key] = value - else: - raise click.BadParameter(f"Invalid --env value '{env_var}'. Use KEY=VALUE.") + def _env_callback(ctx, param, values): + env_dict = {} + for env_var in values: + if '=' in env_var: + key, value = env_var.split('=', 1) + env_dict[key] = value + else: + raise click.BadParameter(f"Invalid --env value '{env_var}'. Use KEY=VALUE.") + return env_dict + + def method_command(args, env): - returncode = getattr(self, method_name)(*args, **env_dict) + returncode = getattr(self, method_name)(*args, **env) @@ - method_command = click.option('--env', '-e', multiple=True, - help='Environment variables in KEY=VALUE format')(method_command) + method_command = click.option( + '--env', '-e', multiple=True, callback=_env_callback, + help='Environment variables in KEY=VALUE format' + )(method_command)Also applies to: 85-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py(2 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(2 hunks)packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py(3 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(4 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(2 hunks)packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py(2 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(2 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py(2 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(3 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(2 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(2 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py(2 hunks)packages/jumpstarter/jumpstarter/client/base.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/client/decorators.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(2 hunks)packages/jumpstarter/jumpstarter/driver/base.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/session_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/jumpstarter/jumpstarter/client/base.py
- packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py
- packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py
- packages/jumpstarter/jumpstarter/exporter/session_test.py
- packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py
- packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py
- packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
- packages/jumpstarter/jumpstarter/driver/base.py
🧰 Additional context used
🧬 Code graph analysis (7)
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (2)
packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py (2)
client(10-11)client(25-26)packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(10-46)
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (2)
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py (1)
client(117-118)packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(10-46)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (2)
driver_click_group(10-46)add_option(82-85)
packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py (2)
packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/driver.py (2)
client(100-101)client(163-164)packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(10-46)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (2)
packages/jumpstarter/jumpstarter/client/decorators.py (2)
driver_click_group(10-46)command(87-99)packages/jumpstarter/jumpstarter/client/base.py (1)
call(42-52)
packages/jumpstarter/jumpstarter/client/client.py (1)
packages/jumpstarter/jumpstarter/driver/base.py (2)
client(101-104)report(198-217)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(10-46)
⏰ 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: pytest-matrix (macos-15, 3.11)
- 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.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (9)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
94-95: Methods description override is consistent.The logic correctly checks if the command name exists in
client.methods_descriptionbefore overriding help text, matching the pattern established indriver_click_group.packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
39-42: LGTM - Clean migration to driver_click_group.The migration from
@click.group()to@driver_click_group(self)is correct. The docstring "Generic power" will be used as the default help text and can be overridden by server-provided descriptions.packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py (2)
38-41: LGTM - Consistent migration pattern.The migration to
@driver_click_group(self)is correct forDigitalOutputClient. The docstring "GPIO power control commands." serves as the default help text.
83-86: LGTM - Matching pattern for DigitalInputClient.Same correct pattern applied consistently to the second client class.
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (1)
40-43: LGTM - Consistent with other driver migrations.The migration follows the established pattern. Docstring "Serial port client" provides the default help text.
packages/jumpstarter/jumpstarter/client/client.py (1)
62-63: Good defensive extraction from report.The use of
getattrwith defaults and theor None/or {}patterns safely handle cases where the report might not have these fields or they might be empty. This is robust against proto/gRPC variations.packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
21-29: LGTM - Standard migration with return added.The migration to
@driver_click_group(self)is correct, and the method now properly returns thesnmpgroup, enabling CLI composition.packages/jumpstarter/jumpstarter/config/exporter.py (1)
31-32: No changes needed: Driver supports these kwargs
TheDriverdataclass declaresdescriptionandmethods_descriptionas keyword-only fields, so passing them to the constructor is valid.packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
45-49: Good switch to driver_click_groupUsing driver_click_group here is correct and aligns help text with server-provided description via DriverClickGroup.
michalskrivanek
force-pushed
the
custom-descriptions
branch
from
0ad1f51 to
345dc18
Compare
introducing new format in addition to the simple
methods:
method: "command"
you can now specify per-method customization with:
methods:
method:
description: "This shows up in the CLI"
method: "echo Hello"
timeout: 5
michalskrivanek
force-pushed
the
custom-descriptions
branch
from
345dc18 to
219a9ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
39-46: Group options lost withdriver_click_groupSwitching to
@driver_click_group(self)drops the--log-leveloption. The decorator currently instantiatesDriverClickGroupwithout propagating the wrapped function’s callback/__click_params__, so the metadata injected by@click.optionis discarded. You can observe this indriver_click_group(packages/jumpstarter/jumpstarter/client/decorators.py), which returnsDriverClickGroup(client, name=f.__name__, **kwargs)without carrying overcallback=for the collected params. Please plumb the callback and params (e.g.callback=fplusparams=getattr(f, "__click_params__", ())) or otherwise reuseclick.groupso the CLI retains its options.
🧹 Nitpick comments (3)
packages/jumpstarter/jumpstarter/client/decorators.py (2)
44-45: Consider explicit None check for description override.The truthiness check means an empty string description (
client.description = "") won't override the docstring fallback. If a server intentionally sets an empty description to hide help text, this behavior might be unexpected.Consider this more explicit check if empty strings should override:
# Server description overrides Click defaults - if getattr(client, 'description', None): + if (desc := getattr(client, 'description', None)) is not None: kwargs['help'] = client.descriptionAlternatively, if empty strings should be treated as "no override," document this in the docstring to clarify the intended behavior.
72-73: Same consideration for explicit None check.Same truthiness behavior as
driver_click_group: an empty string description won't override.For consistency with the group decorator, consider:
- if getattr(client, 'description', None): + if (desc := getattr(client, 'description', None)) is not None: kwargs['help'] = client.descriptionpackages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (1)
31-36: Consider validation for missing "command" key.When a method is configured as a dict, the code defaults to
"echo Hello"if the"command"key is missing. This permissive behavior might hide configuration errors.Consider raising an error for missing commands:
def _get_method_command(self, method: str) -> str: """Extract the command string from a method configuration""" method_config = self.methods[method] if isinstance(method_config, str): return method_config - return method_config.get("command", "echo Hello") + if "command" not in method_config: + raise ValueError(f"Method '{method}' dict config missing required 'command' key") + return method_config["command"]This would fail fast during
call_methodif the configuration is invalid, rather than silently running a placeholder command.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py(2 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(2 hunks)packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py(3 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(4 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(2 hunks)packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py(2 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(2 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py(2 hunks)packages/jumpstarter-driver-shell/README.md(5 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(3 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py(6 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(2 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(2 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py(2 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py(2 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py(8 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.py(1 hunks)packages/jumpstarter/jumpstarter/client/base.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/client/decorators.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(2 hunks)packages/jumpstarter/jumpstarter/driver/base.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/session_test.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2_grpc.py
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
- packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py
- packages/jumpstarter/jumpstarter/client/base.py
- packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py
- packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
- packages/jumpstarter/jumpstarter/exporter/session_test.py
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
- packages/jumpstarter/jumpstarter/driver/base.py
- packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T15:44:51.089Z
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#690
File: packages/jumpstarter/jumpstarter/config/exporter.py:51-56
Timestamp: 2025-10-06T15:44:51.089Z
Learning: In jumpstarter config instantiation (packages/jumpstarter/jumpstarter/config/exporter.py), passing **self.root.config to driver_class constructors is intentional validation behavior: since Driver classes are dataclasses, the __init__ will reject any config keys that don't match declared fields with a TypeError, ensuring only declared/supported fields can be used in config YAML files. This is the desired behavior for config safety.
Applied to files:
packages/jumpstarter/jumpstarter/config/exporter.py
🧬 Code graph analysis (10)
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(13-49)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (2)
driver_click_group(13-49)command(85-97)
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (2)
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py (1)
client(117-118)packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(13-49)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(13-49)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
DriverClient(19-105)
packages/jumpstarter/jumpstarter/client/client.py (1)
packages/jumpstarter/jumpstarter/driver/base.py (2)
client(101-104)report(198-217)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(35-36)packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_command(52-75)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(13-49)
packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (2)
client(79-80)client(228-229)packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(13-49)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (3)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
cli(43-58)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (2)
Shell(12-217)client(46-47)packages/jumpstarter/jumpstarter/common/utils.py (1)
serve(31-39)
⏰ 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). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (5)
packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py (1)
24-99: Nice reuse ofdriver_click_group.Switching to the shared decorator keeps the CLI surface intact while honoring server-provided descriptions—good improvement in consistency across drivers.
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py (1)
1-825: Approved: upstream dependency mergedjumpstarter-protocol PR #28 is merged; the generated gRPC stubs for the new RPCs are correct. Proceed to merge.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (3)
24-29: LGTM!The
__post_init__correctly extracts per-method descriptions into themethods_descriptionfield for CLI help text propagation.
38-43: LGTM! Timeout fallback logic is correct.The timeout resolution follows the expected precedence:
- Per-method timeout (if specified in dict config)
- Global timeout (fallback for string configs or missing per-method timeout)
- Explicit None handling defaults to global timeout
This design provides flexibility while maintaining backward compatibility.
Also applies to: 179-181
148-148: LGTM! Per-method timeout integration is correct.The timeout parameter flows correctly through the call chain:
- Accepts
Noneto use global timeout (Line 148)- Defaults explicitly at Line 179-181
- Uses the resolved timeout in termination checks (Line 184)
- Reports the correct timeout in
TimeoutExpired(Line 199)Also applies to: 184-184, 199-199
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2_grpc.py
Show resolved
Hide resolved
this allows override of the default strings to show up in the jmp shell CLI.
the Driver descriptions can be customized with:
exporter:
driver_x:
type: x
description: "this driver does x"
methods_description:
"method1": "this method does y"
"method2": "this method does z"
config:
...
- Add driver_click_group decorator in client/decorators.py for CLI generation. Annotated method will be used as CLI group description(driver).
- Add driver_click_command decorator to use when there is no group (a single command driver)
- Add description and methods_description field to DriverClient, retrieved via GetReport call from server.
- Add description and methods_description field to Driver base class, only when customized (i.e. defaults are not transmitted).
The infrastructure is using a single DriverInstanceReport message to save on RPC calls.
… decorator Replace @click.group decorators with @driver_click_group and @click.command with @driver_click_command in all drivers, and use it to provide server-side driver description. The CLI client will retrieve customized driver's description via GetReport() from the exporter. When there's no customization it falls back to locally provided default in docstring or help=.
eliminate the extra RPC introduced in https://github.com/jumpstarter-dev/jumpstarter/pull/685
michalskrivanek
force-pushed
the
custom-descriptions
branch
from
219a9ed to
c1ec0c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/jumpstarter/jumpstarter/exporter/session_test.py (1)
277-311: Clarify the test logic for better readability.The simulation of DriverClickGroup logic uses confusing patterns like
if "help" in {}(line 294) which will always be False. While functionally correct for demonstrating the fallback path, this could be clearer.Consider rewriting for clarity:
- # Method with server override - method_name = "on" - if method_name in client.methods_description: - help_text = client.methods_description[method_name] - elif "help" in {}: # Simulate client's help= parameter - help_text = {}["help"] - else: - help_text = "" + # Method with server override + method_name = "on" + client_help = None # Simulate client's help= parameter + if method_name in client.methods_description: + help_text = client.methods_description[method_name] + elif client_help: + help_text = client_help + else: + help_text = ""packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (1)
31-36: Consider the default command choice.The default
"echo Hello"for dict entries without acommandkey may be surprising to users. While this enables dict entries with onlydescriptionortimeout, it might be clearer to either:
- Require
commandin dict format, or- Document this default prominently, or
- Use a more neutral default like
"true"(which simply exits with code 0)The current implementation works as tested in
test_mixed_format_methods, but consider whether this default is intuitive for production use.packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
83-85: LetDriverClickGroupsupply server help text.
DriverClickGroup.command()already overwriteshelpwhen the server providesmethods_description, so this manual lookup is redundant. You can drop the explicithelp=argument and rely on the decorator to inject the description (it will fall back to Click’s defaults otherwise).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py(2 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(2 hunks)packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py(3 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(4 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(2 hunks)packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py(2 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(2 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py(2 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(3 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py(6 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(2 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(2 hunks)packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py(2 hunks)packages/jumpstarter/jumpstarter/client/base.py(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/client/decorators.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(2 hunks)packages/jumpstarter/jumpstarter/driver/base.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/session_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py
- packages/jumpstarter/jumpstarter/client/base.py
- packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py
- packages/jumpstarter/jumpstarter/client/decorators.py
- packages/jumpstarter/jumpstarter/config/exporter.py
- packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py
- packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
- packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
- packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py
- packages/jumpstarter-driver-probe-rs/jumpstarter_driver_probe_rs/client.py
🧰 Additional context used
🧬 Code graph analysis (8)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (3)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
cli(43-58)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (2)
Shell(12-217)client(46-47)packages/jumpstarter/jumpstarter/common/utils.py (1)
serve(31-39)
packages/jumpstarter/jumpstarter/exporter/session_test.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (3)
Driver(56-277)client(101-104)report(198-217)packages/jumpstarter/jumpstarter/exporter/session.py (1)
Session(31-141)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(13-54)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(13-54)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(13-54)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
packages/jumpstarter/jumpstarter/client/decorators.py (2)
driver_click_group(13-54)command(90-102)
packages/jumpstarter/jumpstarter/client/client.py (1)
packages/jumpstarter/jumpstarter/driver/base.py (2)
client(101-104)report(198-217)
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (2)
packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py (2)
client(10-11)client(25-26)packages/jumpstarter/jumpstarter/client/decorators.py (1)
driver_click_group(13-54)
🔇 Additional comments (11)
packages/jumpstarter/jumpstarter/driver/base.py (2)
75-79: LGTM! Well-designed fields with clear documentation.The
descriptionandmethods_descriptionfields are properly typed, have clear docstrings explaining their purpose, and use appropriate defaults. This provides a clean way to customize driver and method help text.
215-216: Good use of defensive programming.The use of
self.description or Noneandself.methods_description or {}ensures empty strings and empty dicts are normalized, preventing clutter in the protocol messages.packages/jumpstarter/jumpstarter/exporter/session_test.py (2)
9-23: LGTM! Clean test doubles.The
SimpleDriverandCompositeDriver_test doubles are minimal and appropriate for testing the description propagation features.
25-68: LGTM! Thorough test of GetReport behavior.This test properly verifies that descriptions are included in GetReport responses for drivers that have them and excluded for drivers that don't.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (3)
155-179: LGTM! Comprehensive test of custom descriptions.This test properly verifies that custom descriptions from the unified dict format are propagated to CLI help text.
202-222: LGTM! Good test of methods_description population.This test correctly verifies that dict-format methods populate
methods_descriptionwhile string-format methods do not, allowing proper fallback to defaults.
224-257: LGTM! Excellent test of mixed method formats.This test demonstrates that string and dict formats can coexist, and properly exercises the default command fallback for dict entries without an explicit command.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (4)
15-19: LGTM! Clear documentation of the unified format.The comment clearly explains the two supported formats (simple string and dict with metadata), making it easy for users to understand the schema.
24-29: LGTM! Proper population of methods_description.The
__post_init__hook correctly extracts descriptions from dict-format method configurations and populates themethods_descriptionfield for server-to-client propagation.
38-43: LGTM! Clean timeout extraction with proper fallback.The
_get_method_timeouthelper correctly extracts per-method timeout from dict format and falls back to the global timeout for string format, providing flexible timeout configuration.
147-217: LGTM! Timeout parameter properly threaded through execution.The timeout parameter is correctly:
- Accepted in the signature (line 148)
- Defaulted to global timeout if None (lines 179-180)
- Used in the timeout check (line 184)
- Passed to TimeoutExpired (line 199)
This enables per-method timeout configuration while maintaining backward compatibility.
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
It looks great, this is such a huge usability improvement
|
Successfully created backport PR for |
2 participants
Summary by CodeRabbit
New Features
Shell
Configuration
Protocol
Documentation
Tests