-
Notifications
You must be signed in to change notification settings - Fork 18
add CLI support for Shell driver methods, stream output #601
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| import sys | ||
| from dataclasses import dataclass | ||
|
|
||
| import click | ||
|
|
||
|
Comment on lines
+4
to
+5
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainEnsure Click is available (or import lazily). Importing click at module import time makes it a hard dependency for this package. Either:
Run to verify dependency declaration: If you prefer lazy import, remove the top-level import and add inside cli(): -from dataclasses import dataclass
-
-import click
+from dataclasses import dataclassAnd at the start of cli(): # inside cli()
import click # local import to keep click optional unless CLI is used🏁 Script executed: #!/bin/bash
# Check for Click dependency in the shell driver package
fd -a 'pyproject.toml|setup.cfg|setup.py' packages/jumpstarter-driver-shell -x sh -c '
echo "==> {}"; rg -n "click" {}
'Length of output: 196 The
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@mangelajo Thanks for confirming! I can see the CI failure is indeed due to the missing Click dependency. Here are two solutions: Option 1: Add Click as a dependency (recommended for CLI-focused feature) dependencies = [
"jumpstarter",
"click>=8.1"
]Option 2: Lazy import (if you want to keep Click optional) from dataclasses import dataclass
# Remove: import click
from jumpstarter.client import DriverClientAnd update the def cli(self):
"""Create CLI interface for dynamically configured shell methods"""
try:
import click # lazy import to keep click optional
except ImportError:
raise ImportError("Click is required for CLI functionality. Install with: pip install click")
@click.group
def base():
"""Shell command executor"""
pass
# ... rest of the method unchangedGiven that this PR specifically adds CLI support, I'd recommend Option 1 since CLI functionality is the main feature being added. 🏁 Script executed: Length of output: 1154 🏁 Script executed: Length of output: 152 Perfect! I can see the issue clearly now. The Solution: Add Click to dependencies Update the dependencies line in dependencies = ["anyio>=4.6.2.post1", "jumpstarter", "click>=8.1"]This is the cleanest solution since:
This single change will resolve the CI failure and make the CLI functionality work properly. |
||
| from jumpstarter.client import DriverClient | ||
|
|
||
|
|
||
|
|
@@ -11,8 +14,8 @@ class ShellClient(DriverClient): | |
| Client interface for Shell driver. | ||
|
|
||
| This client dynamically checks that the method is configured | ||
| on the driver, and if it is, it will call it and get the results | ||
| in the form of (stdout, stderr, returncode). | ||
| on the driver, and if it is, it will call it with live streaming output. | ||
| Output chunks are displayed as they arrive. | ||
| """ | ||
|
|
||
| def _check_method_exists(self, method): | ||
|
|
@@ -24,4 +27,59 @@ def _check_method_exists(self, method): | |
| ## capture any method calls dynamically | ||
| def __getattr__(self, name): | ||
| self._check_method_exists(name) | ||
| return lambda *args, **kwargs: tuple(self.call("call_method", name, kwargs, *args)) | ||
| def execute(*args, **kwargs): | ||
| returncode = 0 | ||
| for stdout, stderr, code in self.streamingcall("call_method", name, kwargs, *args): | ||
| if stdout: | ||
| print(stdout, end='', flush=True) | ||
| if stderr: | ||
| print(stderr, end='', file=sys.stderr, flush=True) | ||
| if code is not None: | ||
| returncode = code | ||
| return returncode | ||
| return execute | ||
|
|
||
| def cli(self): | ||
| """Create CLI interface for dynamically configured shell methods""" | ||
| @click.group | ||
| def base(): | ||
| """Shell command executor""" | ||
| pass | ||
|
|
||
| # Get available methods from the driver | ||
| if self._methods is None: | ||
| self._methods = self.call("get_methods") | ||
|
|
||
| # Create a command for each configured method | ||
| for method_name in self._methods: | ||
|
michalskrivanek marked this conversation as resolved.
|
||
| self._add_method_command(base, method_name) | ||
|
|
||
| return base | ||
|
|
||
| def _add_method_command(self, group, method_name): | ||
| """Add a Click command for a specific shell method""" | ||
| @group.command( | ||
| name=method_name, | ||
| context_settings={"ignore_unknown_options": True, "allow_interspersed_args": False}, | ||
| ) | ||
| @click.argument('args', nargs=-1, type=click.UNPROCESSED) | ||
| @click.option('--env', '-e', multiple=True, | ||
| help='Environment variables in KEY=VALUE format') | ||
| 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.") | ||
|
|
||
| returncode = getattr(self, method_name)(*args, **env_dict) | ||
|
|
||
| # Exit with the same return code as the shell command | ||
| if returncode != 0: | ||
| raise click.exceptions.Exit(returncode) | ||
|
|
||
| # Update the docstring dynamically | ||
| method_command.__doc__ = f"Execute the {method_name} shell method" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use a single CLI invocation consistently (“jmp”), not mixed “j”/“jmp”.
Examples alternate between “j” and “jmp”, which is confusing.
Also applies to: 90-98, 104-115
🤖 Prompt for AI Agents