Conversation
WalkthroughThis pull request refactors the Jumpstarter CLI by removing several deprecated client and exporter commands and consolidating the CLI interface. Key changes include the removal of the Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant ShellCmd as Shell Command
participant Config as Configuration Loader
participant Lease as Lease Manager
participant ShellEngine as Shell Launcher
User->>ShellCmd: Invoke "shell" command
ShellCmd->>Config: Load configuration
alt Client Configuration
ShellCmd->>Lease: Request lease (with selector, duration, lease name)
Lease-->>ShellCmd: Lease granted
ShellCmd->>ShellEngine: Launch shell (client mode)
else Exporter Configuration
ShellCmd->>ShellEngine: Launch shell (trusted local mode)
end
ShellEngine-->>User: Interactive shell started
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/jumpstarter-cli/jumpstarter_cli/common.py (2)
15-26: Consider enhancing error message with examples of valid formats.The
DurationParamTypeclass correctly handles bothtimedeltaobjects and string inputs. However, when validation fails, the error message could be more helpful by including examples of valid formats.- self.fail(f"{value!r} is not a valid duration", param, ctx) + self.fail( + f"{value!r} is not a valid duration. Examples of valid formats: " + f"'1d', 'P1DT2H30M', '02:30:00', '1 days, 12:00:00'", + param, + ctx + )
1-47: Consider adding docstrings for better code documentation.The code is well-structured, but adding docstrings to the class and functions would improve the code's self-documentation and make it easier for other developers to understand and use these components.
+ """Common utilities for the jumpstarter CLI.""" from datetime import timedelta from functools import partial import asyncclick as click from pydantic import TypeAdapter + """Option for filtering resources based on label queries.""" opt_selector = click.option( "-l", "--selector", help="Selector (label query) to filter on, supports '=', '==', and '!=' (e.g. -l key1=value1,key2=value2)." " Matching objects must satisfy all of the specified label constraints.", ) class DurationParamType(click.ParamType): + """Custom parameter type for handling duration inputs in various formats.""" name = "duration" def convert(self, value, param, ctx): + """ + Convert the input value to a timedelta object. + + Args: + value: Input value to convert + param: The parameter being processed + ctx: Click context + + Returns: + timedelta: Converted duration + + Raises: + click.BadParameter: If the value cannot be converted + """ if isinstance(value, timedelta): return value try: return TypeAdapter(timedelta).validate_python(value) except ValueError: self.fail(f"{value!r} is not a valid duration", param, ctx) DURATION = DurationParamType() + """ + Partial function to create a CLI option for specifying durations. + Supports ISO 8601 and various time formats. + """ opt_duration_partial = partial( click.option, "--duration", "duration", type=DURATION, help=""" Accepted duration formats: \b PnYnMnDTnHnMnS - ISO 8601 duration format HH:MM:SS - time in hours, minutes, seconds D days, HH:MM:SS - time prefixed by X days D d, HH:MM:SS - time prefixed by X d See https://docs.rs/speedate/latest/speedate/ for details """, )packages/jumpstarter-cli/jumpstarter_cli/shell.py (2)
38-41: Consider consistent exit handling for ExporterConfigV1Alpha1.The ExporterConfigV1Alpha1 case doesn't have explicit exit handling like the ClientConfigV1Alpha1 case, which could lead to inconsistent behavior.
Consider adding exit handling for consistency:
case ExporterConfigV1Alpha1(): with config.serve_unix() as path: # SAFETY: the exporter config is local thus considered trusted - launch_shell(path, "local", allow=[], unsafe=True) + exit_code = launch_shell(path, "local", allow=[], unsafe=True) + sys.exit(exit_code)
27-41: Consider handling unexpected config types.The match-case statement only handles ClientConfigV1Alpha1 and ExporterConfigV1Alpha1 config types. Consider adding a catch-all case to handle unexpected config types gracefully.
Add a default case to handle unexpected config types:
match config: case ClientConfigV1Alpha1(): # existing code... case ExporterConfigV1Alpha1(): # existing code... + case _: + raise ValueError(f"Unsupported configuration type: {type(config).__name__}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__init__.py(1 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter.py(0 hunks)packages/jumpstarter-cli/jumpstarter_cli/__init__.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/common.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/delete.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/get.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/shell.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)
💤 Files with no reviewable changes (3)
- packages/jumpstarter-cli-client/jumpstarter_cli_client/init.py
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py
🧰 Additional context used
🧬 Code Definitions (1)
packages/jumpstarter-cli/jumpstarter_cli/__init__.py (5)
packages/jumpstarter-cli/jumpstarter_cli/create.py (1) (1)
create(17-20)packages/jumpstarter-cli/jumpstarter_cli/delete.py (1) (1)
delete(9-12)packages/jumpstarter-cli/jumpstarter_cli/get.py (1) (1)
get(9-12)packages/jumpstarter-cli/jumpstarter_cli/shell.py (1) (1)
shell(22-41)packages/jumpstarter-cli/jumpstarter_cli/update.py (1) (1)
update(11-14)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
🔇 Additional comments (13)
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__init__.py (1)
8-8: Import change reflects removal of exporter_shell commandThis line has been modified to remove the import of the
exporter_shellcommand, which aligns with the PR objective of removing deprecated shell commands. The corresponding command addition line has also been removed from the file, ensuring consistency in the removal process.packages/jumpstarter-cli/jumpstarter_cli/common.py (2)
7-12: Well-defined selector option with clear documentation.The selector option is well-defined with appropriate flags and comprehensive help text that explains both the syntax and behavior of the filtering mechanism.
28-46: Well-structured duration option with comprehensive help text.The
opt_duration_partialfunction is well implemented, creating a reusable CLI option with clear documentation of the supported duration formats. Including the external documentation link is a good practice.packages/jumpstarter-cli/jumpstarter_cli/delete.py (1)
2-2: Import reorganization and decorator parameter addition looks goodThe changes include reorganizing imports and adding the parameter
exporter=Falseto theopt_configdecorator, which aligns with the PR objective of unifying the CLI.These changes indicate that the
delete_leasescommand now explicitly opts out of exporter configuration, focusing solely on client operations.Also applies to: 5-5, 16-16
packages/jumpstarter-cli/jumpstarter_cli/create.py (1)
8-8: Import reorganization and decorator parameter addition looks goodSimilar to the changes in delete.py, these modifications reorganize imports and add the
exporter=Falseparameter to theopt_configdecorator, maintaining consistency across command files.This approach clearly indicates that the
create_leasecommand doesn't require exporter configuration.Also applies to: 13-13, 24-24
packages/jumpstarter-cli/jumpstarter_cli/get.py (1)
2-2: Consistent decorator parameter addition across commandsThe changes to both the
exportersandleasescommands ensure consistency with other files by adding theexporter=Falseparameter to theopt_configdecorator.This systematic approach across all commands shows good attention to detail in the CLI unification effort.
Also applies to: 5-5, 16-16, 48-48
packages/jumpstarter-cli/jumpstarter_cli/__init__.py (1)
8-12: Well-structured addition of unified top-level commandsThe introduction of new top-level commands (
create,delete,get,shell, andupdate) implements the core of the unified CLI design. These commands are properly imported and registered with the mainjmpcommand group.The changes maintain a clear separation between the new unified commands and the existing specialized commands (
client,driver, etc.), providing a good balance between new functionality and backward compatibility.Also applies to: 20-25
packages/jumpstarter-cli/jumpstarter_cli/update.py (2)
4-7: Import organization is aligned with PR objectives.The imports have been reorganized to maintain consistency with the unified CLI approach, importing
opt_configfrom the common utilities package rather than a separate location.
18-18: Explicitly disabling exporter functionality.The decorator change from
@opt_configto@opt_config(exporter=False)makes the intended behavior more explicit, which aligns with the PR objective of unifying the CLI by removing deprecated shell commands and their associated configuration handling.packages/jumpstarter-cli/jumpstarter_cli/shell.py (4)
13-22: New shell command implementation looks good.The shell command implementation effectively combines the functionality of the removed client_shell and exporter_shell commands into a unified interface, using the pattern established in other CLI commands.
16-17: Address TODO before finalizing PR.There's a TODO comment about warning users when client-specific options are used with an exporter config. This should be implemented to provide a better user experience.
Would you like me to suggest an implementation for the warning logic?
27-37: ClientConfigV1Alpha1 case handling looks good.The implementation for handling ClientConfigV1Alpha1 configuration properly manages the lease lifecycle, serves the Unix socket, monitors the lease, and handles the exit code correctly.
40-41: Verify safety implications of unsafe=True.The code sets
unsafe=Truewith a comment indicating this is considered trusted because it's local. This deserves careful review to ensure it doesn't introduce security vulnerabilities.Could you provide more details about the security implications of setting
unsafe=Trueand what safeguards are in place to prevent potential exploitation?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (5)
13-19: Consider splitting this complex function for better maintainabilityThis function is marked with
noqa: C901indicating it's too complex according to linting standards. Consider refactoring into smaller, more focused functions to improve maintainability and testability.
31-49: Consider using a loop for similar option definitionsThe client and exporter option definitions have similar structure. This could be refactored to use a loop with a configuration object to reduce code duplication.
- if client: - options += [ - option("--client", help="Alias of client config"), - option("--client-config", type=click.Path(), help="Path to client config"), - ] - options_names += [ - "--client", - "--client-config", - ] - - if exporter: - options += [ - option("--exporter", help="Alias of exporter config"), - option("--exporter-config", type=click.Path(), help="Path of exporter config"), - ] - options_names += [ - "--exporter", - "--exporter-config", - ] + option_configs = [] + if client: + option_configs.append(("client", "client config")) + if exporter: + option_configs.append(("exporter", "exporter config")) + + for name, desc in option_configs: + options += [ + option(f"--{name}", help=f"Alias of {desc}"), + option(f"--{name}-config", type=click.Path(), help=f"Path to {desc}"), + ] + options_names += [ + f"--{name}", + f"--{name}-config", + ]
53-87: Error handling could be improved with more specific exceptionsThe function catches generic exceptions and wraps them in
click.ClickException. Consider catching more specific exceptions separately to provide more helpful error messages.Also, note that this function is marked with
noqa: C901, indicating high complexity. Consider refactoring some of the nested logic into separate functions.
89-89: Consider adding a docstring to explain thereduceusageThe use of
reducewithlambdato apply multiple options to the wrapper function is clever but may not be immediately obvious to all developers. A brief docstring explaining this pattern would improve maintainability.
92-93: Add docstring to document the default behaviorThis wrapper function provides convenient defaults, but lacks a docstring explaining what these defaults imply and when a user might want to override them.
def opt_config(*, client: bool = True, exporter: bool = True, allow_missing=False): + """ + Decorator for Click commands that need configuration options. + + By default, adds both client and exporter config options and requires + a valid configuration to be specified or available. + + Args: + client: Whether to include client config options + exporter: Whether to include exporter config options + allow_missing: Whether to allow missing configuration files + """ return partial(opt_config_inner, client=client, exporter=exporter, allow_missing=allow_missing)packages/jumpstarter-cli/jumpstarter_cli/run.py (1)
14-17: Consider using a logger instead of print for error reportingDirect printing to stderr isn't ideal for a library or application. Consider using the logging module for consistent error reporting, especially since logging is configured in the main module.
- print(f"Exception while serving on the exporter: {excgroup.exceptions}", file=sys.stderr) - for exc in excgroup.exceptions: - traceback.print_exception(type(exc), exc, exc.__traceback__, file=sys.stderr) + logging.error(f"Exception while serving on the exporter: {excgroup.exceptions}") + for exc in excgroup.exceptions: + logging.error("Exception details:", exc_info=(type(exc), exc, exc.__traceback__))packages/jumpstarter-cli/jumpstarter_cli/__init__.py (1)
23-23: Consider implementing the TODO for log level flagsCurrently, the logging level is hardcoded to INFO with a TODO comment indicating that log level flags should be added. Consider implementing command-line options for controlling the log level.
- logging.basicConfig(level=logging.INFO) # TODO: log level flags + # Set up logging based on verbosity level + logging.basicConfig(level=logging.INFO)You could also leverage the existing
opt_log_leveldecorator that's imported in the exporter file.packages/jumpstarter-cli/jumpstarter_cli/login.py (4)
9-25: Address TODO for exporter usage.You have a TODO warning if used with an exporter, but there's no explicit check or warning message. Consider adding a warning or raising an error when users try to invoke these client-specific options in an exporter context.
49-80: Consolidate repeated conditional blocks for client creation.The code checks
kind.startswith("client")twice: once for prompting about unsafe imports (lines 57–63) and again for creating aClientConfigV1Alpha1(lines 64–71). Merging them into a single block can reduce code clutter and improve readability.if kind.startswith("client"): if unsafe is None: unsafe = click.confirm("Allow unsafe driver client imports?") if unsafe is False and allow == "": allow = click.prompt( "Enter a comma-separated list of allowed driver packages (optional)", default="", type=str ) + config = ClientConfigV1Alpha1( + alias=value if kind == "client" else "default", + metadata=ObjectMeta(namespace=namespace, name=name), + endpoint=endpoint, + token="", + drivers=ClientConfigV1Alpha1Drivers(allow=allow.split(","), unsafe=unsafe), + )
86-93: Ensure OIDC exceptions are properly handled.During
token_exchange_grant,password_grant, andauthorization_code_grant, unexpected failures could occur (e.g., invalid credentials or network issues). Consider wrapping or catching these exceptions to provide more meaningful error messages.
94-104: Reduce repetition in final config save logic.Each case in the final match statement saves the config with only minimal differences in parameters. Consider centralizing the save logic in a helper function or dictionary-based dispatch to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py(0 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py(1 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__init__.py(1 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py(0 hunks)packages/jumpstarter-cli/jumpstarter_cli/__init__.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/login.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/run.py(1 hunks)packages/jumpstarter/jumpstarter/common/exceptions.py(1 hunks)
💤 Files with no reviewable changes (3)
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py
🧰 Additional context used
🧬 Code Definitions (3)
packages/jumpstarter-cli/jumpstarter_cli/__init__.py (7)
packages/jumpstarter-cli/jumpstarter_cli/create.py (1) (1)
create(17-20)packages/jumpstarter-cli/jumpstarter_cli/delete.py (1) (1)
delete(9-12)packages/jumpstarter-cli/jumpstarter_cli/get.py (1) (1)
get(9-12)packages/jumpstarter-cli/jumpstarter_cli/login.py (1) (1)
login(27-104)packages/jumpstarter-cli/jumpstarter_cli/run.py (1) (1)
run(24-27)packages/jumpstarter-cli/jumpstarter_cli/shell.py (1) (1)
shell(22-41)packages/jumpstarter-cli/jumpstarter_cli/update.py (1) (1)
update(11-14)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (1)
packages/jumpstarter/jumpstarter/common/exceptions.py (1) (1)
FileNotFoundError(53-56)
packages/jumpstarter-cli/jumpstarter_cli/login.py (2)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (1) (1)
opt_config(92-93)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__init__.py (1) (1)
exporter(12-17)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- 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)
🔇 Additional comments (9)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (3)
22-25: LGTM: Simple callback to collect parameter valuesThe callback function correctly captures non-None parameter values into the params dictionary.
52-52: LGTM: Proper function wrappingGood use of
@wraps(f)to preserve the wrapped function's metadata.
75-79:Details
❓ Verification inconclusive
Verify behavior of allow_missing with non-file configurations
When a FileNotFoundError is caught, the code assigns the parameter tuple to
configifallow_missingis True. This might lead to unexpected behavior if callers expect config to be a configuration object rather than a tuple.
🏁 Script executed:
#!/bin/bash # Find all locations where this function is called with allow_missing=True # to verify how the returned config is used echo "Searching for places where opt_config is called with allow_missing=True:" rg "opt_config.*allow_missing\s*=\s*True" -A 3 -B 1Length of output: 574
Action Required: Verify Handling of Non-File Configurations
In
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py(lines 75–79), when aFileNotFoundErroris caught andallow_missingis enabled, the function retrieves the first parameter tuple from the configuration dictionary. This behavior may be problematic if callers—such as the one inpackages/jumpstarter-cli/jumpstarter_cli/login.pywhere@opt_config(allow_missing=True)is used—expect a full configuration object rather than a simple tuple.
- Review Caller Expectations: Confirm that the downstream code properly handles a tuple instead of a configuration object.
- Potential Adjustment: If the tuple does not meet the expected interface, consider transforming it into the appropriate configuration object format before returning it.
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__init__.py (2)
5-5: Verify removal of related functionalityThe import has been updated to only include
AliasedGroupandopt_log_level, suggesting that other imports were removed. This aligns with the removal of several commands from the exporter command group.
19-20:Details
✅ Verification successful
Several commands have been removed from the exporter group
According to the AI summary, commands like
run_exporter,exporter_login,exporter_shell, andversionhave been removed. This is part of the unification of the CLI structure mentioned in the PR title.Ensure that users are informed about these changes and provided with alternatives if needed.
🏁 Script executed:
#!/bin/bash # Check if there are any references to the removed commands in documentation or tests echo "Searching for references to removed commands:" rg "run_exporter|exporter_login|exporter_shell" --type md --type pythonLength of output: 205
ACTION REQUIRED: Verify Removed Commands in Documentation and Tests
The initial verification script encountered an error with the
--type pythonflag. Please re-run the above script to confirm that there are no lingering references to removed commands (run_exporter,exporter_login, andexporter_shell) in both markdown and Python files. Once confirmed, ensure that the user documentation clearly communicates that these commands have been removed in favor of the new unified CLI structure, and provide any necessary alternatives.
🏁 Script executed:
#!/bin/bash echo "Searching for references in markdown files:" rg "run_exporter|exporter_login|exporter_shell" --type md echo "Searching for references in python files:" rg "run_exporter|exporter_login|exporter_shell" --glob "*.py"Length of output: 313
Ensure CLI documentation clearly reflects the removal of legacy commands and directs users to the new unified interface.
After verifying the codebase, no references to the removed commands (run_exporter,exporter_login,exporter_shell, andversion) were found in both markdown and Python files. As such, please:
- Confirm that the CLI help messages and user documentation are updated to communicate the removal of these commands.
- Provide guidance on the new alternatives or unified commands available to users, if not already documented.
packages/jumpstarter-cli/jumpstarter_cli/run.py (1)
21-27: Good use of the new opt_config decoratorThe command is well-structured with appropriate decorators. The
client=Falseparameter correctly indicates that only exporter configurations are expected for this command.packages/jumpstarter-cli/jumpstarter_cli/__init__.py (2)
10-16: Well-organized imports for new commandsThe imports are organized properly, importing each command function from its respective module.
26-32: Good organization of new commandsThe new command functions are properly added to the
jmpcommand group, enhancing the CLI's capabilities as described in the PR title about unifying the CLI.packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py (1)
6-6: Confirm removal of other commands and update documentation as needed.The import now references only
AliasedGroupandopt_log_level, reflecting the removal of other commands. Please ensure this streamlined interface is documented so users clearly know how to access remaining commands likeconfig.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/source/installation/python-package.md (1)
126-126: Documentation update accurately reflects CLI unification.The updated description aligns with the code changes to unify the CLI. Consider two minor style improvements:
- Remove "all of" for conciseness: "A metapackage containing the Jumpstarter CLI components..."
- Hyphenate the compound adjective: "the user-facing CLI"
-| [`jumpstarter-cli`](https://github.com/jumpstarter-dev/jumpstarter/tree/main/packages/jumpstarter-cli) | A metapackage containing all of the Jumpstarter CLI components including the cluster admin CLI `jumpstarter-cli-admin`, the user facing CLI. | +| [`jumpstarter-cli`](https://github.com/jumpstarter-dev/jumpstarter/tree/main/packages/jumpstarter-cli) | A metapackage containing the Jumpstarter CLI components including the cluster admin CLI `jumpstarter-cli-admin`, the user-facing CLI. |🧰 Tools
🪛 LanguageTool
[style] ~126-~126: Consider removing “of” to be more concise
Context: ... | A metapackage containing all of the Jumpstarter CLI components including th...(ALL_OF_THE)
[uncategorized] ~126-~126: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... admin CLIjumpstarter-cli-admin, the user facing CLI. ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
packages/jumpstarter-cli/jumpstarter_cli/config_client.py (1)
42-42: Fix typo in prompt messageThere's a typo in the prompt message: "nespace" should be "namespace".
- prompt="Enter a valid Jumpstarter client nespace", + prompt="Enter a valid Jumpstarter client namespace",
📜 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 (18)
Makefile(0 hunks)docs/source/installation/python-package.md(1 hunks)packages/jumpstarter-cli-client/README.md(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__main__.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_test.py(0 hunks)packages/jumpstarter-cli-client/pyproject.toml(0 hunks)packages/jumpstarter-cli-exporter/README.md(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__init__.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__main__.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_test.py(0 hunks)packages/jumpstarter-cli-exporter/pyproject.toml(0 hunks)packages/jumpstarter-cli/jumpstarter_cli/__init__.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/config.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_client.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py(4 hunks)packages/jumpstarter-cli/pyproject.toml(0 hunks)pyproject.toml(0 hunks)
💤 Files with no reviewable changes (13)
- packages/jumpstarter-cli-client/README.md
- packages/jumpstarter-cli-exporter/README.md
- Makefile
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/main.py
- packages/jumpstarter-cli/pyproject.toml
- packages/jumpstarter-cli-client/pyproject.toml
- packages/jumpstarter-cli-client/jumpstarter_cli_client/main.py
- pyproject.toml
- packages/jumpstarter-cli-exporter/pyproject.toml
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/init.py
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_test.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/init.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_test.py
🧰 Additional context used
🧬 Code Definitions (1)
packages/jumpstarter-cli/jumpstarter_cli/__init__.py (8)
packages/jumpstarter-cli/jumpstarter_cli/config.py (1) (1)
config(8-9)packages/jumpstarter-cli/jumpstarter_cli/create.py (1) (1)
create(17-20)packages/jumpstarter-cli/jumpstarter_cli/delete.py (1) (1)
delete(9-12)packages/jumpstarter-cli/jumpstarter_cli/get.py (1) (1)
get(9-12)packages/jumpstarter-cli/jumpstarter_cli/login.py (1) (1)
login(27-104)packages/jumpstarter-cli/jumpstarter_cli/run.py (1) (1)
run(24-27)packages/jumpstarter-cli/jumpstarter_cli/shell.py (1) (1)
shell(22-41)packages/jumpstarter-cli/jumpstarter_cli/update.py (1) (1)
update(11-14)
🪛 LanguageTool
docs/source/installation/python-package.md
[style] ~126-~126: Consider removing “of” to be more concise
Context: ... | A metapackage containing all of the Jumpstarter CLI components including th...
(ALL_OF_THE)
[uncategorized] ~126-~126: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... admin CLI jumpstarter-cli-admin, the user facing CLI. ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
🔇 Additional comments (14)
packages/jumpstarter-cli/jumpstarter_cli/config.py (1)
1-14: Clean implementation of hierarchical command structure.This is a well-structured implementation of a parent command group that unifies the client and exporter configuration commands, aligning with the PR objective of creating a unified CLI.
packages/jumpstarter-cli/jumpstarter_cli/__init__.py (3)
1-2: LGTM: Added logging import.Adding logging module is appropriate for enabling the logging configuration below.
8-16: Well-structured command imports.The command imports are organized logically, facilitating the unified CLI structure. This improves organization of the CLI components.
25-32: Good organization of commands in the unified CLI.The CLI structure has been improved by organizing commands in a more consistent way, following common CLI patterns. The adoption of standard command verbs (
create,delete,get,update) along with specialized operations makes for a more intuitive user experience.packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py (5)
16-17: LGTM: Appropriate group name and function rename.Explicitly naming the group "exporter" is a good practice, and the function rename to
config_exporterimproves clarity.
23-23: Command decorator updated correctly.Updated decorator properly references the renamed function.
51-51: Command decorator updated correctly.Updated decorator properly references the renamed function.
65-65: Command decorator updated correctly.Updated decorator properly references the renamed function.
76-76: Command decorator updated correctly.Updated decorator properly references the renamed function.
packages/jumpstarter-cli/jumpstarter_cli/config_client.py (5)
23-24: Command group renaming improves CLI organizationThe renaming of the click group function from
config()toconfig_client()and setting the explicit group name to "client" aligns well with the PR objective to unify the CLI. This change creates a clearer command hierarchy and better reflects the specific purpose of this command group.
30-30: Decorator update consistent with function renamingThe command decorator has been correctly updated to reference the renamed
config_clientfunction.
125-125: Decorator update consistent with function renamingThe command decorator has been correctly updated to reference the renamed
config_clientfunction.
137-137: Decorator update consistent with function renamingThe command decorator has been correctly updated to reference the renamed
config_clientfunction.
171-171: Decorator update consistent with function renamingThe command decorator has been correctly updated to reference the renamed
config_clientfunction.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/jumpstarter-cli/jumpstarter_cli/__init__.py (1)
23-24: 🛠️ Refactor suggestionAdd configurable log levels as noted in the TODO comment.
The code currently uses a fixed logging level with a TODO reminder to implement log level flags:
logging.basicConfig(level=logging.INFO) # TODO: log level flagsThis should be updated to support configurable log levels via command-line arguments, similar to the approach used in other CLI modules.
- logging.basicConfig(level=logging.INFO) # TODO: log level flags + # Use shared logging configuration mechanism from CLI modules + from jumpstarter_cli_common import opt_log_level + # Apply log level configuration from command arguments
🧹 Nitpick comments (3)
packages/jumpstarter-cli/jumpstarter_cli/j.py (3)
9-20: Add documentation to the entry point function.This function appears to be the main entry point for the unified CLI command. Adding a docstring would help other developers understand its purpose and usage.
def j(): + """ + Main entry point for the Jumpstarter CLI. + + Creates a client instance using the env context manager and executes + the CLI commands, handling exceptions appropriately. + """ with env() as client:
10-15: Consider the nested function pattern.The nested function pattern works but might be worth evaluating. If
cli()needs to be separately testable or reusable, consider making it a standalone function.
16-20: Consider adding a return value for the success case.The function exits with status code 1 on error, but doesn't have an explicit return for the success case. Consider adding
return 0at the end for symmetry and clarity.try: cli() except click.ClickException as e: e.show() sys.exit(1) + return 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-cli/jumpstarter_cli/__init__.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/j.py(1 hunks)packages/jumpstarter-cli/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-cli/pyproject.toml
🧰 Additional context used
🧬 Code Definitions (1)
packages/jumpstarter-cli/jumpstarter_cli/__init__.py (9)
packages/jumpstarter-cli/jumpstarter_cli/config.py (1) (1)
config(8-9)packages/jumpstarter-cli/jumpstarter_cli/create.py (1) (1)
create(17-20)packages/jumpstarter-cli/jumpstarter_cli/delete.py (1) (1)
delete(9-12)packages/jumpstarter-cli/jumpstarter_cli/get.py (1) (1)
get(9-12)packages/jumpstarter-cli/jumpstarter_cli/j.py (1) (1)
j(9-20)packages/jumpstarter-cli/jumpstarter_cli/login.py (1) (1)
login(27-104)packages/jumpstarter-cli/jumpstarter_cli/run.py (1) (1)
run(24-27)packages/jumpstarter-cli/jumpstarter_cli/shell.py (1) (1)
shell(22-41)packages/jumpstarter-cli/jumpstarter_cli/update.py (1) (1)
update(11-14)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- 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 Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
🔇 Additional comments (4)
packages/jumpstarter-cli/jumpstarter_cli/j.py (2)
1-7: Imports are well-organized and appropriate.The imports are logically grouped with standard library imports first, followed by third-party libraries and local modules. The necessary dependencies for CLI functionality and exception handling are correctly included.
1-21: Overall implementation is clean and follows the unification goal.The implementation effectively creates a unified entry point for the CLI tool, handling exceptions at appropriate levels. This aligns well with the PR objective of creating a unified CLI.
packages/jumpstarter-cli/jumpstarter_cli/__init__.py (2)
8-16: LGTM! Well-structured CLI command organization.The CLI commands have been properly organized and registered with the main command group. The imports are clean and well-structured, and each command is clearly defined with appropriate separation of concerns.
Also applies to: 26-33
39-40: LGTM! Appropriate export declaration.The
__all__declaration properly defines the public API of this module, exporting only the necessary components.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/jumpstarter-cli/jumpstarter_cli/cli_test.py (1)
11-23: Improved test structure with better maintainability.The refactored test appropriately reflects the unified CLI structure by checking for the new set of action-focused commands. Using a loop instead of individual assertions is more maintainable and makes it easier to update when commands change.
One suggestion for further improvement: consider adding a test that verifies not just the presence of these commands, but also checks that they respond correctly to the
--helpflag to ensure their help text is properly configured.docs/source/installation/python-package.md (1)
126-126: Refinejumpstarter-cliDescription for ClarityThe updated table row for
jumpstarter-cliappropriately emphasizes that it now bundles both the admin and user-facing CLI components. For further clarity and to conform with the static analysis hints, consider revising the sentence to remove the extra "of" and add hyphenation for compound adjectives. For example, change:-| A metapackage containing all of the Jumpstarter CLI components including the cluster admin CLI `jumpstarter-cli-admin`, the user facing CLI. +| A metapackage containing all Jumpstarter CLI components including the cluster-admin CLI `jumpstarter-cli-admin` and the user-facing CLI.This small enhancement will improve readability and conciseness.
🧰 Tools
🪛 LanguageTool
[style] ~126-~126: Consider removing “of” to be more concise
Context: ... | A metapackage containing all of the Jumpstarter CLI components including th...(ALL_OF_THE)
[uncategorized] ~126-~126: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... admin CLIjumpstarter-cli-admin, the user facing CLI. ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
packages/jumpstarter-cli/jumpstarter_cli/login.py (2)
27-40: Consider refactoring this complex functionThe function is marked with
noqa: C901to ignore complexity warnings. Consider breaking it down into smaller helper functions for better maintainability, such as:
- A function to determine configuration type
- A function to handle client-specific configurations
- A function to handle authentication methods
This would make the code easier to test and maintain.
56-71: Enhance security-related user interactionsWhen handling the
unsafeflag, the code provides a confirmation prompt, which is good. However, consider enhancing this with more explicit warnings about the security implications of enabling unsafe driver imports. Also, the current logic doesn't validate theallowlist for potential malicious inputs.if kind.startswith("client"): if unsafe is None: - unsafe = click.confirm("Allow unsafe driver client imports?") + unsafe = click.confirm( + "WARNING: Allowing unsafe driver client imports poses security risks. Are you sure?", + default=False + ) if unsafe is False and allow == "": allow = click.prompt( "Enter a comma-separated list of allowed driver packages (optional)", default="", type=str ) + # Consider validating the allow list for potential malicious inputs
📜 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 (35)
Makefile(0 hunks)docs/source/installation/python-package.md(1 hunks)packages/jumpstarter-cli-client/README.md(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__main__.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_test.py(0 hunks)packages/jumpstarter-cli-client/pyproject.toml(0 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/__init__.py(2 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py(1 hunks)packages/jumpstarter-cli-exporter/README.md(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__init__.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__main__.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_test.py(0 hunks)packages/jumpstarter-cli-exporter/pyproject.toml(0 hunks)packages/jumpstarter-cli/jumpstarter_cli/__init__.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/cli_test.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/common.py(0 hunks)packages/jumpstarter-cli/jumpstarter_cli/config.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_client.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/delete.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/get.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/j.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/login.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/run.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/shell.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)packages/jumpstarter-cli/pyproject.toml(1 hunks)packages/jumpstarter/jumpstarter/common/exceptions.py(1 hunks)pyproject.toml(0 hunks)
💤 Files with no reviewable changes (17)
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/main.py
- packages/jumpstarter-cli-exporter/README.md
- packages/jumpstarter-cli-client/README.md
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/init.py
- Makefile
- pyproject.toml
- packages/jumpstarter-cli-client/jumpstarter_cli_client/main.py
- packages/jumpstarter-cli/jumpstarter_cli/common.py
- packages/jumpstarter-cli-client/pyproject.toml
- packages/jumpstarter-cli-exporter/pyproject.toml
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/init.py
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_test.py
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_test.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/jumpstarter-cli/jumpstarter_cli/j.py
- packages/jumpstarter/jumpstarter/common/exceptions.py
- packages/jumpstarter-cli/jumpstarter_cli/config.py
- packages/jumpstarter-cli/jumpstarter_cli/delete.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/init.py
- packages/jumpstarter-cli/jumpstarter_cli/get.py
- packages/jumpstarter-cli/jumpstarter_cli/run.py
- packages/jumpstarter-cli/jumpstarter_cli/create.py
- packages/jumpstarter-cli/jumpstarter_cli/init.py
- packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py
- packages/jumpstarter-cli/jumpstarter_cli/update.py
- packages/jumpstarter-cli/pyproject.toml
- packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py
- packages/jumpstarter-cli/jumpstarter_cli/shell.py
- packages/jumpstarter-cli/jumpstarter_cli/config_client.py
🧰 Additional context used
🧬 Code Definitions (1)
packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (1) (1)
opt_config(92-93)
🪛 LanguageTool
docs/source/installation/python-package.md
[style] ~126-~126: Consider removing “of” to be more concise
Context: ... | A metapackage containing all of the Jumpstarter CLI components including th...
(ALL_OF_THE)
[uncategorized] ~126-~126: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... admin CLI jumpstarter-cli-admin, the user facing CLI. ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/jumpstarter-cli/jumpstarter_cli/login.py (3)
81-93: 🛠️ Refactor suggestionAdd error handling for OIDC operations
The OIDC authentication operations aren't wrapped in try-except blocks. These network operations could fail for various reasons (network issues, invalid credentials, etc.).
if issuer is None: issuer = click.prompt("Enter the OIDC issuer") oidc = Config(issuer=issuer, client_id=client_id) + try: if token is not None: kwargs = {"connector_id": connector_id} if connector_id is not None else {} tokens = await oidc.token_exchange_grant(token, **kwargs) elif username is not None and password is not None: tokens = await oidc.password_grant(username, password) else: tokens = await oidc.authorization_code_grant() + except Exception as e: + click.echo(f"Authentication failed: {str(e)}") + return
15-15: 🛠️ Refactor suggestionTODO implementation is missing
There's a TODO comment about warning users when client-specific options (--allow, --unsafe) are used with exporter configurations, but this hasn't been implemented yet.
Consider adding a warning message in the configuration handling section:
case (kind, value): + # Check if exporter config is being used with client options + if kind.startswith("exporter") and (unsafe is not None or allow != ""): + click.echo("Warning: Client-specific options (--allow, --unsafe) are being used with exporter configuration, which may have no effect.", err=True)
94-104: 🛠️ Refactor suggestionAdd error handling for configuration saving
The configuration saving operations aren't wrapped in try-except blocks. File operations could fail for various reasons (disk full, permission issues, etc.). Also, the
kindvariable might not be properly defined in all code paths before this match statement.config.token = tokens["access_token"] + try: match kind: case "client": ClientConfigV1Alpha1.save(config) case "client_config": ClientConfigV1Alpha1.save(config, value) case "exporter": ExporterConfigV1Alpha1.save(config) case "exporter_config": ExporterConfigV1Alpha1.save(config, value) + except Exception as e: + click.echo(f"Failed to save configuration: {str(e)}") + return + + click.echo("Login successful!")
🧹 Nitpick comments (5)
docs/source/installation/python-package.md (2)
99-100: Consider adding a comma in this sentence.-If you need access to your hardware, i.e. because you are running the `jmp` command +If you need access to your hardware, i.e. because you are running the `jmp` command,🧰 Tools
🪛 LanguageTool
[uncategorized] ~99-~99: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ecause you are running thejmpcommand or you are following the [local-only workf...(COMMA_COMPOUND_SENTENCE)
126-126: Consider making description more concise.-| [`jumpstarter-cli`](https://github.com/jumpstarter-dev/jumpstarter/tree/main/packages/jumpstarter-cli) | A metapackage containing all of the Jumpstarter CLI components including the cluster admin CLI `jumpstarter-cli-admin`, the user facing CLI. | +| [`jumpstarter-cli`](https://github.com/jumpstarter-dev/jumpstarter/tree/main/packages/jumpstarter-cli) | A metapackage containing all Jumpstarter CLI components including the cluster admin CLI `jumpstarter-cli-admin`, the user-facing CLI. |🧰 Tools
🪛 LanguageTool
[style] ~126-~126: Consider removing “of” to be more concise
Context: ... | A metapackage containing all of the Jumpstarter CLI components including th...(ALL_OF_THE)
[uncategorized] ~126-~126: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... admin CLIjumpstarter-cli-admin, the user facing CLI. ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
docs/source/cli/reference/index.md (1)
28-28: Fix grammatical error in command description.There's a grammatical error in "as a clients or exporter".
-The `jmp` CLI allows interaction with Jumpstarter as a clients or exporter. +The `jmp` CLI allows interaction with Jumpstarter as a client or exporter.packages/jumpstarter-cli/jumpstarter_cli/login.py (2)
8-8: Consider adding a detailed docstringThe function has a brief docstring but lacks detailed information about parameters, return values, and potential exceptions.
@click.command("login", short_help="Login") @click.option("-e", "--endpoint", type=str, help="Enter the Jumpstarter service endpoint.", default=None) @click.option("--namespace", type=str, help="Enter the Jumpstarter exporter namespace.", default=None) @click.option("--name", type=str, help="Enter the Jumpstarter exporter name.", default=None) @opt_oidc # client specific # TODO: warn if used with exporter @click.option( "--allow", type=str, help="A comma-separated list of driver client packages to load.", default="", ) @click.option( "--unsafe", is_flag=True, help="Should all driver client packages be allowed to load (UNSAFE!).", default=None ) # end client specific @opt_config(allow_missing=True) async def login( # noqa: C901 config, endpoint: str, namespace: str, name: str, username: str | None, password: str | None, token: str | None, issuer: str, client_id: str, connector_id: str, unsafe, allow, ): - """Login into a jumpstarter instance""" + """Login into a jumpstarter instance + + This command authenticates with a Jumpstarter instance using OIDC and updates + the configuration with the obtained access token. It supports both client and + exporter configurations. + + Args: + config: The configuration object or a tuple of (kind, value) + endpoint: The Jumpstarter service endpoint + namespace: The Jumpstarter exporter namespace + name: The Jumpstarter exporter name + username: The username for password grant authentication + password: The password for password grant authentication + token: The token for token exchange grant authentication + issuer: The OIDC issuer URL + client_id: The OIDC client ID + connector_id: The connector ID for token exchange grant + unsafe: Flag to allow unsafe driver client imports + allow: Comma-separated list of allowed driver packages + + Returns: + None + """
27-40: Consider refactoring the login function to reduce complexityThe login function is marked with
# noqa: C901, indicating it's too complex. Consider splitting it into smaller, more manageable functions.You could extract several helper functions:
- One to handle configuration type detection and prompting
- One to handle OIDC authentication
- One to save the configuration
This would make the main function simpler and easier to understand.
📜 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 (50)
.github/workflows/e2e.yaml(1 hunks)Makefile(0 hunks)docs/source/api-reference/drivers/flashers.md(1 hunks)docs/source/api-reference/drivers/yepkit.md(1 hunks)docs/source/cli/clients.md(1 hunks)docs/source/cli/reference/index.md(2 hunks)docs/source/cli/reference/jmp-client.md(0 hunks)docs/source/cli/reference/jmp-driver.md(0 hunks)docs/source/cli/reference/jmp-exporter.md(0 hunks)docs/source/cli/run-tests.md(2 hunks)docs/source/config/cli.md(5 hunks)docs/source/config/oidc.md(3 hunks)docs/source/getting-started/setup-exporter-client.md(3 hunks)docs/source/getting-started/setup-local-exporter.md(5 hunks)docs/source/installation/python-package.md(4 hunks)docs/source/introduction/exporters.md(1 hunks)packages/jumpstarter-cli-client/README.md(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__main__.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_test.py(0 hunks)packages/jumpstarter-cli-client/pyproject.toml(0 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/__init__.py(2 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py(0 hunks)packages/jumpstarter-cli-exporter/README.md(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__init__.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__main__.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_test.py(0 hunks)packages/jumpstarter-cli-exporter/pyproject.toml(0 hunks)packages/jumpstarter-cli/jumpstarter_cli/__init__.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/cli_test.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/common.py(0 hunks)packages/jumpstarter-cli/jumpstarter_cli/config.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_client.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/delete.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/get.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/j.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/login.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/run.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/shell.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)packages/jumpstarter-cli/pyproject.toml(1 hunks)packages/jumpstarter/jumpstarter/common/exceptions.py(1 hunks)pyproject.toml(0 hunks)
💤 Files with no reviewable changes (21)
- packages/jumpstarter-cli-exporter/README.md
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/main.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/main.py
- docs/source/cli/reference/jmp-exporter.md
- packages/jumpstarter-cli-client/README.md
- docs/source/cli/reference/jmp-driver.md
- packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/init.py
- Makefile
- pyproject.toml
- docs/source/cli/reference/jmp-client.md
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter.py
- packages/jumpstarter-cli-client/pyproject.toml
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/init.py
- packages/jumpstarter-cli/jumpstarter_cli/common.py
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_test.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_test.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py
- packages/jumpstarter-cli-exporter/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/jumpstarter-cli-common/jumpstarter_cli_common/init.py
- packages/jumpstarter/jumpstarter/common/exceptions.py
- packages/jumpstarter-cli/jumpstarter_cli/j.py
- packages/jumpstarter-cli/jumpstarter_cli/config.py
- packages/jumpstarter-cli/jumpstarter_cli/delete.py
- packages/jumpstarter-cli/pyproject.toml
- packages/jumpstarter-cli/jumpstarter_cli/create.py
- packages/jumpstarter-cli/jumpstarter_cli/update.py
- packages/jumpstarter-cli/jumpstarter_cli/run.py
- packages/jumpstarter-cli/jumpstarter_cli/shell.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py
- packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py
- packages/jumpstarter-cli/jumpstarter_cli/config_client.py
🧰 Additional context used
🧬 Code Definitions (2)
packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (1) (1)
opt_config(92-93)
packages/jumpstarter-cli/jumpstarter_cli/__init__.py (9)
packages/jumpstarter-cli/jumpstarter_cli/config.py (1) (1)
config(8-9)packages/jumpstarter-cli/jumpstarter_cli/create.py (1) (1)
create(17-20)packages/jumpstarter-cli/jumpstarter_cli/delete.py (1) (1)
delete(9-12)packages/jumpstarter-cli/jumpstarter_cli/get.py (1) (1)
get(9-12)packages/jumpstarter-cli/jumpstarter_cli/j.py (1) (1)
j(9-20)packages/jumpstarter-cli/jumpstarter_cli/login.py (1) (1)
login(27-104)packages/jumpstarter-cli/jumpstarter_cli/run.py (1) (1)
run(24-27)packages/jumpstarter-cli/jumpstarter_cli/shell.py (1) (1)
shell(22-41)packages/jumpstarter-cli/jumpstarter_cli/update.py (1) (1)
update(11-14)
🪛 LanguageTool
docs/source/installation/python-package.md
[uncategorized] ~99-~99: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ecause you are running the jmp command or you are following the [local-only workf...
(COMMA_COMPOUND_SENTENCE)
[style] ~126-~126: Consider removing “of” to be more concise
Context: ... | A metapackage containing all of the Jumpstarter CLI components including th...
(ALL_OF_THE)
[uncategorized] ~126-~126: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... admin CLI jumpstarter-cli-admin, the user facing CLI. ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🔇 Additional comments (36)
docs/source/api-reference/drivers/flashers.md (1)
88-90: Updated CLI Command Syntax
The command has been updated to remove the legacyclientkeyword and now uses a key–value style for the board parameter. Please verify that this new syntax is uniformly adopted across all CLI documentation.docs/source/api-reference/drivers/yepkit.md (1)
54-58: Refined CLI Access for Yepkit Driver
The CLI command now uses the--exporter-configflag instead of the legacy configuration flag. This change improves clarity and aligns with the unified CLI design. Verify that users understand the need to supply the correct exporter configuration file path..github/workflows/e2e.yaml (1)
19-23: Updated E2E Action Reference
The workflow now referencesjumpstarter-dev/jumpstarter-e2e@unified-cliinstead of the previous main branch. Ensure that this tag/version is stable and that no unintended E2E test behaviors are introduced by this change.docs/source/introduction/exporters.md (1)
68-70: Unified Command Syntax for Running an Exporter
The command example has been updated to$ jmp run --exporter myexporter, which is clearer and consistent with the new unified CLI structure. This enhances the documentation’s clarity.docs/source/getting-started/setup-local-exporter.md (4)
56-59: Updated Spawn Exporter Shell Command
The shell command now uses$ jmp shell --exporter demo. This update aligns with the unified CLI syntax change. Confirm that the removal of the old command format does not affect existing user workflows.
66-69: Consistent Exporter Shell Invocation
The revised command at this snippet reiterates the unified syntax ($ jmp shell --exporter demo) for accessing the exporter shell. Ensure consistency across all examples in this document.
105-109: Updated Command for Running Python in Exporter Shell
The command that spawns the exporter shell now correctly reflects the unified syntax ($ jmp shell --exporter demo). This update fosters clarity when users execute inline Python scripts.
147-149: Revised Command for Running a Python File within the Exporter Shell
Again, the command uses$ jmp shell --exporter demoto launch the shell before running the Python file. This uniformity in command syntax is beneficial; please verify that all associated documentation and scripts reference this change.packages/jumpstarter-cli/jumpstarter_cli/cli_test.py (1)
11-23: Improved test coverage with a more maintainable approach.This change replaces individual assertions with a loop that checks for all expected subcommands. This approach is more maintainable as it allows for easy addition or removal of subcommands in the future, and it provides a more comprehensive test of the unified CLI's functionality.
docs/source/cli/clients.md (2)
85-85: Documentation reference updated to match new CLI structure.The reference has been correctly updated to point to the new location of the client create command in the documentation, reflecting the unified CLI structure.
88-88: Command syntax updated to follow new CLI conventions.The command has been properly updated from
jmp client config createtojmp config client create, aligning with the unified CLI structure where configuration commands are grouped under theconfigsubcommand.packages/jumpstarter-cli/jumpstarter_cli/get.py (4)
2-2: Import statement updated for explicit import of utilities.The import statement has been updated to explicitly import
opt_configfrom the common package, which improves code readability and aligns with the reorganization of utilities in the common package.
5-5: Import statement updated for local utility.The import statement for
opt_selectorhas been updated to use a relative import, which is appropriate since it's a local utility in the same package.
16-16: Decorator updated to disable exporter configuration.The
opt_configdecorator has been updated to explicitly disable exporter configuration for theget_exporterscommand, which aligns with the CLI unification goal by making configuration options more consistent and explicit.
48-48: Decorator updated to disable exporter configuration.The
opt_configdecorator has been updated to explicitly disable exporter configuration for theget_leasescommand, which aligns with the CLI unification goal by making configuration options more consistent and explicit.docs/source/cli/run-tests.md (2)
32-32: Command syntax updated to follow unified CLI structure.The command has been properly updated from
jmp-exporter shell -c /exporter-config.yamltojmp shell --exporter-config /exporter-config.yaml, which:
- Consolidates the exporter functionality under the main
jmpcommand- Uses a more descriptive long-form parameter
--exporter-configinstead of the short form-cThis change improves both usability and consistency of the CLI.
48-48: Documentation reference updated for lease creation.The reference has been updated from "jmp lease request" to "jmp create lease", which follows a more consistent resource creation pattern (verb-noun) across the CLI. This matches the unified command structure where resource operations are grouped by action type.
docs/source/installation/python-package.md (4)
64-65: LGTM! Command alias reference updated correctly.
81-86: LGTM! Container command alias updated to use consolidated jmp command.The container command alias has been appropriately updated to use the unified
jmpcommand instead of separate client/exporter commands.
91-95: LGTM! Example command syntax updated correctly.The example command has been updated from
jmp-client config listtojmp config client listto match the new unified CLI structure.
111-116: LGTM! Container privileged command alias updated correctly.docs/source/cli/reference/index.md (2)
7-8: LGTM! Updated command installation information.
24-25: LGTM! Updated toctree reference to reflect unified CLI.The reference has been updated from
jmp-client.mdtojmp.mdto align with the unified CLI approach.packages/jumpstarter-cli/jumpstarter_cli/__init__.py (4)
1-16: LGTM! Imports organized properly for unified CLI.The imports have been updated to include all the necessary modules for the unified CLI approach, removing dependency on separate client and exporter CLI packages.
19-28: Great job implementing configurable log levels!You've successfully addressed the previous review comment about implementing configurable log levels by adding the
@opt_log_leveldecorator and handling the log level parameter appropriately.
30-38: LGTM! New command structure follows good design patterns.The new command structure with create, delete, update, get, etc. follows a Kubernetes-like pattern which will be familiar to many users and provides a more consistent interface.
43-43: LGTM! Updated public interface to reflect new command structure.The
__all__list has been correctly updated to include only "jmp" and "j", which are the main entry points for the unified CLI.docs/source/config/oidc.md (3)
36-44: LGTM! Client login command updated to use unified syntax.The command has been updated from
jmp client logintojmp login --clientto reflect the new unified CLI approach.
46-50: LGTM! Exporter login command and comment updated correctly.The exporter login command has been updated to use the new unified syntax, and the comment has been generalized to indicate that the options are accepted for both client and exporter login.
150-162: LGTM! Login command examples updated to reflect unified CLI.Both client and exporter login examples have been updated to use the new unified command structure with appropriate flags.
docs/source/config/cli.md (3)
11-11: Great CLI command structure simplificationThe command structure has been significantly simplified, making it more intuitive for users. The change from
jmp exporter shelltojmp shellprovides a cleaner interface.
93-94: Updated command syntax properly aligns with code changesThe new command
jmp config client create <name>correctly reflects the updated CLI structure in the codebase. This pattern is consistently applied throughout the documentation.
216-217: Command syntax aligned with implementationThe updated command
jmp run --exporter my-exporterproperly reflects the changes made to the CLI implementation, where theruncommand now accepts an--exporterflag.docs/source/getting-started/setup-exporter-client.md (3)
59-60: Command syntax updated correctlyThe modified command
jmp config exporter edit testingaligns with the CLI command structure changes in the implementation.
88-89: Command syntax updated correctlyThe modified command
jmp run --exporter testingaccurately reflects the updated CLI implementation.
132-160: Shell command update improves clarity and functionalityThe updates to the
jmp shellcommand and its options provide clearer documentation of the available features. The command now explicitly shows that it supports both--exporter-configand--client-configoptions, making it more versatile.
This comment was marked as resolved.
This comment was marked as resolved.
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
Some doc-nits but that's it :)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
docs/source/cli/reference/index.md (1)
1-3: 🛠️ Refactor suggestionReposition "Reference" Section Header
The document currently starts with the "Reference" header at the top. However, as noted in the PR feedback, the "Reference" should be placed at the end of the output to improve the logical flow of information.Suggested Change Example:
-# Reference +<!-- (Reference section moved to bottom) -->
🧹 Nitpick comments (5)
packages/jumpstarter-driver-yepkit/README.md (1)
10-12: Reference Placement and Note Clarity
The note on line 11 regarding the necessity of usingsudofor accessing raw USB interfaces is clear. However, based on prior feedback to place the "Reference" at the end of the output, please confirm that any CLI output or help text related to this command adheres to this requirement. If needed, adjust the documentation or CLI behavior accordingly to reflect the correct order as desired by the reviewers.docs/source/cli/reference/index.md (4)
7-8: Correct Typographical Error in Description
There is a typo in the word “featuers” (line 7) which should be corrected to “features” for clarity and professionalism.Suggested Change Example:
-The base jmp command contains a set of subcommands for the different featuers, those +The base jmp command contains a set of subcommands for the different features, which
18-21: Validate Toctree Entry forjmp-admin.md
The toctree block at lines 18–21 includesjmp-admin.md, which is expected to document administrative commands. Please verify that this document is updated as part of the unified CLI refactor to ensure consistency across the docs.
23-23: Refine CLI Description Wording
The sentence “ThejmpCLI allows interaction with Jumpstarter as a clients or exporter.” has a grammatical issue. Consider revising it to “ThejmpCLI allows interaction with Jumpstarter as a client or exporter.”Suggested Change Example:
-The `jmp` CLI allows interaction with Jumpstarter as a clients or exporter. +The `jmp` CLI allows interaction with Jumpstarter as a client or exporter.
25-28: Confirm Toctree Entry forjmp.mdand New Command Visibility
The second toctree block now referencesjmp.md. Please ensure this file reflects the consolidated command descriptions for the unified CLI. Additionally, verify that all new commands introduced in this PR are documented here and display as expected in the CLI output.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/source/cli/reference/index.md(1 hunks)docs/source/cli/reference/jmp-admin.md(1 hunks)docs/source/cli/reference/jmp.md(1 hunks)docs/source/config/cli.md(5 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(3 hunks)packages/jumpstarter-driver-yepkit/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/source/cli/reference/jmp.md
- docs/source/cli/reference/jmp-admin.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-cli/jumpstarter_cli/create.py
- docs/source/config/cli.md
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- 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-utils Dockerfile.utils)
🔇 Additional comments (1)
packages/jumpstarter-driver-yepkit/README.md (1)
7-9: Unify Command Syntax in Documentation
The updated command now usesjmp shell --exporter-config ...which aligns with the unified CLI changes by removing the redundant "exporter" term and updating the flag from--configto--exporter-config. Please verify that this modified syntax is consistent with the unified CLI behavior and that users will see the new commands correctly when they run the CLI.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
docs/source/cli/reference/index.md (1)
1-4: 🛠️ Refactor suggestionRelocate the Reference Section
The “Reference” header is currently at the top of the document, but feedback indicated that it should appear at the end of the output. Please consider repositioning this section to the document’s end to align with user expectations.
🧹 Nitpick comments (9)
docs/source/cli/reference/index.md (2)
7-8: Clarify Command Description and Fix Typo
The modified line now reads:can also be installed and used independently as `jmp-admin` and `jmp`.Ensure that this description clearly reflects the intended usage of both commands. Also, note that in the preceding text “featuers” is a typo; it should be “features.”
23-24: Correct Grammar in CLI Description
The sentence:The `jmp` CLI allows interaction with Jumpstarter as a clients or exporter.appears grammatically incorrect. Consider revising it to either:
- “The
jmpCLI allows interaction with Jumpstarter as a client or exporter.”
or if plural is intended:- “The
jmpCLI allows interaction with Jumpstarter as clients or exporters.”
A diff suggestion for the singular version is provided below:-The `jmp` CLI allows interaction with Jumpstarter as a clients or exporter. +The `jmp` CLI allows interaction with Jumpstarter as a client or exporter.🧰 Tools
🪛 LanguageTool
[uncategorized] ~23-~23: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...llows interaction with Jumpstarter as a clients or exporter. ```{toctree} :maxdepth: 1...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
docs/source/installation/python-package.md (5)
63-68: Clarify alias removal tip placement.
The tip block now reminds users to undefine any existingjmpalias before running the command. Verify that its placement here is ideal for drawing attention to this important step. If user feedback calls for “Reference” information to be moved to the end of the output, ensure that any related reference data is repositioned accordingly.
80-86: Validate the container alias command formatting.
The alias command for running the client in a container now uses a multi-line syntax with substitutions (e.g.,{{version}}). Please check that the line continuations, volume mount definitions, and variable expansions work correctly across different shell environments.
125-126: Enhance table entry clarity for jumpstarter-cli-admin.
The description for thejumpstarter-cli-admincomponent could be tightened. Consider removing extraneous words (e.g., “of”) and, if “user facing” is intended as a compound adjective, hyphenate it as “user-facing” for better readability and consistency.🧰 Tools
🪛 LanguageTool
[formatting] ~125-~125: Insert a comma after ‘cases’: “In most cases,”?
Context: ...uns on the exporter hosts as a service. In most cases installation is not necessary and can b...(IN_MOST_CASES_COMMA)
[style] ~126-~126: Consider removing “of” to be more concise
Context: ... | A metapackage containing all of the Jumpstarter CLI components including th...(ALL_OF_THE)
[uncategorized] ~126-~126: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... admin CLIjumpstarter-cli-admin, the user facing CLI. ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
106-116: Review hardware access alias configuration.
The alias defined for hardware access now includes additional volume mounts, network settings, and privilege flags. Please verify that all command-line escapes and environment variable expansions work consistently across different systems, and that the command behavior aligns with user expectations for hardware interactions.
99-104: Improve sentence clarity with proper punctuation.
In the hardware access section, consider adding a comma before “or” in the phrase “...because you are running thejmpcommand or you are following the [local-only workflow]...” to clearly separate the two independent clauses.🧰 Tools
🪛 LanguageTool
[uncategorized] ~99-~99: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ecause you are running thejmpcommand or you are following the [local-only workf...(COMMA_COMPOUND_SENTENCE)
docs/source/getting-started/setup-exporter-client.md (2)
134-160: Enhanced CLI Help Text for the Shell Command
The help text now includes additional options (such as--exporter-config,--client-config, and--duration) and updated descriptions for parameters like--selector. This comprehensive update improves clarity on available options. One minor suggestion: consider confirming that a dedicated “Reference” section is appended at the end of the output per user feedback, which could further aid users in scanning available commands quickly.
166-188: Interactive Shell Example & Command Reference Ordering
This block demonstrates the complete workflow—from spawning the shell to interacting with it via thejcommand. While the example clearly outlines usage, there is some overlap with the earlier spawn command block. If intentional, please ensure that the “Reference” (i.e., the command list that appears after runningj) is clearly distinguished and placed at the end of the output, as requested by reviewer feedback. A brief note or reordering might improve readability and meet the intended UX guidelines.
📜 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 (52)
Makefile(0 hunks)docs/source/api-reference/drivers/flashers.md(1 hunks)docs/source/api-reference/drivers/yepkit.md(1 hunks)docs/source/cli/clients.md(1 hunks)docs/source/cli/reference/index.md(1 hunks)docs/source/cli/reference/jmp-admin.md(1 hunks)docs/source/cli/reference/jmp-client.md(0 hunks)docs/source/cli/reference/jmp-driver.md(0 hunks)docs/source/cli/reference/jmp-exporter.md(0 hunks)docs/source/cli/reference/jmp.md(1 hunks)docs/source/cli/run-tests.md(2 hunks)docs/source/config/cli.md(5 hunks)docs/source/config/oidc.md(3 hunks)docs/source/getting-started/setup-exporter-client.md(3 hunks)docs/source/getting-started/setup-local-exporter.md(5 hunks)docs/source/installation/python-package.md(4 hunks)docs/source/introduction/exporters.md(1 hunks)packages/jumpstarter-cli-client/README.md(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__main__.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_test.py(0 hunks)packages/jumpstarter-cli-client/pyproject.toml(0 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/__init__.py(2 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py(0 hunks)packages/jumpstarter-cli-exporter/README.md(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__init__.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__main__.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py(0 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_test.py(0 hunks)packages/jumpstarter-cli-exporter/pyproject.toml(0 hunks)packages/jumpstarter-cli/jumpstarter_cli/__init__.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/cli_test.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/common.py(0 hunks)packages/jumpstarter-cli/jumpstarter_cli/config.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_client.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/delete.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/get.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/j.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/login.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/run.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/shell.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)packages/jumpstarter-cli/pyproject.toml(1 hunks)packages/jumpstarter-driver-yepkit/README.md(1 hunks)packages/jumpstarter/jumpstarter/common/exceptions.py(1 hunks)pyproject.toml(0 hunks)
💤 Files with no reviewable changes (21)
- packages/jumpstarter-cli-client/README.md
- Makefile
- docs/source/cli/reference/jmp-driver.md
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/main.py
- docs/source/cli/reference/jmp-exporter.md
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/init.py
- docs/source/cli/reference/jmp-client.md
- packages/jumpstarter-cli-exporter/README.md
- packages/jumpstarter-cli-client/pyproject.toml
- pyproject.toml
- packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_test.py
- packages/jumpstarter-cli/jumpstarter_cli/common.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/main.py
- packages/jumpstarter-cli-exporter/pyproject.toml
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_login.py
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_login.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_test.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/init.py
- packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter.py
🚧 Files skipped from review as they are similar to previous changes (24)
- docs/source/api-reference/drivers/flashers.md
- packages/jumpstarter-cli/jumpstarter_cli/config.py
- docs/source/cli/reference/jmp-admin.md
- packages/jumpstarter-driver-yepkit/README.md
- packages/jumpstarter-cli/pyproject.toml
- packages/jumpstarter-cli/jumpstarter_cli/delete.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/init.py
- docs/source/getting-started/setup-local-exporter.md
- packages/jumpstarter-cli/jumpstarter_cli/cli_test.py
- packages/jumpstarter/jumpstarter/common/exceptions.py
- docs/source/cli/clients.md
- packages/jumpstarter-cli/jumpstarter_cli/run.py
- docs/source/cli/reference/jmp.md
- packages/jumpstarter-cli/jumpstarter_cli/j.py
- packages/jumpstarter-cli/jumpstarter_cli/update.py
- docs/source/config/oidc.md
- docs/source/api-reference/drivers/yepkit.md
- packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py
- docs/source/cli/run-tests.md
- packages/jumpstarter-cli/jumpstarter_cli/create.py
- docs/source/introduction/exporters.md
- packages/jumpstarter-cli/jumpstarter_cli/config_client.py
- docs/source/config/cli.md
🧰 Additional context used
🧬 Code Definitions (2)
packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (1) (1)
opt_config(92-93)
packages/jumpstarter-cli/jumpstarter_cli/__init__.py (9)
packages/jumpstarter-cli-driver/jumpstarter_cli_driver/__init__.py (1) (1)
driver(12-17)packages/jumpstarter-cli/jumpstarter_cli/config.py (1) (1)
config(8-9)packages/jumpstarter-cli/jumpstarter_cli/create.py (1) (1)
create(17-20)packages/jumpstarter-cli/jumpstarter_cli/delete.py (1) (1)
delete(9-12)packages/jumpstarter-cli/jumpstarter_cli/get.py (1) (1)
get(9-12)packages/jumpstarter-cli/jumpstarter_cli/j.py (1) (1)
j(9-20)packages/jumpstarter-cli/jumpstarter_cli/login.py (1) (1)
login(27-104)packages/jumpstarter-cli/jumpstarter_cli/run.py (1) (1)
run(24-27)packages/jumpstarter-cli/jumpstarter_cli/update.py (1) (1)
update(11-14)
🪛 LanguageTool
docs/source/cli/reference/index.md
[uncategorized] ~23-~23: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...llows interaction with Jumpstarter as a clients or exporter. ```{toctree} :maxdepth: 1...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
docs/source/installation/python-package.md
[uncategorized] ~99-~99: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ecause you are running the jmp command or you are following the [local-only workf...
(COMMA_COMPOUND_SENTENCE)
[style] ~126-~126: Consider removing “of” to be more concise
Context: ... | A metapackage containing all of the Jumpstarter CLI components including th...
(ALL_OF_THE)
[uncategorized] ~126-~126: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... admin CLI jumpstarter-cli-admin, the user facing CLI. ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
🔇 Additional comments (20)
docs/source/cli/reference/index.md (2)
18-21: Verify Toctree Reference for jmp-admin.md
The toctree directive now explicitly listsjmp-admin.md(line 20). Confirm that this reference is correct and that the linked document is up to date with the consolidated CLI changes.
25-28: Ensure Consistent Toctree Entry for jmp.md
The toctree in lines 25–28 now referencesjmp.md. Verify that this document correctly consolidates the updated CLI command information and that no deprecated references remain.docs/source/installation/python-package.md (1)
90-95: Verify CLI command output consistency.
The example output for$ jmp config client listappears updated; however, please ensure that it accurately reflects the new unified CLI behavior and that any “Reference” details (if applicable per user feedback) are positioned at the end of the output.packages/jumpstarter-cli/jumpstarter_cli/get.py (2)
2-2: Import refactoring enhances code clarityImporting
opt_configdirectly fromjumpstarter_cli_commoninstead of using a relative import provides better clarity about the origin of this function and aligns with the unified CLI approach.
16-16: Configuration specialized for non-exporter use casesThe decorator change to
@opt_config(exporter=False)properly specifies that these commands don't require exporter configuration, making the command behavior more explicit. This aligns with the unified CLI objectives by standardizing decorator usage across commands.Also applies to: 48-48
packages/jumpstarter-cli/jumpstarter_cli/__init__.py (4)
1-16: Consolidated imports support unified CLI structureThe new imports bring together all command modules under a single CLI interface, effectively unifying previously separate commands. This change directly addresses the PR objective of creating a unified CLI.
20-28: Configurable log levels implemented successfullyYou've successfully implemented configurable log levels by adding the
@opt_log_leveldecorator and configuring logging based on the provided level. This addresses a previous review comment requesting this feature.
30-37: Command structure refined for simplified user experienceThe addition of these commands to the main
jmpcommand group centralizes the CLI interface, making it more intuitive for users. This change represents the core of the unified CLI objective.
43-43: Public API explicitly definedDefining
__all__clearly communicates which functions are part of the public API, enhancing maintainability and making the module's interface explicit.packages/jumpstarter-cli/jumpstarter_cli/shell.py (3)
13-22: Unified shell command with configuration optionsThe new
shellcommand successfully integrates functionality previously spread across different CLI tools. This is a core improvement for the unified CLI.
16-17: Implement the missing warning for client-specific optionsThere's a TODO comment about warning users when client-specific options are used with an exporter configuration, but the implementation is missing.
Consider adding a warning check in the function body that detects when
ExporterConfigV1Alpha1is used with client-specific parameters:@click.command("shell") @opt_config() # client specific # TODO: warn if these are specified with exporter config @click.option("--lease", "lease_name") @opt_selector @opt_duration_partial(default=timedelta(minutes=30), show_default="00:30:00") # end client specific @handle_exceptions def shell(config, lease_name, selector, duration): """ Spawns a shell connecting to a local or remote exporter """ + # Warn if client-specific options are used with exporter config + if isinstance(config, ExporterConfigV1Alpha1) and any([lease_name, selector, duration != timedelta(minutes=30)]): + click.echo("Warning: Client-specific options (--lease, selector, or duration) were specified with exporter config and will be ignored.") match config:
27-42: Match-case pattern efficiently handles configuration typesThe match-case pattern clearly separates handling logic for different configuration types, making the code more maintainable and easier to understand. Good use of context managers for resource management.
packages/jumpstarter-cli/jumpstarter_cli/login.py (5)
9-26: Unified login command with comprehensive optionsThe login command provides a comprehensive set of options for authentication while maintaining consistency with the unified CLI approach.
15-25: Implement missing warning for exporter configurationThere's a TODO comment about warning users when client-specific options are used with an exporter configuration, but the implementation is missing.
Consider adding warning logic similar to the one suggested for the shell command:
@opt_config(allow_missing=True) async def login( config, endpoint: str, namespace: str, name: str, username: str | None, password: str | None, token: str | None, issuer: str, client_id: str, connector_id: str, unsafe, allow, ): """Login into a jumpstarter instance""" + # Warn if client-specific options are used with exporter config + if isinstance(config, ExporterConfigV1Alpha1) and (unsafe is not None or allow != ""): + click.echo("Warning: Client-specific options (--allow, --unsafe) were specified with exporter config and will be ignored.")
43-80: Configuration handling with user promptsThe configuration handling logic effectively manages different configuration types and prompts users for missing information. Based on past review comments, error handling for these prompts has been addressed in subsequent commits.
81-93: OIDC authentication with multiple grant typesThe authentication logic supports multiple grant types (token exchange, password, authorization code) providing flexibility for different authentication scenarios. Past review comments about error handling have been addressed in subsequent commits.
94-104: Configuration saving based on configuration typeThe configuration saving logic correctly handles different types of configurations. Past review comments about error handling have been addressed in subsequent commits.
docs/source/getting-started/setup-exporter-client.md (3)
57-60: Updated Exporter Config Edit Command
The command has been updated to use:$ jmp config exporter edit testingwhich properly reflects the new unified CLI structure. Please verify that all related examples and documentation references consistently use this revised command format.
86-89: Refined Command for Running the Exporter
The change to:$ jmp run --exporter testingis clear and aligns with the new command architecture. This update reduces redundancy in the command groups.
131-133: Revised Shell Spawn Command
The shell command now reads:$ jmp shell --client hello --selector example.com/board=fooThis update removes the deprecated
client shellpattern and introduces the--selectoroption for filtering. Please ensure that any downstream documentation or examples referencing the old command have been updated accordingly.
2 participants
Summary by CodeRabbit