Skip to content
This repository was archived by the owner on Jan 23, 2026. It is now read-only.

Add JSON and YAML output options for the jmp-admin, jmp-client, and jmp-exporter CLIs#335

Merged
mangelajo merged 23 commits intomainfrom
cli-json-output
Mar 10, 2025
Merged

Add JSON and YAML output options for the jmp-admin, jmp-client, and jmp-exporter CLIs#335
mangelajo merged 23 commits intomainfrom
cli-json-output

Conversation

@kirkbrauer
Copy link
Copy Markdown
Member

@kirkbrauer kirkbrauer commented Mar 9, 2025

This PR adds support for JSON, YAML, and name/path output types for all CLIs. This will allow the use of the Jumpstarter CLIs programmatically from other tools that support parsing JSON/YAML output such as VS Code extensions or other CLI tools/scripts.

  • Added -o/--output json,yaml,name option to CLI commands that return multiple/complex values. Behaves similarly to kubectl.
  • Added -o/--output path option to CLI commands that just return a path. Mostly for create/delete operations.
  • Added --nointeractive option to admin CLI commands to prevent interactive prompts when using programatically.

Summary by CodeRabbit

  • New Features

    • Introduced non-interactive modes and flexible output formatting options (JSON, YAML, NAME, or PATH) for streamlined creation, deletion, import, and listing operations.
  • Improvements

    • Enhanced command outputs with direct, formatted displays and clearer feedback without extra prompts.
    • Expanded configuration listing functionality for clients and exporters with multiple output views.
  • Tests

    • Increased test coverage across commands to ensure reliable behavior with the new output options and non-interactive modes.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 9, 2025

Walkthrough

The changes extend support for configurable output formats and non-interactive modes across the codebase. Updates include new parameters (e.g., nointeractive, output) in CLI commands for creating, deleting, importing, and retrieving Kubernetes resources. The modifications refactor output handling by replacing table rendering with dedicated print functions that support JSON, YAML, NAME, and PATH modes. Additionally, models for clients, exporters, and leases have been migrated to Pydantic’s JsonBaseModel with enhanced serialization/deserialization methods, and corresponding test suites have been updated accordingly.

Changes

File(s) Change Summary
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete.py
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.py
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py
Enhanced CLI admin commands to support new output parameters (output, nointeractive) for client/exporter creation, deletion, retrieval, and import. Added dedicated print functions that format output as JSON, YAML, NAME, or PATH, and updated tests accordingly.
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py Updated the list_client_configs command to accept an output parameter, allowing configurations to be printed in JSON, YAML, or NAME format and added the decorator for output-all support.
packages/jumpstarter-cli-common/jumpstarter_cli_common/__init__.py
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py
Introduced new output mode definitions and type aliases (OutputMode, OutputType, NameOutputType, PathOutputType) along with options (opt_output_all, opt_output_name_only, opt_output_path_only, opt_nointeractive), enhancing overall CLI configurability.
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_config.py Enhanced exporter configuration listing with output formatting options, enabling JSON, YAML, or NAME output formats.
packages/jumpstarter-kubernetes/* (e.g., clients.py, exporters.py, leases.py, list.py, json.py, serialize.py, test_leases.py, pyproject.toml) Migrated Kubernetes resource models (clients, exporters, leases, lists) from dataclasses to Pydantic’s JsonBaseModel. Added serialization methods (dump_json, dump_yaml), new list classes, updated API methods to return typed lists, and expanded test coverage.
packages/jumpstarter/config/* (e.g., __init__.py, client.py, exporter.py) Updated client and exporter configuration models by introducing new list classes and refining serialization behavior. Method signatures for saving, loading, and dumping configurations were adjusted to return the correct types and formats.

Sequence Diagram(s)

sequenceDiagram
    participant U as User
    participant C as CLI Command
    participant R as Resource Operation
    participant P as Printer

    U->>C: Execute command (--nointeractive, --output format)
    C->>R: Process resource operation (create, delete, get, import)
    R-->>C: Return resource data
    alt Output option specified (JSON/YAML/NAME/PATH)
      C->>P: Call corresponding print function
      P-->>C: Return formatted output
    else Default table output
      C->>P: Build table representation
      P-->>C: Return table view
    end
    C-->>U: Display result
Loading

Possibly related PRs

  • Add mitmproxy driver for HTTP(S) interception and backend mocking jumpstarter#254: The changes in the main PR, which enhance the functionality of client and exporter creation with new parameters and output handling, are related to the modifications in the retrieved PR that rename the name attribute to alias in the ClientConfigV1Alpha1 class, as both involve updates to the client configuration management.

Suggested reviewers

  • bennyz
  • mangelajo

Poem

A curious rabbit hops along the code trail,
Where JSON, YAML, and names set sail.
No prompts disrupt my playful run,
Output shines like a radiant sun.
With each new print, my heart does leap—
Bugs nibble away while the features keep,
My joyful hops in code a merry tale to tell!


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 696a82c and a128a4c.

📒 Files selected for processing (5)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/__main__.py (1 hunks)
  • packages/jumpstarter-cli-client/jumpstarter_cli_client/__main__.py (1 hunks)
  • packages/jumpstarter-cli-driver/jumpstarter_cli_driver/__main__.py (1 hunks)
  • packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__main__.py (1 hunks)
  • packages/jumpstarter-cli/jumpstarter_cli/__main__.py (1 hunks)
✅ Files skipped from review due to trivial changes (5)
  • packages/jumpstarter-cli-driver/jumpstarter_cli_driver/main.py
  • packages/jumpstarter-cli/jumpstarter_cli/main.py
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/main.py
  • packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/main.py
  • packages/jumpstarter-cli-client/jumpstarter_cli_client/main.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • 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-devspace .devfile/Containerfile.client)
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 9, 2025

Deploy Preview for jumpstarter-docs ready!

Name Link
🔨 Latest commit a128a4c
🔍 Latest deploy log https://app.netlify.com/sites/jumpstarter-docs/deploys/67ce4af49b638b000883d68b
😎 Deploy Preview https://deploy-preview-335--jumpstarter-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (15)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/json.py (3)

5-14: Well-designed JSON and YAML serialization base class

The JsonBaseModel provides a solid foundation for JSON and YAML serialization across the codebase, using Pydantic for robust validation and serialization. This class nicely supports the PR's objective of enabling standardized output formats.

Consider enhancing the class documentation to include examples of usage and explain the purpose of each method in more detail.


8-9: Consider adding explicit error handling for JSON serialization

While Pydantic's model_dump_json handles most serialization errors internally, consider adding explicit try/except blocks to gracefully handle any unexpected serialization failures.

def dump_json(self):
-    return self.model_dump_json(indent=4, by_alias=True)
+    try:
+        return self.model_dump_json(indent=4, by_alias=True)
+    except Exception as e:
+        # Log the error or handle it appropriately
+        raise ValueError(f"Failed to serialize model to JSON: {e}") from e

11-12: Consider adding error handling for YAML serialization

Similar to the JSON serialization method, the YAML serialization could benefit from explicit error handling, especially since it involves both a Pydantic method and the yaml library.

def dump_yaml(self):
-    return yaml.safe_dump(self.model_dump(mode="json", by_alias=True), indent=2)
+    try:
+        model_dict = self.model_dump(mode="json", by_alias=True)
+        return yaml.safe_dump(model_dict, indent=2)
+    except Exception as e:
+        # Log the error or handle it appropriately
+        raise ValueError(f"Failed to serialize model to YAML: {e}") from e
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/list.py (2)

8-13: Generic list implementation looks good

The V1Alpha1List class provides a clean, generic implementation for representing lists of resources. The use of Pydantic's Field for aliases and defaults is appropriate.

Consider using TypeVar for better type safety:

-class V1Alpha1List[T](JsonBaseModel):
+from typing import TypeVar, Generic
+
+T = TypeVar('T')
+
+class V1Alpha1List(JsonBaseModel, Generic[T]):

This makes the generic type parameter more explicit and follows common patterns for generic classes in Python.


9-10: Enhance class documentation

The current documentation is minimal. Consider adding more details about the purpose of this class, how it's intended to be used, and examples of concrete implementations.

-    """A generic list result type."""
+    """A generic list result type for Jumpstarter resources.
+    
+    This class provides a standardized container for lists of resources,
+    following the Kubernetes-style API conventions with apiVersion and kind fields.
+    
+    Attributes:
+        api_version: The API version (always "jumpstarter.dev/v1alpha1")
+        items: The list of resources of type T
+        kind: The resource kind (always "List")
+        
+    Example:
+        ```python
+        class ClientList(V1Alpha1List[Client]):
+            kind: Literal["ClientList"] = Field(default="ClientList")
+        ```
+    """
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_config.py (1)

61-78: Consider handling empty list case for NAME output format.

While JSON, YAML, and table output formats handle empty lists gracefully, the NAME output format (lines 65-67) doesn't output anything when the exporters list is empty. Consider adding a consistent feedback message for this case to improve user experience.

 elif output == OutputMode.NAME:
     if len(exporters) > 0:
         click.echo(exporters[0].alias)
+    else:
+        click.echo("No exporter configs found")
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (1)

43-49: Consider standardizing help messages for output options.

There's a slight inconsistency in the help messages between opt_output_name_only ("resource/name") and opt_output_path_only ("file/path"). Consider standardizing the terminology for a more consistent user experience.

 opt_output_path_only = click.option(
     "-o",
     "--output",
     type=click.Choice([OutputMode.PATH]),
     default=None,
-    help='Output mode. Use "-o path" for shorter output (file/path).',
+    help='Output mode. Use "-o path" for shorter output (resource/path).',
 )

Also applies to: 53-59

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py (3)

7-8: Recommend clarifying relationship between OutputType and OutputMode.
The code checks if output == OutputMode.JSON later, but the argument type here is OutputType. Ensure consistent naming and usage to avoid confusion.


101-103: Clarify operator precedence in the condition.
The expression if save or out is not None or nointeractive is False and click.confirm(...): may be ambiguous. Wrap subexpressions in parentheses to avoid unexpected short-circuiting.

- if save or out is not None or nointeractive is False and click.confirm("Save client configuration?"):
+ if (save or out is not None) or (nointeractive is False and click.confirm("Save client configuration?")):

179-181: Apply parentheses for clarity in condition.
Same concern as in the client creation flow. Adding parentheses avoids ambiguity in the boolean expression.

- if save or out is not None or nointeractive is False and click.confirm("Save exporter configuration?"):
+ if (save or out is not None) or (nointeractive is False and click.confirm("Save exporter configuration?")):
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (1)

34-72: Potential overshadowing of built-in dict.
Using def from_dict(dict: dict) redefines the built-in. This can cause confusion in larger codebases. Consider renaming the parameter to data or payload.

- @staticmethod
- def from_dict(dict: dict):
+ @staticmethod
+ def from_dict(data: dict):
    return V1Alpha1Lease(
-       api_version=dict["apiVersion"],
-       kind=dict["kind"],
        ...
    )
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py (2)

37-48: Potential design choice in NAME output
In each of these printing functions (print_clients, print_exporters, print_leases), only the first item’s name is emitted in NAME mode. This differs from the typical kubectl pattern, which prints all resources by name. If you’d like to match kubectl behavior, consider iterating over all items.

Also applies to: 99-113, 163-174


134-150: Consider safe iteration in make_lease_row
If lease.spec.selector is ever None, iterating labels could fail. Adding a None check for lease.spec.selector might prevent unexpected AttributeError.

packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (1)

21-23: V1Alpha1ClientStatus endpoint is non-optional
If a client might not have an endpoint at creation, you could make endpoint optional. Otherwise, looks fine for guaranteed endpoints.

packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (1)

35-57: Consider leveraging more of Pydantic's parsing capabilities

While the from_dict method works correctly, consider using Pydantic's built-in parsing to handle validation and error reporting more robustly.

@staticmethod
def from_dict(dict: dict):
-    return V1Alpha1Exporter(
-        api_version=dict["apiVersion"],
-        kind=dict["kind"],
-        metadata=V1ObjectMeta(
-            creation_timestamp=dict["metadata"]["creationTimestamp"],
-            generation=dict["metadata"]["generation"],
-            name=dict["metadata"]["name"],
-            namespace=dict["metadata"]["namespace"],
-            resource_version=dict["metadata"]["resourceVersion"],
-            uid=dict["metadata"]["uid"],
-        ),
-        status=V1Alpha1ExporterStatus(
-            credential=V1ObjectReference(name=dict["status"]["credential"]["name"])
-            if "credential" in dict["status"]
-            else None,
-            endpoint=dict["status"]["endpoint"],
-            devices=[V1Alpha1ExporterDevice(labels=d["labels"], uuid=d["uuid"]) for d in dict["status"]["devices"]]
-            if "devices" in dict["status"]
-            else [],
-        ),
-    )
+    # Let Pydantic handle validation and parsing
+    return V1Alpha1Exporter.model_validate(dict)

This approach would use Pydantic's built-in validation, which can handle missing fields more gracefully and provide better error messages when parsing fails.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8b11ad6 and d217883.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (27)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py (7 hunks)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py (5 hunks)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete.py (3 hunks)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete_test.py (4 hunks)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py (3 hunks)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py (14 hunks)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.py (5 hunks)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res_test.py (3 hunks)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py (1 hunks)
  • packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py (3 hunks)
  • packages/jumpstarter-cli-common/jumpstarter_cli_common/__init__.py (2 hunks)
  • packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (2 hunks)
  • packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_config.py (2 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/__init__.py (1 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (3 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients_test.py (1 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (2 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters_test.py (1 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/json.py (1 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (2 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/list.py (1 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/serialize.py (1 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py (1 hunks)
  • packages/jumpstarter-kubernetes/pyproject.toml (2 hunks)
  • packages/jumpstarter/jumpstarter/config/__init__.py (2 hunks)
  • packages/jumpstarter/jumpstarter/config/client.py (5 hunks)
  • packages/jumpstarter/jumpstarter/config/exporter.py (6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: pytest-matrix (3.13)
  • GitHub Check: pytest-matrix (3.12)
  • GitHub Check: e2e
  • GitHub Check: pytest-matrix (3.11)
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (121)
packages/jumpstarter-kubernetes/pyproject.toml (4)

10-13: Good addition of Pydantic dependency

The addition of the Pydantic dependency aligns well with the PR objective of adding JSON and YAML output options, as it provides robust data validation and serialization capabilities. The version constraint is appropriate and ensures compatibility with modern Pydantic features.


5-5: Formatting change looks good

The authors section has been reformatted from a multi-line to a single-line format, which is consistent with modern TOML styling practices.


18-21: Consistent formatting of dev dependencies

The dev dependencies section has been reformatted to match the main dependencies section, providing a consistent style throughout the file.


30-30: Minor formatting fix

The spacing around curly braces in the raw-options property has been adjusted for better readability.

packages/jumpstarter/jumpstarter/config/__init__.py (2)

1-9: Good addition of list types to imports

The addition of ClientConfigListV1Alpha1 and ExporterConfigListV1Alpha1 to the imports aligns well with the PR objective of supporting standardized output formats for collections of resources.


22-26: Proper update to all list

The __all__ list has been correctly updated to include the new types, ensuring they're available when importing from this module.

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res_test.py (4)

2-2: Good addition of Path import

Adding the Path import is necessary for the updated test cases that verify the path output functionality.


59-64: LGTM: Non-interactive mode test is well implemented

The test for the new --nointeractive flag properly verifies that client configurations can be saved without user prompts, matching the PR goal of improving CLI automation.


74-83: Comprehensive test for path output option

This test correctly verifies the new --output path functionality, ensuring that the command correctly returns only the path to the saved file instead of success messages. The mock return value update on line 76 is essential to support this functionality.


137-143: Good parallel implementation for exporter path output

The test for exporter path output matches the client implementation, maintaining consistency in behavior across resource types. This ensures that both client and exporter commands provide the same user experience.

packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters_test.py (3)

1-21: Well-structured test setup with comprehensive test data

The test exporter object is created with complete metadata and status fields, providing thorough coverage for serialization testing. This approach will help catch any serialization issues with complex nested objects.


24-69: Thorough JSON serialization test

The test validates that the dump_json method produces the expected JSON output including all nested properties. The comprehensive expected output validates that the serializer handles complex nested objects correctly.


71-107: Complete YAML serialization test

The test for YAML serialization complements the JSON test, ensuring both output formats work correctly. Having both tests provides complete coverage of the output options in the PR objectives.

packages/jumpstarter/jumpstarter/config/client.py (6)

10-10: Updated imports to support enhanced serialization

The addition of ConfigDict import from Pydantic is necessary for the new configuration options in the added ClientConfigListV1Alpha1 class.


40-41: Field definition improvement for serialization support

Removing the exclude=True parameter allows these fields to be included in model dumps when needed, which supports the enhanced output options being added.


186-186: Method return type enhancement

Updating the save method to return a Path object enables the new path output option, allowing CLI commands to display the path to the saved file.


196-197: Improved YAML dumping with exclusions

The updated YAML dumping excludes path and alias fields, which aren't part of the standard configuration and shouldn't be persisted, while returning the path for programmatic usage.


201-201: Consistent exclusion in dump_yaml method

This change ensures the same fields are excluded in both the save method and the dump_yaml method, maintaining consistency in the serialized output.


234-247: Well-designed ClientConfigListV1Alpha1 class

This new class provides a structured way to represent multiple client configurations, with proper serialization methods for both JSON and YAML. The model configuration enables arbitrary types and naming consistency with the API.

packages/jumpstarter-kubernetes/jumpstarter_kubernetes/serialize.py (3)

1-5: Appropriate imports for serialization functionality

The imports provide all necessary types and utilities for the serialization functionality, including Pydantic's WrapSerializer for custom serialization behavior.


7-10: Clean and efficient serialization function

The k8s_obj_to_dict function effectively converts Kubernetes objects to dictionaries while filtering out None values, which produces cleaner output and is a best practice for serialization.


12-14: Type annotations for consistent serialization

These type annotations using WrapSerializer provide a clean, reusable way to handle serialization of common Kubernetes objects across the codebase, promoting consistency and reducing code duplication.

packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py (4)

1-4: Good imports and module structuring

The imports are well organized, clearly separating the Kubernetes async client models from the Jumpstarter-specific classes.


5-32: Well-structured test fixture with comprehensive test data

The TEST_LEASE constant provides a complete representation of a lease object with all required fields, which is excellent for testing the serialization methods. The test data includes metadata, spec, and status fields with realistic values.


35-99: Good test for JSON serialization

The test validates that the dump_json() method correctly serializes the lease object to a properly formatted JSON string. All expected fields are present in the output, maintaining the proper structure and relationships.


102-156: Good test for YAML serialization

The test confirms that the dump_yaml() method correctly serializes the lease object to a properly formatted YAML string. The YAML representation maintains the same data structure as the JSON format but in YAML syntax.

packages/jumpstarter-kubernetes/jumpstarter_kubernetes/__init__.py (2)

1-11: Well-organized imports with clear grouping

The imports have been updated to include the new list classes for clients, exporters, and leases. The parentheses for the exporters imports improve readability for the multi-line import. The V1Alpha1List base class import is also appropriately added.


13-32: Consistent all list updated with new exports

The all list has been properly updated to include the new list classes, maintaining alphabetical ordering within logical groupings. This ensures that the new classes are properly exposed for import from the package.

packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients_test.py (3)

1-17: Good test fixture for client serialization

The TEST_CLIENT constant is well-structured with all necessary fields for testing serialization. The use of V1ObjectMeta for metadata is consistent with Kubernetes patterns.


20-45: Thorough test for JSON serialization of client object

The test properly validates that the client object can be serialized to a correctly formatted JSON string, with all expected fields included.


47-69: Thorough test for YAML serialization of client object

The test ensures that the client object can be serialized to a correctly formatted YAML string, maintaining data consistency with the JSON representation.

packages/jumpstarter-cli-common/jumpstarter_cli_common/__init__.py (2)

2-16: Well-organized imports with clear grouping of output-related types

The imports have been expanded to include the new output-related types and options, using parentheses for improved readability. The organization makes it clear which options are related to output formatting.


21-40: Properly updated all list with new exports

The all list has been extended to include all the new output-related types and options, maintaining a logical grouping and making these entities available for import from the package.

packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_config.py (1)

55-59: Good implementation of multiple output formats.

The function now accepts an output parameter following the PR objectives to support JSON, YAML, and NAME formats, with proper decoration using @opt_output_all.

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py (6)

37-40: Good update to include credential field in V1Alpha1ClientStatus.

The addition of the credential field with a V1ObjectReference properly aligns with the model structure used in Kubernetes.


42-70: Well-structured JSON and YAML templates for client objects.

These templates provide excellent examples of the expected output format, making them valuable for testing the new output format functionality.


143-169: Comprehensive test coverage for non-interactive client creation.

The tests effectively verify all the new output formats (JSON, YAML, and NAME) and confirm that the configuration isn't saved in non-interactive mode, which is crucial behavior.


184-185: Good enhancement of V1Alpha1ExporterStatus with credential field.

Similar to the client object, this properly includes the credential reference in the exporter status.


188-218: Well-structured JSON and YAML templates for exporter objects.

These templates are consistent with the client templates and provide clear examples of the expected output formats.


269-295: Good test coverage for non-interactive exporter creation.

The tests thoroughly verify all the supported output formats and confirm the expected behavior in non-interactive mode.

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete_test.py (3)

128-157: Good test coverage for non-interactive client deletion.

These tests verify that the non-interactive flag works correctly for client deletion and that the NAME output format returns the expected value.


170-173: Proper update to include credential field in V1Alpha1ExporterStatus.

The addition of the credential field with a V1ObjectReference matches the changes made in other files, ensuring consistency across the codebase.


234-258: Good test coverage for non-interactive exporter deletion.

The tests effectively verify both the non-interactive flag and the NAME output format for exporter deletion, ensuring the new functionality works as expected.

packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (3)

24-29: Good definition of OutputMode class.

The OutputMode class clearly defines the supported output formats as string constants, making them easy to reference throughout the codebase.


33-39: Well-implemented output option for all formats.

The opt_output_all option supports JSON, YAML, and NAME output formats with a clear help message explaining the usage.


61-63: Good implementation of non-interactive option.

The opt_nointeractive option is well-defined with a clear help message indicating its purpose for scripting scenarios.

packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py (5)

4-9: Well-structured imports to support new output formats

The addition of OutputMode, OutputType, and opt_output_all imports is appropriate for implementing the new output formatting options mentioned in the PR.


12-18: Well-organized config imports

Good organization of imports with the addition of ClientConfigListV1Alpha1 which is needed for the structured output formats.


121-123: Good use of decorator for CLI option handling

The @opt_output_all decorator and updated function signature properly implement the output options described in the PR objectives, allowing for JSON, YAML, and NAME formats.


132-139: Clean implementation of JSON, YAML, and NAME output formats

The conditional logic for handling different output formats is clear and follows good practices. Using the ClientConfigListV1Alpha1 class with its built-in serialization methods ensures consistent output formatting.


140-151: Preserved backward compatibility with table output

The implementation maintains backward compatibility by keeping the original table output format as the default behavior, which is a good practice when adding new features.

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete.py (8)

5-14: Good import organization for new CLI options

The addition of NameOutputType, opt_nointeractive, and opt_output_name_only imports properly supports the new CLI options for output control and automation.


36-50: Well-structured decorators for the delete_client command

The addition of the @opt_output_name_only and @opt_nointeractive decorators enhances the CLI interface by supporting structured output formats and non-interactive mode, which aligns perfectly with the PR objectives.


51-58: Clear function signature updates for enhanced CLI options

The function signature is properly updated to include the new output and nointeractive parameters, maintaining backward compatibility while adding new functionality.


63-66: Good conditional output formatting

The implementation correctly checks the output mode and formats the response accordingly. Using resource-typed paths (client.jumpstarter.dev/{name}) follows good Kubernetes-like conventions.


68-70: Proper handling of non-interactive mode for automation

The condition to check for non-interactive mode before prompting for confirmation is well-implemented, allowing for automation when using the --nointeractive flag.


78-80: Consistent output handling for confirmation messages

The code consistently checks the output mode before printing confirmation messages, maintaining a clean user experience in both interactive and non-interactive modes.


98-108: Consistent implementation for delete_exporter command

The changes to the delete_exporter command follow the same pattern as delete_client, ensuring a consistent CLI experience across different resource types.


113-124: Consistent output handling in delete_exporter

The output handling in the delete_exporter function mirrors that of delete_client, maintaining a consistent approach throughout the codebase.

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.py (7)

4-11: Good import organization for new CLI options

The addition of PathOutputType, opt_nointeractive, and opt_output_path_only imports properly supports the new CLI options for output control and automation in import operations.


47-52: Well-structured decorators for the import_client command

The addition of the @opt_output_path_only and @opt_nointeractive decorators enhances the CLI interface by supporting structured output formats and non-interactive mode for import operations.


53-62: Clear function signature updates for enhanced CLI options

The function signature is properly updated to include the new output and nointeractive parameters, maintaining backward compatibility while adding new functionality to the import_client command.


69-74: Proper handling of non-interactive mode

The conditional logic for handling non-interactive mode is well-implemented, bypassing interactive prompts when the --nointeractive flag is set, which is essential for automation.


75-88: Good conditional output handling

The implementation correctly checks the output mode before printing messages and handles the output path appropriately, providing a clean user experience in both interactive and non-interactive modes.


105-115: Consistent implementation for import_exporter command

The changes to the import_exporter command follow the same pattern as import_client, ensuring a consistent CLI experience across different resource types.


125-132: Consistent output handling in import_exporter

The output handling in the import_exporter function is implemented consistently with the approach used in import_client, maintaining a uniform pattern throughout the codebase.

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py (12)

41-78: Well-structured test data for client output formats

The test data constants for client objects in various formats (JSON, YAML) are well-defined and accurately represent the expected output structures, providing a solid foundation for testing.


95-114: Comprehensive test cases for client output formats

The test cases for client JSON, YAML, and NAME output formats are thorough and verify the correct behavior of the CLI commands with different output options.


130-226: Thorough test data for client list output formats

The test data constants for client lists in various formats are comprehensive and include both populated and empty list scenarios, which is excellent for complete test coverage.


244-291: Good coverage of client list output formats

The test cases properly verify all output formats for client lists, including edge cases like empty lists, ensuring that the CLI behaves correctly in all scenarios.


294-333: Well-structured test data for exporter output formats

Similar to the client test data, the exporter test data is well-organized and covers all output formats, maintaining consistency in testing approach.


350-369: Good test coverage for exporter output formats

The test cases for exporter output formats follow the same comprehensive approach as the client tests, ensuring consistent behavior across resource types.


384-447: Comprehensive test data for exporters with devices

The test data for exporters with devices is thorough and accurately represents complex nested structures in different output formats.


466-485: Good test coverage for exporter devices output formats

The tests properly verify all output formats for exporters with devices, ensuring that complex nested structures are correctly serialized in different formats.


865-926: Comprehensive test data for lease output formats

The test data constants for leases are thorough and accurately represent the expected structures in different output formats.


964-983: Good test coverage for lease output formats

The test cases for lease output formats are comprehensive and verify the correct behavior of the CLI commands with different output options.


998-1129: Thorough test data for lease lists

The test data for lease lists is well-structured and comprehensive, covering the complexity of lease objects in different output formats.


1157-1176: Good test coverage for lease list output formats

The test cases for lease list output formats follow the same comprehensive approach as other resource types, ensuring consistent behavior across the CLI.

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py (12)

14-15: Validate interactive logic with non-interactive flag.
Adding opt_nointeractive and opt_output_all is useful, but confirm that related prompts are skipped or included appropriately, preventing unexpected user interaction.


17-17: No concerns.
The new imports from jumpstarter_kubernetes look correct for the usage below.


40-47: Output printing function design looks good.
This function correctly handles JSON, YAML, and name-based outputs according to the specified mode. The code cleanly separates different output formats.


77-78: Streamline CLI decorators.
Stacking @opt_nointeractive and @opt_output_all is consistent with the overall pattern. No issues found.


90-91: Ensure default output behavior is intuitive.
The function signature includes nointeractive and output. Check if a None output gracefully defaults to a console/table format, matching user expectations.


96-99: Restrict extraneous console logs for non-JSON/YAML outputs.
The condition if output is None: is a neat way to limit console messages. This aligns with the new format-based approach.


120-122: Confirm final output messages are suppressed for JSON/YAML.
The condition if output is None: ensures minimal console feedback when users prefer structured output.


129-136: Output function design for exporters is consistent.
Mirroring the approach used for clients helps maintain consistent UX. No issues detected.


158-159: Retaining consistent decorators.
Same logic as the client creation. Looks good.


169-170: Same parameter arrangement for exporters.
Parallel structure for nointeractive and output is appropriate, matching the client creation workflow.


175-177: Limit console feedback in non-JSON/YAML modes.
if output is None: check ensures structured outputs remain clean.


184-186: Consistent save logic with user notification.
The final message is displayed only for the default (non-structured) mode, achieving the desired interactive experience.

packages/jumpstarter/jumpstarter/config/exporter.py (10)

10-10: No issues with import changes.
The import from pydantic is appropriate for model classes.


72-72: Include alias in the model.
Defining alias: str = "default" is convenient to identify each exporter config. This ensures clarity in referencing.


74-75: Usage of default apiVersion and kind.
Setting these as fields with default literal values is consistent with other CRD-based models.


84-84: Optional path field.
Allowing path: Path | None to be set later is a flexible approach. No concerns.


102-102: Type hint using -> Self.
Makes the code more concise in Python 3.11+, and clarifies that the class returns its own instance.


108-108: Implementing list() using list[Self].
Good approach for type-checking. No immediate concerns.


116-117: Excluding fields from YAML dump.
Excluding alias and path matches existing patterns. Verify that the omission of these fields is intentional.


120-121: Save method now returning a path is clear.
Returning the storage path clarifies usage for call sites that need confirmation of where the file is saved.


128-129: Safe dumping excludes certain fields.
Mirror logic from dump_yaml. This ensures the model's alias and path remain internal while saving.


171-183: New ExporterConfigListV1Alpha1 class is well-structured.
Providing dump_json and dump_yaml with model_dump ensures consistent output. This fosters easier batch export of exporter configs.

packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (8)

4-4: Expanded import usage.
Switching to Field from pydantic is consistent with the changes below.


6-8: Adoption of JsonBaseModel & new utilities.
Migrating to these classes is aligned with your approach to unify serialization logic.


12-17: Revised V1Alpha1LeaseStatus fields.
Aliasing with Field(alias="...") helps ensure JSON keys match the CRD spec. Properly marks optional fields.


20-21: Adding a typed field for client.
Using SerializeV1ObjectReference clarifies the reference type. No issues.


26-31: New Pydantic model for V1Alpha1Lease.
The default api_version and kind fields are a standard approach for CRD-based models.


77-82: V1Alpha1LeaseList class mirrors the single object structure.
Returning items as typed instances enhances clarity for consumers.


85-86: LeasesV1Alpha1Api docstring & concurrency approach.
The docstring clarifies this CRD's usage, and the approach for listing leases asynchronously is consistent with existing patterns.

Also applies to: 88-88, 90-90, 93-93


100-101: Single lease retrieval.
Invoking from_dict is correct to maintain consistent Pydantic-based initialization. No concerns.

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py (6)

1-16: Imports and constants look consistent
This block correctly sets up the environment and includes the essential dependencies for output formatting.


18-23: make_client_row handles missing status gracefully
Returning an empty string for the endpoint when client.status is None is good, preventing attribute errors. No issues found.


26-35: print_client output logic is clear
All supported output modes (JSON, YAML, NAME, table) look well-implemented.


85-97: print_exporter follows the same output pattern
Features consistent JSON/YAML/NAME/table logic. No new issues identified.


118-132: get_reason logic covers known lease statuses
The conditional checks for “Ready” and “Expired” are implemented well.


152-160: print_lease is consistent
Follows the output modes pattern (JSON, YAML, NAME, table) with no issues.

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py (1)

44-47: New output and devices parameters integrated consistently
The updates in get_client, get_exporter, and get_lease commands align well with the new print functions, providing flexible output modes. No issues found.

Also applies to: 63-63, 68-68, 71-76, 100-101

packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (6)

6-11: Good refactoring toward structured model serialization

The imports have been updated to support the transition from standard classes to Pydantic's JsonBaseModel. This change aligns with the PR objective of adding structured output formats (JSON, YAML) to the CLI tools.


18-20: Transition to JsonBaseModel improves serialization capabilities

Converting the exporter-related classes to inherit from JsonBaseModel is a good approach for supporting multiple output formats. The use of Pydantic's Field with aliases for apiVersion and other camelCase fields ensures proper serialization.

Also applies to: 23-26, 29-33


60-66: Excellent addition of List model for supporting multiple results

The new V1Alpha1ExporterList class integrates well with the V1Alpha1List generic type and provides a clean way to handle collections of exporters. This enables proper serialization of lists for the CLI output formats.


71-77: Updated return type and deserialization approach

The list_exporters method now correctly returns a typed list and uses the new from_dict method for deserialization. This change supports the new CLI output formats while maintaining backward compatibility.


78-84: Consistent deserialization pattern

The get_exporter method has been updated to use the same deserialization pattern as list_exporters, which ensures consistent handling of exporter objects throughout the codebase.


114-116: Final deserialization update for create operation

Updating the final return in create_exporter completes the refactoring across all API methods. This consistent approach ensures all exporter objects can be properly serialized for the new CLI output formats.

Comment thread packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py
Comment thread packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py
Comment thread packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py
Comment thread packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients_test.py (1)

1-59: Consider adding tests for edge cases and error conditions.

While the current tests verify the basic functionality of the serialization methods, consider adding tests for:

  1. Objects with minimal required fields
  2. Objects with all optional fields populated
  3. Error handling for invalid inputs

This would improve test coverage and ensure robust handling of various scenarios.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d217883 and 5276158.

📒 Files selected for processing (3)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients_test.py (1 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters_test.py (1 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters_test.py
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: pytest-matrix (3.13)
  • GitHub Check: pytest-matrix (3.12)
  • GitHub Check: e2e
  • GitHub Check: pytest-matrix (3.11)
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (3)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients_test.py (3)

5-17: Well-structured test fixture creation.

The TEST_CLIENT constant provides a comprehensive test fixture with realistic values for all required fields. This is a good practice for testing serialization methods.


20-39: Well-implemented JSON serialization test.

The test effectively validates that the dump_json() method produces correctly formatted JSON output with the expected structure and indentation.


42-58: Well-implemented YAML serialization test.

The test properly validates that the dump_yaml() method produces correctly formatted YAML output that matches the expected structure.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_config.py (1)

75-92: Output handling provides flexible formats

The implementation cleanly supports multiple output formats (JSON, YAML, NAME) with appropriate formatting for each. The table output fallback provides good human-readable output when specific formats aren't requested.

Document NAME output behavior

When using OutputMode.NAME, only the first exporter's alias is displayed, even if multiple exporters exist. Consider documenting this behavior or modifying it to handle multiple results more explicitly if this output is intended for automation.

packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py (2)

142-161: Well-structured output handling

The implementation effectively supports multiple output formats with appropriate formatting for each type. The table output provides a clean human-readable format when specific machine formats aren't requested.

Document NAME output behavior

When using OutputMode.NAME, only the first client's alias is displayed, even if multiple clients exist. Consider documenting this behavior or modifying it to handle multiple results more explicitly if this output is intended for automation.


171-173: Handle potential None return

The use_client method might return None if there's an issue with the client path, but there's no error handling here. Consider adding a check to handle this case gracefully.

def use_client_config(name: str, output: PathOutputType):
    """Select the current Jumpstarter client configuration to use."""
    user_config = UserConfigV1Alpha1.load_or_create()
    path = user_config.use_client(name)
-    if output == OutputMode.PATH:
+    if output == OutputMode.PATH and path:
        click.echo(path)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5276158 and 08958d8.

📒 Files selected for processing (5)
  • packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py (6 hunks)
  • packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_config.py (4 hunks)
  • packages/jumpstarter/jumpstarter/config/client.py (5 hunks)
  • packages/jumpstarter/jumpstarter/config/exporter.py (6 hunks)
  • packages/jumpstarter/jumpstarter/config/user.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/jumpstarter/jumpstarter/config/exporter.py
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: Redirect rules - jumpstarter-docs
  • GitHub Check: Header rules - jumpstarter-docs
  • GitHub Check: Pages changed - jumpstarter-docs
  • 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)
🔇 Additional comments (4)
packages/jumpstarter/jumpstarter/config/client.py (2)

235-247: LGTM! New ClientConfigListV1Alpha1 class enhances serialization capabilities.

The added class implements proper serialization methods for both JSON and YAML formats, aligning perfectly with the PR's objective to support multiple output options. The class structure provides a clean way to handle collections of client configurations.


186-197: Return type and behavior change enhances the API.

Adding the -> Path return type and explicitly returning the config path improves the function's usability, especially when combined with the new output options. This change enables the CLI to properly display the path when requested via the path output mode.

packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_config.py (1)

38-41: Path output handling works as expected

The addition of conditional path output aligns with the PR objectives and provides a clean way to display the path when explicitly requested.

packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py (1)

125-127: Path output follows consistent pattern

The implementation of path output for delete operations follows the same pattern as create operations, maintaining consistency across the codebase.

Comment thread packages/jumpstarter/jumpstarter/config/client.py
Comment thread packages/jumpstarter/jumpstarter/config/user.py Outdated
Comment thread packages/jumpstarter/jumpstarter/config/user.py Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (2)

33-52: Consider safer dictionary lookups and avoid overshadowing built-in name.

  1. This method accesses fields like "apiVersion", "kind", and "metadata" directly, which can raise KeyError if the raw dictionary lacks these keys. Consider using .get or validating input earlier.
  2. At line 46, dict["status"]["credential"]["name"] may raise a KeyError if "name" is missing or credential is empty. A safer approach is:
    credential=(
        V1ObjectReference(name=dict["status"]["credential"].get("name", ""))
        if "credential" in dict["status"]
        else None
    )
  3. The function parameter dict: dict overshadows the built-in dict type, which can cause confusion. Consider renaming the function argument to something like data or raw_dict.

64-65: Class docstring offers good clarity.

The short docstring succinctly describes the purpose of this API class for clients. Additional detail can be added if needed, but this is acceptable.

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py (2)

19-23: Consider handling a None endpoint more gracefully
If client.status is not None but client.status.endpoint is None, the table would display "None". A small refinement could default to an empty string or "N/A" to avoid confusion in the UI.

    return {
        "NAME": client.metadata.name,
-       "ENDPOINT": client.status.endpoint if client.status is not None else "",
+       "ENDPOINT": client.status.endpoint if (client.status and client.status.endpoint) else "",
        "AGE": time_since(client.metadata.creation_timestamp),
    }

37-49: Refactor repeated single/multiple resource printing paths
Each function handling multiple resources has similar logic for JSON, YAML, NAME, or table outputs. Consider a unified helper or shared approach to avoid duplication and centralize future updates.

Also applies to: 101-115, 165-177

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 99f09db and 59dcf46.

📒 Files selected for processing (3)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py (1 hunks)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (3 hunks)
  • packages/jumpstarter/jumpstarter/config/user.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/jumpstarter/jumpstarter/config/user.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: pytest-matrix (3.13)
  • GitHub Check: pytest-matrix (3.12)
  • GitHub Check: e2e
  • GitHub Check: pytest-matrix (3.11)
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (11)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (7)

7-11: Use of Pydantic and local modules is appropriate.

The new imports from Pydantic and local modules are well-structured and correctly reference the necessary classes for model serialization. No issues found here.


21-22: Optional credential field is clear and consistent.

Declaring credential as an optional field of type SerializeV1ObjectReference clearly indicates that a client may not always have an associated credential. This design choice is coherent with the rest of the code.


26-30: Pydantic model for client resource is well-defined.

Defining api_version and kind as Literal types, along with metadata and an optional status, aligns with best practices for typed resource models. Using Field for aliasing and defaults is also correct.


56-57: Client list model is consistent with standard Kubernetes resource lists.

Declaring the kind field as "ClientList" and extending V1Alpha1List ensures consistent behavior with other Kubernetes-style list objects.


59-61: Robust fallback for missing “items” key.

Using dict.get("items", []) prevents potential KeyErrors and gracefully handles empty or missing items. This aligns well with prior suggestions from older commits.


97-97: Reusing static method for updated client is consistent.

Returning V1Alpha1Client.from_dict(updated_client) neatly centralizes client instantiation logic. This is well-structured for maintainability.


102-102: Consistent list & get operations.

The methods list_clients and get_client also leverage from_dict, ensuring a uniform creation process for all resources. This promotes reusability and consistent handling of the client data model.

Also applies to: 107-107, 114-114

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py (4)

43-45: Revisit NAME output for multiple client resources
Currently, you only print the first client’s name when OutputMode.NAME is selected, which deviates from typical CLI behavior (e.g., kubectl get pods -o name prints all items).

Should this be updated to print each client on a new line? For instance:

-        if len(clients.items) > 0:
-            click.echo(f"client.jumpstarter.dev/{clients.items[0].metadata.name}")
+        for c in clients.items:
+            click.echo(f"client.jumpstarter.dev/{c.metadata.name}")

69-70: Handle None devices to avoid iteration errors
If e.status.devices is None, iterating over it will raise a TypeError.

-            for d in e.status.devices:
+            if e.status.devices:
+                for d in e.status.devices:
+                    ...

107-108: Revisit NAME output for multiple exporter resources
You only print the first exporter’s name rather than all exporters. Confirm if that is intended or if you prefer a list-like output.

-        if len(exporters.items) > 0:
-            click.echo(f"exporter.jumpstarter.dev/{exporters.items[0].metadata.name}")
+        for exp in exporters.items:
+            click.echo(f"exporter.jumpstarter.dev/{exp.metadata.name}")

171-172: Revisit NAME output for multiple lease resources
Similar to clients and exporters, you only print the first lease’s name in NAME mode. Please confirm whether you want to print all leases.

-        if len(leases.items) > 0:
-            click.echo(f"lease.jumpstarter.dev/{leases.items[0].metadata.name}")
+        for l in leases.items:
+            click.echo(f"lease.jumpstarter.dev/{l.metadata.name}")

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/jumpstarter-cli-common/jumpstarter_cli_common/version.py (1)

36-40: Consider standardizing indentation between JSON and YAML outputs.

The JSON output uses 4-space indentation while YAML uses 2-space indentation. Consider standardizing both to 2-space for consistency, or documenting why they differ.

-    def dump_json(self):
-        return self.model_dump_json(indent=4, by_alias=True)
+    def dump_json(self):
+        return self.model_dump_json(indent=2, by_alias=True)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 59dcf46 and 696a82c.

📒 Files selected for processing (3)
  • packages/jumpstarter-cli-common/jumpstarter_cli_common/version.py (2 hunks)
  • packages/jumpstarter-cli-common/pyproject.toml (2 hunks)
  • packages/jumpstarter-kubernetes/pyproject.toml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/jumpstarter-kubernetes/pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: pytest-matrix (3.13)
  • GitHub Check: pytest-matrix (3.12)
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
  • GitHub Check: pytest-matrix (3.11)
  • GitHub Check: e2e
  • GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (9)
packages/jumpstarter-cli-common/pyproject.toml (4)

5-5: Author Consolidation Update

The authors field is now consolidated into a single entry with the specified name and email. Please verify that "kbrauer@hatci.com" is the correct email address intended for contact.


10-16: Dependency List Update and Reformatting

The dependencies list has been reformatted for consistency and now includes the new dependency "pydantic>=2.8.2". Confirm that the versions for all listed dependencies meet your project’s requirements.


21-24: Development Dependencies Formatting

The dev dependency group has been updated to a more concise list format. Please ensure that the specified versions for pytest and its plugins are appropriate for your testing framework.


36-36: Raw-options Formatting Improvement

The raw-options entry now includes a space before the closing brace for improved consistency. Verify that this change does not interfere with any tooling or parsing mechanisms in your build process.

packages/jumpstarter-cli-common/jumpstarter_cli_common/version.py (5)

6-7: LGTM: Good choice of libraries for the new output formats.

The addition of YAML and Pydantic provides robust serialization capabilities consistent with the PR objectives of adding JSON and YAML output options.


30-35: Well-structured Pydantic model for versioning.

The JumpstarterVersion class effectively uses Pydantic features:

  • Field aliases for standard JSON naming conventions
  • ConfigDict with populate_by_name for flexible initialization
  • Clean organization of version attributes

This makes the version information easily serializable to structured formats.


43-44: LGTM: Simple and effective factory function.

This function efficiently creates a version object with the current package and Python versions.


47-56: Effective implementation of the output formatting requirements.

The updated version command correctly implements the requirements of the PR by:

  1. Adding support for different output formats (JSON/YAML)
  2. Maintaining backward compatibility with the text output
  3. Using the new decorator for output format options

This implementation pattern will likely be reused across other CLI commands as mentioned in the PR objectives.


9-9:

Details

❓ Verification inconclusive

Verify that the imported OutputMode and OutputType include all required formats.

The imports from .opt module align with the PR objective of supporting various output formats.


🏁 Script executed:

#!/bin/bash
# Check what output modes are defined in the .opt module
cat packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py | grep -A 20 "class OutputMode" || echo "OutputMode not found"

Length of output: 551


Action Required: Confirm All Output Formats Are Exposed via CLI

The lookup in the .opt module shows that the OutputMode class defines four formats—JSON, YAML, NAME, and PATH—but the CLI option opt_output_all currently only allows JSON, YAML, and NAME. Please verify whether the omission of PATH from the CLI’s valid choices is intentional or if it should also be supported to meet the PR’s objective for comprehensive output formats.

  • Check if PATH is meant for internal use only or should be added to the click.Choice list in opt_output_all.
  • If PATH should be exposed, update the CLI option accordingly and ensure OutputType reflects the change.

)
@click.option("--unsafe", is_flag=True, help="Should all driver client packages be allowed to load (UNSAFE!).")
@click.option(
"-o",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to aligning with kubectl :)

Copy link
Copy Markdown
Member

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great

@mangelajo mangelajo changed the title Add JSON and YAML output options for the jmp-admin, jmp-admin, and jmp-exporter CLIs Add JSON and YAML output options for the jmp-admin, jmp-client, and jmp-exporter CLIs Mar 10, 2025
@mangelajo mangelajo merged commit 23254f4 into main Mar 10, 2025
@NickCao NickCao deleted the cli-json-output branch March 31, 2025 15:44
@mangelajo mangelajo added this to the 0.6.0 milestone May 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants