Add Automatic Local Cluster Management to Jumpstarter Admin CLI#529
Add Automatic Local Cluster Management to Jumpstarter Admin CLI#529
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis update restructures and expands the Jumpstarter service installation documentation into separate local and production guides. It enhances the CLI to manage local Kubernetes clusters (Kind and Minikube) with creation, deletion, and validation features. New asynchronous cluster management utilities and comprehensive tests are added. Minor docstring improvements and link updates complete the changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ClusterMgr as Cluster Manager (Kind/Minikube)
participant Helm
User->>CLI: jmp admin install --kind --create-cluster
CLI->>ClusterMgr: Validate Kind installed
CLI->>ClusterMgr: Check if cluster exists
alt Cluster does not exist or --force-recreate
CLI->>ClusterMgr: Create Kind cluster (with extra args)
end
CLI->>ClusterMgr: Retrieve cluster IP
CLI->>Helm: Install Jumpstarter Helm chart (with endpoints)
Helm-->>CLI: Installation result
CLI-->>User: Installation summary
sequenceDiagram
participant User
participant CLI
participant ClusterMgr as Cluster Manager (Kind/Minikube)
participant Helm
User->>CLI: jmp admin uninstall --delete-cluster
CLI->>Helm: Uninstall Jumpstarter Helm release
Helm-->>CLI: Uninstall result
CLI->>ClusterMgr: Delete Kind/Minikube cluster
ClusterMgr-->>CLI: Deletion result
CLI-->>User: Uninstallation summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py (1)
218-232: Add proper default value for cluster_name parameter.The
cluster_nameparameter defaults toNone, but it's used directly inget_minikube_ip. This could cause issues if the function expects a string.-async def get_ip_generic(cluster_type: Optional[str], minikube: str, cluster_name: str = None) -> str: +async def get_ip_generic(cluster_type: Optional[str], minikube: str, cluster_name: str = "jumpstarter-lab") -> str:
🧹 Nitpick comments (7)
packages/jumpstarter/jumpstarter/common/ipaddr.py (1)
26-26: Improve type hint for the profile parameter.The
profileparameter should have a proper type hint. UseOptional[str]for consistency with modern Python typing practices.-async def get_minikube_ip(profile: str = None, minikube: str = "minikube"): +async def get_minikube_ip(profile: Optional[str] = None, minikube: str = "minikube"):You'll also need to add the import:
+from typing import Optionaldocs/source/getting-started/installation/service/service-production.md (1)
43-46: Fix formatting issue in TLS section.There's a formatting issue with repeated "TLS" text that affects readability.
Apply this fix:
-**Option 2: End-to-End TLS** -- TLS from client to Jumpstarter service +**Option 2: End-to-End TLS** +- TLS from client to Jumpstarter servicepackages/jumpstarter-kubernetes/jumpstarter_kubernetes/cluster.py (2)
71-74: Simplify control flow by removing unnecessary else blocks.The code can be improved by removing unnecessary else blocks after return statements, as suggested by the linter.
Apply these simplifications:
- if returncode == 0: - return True - else: - raise RuntimeError(f"Failed to delete Minikube cluster '{cluster_name}'") + if returncode == 0: + return True + raise RuntimeError(f"Failed to delete Minikube cluster '{cluster_name}'")Similar patterns exist in lines 87-90, 124-127, and 189-192.
Also applies to: 87-90, 124-127, 189-192
107-111: Simplify control flow after raise statements.Remove unnecessary else blocks after raise statements for cleaner code.
Apply these simplifications:
- if not force_recreate: - raise RuntimeError(f"Minikube cluster '{cluster_name}' already exists.") - else: - if not await delete_minikube_cluster(minikube, cluster_name): - return False + if not force_recreate: + raise RuntimeError(f"Minikube cluster '{cluster_name}' already exists.") + if not await delete_minikube_cluster(minikube, cluster_name): + return FalseSimilar pattern exists in lines 144-148.
Also applies to: 144-148
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py (3)
29-40: Simplify the conditional logic.The
elifis unnecessary after the return statement.def _validate_cluster_type( kind: Optional[str], minikube: Optional[str] ) -> Optional[Literal["kind"] | Literal["minikube"]]: if kind and minikube: raise click.ClickException('You can only select one local cluster type "kind" or "minikube"') if kind is not None: return "kind" - elif minikube is not None: + if minikube is not None: return "minikube" return None
96-119: Simplify the error handling logic.Remove unnecessary
elseblocks afterraisestatements.async def _create_kind_cluster( kind: str, cluster_name: str, kind_extra_args: str, force_recreate_cluster: bool ) -> None: if not kind_installed(kind): raise click.ClickException("kind is not installed (or not in your PATH)") cluster_action = "Recreating" if force_recreate_cluster else "Creating" click.echo(f'{cluster_action} Kind cluster "{cluster_name}"...') extra_args_list = kind_extra_args.split() if kind_extra_args.strip() else [] try: await create_kind_cluster(kind, cluster_name, extra_args_list, force_recreate_cluster) if force_recreate_cluster: click.echo(f'Successfully recreated Kind cluster "{cluster_name}"') else: click.echo(f'Successfully created Kind cluster "{cluster_name}"') except RuntimeError as e: if "already exists" in str(e) and not force_recreate_cluster: click.echo(f'Kind cluster "{cluster_name}" already exists, continuing...') else: if force_recreate_cluster: raise click.ClickException(f"Failed to recreate Kind cluster: {e}") from e - else: - raise click.ClickException(f"Failed to create Kind cluster: {e}") from e + raise click.ClickException(f"Failed to create Kind cluster: {e}") from e
121-144: Apply the same error handling simplification.For consistency with the Kind cluster function, remove the unnecessary
elseblock.except RuntimeError as e: if "already exists" in str(e) and not force_recreate_cluster: click.echo(f'Minikube cluster "{cluster_name}" already exists, continuing...') else: if force_recreate_cluster: raise click.ClickException(f"Failed to recreate Minikube cluster: {e}") from e - else: - raise click.ClickException(f"Failed to create Minikube cluster: {e}") from e + raise click.ClickException(f"Failed to create Minikube cluster: {e}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/source/getting-started/guides/setup-distributed-mode.md(1 hunks)docs/source/getting-started/installation/index.md(1 hunks)docs/source/getting-started/installation/service.md(0 hunks)docs/source/getting-started/installation/service/index.md(1 hunks)docs/source/getting-started/installation/service/service-local.md(1 hunks)docs/source/getting-started/installation/service/service-production.md(1 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete.py(1 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/config.py(1 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/__init__.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/cluster.py(1 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/install.py(1 hunks)packages/jumpstarter/jumpstarter/common/ipaddr.py(1 hunks)
💤 Files with no reviewable changes (1)
- docs/source/getting-started/installation/service.md
🧰 Additional context used
🪛 LanguageTool
docs/source/getting-started/installation/service/service-local.md
[uncategorized] ~10-~10: A punctuation mark might be missing here.
Context: ...ed and configured to access your cluster - [Helm](https://helm.sh/docs/intro/install...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
[uncategorized] ~52-~52: Loose punctuation mark.
Context: ...or cluster creation: - --cluster-name: Specify a custom cluster name (default:...
(UNLIKELY_OPENING_PUNCTUATION)
docs/source/getting-started/installation/service/service-production.md
[duplication] ~43-~43: Possible typo: you repeated a word.
Context: ...encryption hops Option 2: End-to-End TLS - TLS from client to Jumpstarter service - Hi...
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.17.2)
docs/source/getting-started/installation/service/service-local.md
97-97: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
233-233: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
🪛 Pylint (3.3.7)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/cluster.py
[refactor] 71-74: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 87-90: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 107-111: Unnecessary "else" after "raise", remove the "else" and de-indent the code inside it
(R1720)
[refactor] 124-127: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 144-148: Unnecessary "else" after "raise", remove the "else" and de-indent the code inside it
(R1720)
[refactor] 189-192: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py
[refactor] 35-38: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 42-42: Too many arguments (7/5)
(R0913)
[refactor] 42-42: Too many positional arguments (7/5)
(R0917)
[refactor] 63-63: Too many arguments (8/5)
(R0913)
[refactor] 63-63: Too many positional arguments (8/5)
(R0917)
[refactor] 115-118: Unnecessary "else" after "raise", remove the "else" and de-indent the code inside it
(R1720)
[refactor] 140-143: Unnecessary "else" after "raise", remove the "else" and de-indent the code inside it
(R1720)
[refactor] 188-188: Too many arguments (12/5)
(R0913)
[refactor] 188-188: Too many positional arguments (12/5)
(R0917)
[refactor] 372-372: Too many arguments (9/5)
(R0913)
[refactor] 372-372: Too many positional arguments (9/5)
(R0917)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
🔇 Additional comments (26)
packages/jumpstarter-cli/jumpstarter_cli/config.py (1)
9-11: Descriptive docstring addedThe new docstring clearly defines the purpose of the
configcommand group and will be surfaced by Click in the CLI help.packages/jumpstarter/jumpstarter/common/ipaddr.py (3)
27-31: LGTM! Command construction logic is well implemented.The dynamic command construction properly handles the optional profile parameter and maintains clean, readable code.
35-35: Good improvement to capture stderr for better error diagnostics.Capturing both stdout and stderr provides better debugging information when the minikube command fails.
42-42: Enhanced error handling provides better debugging information.Including stderr in the RuntimeError message will help users understand what went wrong when the minikube command fails.
docs/source/getting-started/guides/setup-distributed-mode.md (1)
30-30: Documentation link correctly updated for new structure.The path update properly reflects the restructuring from a single
service.mdfile to aservice/directory with anindex.mdentry point.packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete.py (1)
29-29: Docstring correction properly reflects the command group's purpose.Good catch! The docstring now correctly describes the delete command group's functionality.
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/install.py (1)
7-7: Good documentation improvement.The added docstring clearly describes the function's purpose and improves code readability.
docs/source/getting-started/installation/index.md (1)
7-7: Documentation paths correctly updated for restructured service documentation.Both the main text link and toctree entry have been properly updated to reflect the new
service/directory structure withindex.mdas the entry point.Also applies to: 13-13
docs/source/getting-started/installation/service/index.md (1)
1-21: Excellent documentation structure and organization.The new index file effectively organizes the service installation documentation into focused local and production guides. The clear recommendations help users choose the appropriate installation path, and the toctree structure provides good navigation.
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/__init__.py (1)
2-9: Clean extension of the module's public API.The cluster management functions are properly imported and exported, maintaining consistency with the existing module structure. The addition of these functions aligns well with the new cluster lifecycle management capabilities.
Also applies to: 52-57
docs/source/getting-started/installation/service/service-production.md (1)
1-332: Comprehensive and well-structured production deployment guide.This documentation provides excellent coverage of production deployment scenarios including TLS configuration, ingress controllers, and both Helm and ArgoCD installation methods. The warnings about gRPC requirements and manual secret management are particularly valuable.
docs/source/getting-started/installation/service/service-local.md (1)
1-234: Excellent local installation documentation with comprehensive examples.The documentation provides clear guidance for both automated CLI-based installation and manual setup. The coverage of both kind and minikube with specific configuration examples is particularly valuable for local development workflows.
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/cluster.py (5)
6-13: Clean utility functions for checking tool availability.The installation check functions are simple and effective, properly using
shutil.which()to verify tool availability.
16-34: Well-implemented async command execution utilities.The command execution functions properly handle both silent and streaming output scenarios with good error handling for missing commands.
37-58: Robust cluster existence checking functions.The existence check functions properly handle tool availability and command errors, returning appropriate boolean values.
150-178: Excellent kind cluster configuration.The cluster configuration includes all necessary settings for Jumpstarter including proper NodePort ranges and port mappings. The configuration aligns well with the documentation examples.
183-187: Proper handling of stdin piping for kind configuration.The implementation correctly pipes the cluster configuration to kind via stdin, which is an elegant approach for passing the YAML configuration.
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py (9)
22-27: LGTM!Clear validation logic with a helpful error message.
42-61: Well-structured endpoint configuration.The function correctly handles default values and constructs the endpoints appropriately. The parameter count is high but each serves a distinct purpose.
63-94: Excellent user experience with clear warnings.The function properly validates inputs and provides detailed warnings about data loss when force recreating clusters. The confirmation prompt is a good safety measure.
146-168: Clean implementation of cluster deletion functions.Both functions properly validate prerequisites and provide clear feedback during the deletion process.
170-186: Excellent safety measures for cluster deletion.The confirmation prompt with clear warnings about data loss is a crucial safety feature. Good implementation.
188-216: Clear and informative helm installation process.The function provides excellent visibility into the installation parameters before proceeding.
234-323: Well-integrated cluster management features.The install command successfully incorporates the new cluster creation capabilities while maintaining a logical flow. The organization of validation, cluster creation, and installation steps is excellent.
326-349: Clean implementation of IP detection command.The command properly supports both cluster types and delegates to the appropriate IP detection logic.
351-394: Good integration of cluster deletion with uninstall.The uninstall command properly handles both helm chart removal and optional cluster deletion with appropriate safety measures.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/source/getting-started/installation/service/service-local.md (1)
3-3: Consider enhancing the introductory text for clarity.The current text could be more descriptive about the use cases for local installation.
-For local development and testing, you can install Jumpstarter on local Kubernetes clusters using tools like kind or minikube. This is ideal for getting started quickly or for CI/CD pipelines. +For local development and testing, you can install Jumpstarter on local Kubernetes clusters using tools like kind or minikube. This is ideal for learning about the distributed service quickly or for creating CI/CD pipelines to validate your own Jumpstarter drivers.
🧹 Nitpick comments (6)
packages/jumpstarter/jumpstarter/common/ipaddr_test.py (1)
78-78: Add trailing newline to the file.The file should end with a newline character.
- mock_subprocess.assert_called_once_with("my-minikube", "ip", "-p", "my-profile", stdout=-1, stderr=-1) + mock_subprocess.assert_called_once_with("my-minikube", "ip", "-p", "my-profile", stdout=-1, stderr=-1) +packages/jumpstarter-kubernetes/jumpstarter_kubernetes/cluster_test.py (1)
396-396: Add trailing newline to the file.The file should end with a newline character.
- await delete_minikube_cluster("minikube", "test-cluster") + await delete_minikube_cluster("minikube", "test-cluster") +packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install_test.py (1)
696-696: Add trailing newline to the file.The file should end with a newline character.
- assert "--minikube" in result.output + assert "--minikube" in result.output +packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py (3)
35-39: Simplify conditional logic by removing unnecessary elif.Since the function returns early, the elif can be replaced with if.
if kind is not None: return "kind" - elif minikube is not None: + if minikube is not None: return "minikube" return None
111-119: Simplify error handling by removing unnecessary else block.The else block is not needed after a raise statement.
except RuntimeError as e: if "already exists" in str(e) and not force_recreate_cluster: click.echo(f'Kind cluster "{cluster_name}" already exists, continuing...') + elif force_recreate_cluster: + raise click.ClickException(f"Failed to recreate Kind cluster: {e}") from e else: - if force_recreate_cluster: - raise click.ClickException(f"Failed to recreate Kind cluster: {e}") from e - else: - raise click.ClickException(f"Failed to create Kind cluster: {e}") from e + raise click.ClickException(f"Failed to create Kind cluster: {e}") from e
136-144: Simplify error handling by removing unnecessary else block.The else block is not needed after a raise statement.
except RuntimeError as e: if "already exists" in str(e) and not force_recreate_cluster: click.echo(f'Minikube cluster "{cluster_name}" already exists, continuing...') + elif force_recreate_cluster: + raise click.ClickException(f"Failed to recreate Minikube cluster: {e}") from e else: - if force_recreate_cluster: - raise click.ClickException(f"Failed to recreate Minikube cluster: {e}") from e - else: - raise click.ClickException(f"Failed to create Minikube cluster: {e}") from e + raise click.ClickException(f"Failed to create Minikube cluster: {e}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/source/getting-started/installation/service/service-local.md(1 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py(4 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install_test.py(1 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/cluster_test.py(1 hunks)packages/jumpstarter/jumpstarter/common/ipaddr_test.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install_test.py
[refactor] 308-308: Too many arguments (7/5)
(R0913)
[refactor] 308-308: Too many positional arguments (7/5)
(R0917)
[refactor] 350-350: Too many arguments (7/5)
(R0913)
[refactor] 350-350: Too many positional arguments (7/5)
(R0917)
[refactor] 385-385: Too many arguments (7/5)
(R0913)
[refactor] 385-385: Too many positional arguments (7/5)
(R0917)
[refactor] 445-445: Too many arguments (7/5)
(R0913)
[refactor] 445-445: Too many positional arguments (7/5)
(R0917)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py
[refactor] 35-38: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 42-42: Too many arguments (7/5)
(R0913)
[refactor] 42-42: Too many positional arguments (7/5)
(R0917)
[refactor] 63-63: Too many arguments (8/5)
(R0913)
[refactor] 63-63: Too many positional arguments (8/5)
(R0917)
[refactor] 115-118: Unnecessary "else" after "raise", remove the "else" and de-indent the code inside it
(R1720)
[refactor] 140-143: Unnecessary "else" after "raise", remove the "else" and de-indent the code inside it
(R1720)
[refactor] 188-188: Too many arguments (12/5)
(R0913)
[refactor] 188-188: Too many positional arguments (12/5)
(R0917)
[refactor] 372-372: Too many arguments (9/5)
(R0913)
[refactor] 372-372: Too many positional arguments (9/5)
(R0917)
🪛 LanguageTool
docs/source/getting-started/installation/service/service-local.md
[uncategorized] ~10-~10: A punctuation mark might be missing here.
Context: ...ed and configured to access your cluster - [Helm](https://helm.sh/docs/intro/install...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
[uncategorized] ~52-~52: Loose punctuation mark.
Context: ...or cluster creation: - --cluster-name: Specify a custom cluster name (default:...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
docs/source/getting-started/installation/service/service-local.md
96-96: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
232-232: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
🔇 Additional comments (2)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/cluster_test.py (1)
1-395: Excellent test coverage for cluster management utilities!The test file provides comprehensive coverage for all cluster management operations including tool detection, command execution, cluster existence checks, and full lifecycle operations for both Kind and Minikube clusters. The async test patterns and mocking are well-implemented.
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install_test.py (1)
1-695: Outstanding test coverage for the install CLI module!The test file provides comprehensive coverage for all aspects of the installation and uninstallation workflows, including validation, cluster management, endpoint configuration, and CLI commands. Excellent use of mocking and Click's testing utilities.
| async def _install_jumpstarter_helm_chart( | ||
| chart: str, | ||
| name: str, | ||
| namespace: str, | ||
| basedomain: str, | ||
| grpc_endpoint: str, | ||
| router_endpoint: str, | ||
| mode: str, | ||
| version: str, | ||
| kubeconfig: Optional[str], | ||
| context: Optional[str], | ||
| helm: str, | ||
| ip: str, | ||
| ) -> None: | ||
| click.echo(f'Installing Jumpstarter service v{version} in namespace "{namespace}" with Helm\n') | ||
| click.echo(f"Chart URI: {chart}") | ||
| click.echo(f"Chart Version: {version}") | ||
| click.echo(f"IP Address: {ip}") | ||
| click.echo(f"Basedomain: {basedomain}") | ||
| click.echo(f"Service Endpoint: {grpc_endpoint}") | ||
| click.echo(f"Router Endpoint: {router_endpoint}") | ||
| click.echo(f"gPRC Mode: {mode}\n") | ||
|
|
||
| await install_helm_chart( | ||
| chart, name, namespace, basedomain, grpc_endpoint, router_endpoint, mode, version, kubeconfig, context, helm | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce the number of function parameters.
This function takes 12 parameters, which makes it difficult to maintain and call correctly. Consider grouping related parameters into a configuration object or dataclass.
For example, you could create a dataclass to group the configuration:
@dataclass
class HelmInstallConfig:
chart: str
name: str
namespace: str
basedomain: str
grpc_endpoint: str
router_endpoint: str
mode: str
version: str
kubeconfig: Optional[str]
context: Optional[str]
helm: str
ip: strThis would reduce the function signature and make the code more maintainable.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 188-188: Too many arguments (12/5)
(R0913)
[refactor] 188-188: Too many positional arguments (12/5)
(R0917)
🤖 Prompt for AI Agents
In packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py from lines
188 to 214, the _install_jumpstarter_helm_chart function has 12 parameters
making it hard to maintain and call. Refactor by creating a dataclass, for
example HelmInstallConfig, to group all these related parameters into a single
object. Then update the function to accept this config object instead of
individual parameters, and adjust the function body to access attributes from
the config. This will simplify the function signature and improve code
maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/source/getting-started/installation/service/service-production.md (2)
36-44: Use proper heading levels instead of bold-styled “headings”.
markdownlint(MD036) warns because**Option 1 …**/**Option 2 …**are emphasis, not real headings, which hurts navigation and ToC generation in Sphinx/ReadTheDocs.Recommended fix:
-**Option 1: TLS Termination at Ingress/Route (Recommended)** +### Option 1: TLS Termination at Ingress/Route (Recommended) ... -**Option 2: End-to-End TLS** +### Option 2: End-to-End TLSThis keeps the visual weight while satisfying lint rules and improving anchor links.
40-41: Insert a blank line before the “Option 2” heading to avoid word-merge & duplication warning.
LanguageToolreports a repeated-word false positive because line 40 (last bullet) and line 41 (“Option 2…”) are glued together without an empty line. Add a newline after the final bullet:-Better performance with fewer encryption hops - -**Option 2: End-to-End TLS** +Better performance with fewer encryption hops + +### Option 2: End-to-End TLSThis also separates list context from the next section, improving readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/source/getting-started/installation/service/service-production.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#425
File: packages/jumpstarter-driver-tasmota/README.md:23-23
Timestamp: 2025-05-07T18:33:56.914Z
Learning: In Jumpstarter documentation, configuration parameters should be presented in markdown tables rather than bulleted lists for better readability.
docs/source/getting-started/installation/service/service-production.md (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
🪛 LanguageTool
docs/source/getting-started/installation/service/service-production.md
[duplication] ~41-~41: Possible typo: you repeated a word.
Context: ...encryption hops Option 2: End-to-End TLS - TLS from client to Jumpstarter service - Hi...
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.17.2)
docs/source/getting-started/installation/service/service-production.md
36-36: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
41-41: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/source/getting-started/installation/service/service-local.md (3)
20-22: Fix the garbled sentence in the warning block.The phrase “automatic IP address detection for will not work” is grammatically broken and may confuse readers.
- Sometimes the automatic IP address detection for will not work correctly, to check if Jumpstarter can determine your IP address, run `jmp admin ip`. If the IP address cannot be determined, use the `--ip` argument to manually set your IP address. + Sometimes the automatic IP-address detection may not work correctly. To check whether Jumpstarter can determine your IP address, run `jmp admin ip`. If detection fails, supply it manually with `--ip`.
9-12: Keep prerequisite bullets parallel.One bullet (“Helm”) lacks a verb, so the list oscillates between “installed” vs no verb. Add “installed” for consistency.
- - [Helm](https://helm.sh/docs/intro/install/) (version 3.x or newer) + - [Helm](https://helm.sh/docs/intro/install/) installed (version 3.x or newer)
74-74: Split the run-on sentence in the warning block.Comma-splicing here hurts readability.
- Jumpstarter requires specific `NodePort` configurations, it is recommended to create a new cluster for Jumpstarter or use the automatic creation above. + Jumpstarter requires specific `NodePort` configurations. We recommend creating a new cluster for Jumpstarter or using the automatic creation method described above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/source/getting-started/installation/service/service-local.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
docs/source/getting-started/installation/service/service-local.md (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
🪛 LanguageTool
docs/source/getting-started/installation/service/service-local.md
[uncategorized] ~10-~10: A punctuation mark might be missing here.
Context: ...ed and configured to access your cluster - [Helm](https://helm.sh/docs/intro/install...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
[uncategorized] ~52-~52: Loose punctuation mark.
Context: ...or cluster creation: - --cluster-name: Specify a custom cluster name (default:...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
docs/source/getting-started/installation/service/service-local.md
96-96: Dollar signs used before commands without showing output
(MD014, commands-show-output)
232-232: Dollar signs used before commands without showing output
(MD014, commands-show-output)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (1)
docs/source/getting-started/installation/service/service-local.md (1)
50-55: Duplicate flag entry resolved — looks good.Previous revision listed
--kind-extra-argstwice; this duplication is gone and the options list is now concise and correct.
|
Oh sorry, I thought this was waiting on your changes. |
|
@mangelajo I think we can try to merge these changes if they aren't too out-of-date. |
Co-authored-by: Miguel Angel Ajo Pelayo <majopela@redhat.com>
Co-authored-by: Miguel Angel Ajo Pelayo <majopela@redhat.com>
Co-authored-by: Miguel Angel Ajo Pelayo <majopela@redhat.com>
kirkbrauer
force-pushed
the
improve-admin-install
branch
from
3fb6b67 to
4d3e1e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py (1)
188-201: Too many parameters in _install_jumpstarter_helm_chart; use a config object.This function takes 12 params, making it hard to maintain and call correctly. Favor a dataclass to group related settings.
For example:
+from dataclasses import dataclass + +@dataclass +class HelmInstallConfig: + chart: str + name: str + namespace: str + basedomain: str + grpc_endpoint: str + router_endpoint: str + mode: str + version: str + kubeconfig: Optional[str] + context: Optional[str] + helm: str + ip: str @@ -async def _install_jumpstarter_helm_chart( - chart: str, - name: str, - namespace: str, - basedomain: str, - grpc_endpoint: str, - router_endpoint: str, - mode: str, - version: str, - kubeconfig: Optional[str], - context: Optional[str], - helm: str, - ip: str, -) -> None: +async def _install_jumpstarter_helm_chart(cfg: HelmInstallConfig) -> None: - click.echo(f'Installing Jumpstarter service v{version} in namespace "{namespace}" with Helm\n') - click.echo(f"Chart URI: {chart}") - click.echo(f"Chart Version: {version}") - click.echo(f"IP Address: {ip}") - click.echo(f"Basedomain: {basedomain}") - click.echo(f"Service Endpoint: {grpc_endpoint}") - click.echo(f"Router Endpoint: {router_endpoint}") - click.echo(f"gPRC Mode: {mode}\n") + click.echo(f'Installing Jumpstarter service v{cfg.version} in namespace "{cfg.namespace}" with Helm\n') + click.echo(f"Chart URI: {cfg.chart}") + click.echo(f"Chart Version: {cfg.version}") + click.echo(f"IP Address: {cfg.ip}") + click.echo(f"Basedomain: {cfg.basedomain}") + click.echo(f"Service Endpoint: {cfg.grpc_endpoint}") + click.echo(f"Router Endpoint: {cfg.router_endpoint}") + click.echo(f"gRPC Mode: {cfg.mode}\n") @@ - await install_helm_chart( - chart, name, namespace, basedomain, grpc_endpoint, router_endpoint, mode, version, kubeconfig, context, helm - ) + await install_helm_chart( + cfg.chart, cfg.name, cfg.namespace, cfg.basedomain, cfg.grpc_endpoint, cfg.router_endpoint, + cfg.mode, cfg.version, cfg.kubeconfig, cfg.context, cfg.helm + ) @@ - click.echo(f'Installed Helm release "{name}" in namespace "{namespace}"') + click.echo(f'Installed Helm release "{cfg.name}" in namespace "{cfg.namespace}"')Then construct
HelmInstallConfigat the call site.Also applies to: 211-213
🧹 Nitpick comments (8)
docs/source/getting-started/installation/service/service-production.md (1)
36-41: Use proper headings instead of bold text for options (fixes MD036).Switch emphasis lines to headings for readability and to satisfy markdownlint.
-**Option 1: TLS Termination at Ingress/Route (Recommended)** +### Option 1: TLS Termination at Ingress/Route (Recommended) -**Option 2: End-to-End TLS** +### Option 2: End-to-End TLSpackages/jumpstarter-cli-admin/jumpstarter_cli_admin/install_test.py (1)
200-211: Add a test covering quoted extra args parsing.current code uses
str.split()which breaks on quoted values; we’re proposingshlex.split()in the CLI (see separate comment). Add a test to ensure arguments with spaces/quotes are handled.Example test to add:
@pytest.mark.asyncio @patch("jumpstarter_cli_admin.install.kind_installed") @patch("jumpstarter_cli_admin.install.create_kind_cluster") async def test_create_kind_cluster_with_quoted_args(mock_create_kind, mock_kind_installed): mock_kind_installed.return_value = True await _create_kind_cluster("kind", "test", '--config "path with space/kind.yaml" --verbosity=1', False) mock_create_kind.assert_called_once_with("kind", "test", ["--config", "path with space/kind.yaml", "--verbosity=1"], False)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py (2)
209-209: Typo: gPRC → gRPC.- click.echo(f"gPRC Mode: {mode}\n") + click.echo(f"gRPC Mode: {mode}\n")
29-39: Type hint simplification.Small readability tweak:
Optional[Literal["kind"] | Literal["minikube"]]can beOptional[Literal["kind", "minikube"]].docs/source/getting-started/installation/service/service-local.md (4)
20-22: Fix grammar and clarity in the IP detection warning.Tighten wording and remove the stray “for”.
-```{warning} -Sometimes the automatic IP address detection for will not work correctly, to check if Jumpstarter can determine your IP address, run `jmp admin ip`. If the IP address cannot be determined, use the `--ip` argument to manually set your IP address. -``` +```{warning} +Automatic IP address detection may not work in some environments. To check whether Jumpstarter can determine your IP, run `jmp admin ip`. If it cannot, use the `--ip` flag to set it manually. +```
73-75: Resolve comma splice in the warning.Split into two sentences for readability.
-```{warning} -Jumpstarter requires specific `NodePort` configurations, it is recommended to create a new cluster for Jumpstarter or use the automatic creation above. -``` +```{warning} +Jumpstarter requires specific `NodePort` configurations. It is recommended to create a new cluster for Jumpstarter or use the automatic creation above. +```
96-96: Address markdownlint MD014: remove leading “$” or include output.Static analysis flagged commands with prompts but no output. Prefer removing the prompt for copy-paste friendliness.
-$ jmp admin uninstall +jmp admin uninstall-$ helm uninstall jumpstarter --namespace jumpstarter-lab +helm uninstall jumpstarter --namespace jumpstarter-labAlternatively, keep “$” consistently and suppress MD014 for console blocks in your linter config.
Also applies to: 232-232
52-56: Clarify cluster name vs. namespace to avoid confusion.Default cluster name is “jumpstarter-lab”, same as the namespace used later. Consider a short note that cluster name is independent from the Helm namespace.
- `--minikube-extra-args`: Pass additional arguments to minikube cluster creation + +Note: The cluster name is independent from the Kubernetes namespace used by Helm (e.g., `jumpstarter-lab` in the examples below).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
docs/source/getting-started/guides/setup-distributed-mode.md(1 hunks)docs/source/getting-started/installation/index.md(1 hunks)docs/source/getting-started/installation/service.md(0 hunks)docs/source/getting-started/installation/service/index.md(1 hunks)docs/source/getting-started/installation/service/service-local.md(1 hunks)docs/source/getting-started/installation/service/service-production.md(1 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete.py(1 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py(4 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install_test.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/config.py(1 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/__init__.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/cluster.py(1 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/cluster_test.py(1 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/install.py(1 hunks)packages/jumpstarter/jumpstarter/common/ipaddr.py(1 hunks)packages/jumpstarter/jumpstarter/common/ipaddr_test.py(1 hunks)
💤 Files with no reviewable changes (1)
- docs/source/getting-started/installation/service.md
✅ Files skipped from review due to trivial changes (2)
- docs/source/getting-started/guides/setup-distributed-mode.md
- docs/source/getting-started/installation/service/index.md
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete.py
- docs/source/getting-started/installation/index.md
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/install.py
- packages/jumpstarter-cli/jumpstarter_cli/config.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/init.py
- packages/jumpstarter/jumpstarter/common/ipaddr.py
- packages/jumpstarter/jumpstarter/common/ipaddr_test.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/cluster_test.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/cluster.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
📚 Learning: 2025-05-28T15:09:35.768Z
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#463
File: packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py:270-270
Timestamp: 2025-05-28T15:09:35.768Z
Learning: The jumpstarter CLI is designed to match kubectl's behavior, including returning exit code 0 (success) when no resources are found, rather than exit code 1 (failure).
Applied to files:
docs/source/getting-started/installation/service/service-production.mdpackages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.pydocs/source/getting-started/installation/service/service-local.md
🪛 markdownlint-cli2 (0.17.2)
docs/source/getting-started/installation/service/service-production.md
36-36: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
41-41: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/source/getting-started/installation/service/service-local.md
96-96: Dollar signs used before commands without showing output
(MD014, commands-show-output)
232-232: Dollar signs used before commands without showing output
(MD014, commands-show-output)
🔇 Additional comments (4)
docs/source/getting-started/installation/service/service-production.md (1)
36-45: Confirm the recommended TLS approach aligns with team decision.Past discussion suggested prioritizing end-to-end TLS first. This doc currently recommends termination at ingress/route. Please confirm the desired default and adjust the “Recommended” tag if needed.
docs/source/getting-started/installation/service/service-local.md (3)
171-173: LGTM: Minikube NodePort range matches documented ports.Range 8000–9000 covers 8082/8083 as used below.
200-217: LGTM: Minikube Helm values are explicit and consistent.Explicit NodePort values (8082/8083) align with the configured range.
36-49: LGTM: Clear one-liners for automatic cluster creation and install.Examples for both kind and minikube are concise and consistent.
| kind: ClusterConfiguration | ||
| apiServer: | ||
| extraArgs: | ||
| "service-node-port-range": "3000-32767" |
There was a problem hiding this comment.
Correct NodePort range for kind config.
“3000-32767” looks like a typo. Standard Kubernetes default is 30000–32767. Using 3000 lowers the floor unnecessarily and is inconsistent with the rest of the guide.
- "service-node-port-range": "3000-32767"
+ "service-node-port-range": "30000-32767"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "service-node-port-range": "3000-32767" | |
| "service-node-port-range": "30000-32767" |
🤖 Prompt for AI Agents
In docs/source/getting-started/installation/service/service-local.md at line
136, the NodePort range is incorrectly set to "3000-32767". Update this value to
the standard Kubernetes default "30000-32767" to maintain consistency and avoid
lowering the port range floor unnecessarily.
| $ helm upgrade jumpstarter --install oci://quay.io/jumpstarter-dev/helm/jumpstarter \ | ||
| --create-namespace --namespace jumpstarter-lab \ | ||
| --set global.baseDomain=${BASEDOMAIN} \ | ||
| --set jumpstarter-controller.grpc.endpoint=${GRPC_ENDPOINT} \ | ||
| --set jumpstarter-controller.grpc.routerEndpoint=${GRPC_ROUTER_ENDPOINT} \ | ||
| --set global.metrics.enabled=false \ | ||
| --set jumpstarter-controller.grpc.nodeport.enabled=true \ | ||
| --set jumpstarter-controller.grpc.mode=nodeport \ | ||
| --version={{controller_version}} | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set explicit NodePort values for kind to match port mappings.
You map 30010/30011 in the kind config. Make these explicit in Helm values to avoid drift if chart defaults change.
--set jumpstarter-controller.grpc.endpoint=${GRPC_ENDPOINT} \
--set jumpstarter-controller.grpc.routerEndpoint=${GRPC_ROUTER_ENDPOINT} \
--set global.metrics.enabled=false \
--set jumpstarter-controller.grpc.nodeport.enabled=true \
+ --set jumpstarter-controller.grpc.nodeport.port=30010 \
+ --set jumpstarter-controller.grpc.nodeport.routerPort=30011 \
--set jumpstarter-controller.grpc.mode=nodeport \
--version={{controller_version}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $ helm upgrade jumpstarter --install oci://quay.io/jumpstarter-dev/helm/jumpstarter \ | |
| --create-namespace --namespace jumpstarter-lab \ | |
| --set global.baseDomain=${BASEDOMAIN} \ | |
| --set jumpstarter-controller.grpc.endpoint=${GRPC_ENDPOINT} \ | |
| --set jumpstarter-controller.grpc.routerEndpoint=${GRPC_ROUTER_ENDPOINT} \ | |
| --set global.metrics.enabled=false \ | |
| --set jumpstarter-controller.grpc.nodeport.enabled=true \ | |
| --set jumpstarter-controller.grpc.mode=nodeport \ | |
| --version={{controller_version}} | |
| ``` | |
| $ helm upgrade jumpstarter --install oci://quay.io/jumpstarter-dev/helm/jumpstarter \ | |
| --create-namespace --namespace jumpstarter-lab \ | |
| --set global.baseDomain=${BASEDOMAIN} \ | |
| --set jumpstarter-controller.grpc.endpoint=${GRPC_ENDPOINT} \ | |
| --set jumpstarter-controller.grpc.routerEndpoint=${GRPC_ROUTER_ENDPOINT} \ | |
| --set global.metrics.enabled=false \ | |
| --set jumpstarter-controller.grpc.nodeport.enabled=true \ | |
| --set jumpstarter-controller.grpc.nodeport.port=30010 \ | |
| --set jumpstarter-controller.grpc.nodeport.routerPort=30011 \ | |
| --set jumpstarter-controller.grpc.mode=nodeport \ | |
| --version={{controller_version}} |
🤖 Prompt for AI Agents
In docs/source/getting-started/installation/service/service-local.md around
lines 187 to 196, the Helm upgrade command sets
jumpstarter-controller.grpc.nodeport.enabled to true but does not explicitly
specify the NodePort values. To prevent drift with the kind port mappings
(30010/30011), add explicit Helm values for
jumpstarter-controller.grpc.nodeport.grpcPort=30010 and
jumpstarter-controller.grpc.nodeport.routerPort=30011 in the command line using
--set flags.
| extra_args_list = kind_extra_args.split() if kind_extra_args.strip() else [] | ||
| try: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Robustly parse extra args using shlex.split().
str.split() breaks with quoted args (paths with spaces, key=value pairs). Use shlex.split() instead.
+import shlex
@@
- extra_args_list = kind_extra_args.split() if kind_extra_args.strip() else []
+ extra_args_list = shlex.split(kind_extra_args) if kind_extra_args.strip() else []
@@
- extra_args_list = minikube_extra_args.split() if minikube_extra_args.strip() else []
+ extra_args_list = shlex.split(minikube_extra_args) if minikube_extra_args.strip() else []Also applies to: 129-130
🤖 Prompt for AI Agents
In packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py at lines
104-105 and 129-130, replace the use of str.split() for parsing extra arguments
with shlex.split() to correctly handle quoted strings and spaces within
arguments. Import the shlex module if not already imported, then use
shlex.split(kind_extra_args) instead of kind_extra_args.split() to robustly
parse the extra arguments.
| "--kind", is_flag=False, flag_value="kind", default=None, help="Use default settings for a local Kind cluster" | ||
| ) | ||
| @click.option( | ||
| "--minikube", | ||
| is_flag=False, | ||
| flag_value="minikube", | ||
| default=None, | ||
| help="Use default settings for a local Minikube cluster", | ||
| ) | ||
| @click.option("--create-cluster", is_flag=True, help="Create a local Kind or Minikube cluster if it does not exist") | ||
| @click.option( | ||
| "--force-recreate-cluster", | ||
| is_flag=True, | ||
| help="Force recreate the cluster if it already exists (WARNING: This will destroy all data in the cluster)", | ||
| ) | ||
| @click.option("--cluster-name", type=str, help="The name of the local cluster to create", default="jumpstarter-lab") | ||
| @click.option("--kind-extra-args", type=str, help="Extra arguments for the Kind cluster creation", default="") | ||
| @click.option("--minikube-extra-args", type=str, help="Extra arguments for the Minikube cluster creation", default="") | ||
| @click.option("-v", "--version", help="The version of the service to install", default=None) |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
CLI semantics for --kind/--minikube are confusing; align code with docs or update docs.
Currently these are defined as options that take a value (executable path), not boolean flags. Docs show them as flags. This is error-prone.
Options to fix:
- Minimal change: Keep current code, update docs/examples to pass a value (
--kind kind,--minikube minikube) and document the ability to pass a custom executable path. - Better UX (breaking change): Make them boolean flags for cluster type, and add
--kind-binary/--minikube-binaryoptions for custom paths.
If choosing better UX, I can provide a full patch updating options, call sites, and tests.
Also applies to: 328-336, 359-367
Inconsistent semantics for --kind and --minikube options
These options are currently defined to take a value (i.e. a path to the executable) but are documented and treated as boolean flags, which will confuse users.
Locations to update:
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py: lines 255–273
- Same pattern repeated at lines 328–336 and 359–367
Suggested fixes:
- Non-breaking (minimal):
• Update docs and examples to show that--kind/--minikuberequire a value (e.g.--kind /usr/local/bin/kind)
• Clarify that omitting them falls back to the default binary in$PATH - Better UX (breaking):
• Change--kind/--minikubeto boolean flags for selecting cluster type
• Introduce new options--kind-binaryand--minikube-binaryfor users to pass custom executable paths
Please choose your preferred approach—happy to draft the corresponding patch.
🤖 Prompt for AI Agents
In packages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py around lines
255 to 273, the --kind and --minikube options are defined to take values but are
documented and used as boolean flags, causing confusion. To fix this, either
update the help text and documentation to clarify these options require a path
value and that omitting them uses the default binary in $PATH, or refactor the
options to be boolean flags for selecting cluster type and add new options like
--kind-binary and --minikube-binary to accept custom executable paths. Apply the
same fix consistently at lines 328–336 and 359–367 as well.
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
Changes were addressed, sorry for the late review.
2 participants
Summary
Added support for automatically creating/deleting local kind/minikube clusters with the correct configuration for Jumpstarter to the
jmp admin installandjmp admin uninstallcommands.Changes
🚀 CLI Enhancements
--create-clusterflag tojmp admin installcommand that automatically creates a properly configured local cluster--delete-clusterflag tojmp admin uninstallcommand for complete cleanup--cluster-nameparameter--force-recreate-clusterflag to handle existing clusters--kind-extra-argsand--minikube-extra-argsfor advanced cluster customization🏗️ Core Infrastructure
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/cluster.py): 192 lines of cluster management logic for both kind and minikubepackages/jumpstarter-cli-admin/jumpstarter_cli_admin/install.py): Extended with cluster lifecycle managementpackages/jumpstarter/jumpstarter/common/ipaddr.py): Better handling of network interface detection📚 Documentation Restructure
service-local.md: 234 lines covering local development setupservice-production.md: 332 lines covering production deployment with TLS, ingress, and ArgoCDservice/index.md: 20 lines providing clear navigation between local and production setupsTechnical Details
The implementation provides a seamless local development experience by:
Usage Examples
Quick Start (New Users)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores