add per-method description and timeout to shell commands#685
add per-method description and timeout to shell commands#685mangelajo merged 1 commit intojumpstarter-dev:mainfrom
Conversation
WalkthroughAdds support for two method formats (simple string or dict with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI (client.py)
participant Driver as Shell Driver (driver.py)
participant Config as Methods Config
rect rgb(245,248,255)
note over CLI: Startup — register method commands
CLI->>Config: Read methods (string or dict)
CLI->>Driver: get_method_description(method)
Driver-->>CLI: description (custom or default)
CLI->>CLI: apply click decorators with help=description
end
User->>CLI: run command: <method> [args]
CLI->>Driver: call_method(method, args)
alt method has per-method timeout
Driver->>Driver: extract command + per-method timeout
else fallback to global timeout
Driver->>Driver: extract command + global timeout
end
Driver->>Driver: _run_inline_shell_script(script, timeout)
Driver-->>CLI: result (exit code, output)
CLI-->>User: print result / help
sequenceDiagram
autonumber
participant Driver as Shell Driver
participant Runner as Inline Shell Runner
participant OS as Subprocess/Env
Driver->>Runner: run(script, env, cwd, timeout)
Runner->>OS: spawn process
par stream stdout/stderr
OS-->>Runner: stream data
and monitor timeout
Runner->>Runner: enforce timeout
end
alt timeout expires
Runner-->>Driver: TimeoutExpired (per-method or global)
else completes
Runner-->>Driver: exit code, output
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (22)
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 |
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-driver-shell/README.md(5 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(1 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py(7 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (4)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (2)
client(46-47)Shell(12-222)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
cli(42-57)packages/jumpstarter/jumpstarter/common/utils.py (1)
serve(31-39)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-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). (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 (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py
Outdated
Show resolved
Hide resolved
michalskrivanek
force-pushed
the
shell-customization
branch
from
ebcdb3a to
f958878
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (5)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py(1 hunks)packages/jumpstarter-driver-shell/README.md(5 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(1 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py(7 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-shell/README.md
🧰 Additional context used
🧬 Code graph analysis (3)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (4)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (2)
client(46-47)Shell(12-222)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
cli(42-57)packages/jumpstarter/jumpstarter/common/utils.py (1)
serve(31-39)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)
cli(34-55)
⏰ 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: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (2)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
77-91: Fallback preserves older driver compatibility.
Wrapping the RPC in a broad try/except keeps CLI generation working against pre-0.7 servers while still surfacing richer help text when available. Nicely handled.packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (1)
130-255: Expanded tests cover new CLI metadata paths.
Great job validating exit codes, custom descriptions, default fallbacks, and mixed method formats—this suite gives strong confidence in the new configuration shape.
| for k, v in self.children.items(): | ||
| if hasattr(v, "cli"): | ||
| # Check if the cli method accepts a click_group parameter | ||
| sig = inspect.signature(v.cli) | ||
| if "click_group" in sig.parameters: | ||
| base.add_command(v.cli(click_group=base), k) | ||
| else: | ||
| base.add_command(v.cli(), k) | ||
| base.add_command(v.cli(), k) | ||
|
|
There was a problem hiding this comment.
Regression: child CLIs needing the parent group now break.
Many drivers’ cli factories (e.g. jumpstarter_driver_tmt.client.ShellClient.cli(self, click_group)) still expect the parent click.Group to be passed in. Removing the signature check and calling v.cli() unconditionally now raises TypeError for every such driver, so composite CLIs stop booting. Please keep supporting both styles—detect whether the child cli expects a group parameter and pass base accordingly.
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py
around lines 50 to 53, the loop unconditionally calls v.cli() which breaks
drivers whose cli factories expect the parent click.Group parameter; update the
code to detect whether v.cli accepts a parameter and call v.cli(base) when it
does, otherwise call v.cli() — e.g., use inspect.signature (or similar) to check
the number of parameters or try calling with base and fall back on calling
without it, then add the resulting command to base with base.add_command(...,
k).
| def test_multi_line_scripts(client): | ||
| stdout, stderr, returncode = _collect_streaming_output(client, "multi_line", {}, "a", "b", "c") | ||
| assert stdout == "a\nb\nc\n" | ||
| def test_normal_multi_line(client): | ||
| stdout, stderr, returncode = _collect_streaming_output(client, "multi_line", {}, "hello", "world", "!") | ||
| assert stdout == "hello\nworld\n!\n" | ||
| assert stderr == "" | ||
| assert returncode == 0 | ||
|
|
||
|
|
||
| def test_return_codes(client): | ||
| stdout, stderr, returncode = _collect_streaming_output(client, "exit1") | ||
| assert stdout == "" | ||
| assert stderr == "" | ||
| def test_exit_code(client): | ||
| stdout, stderr, returncode = _collect_streaming_output(client, "exit1", {}) | ||
| assert returncode == 1 |
There was a problem hiding this comment.
I think AI is being creative here with unnecessary changes :D
| client.unknown() | ||
| except AttributeError as e: | ||
| assert "method unknown not found in" in str(e) | ||
| _collect_streaming_output(client, "unknown_method", {}) | ||
| except Exception as e: | ||
| assert "not found" in str(e) | ||
|
|
There was a problem hiding this comment.
why is this change needed?
won't a regular call to .unknown method fail in the same way?
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
only some nits about testing, it looks good,
only one question about test_unknown_method test.
| "method1": { | ||
| "command": "echo", | ||
| "description": "Custom description for method1" | ||
| }, | ||
| "method2": "ls", # String format, should use default description | ||
| } |
There was a problem hiding this comment.
ah cool so it's backwards compatible 💯
There was a problem hiding this comment.
yeah it's not too ugly to keep supporting the old way as well. maybe it will still be useful for simple things like
methods:
ls: ls
michalskrivanek
force-pushed
the
shell-customization
branch
from
f958878 to
8322e57
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/jumpstarter-driver-shell/README.md (1)
16-16: Fix heading punctuation.The heading "Format 1: Simple String e.g. for self-descriptive short commands)" has a stray closing parenthesis. Either add the opening parenthesis after "Simple String" or remove the trailing one.
📜 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 (17)
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-power/jumpstarter_driver_power/client_test.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/jumpstarter/client/base.py(1 hunks)packages/jumpstarter/jumpstarter/driver/base.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py
🧰 Additional context used
🧬 Code graph analysis (12)
packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
get_cli_description(92-111)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
get_cli_description(92-111)
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(21-33)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
get_cli_description(92-111)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (2)
packages/jumpstarter/jumpstarter/client/base.py (1)
get_cli_description(92-111)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-gpiod/jumpstarter_driver_gpiod/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
get_cli_description(92-111)
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
get_cli_description(92-111)
packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py (3)
packages/jumpstarter/jumpstarter/common/utils.py (1)
serve(31-39)packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py (3)
MockPower(39-64)client(10-11)client(25-26)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (2)
cli(37-59)cli(66-75)
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
get_cli_description(92-111)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
get_cli_description(92-111)
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(36-46)
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)
⏰ 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.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (26)
packages/jumpstarter/jumpstarter/driver/base.py (1)
76-110: LGTM! Clean addition of driver description support.The new
descriptionfield andget_description()method provide a clean mechanism for drivers to expose custom descriptions for CLI help text. The implementation is correct:
- Optional field with clear docstring
- Proper use of
@exportdecorator for RPC access- Returns
str | Noneas expected by consumerspackages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
20-20: LGTM! Consistent use of dynamic CLI descriptions.The CLI group now uses
self.get_cli_description("SNMP power control commands")to provide dynamic help text with a sensible default fallback.packages/jumpstarter/jumpstarter/client/base.py (1)
92-111: LGTM! Well-designed fallback mechanism.The
get_cli_description()method provides a robust way to retrieve driver descriptions with proper error handling:
- Attempts to fetch custom description via RPC
- Gracefully handles failures (driver may not support get_description)
- Returns sensible default as fallback
- Clear documentation
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (1)
105-105: LGTM! Consistent CLI description pattern.The storage group now uses dynamic help text via
get_cli_description("RideSX storage operations").packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
753-753: LGTM! Consistent CLI description pattern.The base CLI group now uses dynamic help text via
get_cli_description("Software-defined flasher interface").packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py (2)
37-37: LGTM! Consistent CLI description pattern.The GPIO power control CLI group now uses dynamic help text.
81-81: LGTM! Consistent CLI description pattern.The GPIO input CLI group now uses dynamic help text.
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
21-33: LGTM! Simplified CLI registration.The SSH client now returns a command directly using
@click.command(...)rather than accepting a parent group parameter. This aligns with the TMT driver pattern and simplifies the interface by removing theclick_groupparameter from the signature.packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (2)
37-37: LGTM! Consistent CLI description pattern.The composite device CLI group now uses dynamic help text via
get_cli_description("Generic composite device").
49-51: All child drivers now support parameterless cli() jumpstarter-driver-opendal/client.py still defines cli(self, base=None) to preserve backward compatibility; all other drivers use a parameterless cli().packages/jumpstarter-driver-shell/README.md (3)
28-66: LGTM! Comprehensive examples.The unified format examples effectively demonstrate all supported configurations: custom descriptions, multi-line commands, description-only methods, custom timeouts, and format mixing. The inline comments enhance clarity.
116-166: LGTM! Clear CLI documentation.The CLI sections effectively illustrate the difference between unified format (custom descriptions) and simple format (default descriptions), including a helpful mixed-format example.
67-84: Default command documentation matches implementationThe
driver.pydefault of"echo Hello"aligns with the README.packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (7)
100-107: LGTM! Safe indexing change.Switching from
.get()to direct indexing is acceptable here since the test fixture guarantees the 'echo' method exists. The assertion on line 106 confirms the command structure.
109-128: LGTM! Thorough CLI method presence test.The test properly verifies that all configured methods are exposed as CLI commands using set equality, ensuring no methods are missing or unexpectedly added.
131-153: LGTM! Exit code validation test.This test properly verifies that shell command exit codes are correctly propagated through the driver, covering success (0), standard failure (1), and custom exit codes (42).
155-179: LGTM! Custom description validation.The test effectively validates that custom descriptions from the unified format are properly exposed in CLI command help text.
181-200: LGTM! Default description validation.This test ensures backwards compatibility by verifying that simple string-format methods receive appropriate default descriptions following the established pattern.
202-220: LGTM! Export method validation.The test properly validates the
get_method_descriptionexport method for both unified format (custom description) and simple format (default description).
222-256: LGTM! Comprehensive mixed-format test.This test thoroughly validates the core feature by mixing all supported formats (simple string, dict with command, dict with description-only) and verifying both execution and CLI help text. The test confirms that methods without a command specification can still execute successfully.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (6)
24-29: LGTM! Clean command extraction.The helper correctly handles both string and dict formats, with an appropriate default command. The default
"echo Hello"is simple and safe for testing.
31-36: LGTM! Consistent description extraction.The helper properly generates default descriptions for simple formats and extracts custom descriptions from the unified format, maintaining consistency with the documented behavior.
38-43: LGTM! Proper timeout resolution.The helper correctly implements the timeout fallback chain: per-method timeout → global timeout. This allows fine-grained control while maintaining a sensible default.
55-58: LGTM! Clean public API.The exported method provides a clean interface for clients to retrieve method descriptions, essential for generating CLI help text at runtime.
60-89: LGTM! Proper helper integration.The method correctly uses the new helper functions to extract command and timeout, and properly threads the per-method timeout through to the execution layer. The logging enhancement aids debugging.
152-222: LGTM! Clean timeout integration.The timeout parameter is properly integrated into the execution loop:
- Backwards-compatible signature with
timeout=None- Correct fallback to global timeout
- Proper use in timing logic and exception handling
michalskrivanek
force-pushed
the
shell-customization
branch
from
8322e57 to
4e8aa05
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (1)
15-44: Broadenmethodstype to cover per-method timeoutsThe new unified format allows
timeout: int, but the annotation still constrains dict values todict[str, str]. Static type-checkers (and IDEs) now flag every config using a numeric timeout. Please widen the type, e.g. by introducing aTypedDictwith optionalcommand,description, andtimeoutkeys, or by extending the union to includeintvalues.-from dataclasses import dataclass, field -from typing import AsyncGenerator +from dataclasses import dataclass, field +from typing import AsyncGenerator, TypedDict ... - methods: dict[str, str | dict[str, str]] +class MethodConfig(TypedDict, total=False): + command: str + description: str + timeout: int + + +@dataclass(kw_only=True) +class Shell(Driver): + ... + methods: dict[str, str | MethodConfig]
🧹 Nitpick comments (1)
packages/jumpstarter-driver-shell/README.md (1)
50-56: Clarify placeholder command behaviorDocumentation claims “Description-only (uses default
echo Hellocommand)”, but the actual driver defaults tocommand = "echo Hello"only whencommandis omitted and configuration is the dict form. For this placeholder example you should either show the default command explicitly or note that omittingcommandcauses the driver to run"echo Hello"so readers know what will execute.
📜 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(2 hunks)packages/jumpstarter/jumpstarter/driver/base.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/session.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
- packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py
🧰 Additional context used
🧬 Code graph analysis (7)
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)
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/driver_test.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)
packages/jumpstarter/jumpstarter/exporter/session.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (2)
enumerate(214-225)report(195-212)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver.py (2)
enumerate(40-41)report(37-38)
packages/jumpstarter/jumpstarter/client/client.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (1)
report(195-212)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver.py (1)
report(37-38)
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(21-33)
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)
🪛 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' (during Sphinx docs build)
🪛 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'. This occurs while importing generated protobuf modules during test discovery.
⏰ 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 (15)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
21-22: Approve CLI signature change
No callers ofSSHWrapperClient.cli()pass a group argument; removing theclick_groupparameter is safe and aligns with other drivers.packages/jumpstarter/jumpstarter/driver/base.py (1)
75-77: LGTM! Clean addition of description field.The new optional
descriptionfield is well-documented and properly typed. The placement between existing fields is logical, and the default value ofNoneensures backward compatibility.packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
38-40: LGTM! Consistent implementation of dynamic help text.The decorator now sources help text from
self.descriptionwith a sensible fallback, aligning with the PR's objective to surface driver descriptions in CLI help.packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py (1)
23-25: LGTM! Consistent pattern applied.The dynamic help text implementation matches the pattern established across other drivers in this PR.
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
20-22: LGTM! Appropriate help text for SNMP driver.The help text implementation is consistent with other drivers and provides a clear description of the SNMP power control functionality.
packages/jumpstarter/jumpstarter/client/base.py (1)
36-38: LGTM! Parallel implementation for client side.The
descriptionfield onDriverClientmirrors the server-side Driver field, enabling CLI help text to be propagated from the driver through GetReport. The docstring clearly explains the data flow.packages/jumpstarter-driver-gpiod/jumpstarter_driver_gpiod/client.py (2)
37-39: LGTM! Consistent help text for GPIO output.The dynamic help text implementation provides appropriate context for GPIO power control operations.
81-83: LGTM! Consistent help text for GPIO input.The dynamic help text implementation appropriately differentiates input operations from output operations.
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (1)
105-107: LGTM! Descriptive help text for RideSX storage.The help text appropriately describes the storage operations context for the RideSX driver.
packages/jumpstarter/jumpstarter/exporter/session.py (2)
118-118: LGTM! Descriptions added to GetReportResponse.The descriptions map is properly included in the response, enabling clients to retrieve all driver descriptions in a single RPC call as documented in the comment.
104-109: Approve description collection logic.enumerate()consistently returns a 4-tuple(uuid, parent, name, instance)across driver implementations, so the filtering and mapping ofinstance.descriptionis safe.packages/jumpstarter/jumpstarter/client/client.py (2)
39-40: LGTM!The defensive use of
getattrwith a default empty dict ensures backward compatibility when thedescriptionsfield is absent from the response.
64-64: Verify client constructors acceptdescription
Manually confirm that every*Clientclass inpackages/jumpstarter/jumpstarter/client/(and its base classes) defines an__init__signature accepting thedescriptionkeyword (or**kwargs) to prevent runtime errors.packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/driver_test.py (1)
26-435: LGTM! Test updates align with CLI signature changes.The test suite has been correctly updated to reflect the new CLI attachment model where TMT commands are invoked directly without a top-level "tmt" subcommand group. All test invocations consistently remove the "tmt" prefix from argument lists, and the test logic remains sound.
packages/jumpstarter-driver-tmt/jumpstarter_driver_tmt/client.py (1)
34-36: LGTM! CLI signature simplification aligns with the new attachment model.Removing the
click_groupparameter and using@click.commanddirectly simplifies the CLI integration and aligns with the pattern used in other drivers (e.g., SSH driver). The test suite confirms this change works correctly.
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py
Outdated
Show resolved
Hide resolved
michalskrivanek
force-pushed
the
shell-customization
branch
2 times, most recently
from
fd403f3 to
0f229d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
77-81: Fallback description does not match legacy wording.The fallback description at line 81 uses
f"Execute {method_name}", but the past review comment specifically requestedf"Execute the {method_name} shell method"to preserve exact legacy wording for pre-0.7 servers. This discrepancy means the help text will differ from documented behavior when talking to older servers.Apply this diff to restore the legacy wording:
- description = f"Execute {method_name}" + description = f"Execute the {method_name} shell method"
🧹 Nitpick comments (1)
packages/jumpstarter-driver-shell/README.md (1)
71-72: Call out per-methodtimeoutin the methods table for completenessThe table row for
methodsmentions onlycommandanddescription, so readers can miss that dict entries may also overridetimeout. Please surface the new field here to keep the quick-reference accurate.-| `methods` | Dictionary of methods. Values can be:<br/>- String: just the command<br/>- Dict: `{command: "...", description: "..."}` | `dict[str, str \| dict]` | Yes | - | +| `methods` | Dictionary of methods. Values can be:<br/>- String: just the command<br/>- Dict: `{command: "...", description: "...", timeout: ...}` | `dict[str, str \| dict]` | Yes | - |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-driver-shell/README.md(5 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(1 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py(7 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (4)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
cli(42-57)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(36-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). (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 (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (8)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
84-91: LGTM! Dynamic decorator application is correctly implemented.The decorator chain properly applies click decorators after retrieving the method description, and the command registration uses the retrieved description as help text. This enables runtime description resolution as intended by the PR.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (7)
104-106: LGTM! Direct indexing is appropriate for test assertions.The change from
.get('echo')to direct indexing['echo']is acceptable in test context. If the command doesn't exist, the KeyError provides a clear failure signal, which is appropriate for testing expected behavior.
109-128: LGTM! Test properly validates CLI method inclusion.The test correctly verifies that all configured methods appear as CLI commands. The assertions are clear and the test structure is sound.
131-153: LGTM! Exit code handling is properly tested.The test effectively validates that shell command exit codes are correctly propagated through the client interface, covering success, failure, and custom exit codes.
155-179: LGTM! Custom descriptions are properly tested.The test effectively validates the new unified dict format for method configuration with custom descriptions. The assertions confirm that custom descriptions appear correctly in CLI help text.
181-200: LGTM! Default descriptions are properly tested.The test correctly validates that methods using the simple string format receive default descriptions with the legacy wording "Execute the {method} shell method". This ensures backward compatibility is maintained.
202-220: LGTM! RPC method for descriptions is properly tested.The test effectively validates the
get_method_descriptionexport method, covering both custom descriptions from dict format and default descriptions from string format. The assertions correctly verify the RPC behavior.
222-255: LGTM! Mixed format support is thoroughly tested.This comprehensive test validates that string format, dict format with custom command and description, and dict format with only description (using default command) all work correctly together. The test covers both execution and CLI description generation for all three formats, ensuring the feature is robust and backward compatible.
michalskrivanek
force-pushed
the
shell-customization
branch
2 times, most recently
from
cde250b to
6767fd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter-driver-shell/README.md (1)
79-82: Consider clarifying per-method timeout behavior.While the current text correctly states that per-method
timeout"defaults to globaltimeoutvalue", it might be clearer to explicitly mention that the global default is 300 seconds. This helps users understand what happens when they omit both global and per-method timeout.Consider this minor clarification:
- `timeout`: Command-specific timeout in seconds (optional, defaults to global `timeout` value) +- `timeout`: Command-specific timeout in seconds (optional, defaults to global `timeout` value of 300 seconds)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-driver-shell/README.md(5 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(1 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py(7 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (4)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
cli(42-57)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(36-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). (1)
- GitHub Check: pytest-matrix (macos-15, 3.13)
🔇 Additional comments (4)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
77-91: LGTM! Backward compatibility preserved.The try/except block correctly handles the scenario where
get_method_descriptionisn't available on pre-0.7 servers. The fallback to the legacy help text format ("Execute the {method_name} shell method") ensures consistent behavior with older versions. The dynamic decoration after description retrieval is correctly structured.packages/jumpstarter-driver-shell/README.md (1)
118-166: LGTM! Documentation accurately reflects the CLI behavior.The CLI usage documentation clearly demonstrates how descriptions appear in help output for unified format (custom descriptions), simple format (default descriptions), and mixed format. The examples are consistent with the implementation.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (1)
222-255: LGTM! Comprehensive test coverage for mixed formats.This test thoroughly validates the mixed format feature by:
- Testing execution of all three format variants (simple string, detailed dict, dict with description-only)
- Verifying CLI help text for each format
- Ensuring the default command fallback works correctly
The test coverage ensures the feature works correctly in all documented scenarios.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (1)
152-222: LGTM! Timeout handling correctly implemented.The per-method timeout feature is well-integrated:
- The
timeoutparameter is properly threaded through the execution path- Fallback to
self.timeoutprovides a sensible default- Timeout enforcement uses the correct local value
- The
TimeoutExpiredexception accurately reflects the timeout that was enforcedThe defensive None check (lines 184-185) is good practice even though the current call path always provides a timeout.
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py
Outdated
Show resolved
Hide resolved
michalskrivanek
force-pushed
the
shell-customization
branch
from
6767fd3 to
97fbefb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-driver-shell/README.md(5 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(1 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py(7 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (4)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
cli(42-57)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(36-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: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.13)
- 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: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
eliminate the extra RPC introduced in https://github.com/jumpstarter-dev/jumpstarter/pull/685
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
shell-customization
branch
from
97fbefb to
e94dc35
Compare
eliminate the extra RPC introduced in https://github.com/jumpstarter-dev/jumpstarter/pull/685
eliminate the extra RPC introduced in https://github.com/jumpstarter-dev/jumpstarter/pull/685
eliminate the extra RPC introduced in https://github.com/jumpstarter-dev/jumpstarter/pull/685 (cherry picked from commit c1ec0c8)
eliminate the extra RPC introduced in https://github.com/jumpstarter-dev/jumpstarter/pull/685 (cherry picked from commit c1ec0c8)
2 participants
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
Summary by CodeRabbit