Shell driver for the exporter #200
Conversation
mangelajo
force-pushed
the
shell-driver
branch
2 times, most recently
from
f95a047 to
25d882e
Compare
|
Trying an improved doc format for drivers: |
mangelajo
force-pushed
the
shell-driver
branch
from
25d882e to
dcc4adb
Compare
|
We should add a CWD option to specify the working directory to call the command in case we're using scripts or other tools that are picky about that. |
Oh... true, that's a good one. |
mangelajo
force-pushed
the
shell-driver
branch
3 times, most recently
from
355b9be to
5b7e35e
Compare
|
@kirkbrauer done :D |
mangelajo
changed the title
mangelajo
force-pushed
the
shell-driver
branch
from
42efbc6 to
ff3091c
Compare
WalkthroughThe changes introduce a new shell driver for the Jumpstarter framework. Documentation has been updated to include the shell driver in the table of contents, with an accompanying detailed API description. New implementation files provide the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant SC as ShellClient
participant SD as ShellDriver
participant EX as Shell Script Executor
C->>SC: Invoke method (e.g., ls)
SC->>SC: __getattr__ intercepts call and validates method existence
SC->>SD: call_method(method, args)
SD->>EX: _run_inline_shell_script(method, script, args)
EX-->>SD: Return (stdout, stderr, return code)
SD-->>SC: Return results tuple
SC-->>C: Deliver command output
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mangelajo
force-pushed
the
shell-driver
branch
from
ff3091c to
8ecaec8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 7
🧹 Nitpick comments (9)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (4)
45-47: Handle Potential Escape Characters in LoggingWhen logging
stderrandstdout, any escape characters or special formatting in the output may disrupt the log formatting.Consider using
reprto safely log the output:if result.stderr != "": - logger.debug(f"{method} stderr:\n{result.stderr.rstrip("\n")}") + logger.debug(f"{method} stderr:\n{repr(result.stderr.rstrip('\n'))}") if result.stdout != "": - logger.debug(f"{method} stdout:\n{result.stdout.rstrip("\n")}") + logger.debug(f"{method} stdout:\n{repr(result.stdout.rstrip('\n'))}")This change ensures that special characters are properly represented in the logs without affecting readability.
10-11: Class Documentation MissingThe class
Shelllacks a comprehensive docstring explaining its purpose and usage.Add a docstring to the class to improve code readability and maintainability:
class Shell(Driver): + """ + Shell driver for Jumpstarter framework that allows executing shell scripts + defined in a configuration. Provides methods to list available scripts and + execute them with given arguments and environment variables. + """
65-66: Avoid Overwriting Existing Environment VariablesWhen updating
combined_envwithenv_vars, there's a possibility of overwriting essential environment variables.Consider using a more controlled approach to updating the environment:
if env_vars: - combined_env.update(env_vars) + for key, value in env_vars.items(): + if key not in combined_env: + combined_env[key] = value + else: + logger.warning(f"Environment variable '{key}' is being overwritten.") + combined_env[key] = valueThis change logs a warning when an environment variable is overwritten, helping to prevent unintended side effects.
24-24: Set Logger to Use Class-Specific Logger InstanceCurrently,
loggeris a module-level logger. To improve logging granularity, set the logger to use the class-specific logger instance.Modify the logger initialization:
-logger = logging.getLogger(__name__) +self.logger = logging.getLogger(f"{__name__}.{self.__class__.__name__}")And update all logging calls to use
self.loggerinstead oflogger.contrib/drivers/shell/jumpstarter_driver_shell/client.py (1)
8-8: Initialize Mutable Default Arguments SafelyThe
_methodsattribute is initialized with a mutable default valueNone. WhileNoneis immutable, it's good practice to define default mutable attributes usingfield(default_factory=...).Adjust the dataclass field for
_methods:_methods: list[str] = field(default_factory=list)Also, adjust the logic in
_check_method_existsto accommodate the change.contrib/drivers/shell/jumpstarter_driver_shell/driver_test.py (4)
10-15: Improve Readability by Formatting Dictionary ElementsIn the
clientfixture, themethodsdictionary is not aligned properly, affecting readability.Reformat the
methodsdictionary:instance = Shell( log_level="DEBUG", methods={ - "echo": "echo $1", - "env": "echo $ENV1", - "multi_line": "echo $1\necho $2\necho $3", - "exit1": "exit 1", - "stderr": "echo $1 >&2" + "echo": "echo $1", + "env": "echo $ENV1", + "multi_line": "echo $1\necho $2\necho $3", + "exit1": "exit 1", + "stderr": "echo $1 >&2", } )
38-41: Use Context Manager for Exception Handling in TestsIn
test_unknown_method, using a try-except-else block is less Pythonic for testing exceptions.Use
pytest.raisesfor clearer exception testing:def test_unknown_method(client): - try: - client.unknown() - except AttributeError as e: - assert "method unknown not found in" in str(e) - else: - raise AssertionError("Expected AttributeError") + with pytest.raises(AttributeError) as exc_info: + client.unknown() + assert "method unknown not found in" in str(exc_info.value)
27-27: Add Test for No Arguments in Multi-line ScriptIn
test_multi_line_scripts, currently, the test passes three arguments. It's beneficial to test the behavior when no arguments are provided.Add an additional test case to cover this scenario:
def test_multi_line_no_args(client): assert client.multi_line() == ("\n\n\n", "", 0)
30-30: Clarify Test Intent with Descriptive Assertion MessagesIn
test_return_codes, the assertion lacks an explanatory message, which could aid in debugging if the test fails.Update the assertion to include a message:
assert client.exit1() == ("", "", 1), "Expected return code 1 for exit1 method"
🛑 Comments failed to post (7)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (1)
68-68:
⚠️ Potential issuePotential Argument Misalignment in Command Execution
In line 68, the
cmdvariable includes themethodname as an argument:cmd = self.shell + [script, method] + list(args)Including
methodin the command arguments may shift the positional parameters, causing misalignment when the script accesses$1,$2, etc. This could lead to unexpected behavior or errors in the shell script execution.Apply this diff to correct the argument list:
-def _run_inline_shell_script(self, method, script, *args, env_vars=None): +def _run_inline_shell_script(self, script, *args, env_vars=None): -def call_method(self, method: str, env, *args): +def call_method(self, method: str, env, *args): # Inside call_method result = self._run_inline_shell_script(script, *args, env_vars=env) # Inside _run_inline_shell_script - cmd = self.shell + [script, method] + list(args) + cmd = self.shell + [script] + list(args)This adjustment ensures that the arguments passed to the shell script align correctly with the
$1,$2, etc., placeholders.contrib/drivers/shell/jumpstarter_driver_shell/client.py (1)
24-26:
⚠️ Potential issueHandle Exceptions Appropriately in Dynamic Method Invocation
In the
__getattr__method, ifself.callfails, it may raise exceptions that are not handled.Consider adding exception handling to provide more informative error messages to the user.
def __getattr__(self, name): self._check_method_exists(name) - return lambda *args, **kwargs: tuple(self.call("call_method", name, kwargs, *args)) + def method(*args, **kwargs): + try: + return tuple(self.call("call_method", name, kwargs, *args)) + except Exception as e: + raise RuntimeError(f"Error calling method '{name}': {e}") from e + return method📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def __getattr__(self, name): self._check_method_exists(name) def method(*args, **kwargs): try: return tuple(self.call("call_method", name, kwargs, *args)) except Exception as e: raise RuntimeError(f"Error calling method '{name}': {e}") from e return methodcontrib/drivers/shell/examples/exporter.yaml (2)
9-16:
⚠️ Potential issueSecurity: Add command validation and restrictions.
The current configuration allows unrestricted shell command execution, which could be a security risk. Consider:
- Implementing an allowlist of permitted commands
- Adding input validation for parameters
- Restricting environment variable access
Add a
permitted_commandssection to restrict which commands can be executed:config: permitted_commands: - ls - echo methods: # ... existing methods ...
3-4:
⚠️ Potential issueSecurity: Avoid hardcoding sensitive information.
The token should not be committed in plain text. Consider using environment variables or a secure secret management system.
-token: "<token>" +token: ${JUMPSTARTER_TOKEN}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.endpoint: grpc.jumpstarter.192.168.0.203.nip.io:8082 token: ${JUMPSTARTER_TOKEN}contrib/drivers/shell/pyproject.toml (1)
4-4: 🛠️ Refactor suggestion
Add missing dependencies and version constraints.
- Add security-related dependencies for shell command execution
- Add upper version bounds to prevent breaking changes
- Replace placeholder description
-description = "Add your description here" +description = "Shell driver for Jumpstarter exporters enabling remote command execution" dependencies = [ "anyio>=4.6.2.post1", "jumpstarter", + "secure-shell>=1.0.0,<2.0.0", # For secure shell command execution + "input-validation>=2.0.0,<3.0.0", # For parameter validation ]Also applies to: 10-13
docs/source/api-reference/drivers/shell.md (1)
8-28: 🛠️ Refactor suggestion
Enhance documentation with security guidelines and best practices.
Add sections covering:
- Security considerations and command restrictions
- Input validation requirements
- Error handling
- Best practices for shell command configuration
Add these sections to the documentation:
## Security Considerations - Use allowlists to restrict permitted commands - Validate all input parameters - Avoid shell injection vulnerabilities - Restrict environment variable access ## Error Handling Commands may return non-zero exit codes. Handle these cases: - Command not found - Permission denied - Timeout - Invalid input ## Best Practices 1. Use absolute paths for commands 2. Implement proper timeout handling 3. Log command execution for audit 4. Use principle of least privilegecontrib/drivers/shell/README.md (1)
3-5: 🛠️ Refactor suggestion
Enhance README with security warnings and examples.
- Fix grammar: "provides simple shell access" (remove "a")
- Add security warnings
- Include usage examples
-This driver provides a simple shell access to the target exporter, and it is +This driver provides simple shell access to the target exporter, and it is intended to be used when command line tools exist to manage existing interfaces or hardware, but no drivers exist yet in Jumpstarter. + +## Security Considerations + +⚠️ WARNING: This driver executes shell commands on the target system. Follow these security guidelines: +- Use allowlists to restrict permitted commands +- Validate all input parameters +- Avoid shell injection vulnerabilities + +## Usage Example + +```yaml +export: + example: + type: jumpstarter_driver_shell.driver.Shell + config: + permitted_commands: + - ls + methods: + list_files: "ls -l" +```🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Uncountable nouns are usually not used with an indefinite article. Use simply “simple shell access”.
Context: ... for shell access This driver provides a simple shell access to the target exporter, and it is inten...(A_UNCOUNTABLE)
|
@coderabbitai full review |
|
@mangelajo i'll start a fresh review of the changes and generate a new summary for the pr. ✅ Actions performedSummary regeneration triggered. |
mangelajo
force-pushed
the
shell-driver
branch
from
8ecaec8 to
6857c76
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
contrib/drivers/shell/README.md (1)
3-5: Fix grammar in the documentation.The phrase "a simple shell access" contains a grammar error with the indefinite article.
Apply this diff to fix the grammar:
-This driver provides a simple shell access to the target exporter, and it is +This driver provides simple shell access to the target exporter, and it is🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Uncountable nouns are usually not used with an indefinite article. Use simply “simple shell access”.
Context: ... for shell access This driver provides a simple shell access to the target exporter, and it is inten...(A_UNCOUNTABLE)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (1)
16-19: Add type annotation for methods field default.The
methodsfield lacks a default value annotation. Consider adding a default empty dictionary to maintain consistency with other fields.Apply this diff:
- methods: dict[str, str] + methods: dict[str, str] = field(default_factory=dict)
📜 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 (9)
contrib/drivers/shell/.gitignore(1 hunks)contrib/drivers/shell/README.md(1 hunks)contrib/drivers/shell/examples/exporter.yaml(1 hunks)contrib/drivers/shell/jumpstarter_driver_shell/client.py(1 hunks)contrib/drivers/shell/jumpstarter_driver_shell/driver.py(1 hunks)contrib/drivers/shell/jumpstarter_driver_shell/driver_test.py(1 hunks)contrib/drivers/shell/pyproject.toml(1 hunks)docs/source/api-reference/drivers/index.md(1 hunks)docs/source/api-reference/drivers/shell.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- contrib/drivers/shell/.gitignore
- docs/source/api-reference/drivers/index.md
- contrib/drivers/shell/examples/exporter.yaml
- docs/source/api-reference/drivers/shell.md
- contrib/drivers/shell/jumpstarter_driver_shell/client.py
- contrib/drivers/shell/pyproject.toml
- contrib/drivers/shell/jumpstarter_driver_shell/driver_test.py
🧰 Additional context used
🪛 LanguageTool
contrib/drivers/shell/README.md
[grammar] ~3-~3: Uncountable nouns are usually not used with an indefinite article. Use simply “simple shell access”.
Context: ... for shell access This driver provides a simple shell access to the target exporter, and it is inten...
(A_UNCOUNTABLE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (3)
12-12: Enhance class docstring with more details.The current docstring is too brief. Consider adding more details about the driver's purpose, configuration options, and usage examples.
- """shell driver for Jumpstarter""" + """Shell driver for Jumpstarter framework. + + This driver enables execution of shell commands through Jumpstarter by mapping + method names to shell scripts. It supports configurable shell interpreter, + timeout, working directory, and logging levels. + + Configuration example: + shell: + driver: jumpstarter_driver_shell.driver.Shell + methods: + list_files: ls -la + shell: ["bash", "-c"] + timeout: 300 + cwd: /path/to/working/dir + log_level: INFO + """
37-51: Enhance error handling in call_method.While the method handles basic errors, it could benefit from more specific exception handling and detailed error messages.
@export def call_method(self, method: str, env, *args): logger.info(f"calling {method} with args: {args} and kwargs as env: {env}") if method not in self.methods: raise ValueError(f"Method '{method}' not found in available methods: {list(self.methods.keys())}") script = self.methods[method] logger.debug(f"running script: {script}") - result = self._run_inline_shell_script(method, script, *args, env_vars=env) - if result.returncode != 0: - logger.info(f"{method} return code: {result.returncode}") - if result.stderr != "": - logger.debug(f"{method} stderr:\n{result.stderr.rstrip("\n")}") - if result.stdout != "": - logger.debug(f"{method} stdout:\n{result.stdout.rstrip("\n")}") - return result.stdout, result.stderr, result.returncode + try: + result = self._run_inline_shell_script(method, script, *args, env_vars=env) + if result.returncode != 0: + logger.warning(f"{method} failed with return code: {result.returncode}") + if result.stderr: + logger.debug(f"{method} stderr:\n{result.stderr.rstrip()}") + if result.stdout: + logger.debug(f"{method} stdout:\n{result.stdout.rstrip()}") + return result.stdout, result.stderr, result.returncode + except subprocess.TimeoutExpired as e: + logger.error(f"{method} timed out after {self.timeout} seconds") + raise + except subprocess.SubprocessError as e: + logger.error(f"{method} failed with error: {str(e)}") + raise
71-84: Strengthen security measures for shell command execution.While input validation is present, additional security measures could be beneficial.
if not isinstance(script, str) or not script.strip(): raise ValueError("Shell script must be a non-empty string") + # Sanitize environment variables to prevent injection + if env_vars: + env_vars = {str(k): str(v) for k, v in env_vars.items()} + + # Validate arguments + args = [str(arg) for arg in args] # Ensure all args are strings + cmd = self.shell + [script, method] + list(args) # Run the command result = subprocess.run( cmd, capture_output=True, # Captures stdout and stderr text=True, # Returns stdout/stderr as strings (not bytes) env=combined_env, # Pass our merged environment cwd=self.cwd, # Run in the working directory (if set) - timeout=self.timeout, + timeout=self.timeout, + shell=False, # Explicitly disable shell=True for security )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py(1 hunks)
🧰 Additional context used
📓 Learnings (1)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (1)
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#200
File: contrib/drivers/shell/jumpstarter_driver_shell/driver.py:52-63
Timestamp: 2025-01-16T22:26:03.037Z
Learning: Functions prefixed with underscore (e.g., `_run_inline_shell_script`) are internal implementation details and don't require extensive public API documentation.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (2)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (2)
22-30: LGTM! Clean initialization and client implementation.The initialization properly calls the parent class and sets up logging. The client method follows the expected pattern.
1-86: Consider adding comprehensive test coverage.While the implementation is solid, ensure comprehensive test coverage for:
- Edge cases in argument handling
- Environment variable processing
- Timeout scenarios
- Error conditions
- Security-related scenarios
Would you like me to help generate a test suite for these scenarios?
|
Handled the timeout exception, but documentation is already handled as an .md file in the documentation directory. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (1)
78-78: Improve command construction safety.The command construction could be improved to handle special characters in method names and arguments.
Apply this diff:
- cmd = self.shell + [script, method] + list(args) + # Ensure all arguments are strings to prevent injection + cmd = self.shell + [script, str(method)] + [str(arg) for arg in args]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py(1 hunks)
🧰 Additional context used
📓 Learnings (1)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (1)
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#200
File: contrib/drivers/shell/jumpstarter_driver_shell/driver.py:52-63
Timestamp: 2025-01-16T22:26:03.037Z
Learning: Functions prefixed with underscore (e.g., `_run_inline_shell_script`) are internal implementation details and don't require extensive public API documentation.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (4)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (4)
22-30: LGTM!The initialization and client configuration look good. The logger setup is appropriate, and the client path is well-structured.
31-35: LGTM!The get_methods implementation is clean and includes helpful debug logging.
16-20: 🛠️ Refactor suggestionDocument security implications in class docstring.
The class allows arbitrary shell command execution. The docstring should document security implications and usage guidelines.
Add security notes to the class docstring:
class Shell(Driver): - """shell driver for Jumpstarter""" + """Shell driver for Jumpstarter. + + This driver enables execution of shell commands defined in the methods dictionary. + + Security Considerations: + - Methods dictionary should only contain trusted shell scripts + - Shell commands have full system access - use with caution + - Consider using more restrictive shells than bash for better security + - Validate and sanitize all input parameters and environment variables + + :param methods: Dictionary mapping method names to shell scripts + :param shell: Shell command and arguments (default: ["bash", "-c"]) + :param timeout: Maximum execution time in seconds (default: 300) + :param log_level: Logging level (default: "INFO") + :param cwd: Working directory for command execution (optional) + """Likely invalid or redundant comment.
17-17: Consider shell availability across environments.The default shell is set to
["bash", "-c"], but bash might not be available in all environments. Consider falling back to a more universally available shell likesh.Let's check the shell availability in the codebase and tests:
✅ Verification successful
Default shell configuration is appropriate
The current implementation using
["bash", "-c"]as default is acceptable because:
- No bash-specific features are used in command execution
- Commands are passed via the standard
-cflag which is compatible with other shells- Users can override the shell configuration if needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any shell-specific features being used rg -l 'bash' . # Check for any shell-specific syntax in the methods ast-grep --pattern 'methods: dict[str, str] = $_'Length of output: 1037
Script:
#!/bin/bash # Check how shell commands are executed and if there's any bash-specific syntax rg -A 5 "subprocess\.(?:run|Popen)" . # Look for shell-specific features or fallback mechanisms rg -A 3 "shell.*=.*\[" .Length of output: 2106
|
@NickCao if you have time, give me a hand to review this one, we'd like to ship it to next version. |
mangelajo
force-pushed
the
shell-driver
branch
from
1541bc1 to
fdbf82d
Compare
|
Just rebased to fix the uv.lock conflict, I wonder if there is a better way to handle uv.lock |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (1)
37-56: 🛠️ Refactor suggestionEnhance error handling for system-level exceptions.
While timeout handling is implemented, other system-level exceptions (PermissionError, FileNotFoundError, etc.) should be handled similarly.
Note: This was previously suggested in a past review. Please refer to the existing comment for the implementation details.
🧹 Nitpick comments (2)
contrib/drivers/shell/README.md (1)
3-5: Fix grammar in the introduction.Remove the article "a" before "simple shell access" for better grammar.
-This driver provides a simple shell access to the target exporter, and it is +This driver provides simple shell access to the target exporter, and it is🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Uncountable nouns are usually not used with an indefinite article. Use simply “simple shell access”.
Context: ... for shell access This driver provides a simple shell access to the target exporter, and it is inten...(A_UNCOUNTABLE)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (1)
17-18: Document shell command structure and validate timeout.
- The shell command structure could be clearer with documentation.
- The timeout should be validated to prevent negative values.
- shell: list[str] = field(default_factory=lambda: ["bash", "-c"]) - timeout: int = 300 + shell: list[str] = field( + default_factory=lambda: ["bash", "-c"], + metadata={"help": "Shell command as list. E.g., ['bash', '-c'] runs commands via bash"} + ) + timeout: int = field( + default=300, + metadata={"help": "Command timeout in seconds"}, + repr=True + ) + + def __post_init__(self): + super().__post_init__() + if self.timeout <= 0: + raise ValueError("timeout must be positive") + logger.setLevel(self.log_level)
📜 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 (9)
contrib/drivers/shell/.gitignore(1 hunks)contrib/drivers/shell/README.md(1 hunks)contrib/drivers/shell/examples/exporter.yaml(1 hunks)contrib/drivers/shell/jumpstarter_driver_shell/client.py(1 hunks)contrib/drivers/shell/jumpstarter_driver_shell/driver.py(1 hunks)contrib/drivers/shell/jumpstarter_driver_shell/driver_test.py(1 hunks)contrib/drivers/shell/pyproject.toml(1 hunks)docs/source/api-reference/drivers/index.md(1 hunks)docs/source/api-reference/drivers/shell.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/source/api-reference/drivers/index.md
- docs/source/api-reference/drivers/shell.md
- contrib/drivers/shell/.gitignore
- contrib/drivers/shell/examples/exporter.yaml
- contrib/drivers/shell/jumpstarter_driver_shell/client.py
- contrib/drivers/shell/jumpstarter_driver_shell/driver_test.py
- contrib/drivers/shell/pyproject.toml
🧰 Additional context used
📓 Learnings (1)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (1)
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#200
File: contrib/drivers/shell/jumpstarter_driver_shell/driver.py:52-63
Timestamp: 2025-01-16T22:26:03.037Z
Learning: Functions prefixed with underscore (e.g., `_run_inline_shell_script`) are internal implementation details and don't require extensive public API documentation.
🪛 LanguageTool
contrib/drivers/shell/README.md
[grammar] ~3-~3: Uncountable nouns are usually not used with an indefinite article. Use simply “simple shell access”.
Context: ... for shell access This driver provides a simple shell access to the target exporter, and it is inten...
(A_UNCOUNTABLE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (2)
contrib/drivers/shell/jumpstarter_driver_shell/driver.py (2)
31-36: LGTM!The implementation is clean and includes appropriate debug logging.
57-90: LGTM!The implementation includes proper:
- Input validation
- Environment variable handling
- Subprocess configuration
- Timeout handling
contrib/drivers/shell/.gitignore
Outdated
There was a problem hiding this comment.
The toplevel .gitignore already covers these.
|
I need to rebase and cleanup, we have the logging part generalized now. |
mangelajo
force-pushed
the
shell-driver
branch
2 times, most recently
from
c2988b1 to
ad0167e
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Nitpick comments (10)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (5)
38-38: Add type annotation for theenvparameterConsider adding a type annotation for the
envparameter in thecall_methodfunction to improve code clarity and enable type checking.Apply this diff to add the type annotation:
- def call_method(self, method: str, env, *args): + def call_method(self, method: str, env: dict[str, str], *args):
46-47: Log warnings for non-zero return codesConsider logging a warning instead of info when the return code is non-zero to highlight potential issues during script execution.
Apply this diff:
- logger.info(f"{method} return code: {result.returncode}") + logger.warning(f"{method} returned non-zero exit code: {result.returncode}")
48-51: Simplify checks for stdout and stderrInstead of comparing
result.stderrandresult.stdoutwith empty strings, you can simplify the conditionals:Apply this diff to simplify the conditions:
- if result.stderr != "": + if result.stderr:Apply the same change for
result.stdout.
54-55: Consider handling additional subprocess exceptionsCurrently, only
subprocess.TimeoutExpiredis caught. To make the code more robust, consider handling additional exceptions such assubprocess.CalledProcessErroror generalExceptionto catch unforeseen errors during script execution.
10-13: Position class docstring immediately after class definitionAccording to PEP 257, the class docstring should be placed immediately after the class definition line. This ensures that the documentation is correctly associated with the class.
Apply this diff to move the docstring:
@dataclass(kw_only=True) class Shell(Driver): - """shell driver for Jumpstarter""" + """shell driver for Jumpstarter"""packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (2)
8-8: Adjust type annotation for_methodsAssigning
Noneto_methodsannotated aslist[str]may cause type checking issues. Consider usingOptional[list[str]]or initializing it as an empty list.Apply this diff:
-from dataclasses import dataclass +from dataclasses import dataclass +from typing import Optional @dataclass(kw_only=True) class ShellClient(DriverClient): - _methods: list[str] = None + _methods: Optional[list[str]] = None
10-16: Move class docstring immediately after class definitionThe class docstring should be placed immediately after the class definition for proper documentation association.
Apply this diff to reposition the docstring:
class ShellClient(DriverClient): - _methods: list[str] = None - - """ + """ Client interface for Shell driver. This client dynamically checks that the method is configuredpackages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (1)
35-41: Simplify exception testing usingpytest.raisesYou can simplify the exception test by using
pytest.raises, which makes the test more readable and idiomatic.Apply this diff:
def test_unknown_method(client): - try: - client.unknown() - except AttributeError as e: - assert "method unknown not found in" in str(e) - else: - raise AssertionError("Expected AttributeError") + with pytest.raises(AttributeError) as e: + client.unknown() + assert "method unknown not found in" in str(e.value)packages/jumpstarter-driver-shell/examples/exporter.yaml (1)
8-20: Shell Command Methods and Optional 'cwd' Configuration
Theexportsection defines the shell command methods clearly. Each method (e.g.ls,method2,method3) is correctly mapped to its shell command. One suggestion: if you intend to leverage the "CWD" (current working directory) capability—as discussed in the PR objectives—consider adding an optionalcwdkey within the configuration block. For example, adding:cwd: "/desired/directory"would allow users to specify the working directory for command execution. If this functionality is implemented at a higher level or defaults to a sensible working directory, ensure that it is clearly documented.
packages/jumpstarter-driver-shell/README.md (1)
1-5: Grammar and Documentation Enhancement
In the first paragraph, the phrase "a simple shell access" uses an indefinite article with an uncountable noun. Consider changing it to "simple shell access" to improve the clarity of the documentation. For example:-This driver provides a simple shell access to the target exporter, and it is +This driver provides simple shell access to the target exporter, and it isAdditionally, it may be helpful to include a brief note about the new CWD option and how users can configure it for command execution.
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Uncountable nouns are usually not used with an indefinite article. Use simply “simple shell access”.
Context: ... for shell access This driver provides a simple shell access to the target exporter, and it is inten...(A_UNCOUNTABLE)
🛑 Comments failed to post (3)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (2)
25-25:
⚠️ Potential issueFix logger level setting to use integer level
Setting the logger level using a string may not work as expected. The
logger.setLevel()method expects an integer level, not a string. You should convert the string level to the corresponding integer level.Apply this diff to fix the issue:
+ level = getattr(logging, self.log_level.upper(), None) + if not isinstance(level, int): + raise ValueError(f"Invalid log level: {self.log_level}") + logger.setLevel(level) - logger.setLevel(self.log_level)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.level = getattr(logging, self.log_level.upper(), None) if not isinstance(level, int): raise ValueError(f"Invalid log level: {self.log_level}") logger.setLevel(level)
78-78: 💡 Verification agent
🧩 Analysis chain
Verify necessity of passing method name to the shell script
In the command construction, you include the method name as an argument to the shell script:
cmd = self.shell + [script, method] + list(args)Please verify if passing
methodto the script is intentional and necessary.
I'll start by gathering additional context from the file to better understand how the
methodparameter is used. In particular, I'll check the surrounding lines inpackages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.pyaround line 78 to see whether themethodargument is indeed needed.
🏁 Script executed:
#!/bin/bash # Display context from line 70 to 90 to see how `method` is used sed -n '70,90p' packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.pyLength of output: 871
I'll run a shell script to search for the usage of the first command-line argument (
$1) in any shell scripts in the repository to see if the passedmethodparameter is consumed by those scripts.
🏁 Script executed:
#!/bin/bash rg "\$1" -g "*.sh"Length of output: 19
I'll run additional searches to check if any shell scripts in the repository reference the passed
methodargument (for example by checking for patterns like$1or the literal stringmethod) to understand if it's used downstream from the command construction.
🏁 Script executed:
#!/bin/bash # Search for "$1" in all .sh files to see if the first argument (possibly our method) is used. rg "\$1" -g "*.sh" # Also search for the literal word "method" in .sh files. rg "method" -g "*.sh"Length of output: 41
Based on the searches so far, no shell scripts in the repository reference the passed method value (e.g. via
$1or$2). This suggests one of two possibilities:
- Either the shell scripts currently don’t use the method name, making it extraneous.
- Or the method is reserved for future use or used in custom scripts external to the repository that expect that argument.
Without further context about external expectations or documentation describing this behavior, it appears likely that passing the method name isn’t currently necessary.
Actionable feedback:
• Consider removing themethodargument from the command construction if it’s not consumed.
• Alternatively, document the intended use ofmethodif it is meant for future extensibility or external scripts.packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py (1)
1-5: 🛠️ Refactor suggestion
Fix import formatting and sorting
The import block is un-sorted or un-formatted, as indicated by the static analysis tool Ruff (I001). Please sort and format the imports according to PEP 8 guidelines by grouping standard library imports, third-party imports, and local imports.
Apply this diff to fix the import block:
-import pytest - -from jumpstarter.common.utils import serve - -from .driver import Shell - +import pytest + +from jumpstarter.common.utils import serve +from .driver import Shell📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import pytest from jumpstarter.common.utils import serve from .driver import Shell🧰 Tools
🪛 GitHub Check: ruff
[failure] 1-8: Ruff (I001)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py:1:1: I001 Import block is un-sorted or un-formatted🪛 GitHub Actions: Lint
[error] 1-1: Import block is un-sorted or un-formatted.
This drivers enables shell methods on exporters, so local tools can be used remotelly when no specific drivers are available.
mangelajo
force-pushed
the
shell-driver
branch
from
ad0167e to
f79d54f
Compare
|
Ok, updated to address comments, as well as the new self.logger interface :) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (2)
8-18: Enhance class documentation with attribute descriptions.The class structure is well-organized, but the docstring could be more descriptive. Consider adding detailed descriptions for each attribute including their purpose and expected values.
- """shell driver for Jumpstarter""" + """Shell driver for Jumpstarter. + + Attributes: + methods: Dictionary mapping method names to shell scripts + shell: Shell command and flags to use for execution (default: ["bash", "-c"]) + timeout: Maximum execution time in seconds (default: 300) + cwd: Working directory for command execution (optional) + """
29-48: Improve error handling and logging security.A few suggestions for the
call_methodimplementation:
- Document the special return code 199 for timeout errors
- Use string formatting that's more resistant to injection in logs
- Consider adding more specific error types
@export def call_method(self, method: str, env, *args): - self.logger.info(f"calling {method} with args: {args} and kwargs as env: {env}") + self.logger.info("calling %r with args: %r and kwargs as env: %r", method, args, env) if method not in self.methods: raise ValueError(f"Method '{method}' not found in available methods: {list(self.methods.keys())}") script = self.methods[method] - self.logger.debug(f"running script: {script}") + self.logger.debug("running script: %r", script) try: result = self._run_inline_shell_script(method, script, *args, env_vars=env) if result.returncode != 0: - self.logger.info(f"{method} return code: {result.returncode}") + self.logger.info("%r return code: %d", method, result.returncode) if result.stderr != "": - self.logger.debug(f"{method} stderr:\n{result.stderr.rstrip("\n")}") + self.logger.debug("%r stderr:\n%s", method, result.stderr.rstrip("\n")) if result.stdout != "": - self.logger.debug(f"{method} stdout:\n{result.stdout.rstrip("\n")}") + self.logger.debug("%r stdout:\n%s", method, result.stdout.rstrip("\n")) return result.stdout, result.stderr, result.returncode except subprocess.TimeoutExpired as e: - self.logger.error(f"Timeout expired while running {method}: {e}") + # Return code 199 indicates a timeout error + self.logger.error("Timeout expired while running %r: %s", method, e) return "", f"Timeout expired while running {method}: {e}", 199packages/jumpstarter-driver-shell/README.md (1)
3-5: Fix grammar in the introduction.The sentence structure needs improvement.
-This driver provides a simple shell access to the target exporter, and it is -intended to be used when command line tools exist to manage existing interfaces -or hardware, but no drivers exist yet in Jumpstarter. +This driver provides simple shell access to the target exporter. It is +intended for use when command line tools exist to manage interfaces +or hardware, but no corresponding drivers exist yet in Jumpstarter.🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Uncountable nouns are usually not used with an indefinite article. Use simply “simple shell access”.
Context: ... for shell access This driver provides a simple shell access to the target exporter, and it is inten...(A_UNCOUNTABLE)
📜 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 (8)
docs/source/api-reference/drivers/index.md(1 hunks)docs/source/api-reference/drivers/shell.md(1 hunks)packages/jumpstarter-driver-shell/README.md(1 hunks)packages/jumpstarter-driver-shell/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(1 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py(1 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py(1 hunks)packages/jumpstarter-driver-shell/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/source/api-reference/drivers/index.md
- packages/jumpstarter-driver-shell/pyproject.toml
- packages/jumpstarter-driver-shell/examples/exporter.yaml
- packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py
- packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py
🧰 Additional context used
🪛 LanguageTool
packages/jumpstarter-driver-shell/README.md
[grammar] ~3-~3: Uncountable nouns are usually not used with an indefinite article. Use simply “simple shell access”.
Context: ... for shell access This driver provides a simple shell access to the target exporter, and it is inten...
(A_UNCOUNTABLE)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (2)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (1)
8-83: Add security documentation and consider additional safeguards.The shell driver executes arbitrary commands, which poses security risks. Consider:
- Adding security warnings in documentation
- Implementing command allowlisting
- Adding support for command argument templates
docs/source/api-reference/drivers/shell.md (1)
1-59: Enhance documentation with security guidelines and more examples.The documentation is well-structured but could benefit from:
- Security considerations and best practices
- More complex examples showing error handling
- Documentation for the timeout error code (199)
Add a new section after the configuration example:
## Security Considerations When using the shell driver, consider the following security best practices: 1. Avoid executing user-provided commands directly 2. Use absolute paths for binaries when possible 3. Validate and sanitize all inputs 4. Restrict environment variables to those needed 5. Consider using command templates instead of direct command strings ## Error Handling The shell driver uses specific return codes: - 199: Command execution timeout - Other non-zero codes: Command-specific errors Example with error handling: ```yaml methods: safe_command: | if ! command -v myapp >/dev/null 2>&1; then echo "Required command not found" >&2 exit 1 fi myapp "$@"</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (1)
1-96:🧩 Analysis chain
Verify test coverage for error scenarios.
Please ensure the test suite includes coverage for:
- Timeout scenarios
- Invalid environment variables
- Resource limit exceeded scenarios
- Working directory validation
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for error scenarios # Search for test cases covering error scenarios rg -A 5 "def test.*(?:timeout|invalid|error|fail)" packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.pyLength of output: 126
Let's gather more data to confirm what error scenario tests (if any) are present. I'll run a script to list all test function definitions in
driver_test.pyso we can verify if tests exist for handling timeouts, invalid environment variables, working directory issues, etc.
🏁 Script executed:
#!/bin/bash # List all test functions in the driver_test.py file rg -n "def test_" packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.pyLength of output: 294
Test suite missing several critical error scenario cases.
The current test file only covers basic scenarios such as normal arguments, environment variables (likely valid cases), multi-line scripts, return codes, stderr, and unknown methods. There are no tests verifying:
- Timeout handling (triggering a subprocess.TimeoutExpired),
- Validation of invalid environment variable names (ensuring ValueError is raised),
- Resource limit exceeded scenarios,
- Working directory validation (ensuring that an invalid/non-existent working directory causes an error).
Please add tests for these scenarios to ensure robust error handling.
🧹 Nitpick comments (3)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (3)
8-17: Add runtime type validation and document timeout value.Consider adding runtime type validation for the
methodsdictionary and document the rationale for the 300-second timeout value.@dataclass(kw_only=True) class Shell(Driver): """shell driver for Jumpstarter""" # methods field is used to define the methods exported, and the shell script # to be executed by each method methods: dict[str, str] + def __post_init__(self): + if not isinstance(self.methods, dict): + raise TypeError("methods must be a dictionary") + if not all(isinstance(k, str) and isinstance(v, str) for k, v in self.methods.items()): + raise TypeError("methods dictionary must contain only string keys and values") shell: list[str] = field(default_factory=lambda: ["bash", "-c"]) - timeout: int = 300 + # Default timeout of 300 seconds (5 minutes) for shell command execution + timeout: int = 300 cwd: str | None = None
29-48: Add type hints and improve documentation.The implementation would benefit from:
- Type hints for
envandargsparameters- Documentation of the return tuple structure
- More specific timeout error message
@export -def call_method(self, method: str, env, *args): +def call_method(self, method: str, env: dict[str, str], *args: str) -> tuple[str, str, int]: + """Execute a shell method with the given environment variables and arguments. + + Args: + method: The name of the method to execute + env: Dictionary of environment variables + args: Additional arguments to pass to the shell script + + Returns: + tuple: (stdout: str, stderr: str, return_code: int) + + Raises: + ValueError: If the method is not found + subprocess.TimeoutExpired: If execution exceeds {self.timeout} seconds + """ self.logger.info(f"calling {method} with args: {args} and kwargs as env: {env}") if method not in self.methods: raise ValueError(f"Method '{method}' not found in available methods: {list(self.methods.keys())}") script = self.methods[method] self.logger.debug(f"running script: {script}") try: result = self._run_inline_shell_script(method, script, *args, env_vars=env) if result.returncode != 0: self.logger.info(f"{method} return code: {result.returncode}") if result.stderr != "": self.logger.debug(f"{method} stderr:\n{result.stderr.rstrip("\n")}") if result.stdout != "": self.logger.debug(f"{method} stdout:\n{result.stdout.rstrip("\n")}") return result.stdout, result.stderr, result.returncode except subprocess.TimeoutExpired as e: self.logger.error(f"Timeout expired while running {method}: {e}") - return "", f"Timeout expired while running {method}: {e}", 199 + return "", f"Command timed out after {self.timeout} seconds while running {method}: {e}", 199
49-96: Enhance security measures for shell command execution.While the input validation is good, consider adding these security measures:
- Set a restrictive umask
- Drop unnecessary capabilities
- Set resource limits
def _run_inline_shell_script(self, method, script, *args, env_vars=None): """ Run the given shell script (as a string) with optional arguments and environment variables. Returns a CompletedProcess with stdout, stderr, and returncode. :param script: The shell script contents as a string. :param args: Arguments to pass to the script (mapped to $1, $2, etc. in the script). :param env_vars: A dict of environment variables to make available to the script. :return: A subprocess.CompletedProcess object (Python 3.5+). """ + # Set a restrictive umask for any files created by the script + old_umask = os.umask(0o077) + # Merge parent environment with the user-supplied env_vars # so that we don't lose existing environment variables. combined_env = os.environ.copy() if env_vars: # Validate environment variable names for key in env_vars: if not isinstance(key, str) or not key.isidentifier(): raise ValueError(f"Invalid environment variable name: {key}") combined_env.update(env_vars) if not isinstance(script, str) or not script.strip(): raise ValueError("Shell script must be a non-empty string") # Validate arguments for arg in args: if not isinstance(arg, str): raise ValueError(f"All arguments must be strings, got {type(arg)}") # Validate working directory if set if self.cwd and not os.path.isdir(self.cwd): raise ValueError(f"Working directory does not exist: {self.cwd}") cmd = self.shell + [script, method] + list(args) + try: + import resource + # Set resource limits + resource.setrlimit(resource.RLIMIT_NOFILE, (1024, 1024)) # Limit open files + resource.setrlimit(resource.RLIMIT_FSIZE, (100 * 1024 * 1024, 100 * 1024 * 1024)) # Limit file size to 100MB + except ImportError: + self.logger.warning("resource module not available, skipping resource limits") # Run the command try: result = subprocess.run( cmd, capture_output=True, # Captures stdout and stderr text=True, # Returns stdout/stderr as strings (not bytes) env=combined_env, # Pass our merged environment cwd=self.cwd, # Run in the working directory (if set) timeout=self.timeout, ) finally: + # Restore the original umask + os.umask(old_umask) return result
📜 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 (1)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (2)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py (2)
19-22: LGTM!The client method is well-implemented with proper type annotation.
23-28: LGTM!The get_methods implementation is clean with proper logging and type annotation.
This drivers enables shell methods on exporters, so local tools can be used remotelly when no specific drivers are available.
Summary by CodeRabbit
shell.mdin the API reference documentation.