Dedup output mode handling #386
Conversation
WalkthroughThe pull request refactors the output handling and serialization mechanisms across multiple components. Local definitions and repeated serialization methods have been removed. Instead, a new common module now provides an Changes
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
4-4: 💡 Verification agent🧩 Analysis chain
Fix import error causing test failures
There's an import error in the CI pipeline:
ImportError: cannot import name 'OutputType' from 'jumpstarter_cli_common.opt'. The import is likely broken becauseOutputTypehas been moved from its original location as part of this refactoring.
🏁 Script executed:
#!/bin/bash # Check where OutputType is actually defined now grep -r "OutputType" --include="*.py" .Length of output: 6064
Fix incorrect re-export of OutputType
Our CI failure (“ImportError: cannot import name 'OutputType' from 'jumpstarter_cli_common.opt'”) indicates that the
OutputTypesymbol is still being imported from the old location injumpstarter_cli_common.opt. However, the grep results show that its new definition now lives inpackages/jumpstarter/jumpstarter/common/pydantic.py(where it’s defined asOutputType = OutputMode | None). To resolve the issue, please update the re-export in the common module—specifically in files likepackages/jumpstarter-cli-common/jumpstarter_cli_common/__init__.py(and any other modules that import from.opt, such asversion.py)—so thatOutputTypeis imported from its new location rather than fromjumpstarter_cli_common.opt.
- Action Required: Change the corresponding import(s) in the common module to something like:
instead of importing it fromfrom jumpstarter.jumpstarter.common.pydantic import OutputType.opt.packages/jumpstarter-cli/jumpstarter_cli/create.py (1)
6-6: 💡 Verification agent🧩 Analysis chain
Fix import error causing test failures
There's an import error in the CI pipeline:
ImportError: cannot import name 'OutputType' from 'jumpstarter_cli_common.opt'. The import is likely broken becauseOutputTypehas been moved from its original location as part of this refactoring.
🏁 Script executed:
#!/bin/bash # Check in common files to see where OutputType is defined find packages/jumpstarter-cli-common -name "*.py" -exec grep -l "OutputType" {} \;Length of output: 280
Update the import in
packages/jumpstarter-cli/jumpstarter_cli/create.pyto use the package’s primary export instead of the submodule
It appears that due to the recent refactoring,OutputTypeis now intended to be accessed from the package’s top-level module (as evidenced by its presence in__init__.py), rather than directly fromjumpstarter_cli_common.opt. To fix the CI import error, please update the import statement as follows:
Old import:
from jumpstarter_cli_common.opt import OutputTypeNew import:
from jumpstarter_cli_common import OutputTypeThis change should resolve the
ImportErrorseen in the CI pipeline.packages/jumpstarter-cli/jumpstarter_cli/get.py (1)
2-2: 💡 Verification agent🧩 Analysis chain
Fix import error causing test failures
There's an import error in the CI pipeline:
ImportError: cannot import name 'OutputType' from 'jumpstarter_cli_common.opt'. The import is likely broken becauseOutputTypehas been moved from its original location as part of this refactoring.
🏁 Script executed:
#!/bin/bash # Let's check where OutputType should be imported from find packages -name "*.py" -exec grep -l "OutputType.*=" {} \;Length of output: 181
Update import to use the new location for OutputType
The CI failure stems from the fact that
OutputTypeis no longer defined in the old module location. Instead of importing it from the aggregated module (which internally looks atjumpstarter_cli_common.opt), please update the import inpackages/jumpstarter-cli/jumpstarter_cli/get.pyto explicitly importOutputTypefrom its new location. For example:
Change the current code from:
from jumpstarter_cli_common import OutputMode, OutputType, make_table, opt_config, opt_output_allTo:
from jumpstarter_cli_common import OutputMode, make_table, opt_config, opt_output_all from jumpstarter.common.pydantic import OutputTypeThis update fixes the import error observed in the CI pipeline.
🧹 Nitpick comments (1)
packages/jumpstarter-cli-common/jumpstarter_cli_common/version.py (1)
41-50: Consider updating to use the dump method with OutputMode.While the current code will work because
JumpstarterVersioninheritsdump_jsonanddump_yamlmethods fromSerializableBaseModel, it would be more consistent with the refactoring to useversion_obj().dump(OutputMode.JSON)andversion_obj().dump(OutputMode.YAML).@click.command() @opt_output_all def version(output: OutputType): """Get the current Jumpstarter version""" if output == OutputMode.JSON: - click.echo(version_obj().dump_json()) + click.echo(version_obj().dump(OutputMode.JSON)) elif output == OutputMode.YAML: - click.echo(version_obj().dump_yaml()) + click.echo(version_obj().dump(OutputMode.YAML)) else: click.echo(version_msg())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/version.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/get.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(1 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(5 hunks)packages/jumpstarter/jumpstarter/common/pydantic.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(2 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (7)
packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
packages/jumpstarter/jumpstarter/common/pydantic.py (2)
OutputMode(5-9)dump(16-23)
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (1)
packages/jumpstarter/jumpstarter/common/pydantic.py (1)
OutputMode(5-9)
packages/jumpstarter-cli/jumpstarter_cli/get.py (1)
packages/jumpstarter/jumpstarter/common/pydantic.py (2)
OutputMode(5-9)dump(16-23)
packages/jumpstarter/jumpstarter/config/exporter.py (1)
packages/jumpstarter/jumpstarter/common/pydantic.py (1)
SerializableBaseModel(15-29)
packages/jumpstarter-cli/jumpstarter_cli/create.py (1)
packages/jumpstarter/jumpstarter/common/pydantic.py (2)
OutputMode(5-9)dump(16-23)
packages/jumpstarter-cli-common/jumpstarter_cli_common/version.py (1)
packages/jumpstarter/jumpstarter/common/pydantic.py (2)
OutputMode(5-9)SerializableBaseModel(15-29)
packages/jumpstarter/jumpstarter/config/client.py (1)
packages/jumpstarter/jumpstarter/common/pydantic.py (1)
SerializableBaseModel(15-29)
🪛 GitHub Actions: Run Tests
packages/jumpstarter-cli/jumpstarter_cli/update.py
[error] 1-1: ImportError: cannot import name 'OutputType' from 'jumpstarter_cli_common.opt'
packages/jumpstarter-cli/jumpstarter_cli/get.py
[error] 1-1: ImportError: cannot import name 'OutputType' from 'jumpstarter_cli_common.opt'
packages/jumpstarter-cli/jumpstarter_cli/create.py
[error] 1-1: ImportError: cannot import name 'OutputType' from 'jumpstarter_cli_common.opt'
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (22)
packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
30-32: Simplified output handling - looks good!The refactoring looks good. By using the bitwise OR operator for
OutputMode.JSON | OutputMode.YAML, you've consolidated duplicate code paths into a single case that calls the unifiedlease.dump(output)method, making the code more maintainable.packages/jumpstarter-cli/jumpstarter_cli/create.py (1)
57-59: Simplified output handling - looks good!The refactoring looks good. By using the bitwise OR operator for
OutputMode.JSON | OutputMode.YAML, you've consolidated duplicate code paths into a single case that calls the unifiedlease.dump(output)method, making the code more maintainable.packages/jumpstarter-cli/jumpstarter_cli/get.py (2)
27-29: Simplified output handling for exporters - looks good!The refactoring looks good. By using the bitwise OR operator for
OutputMode.JSON | OutputMode.YAML, you've consolidated duplicate code paths into a single case that calls the unifiedexporters.dump(output)method, making the code more maintainable.
57-59: Simplified output handling for leases - looks good!The refactoring looks good. By using the bitwise OR operator for
OutputMode.JSON | OutputMode.YAML, you've consolidated duplicate code paths into a single case that calls the unifiedleases.dump(output)method, making the code more maintainable.packages/jumpstarter/jumpstarter/config/client.py (2)
19-19: Added import for SerializableBaseModelGood addition of the import for the
SerializableBaseModelclass, which is now used as the base class forClientConfigListV1Alpha1.
278-278: Updated base class to SerializableBaseModelGood switch from
BaseModeltoSerializableBaseModelforClientConfigListV1Alpha1. This change leverages the common serialization logic (likedump,dump_json, anddump_yamlmethods) provided by theSerializableBaseModelclass, thereby eliminating the need for duplicate serialization code.packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (4)
5-5: Good centralization of OutputMode dependency.The import of
OutputModefrom the common module reduces code duplication and centralizes this enumeration to a single location, promoting better maintenance.
25-31: LGTM - References to OutputMode updated correctly.The references to
OutputModeconstants in theopt_output_alloption have been updated to use the imported class without changing functionality.
35-41: LGTM - References to OutputMode updated correctly.The references to
OutputMode.NAMEin theopt_output_name_onlyoption have been updated to use the imported class without changing functionality.
45-51: LGTM - References to OutputMode updated correctly.The references to
OutputMode.PATHin theopt_output_path_onlyoption have been updated to use the imported class without changing functionality.packages/jumpstarter/jumpstarter/config/exporter.py (2)
17-17: LGTM - Import added for SerializableBaseModel.The import for
SerializableBaseModelis added to support the inheritance change inExporterConfigListV1Alpha1.
179-179: Good refactoring to centralize serialization logic.The change from
BaseModeltoSerializableBaseModelremoves the need for customdump_jsonanddump_yamlmethods, centralizing the serialization logic in the base class.packages/jumpstarter-cli-common/jumpstarter_cli_common/version.py (2)
6-9: LGTM - Imports updated to match new inheritance.The imports have been updated to support the change in inheritance, with the
yamlimport removed since it's now handled in theSerializableBaseModel.
30-34: Good refactoring to centralize serialization logic.Changing from
BaseModeltoSerializableBaseModelprovides consistent serialization behavior with the rest of the codebase.packages/jumpstarter/jumpstarter/common/pydantic.py (2)
5-12: LGTM - OutputMode and OutputType definitions.The
OutputModeenum andOutputTypealias are well-defined and consistent with their previous usage in other files.
25-29: LGTM - Serialization methods implementation.The implementation of
dump_jsonanddump_yamlmethods is clean and straightforward, with consistent formatting options.packages/jumpstarter/jumpstarter/client/grpc.py (6)
9-12: New imports align with the design pattern changes.The imports here properly support the refactoring of output handling by bringing in
SerializableBaseModelfrom the common module, which will be used by the model classes.
38-38: Model inheritance change looks good.The
Exporterclass now inherits fromSerializableBaseModelinstead ofBaseModel, which is consistent with the PR's objective to centralize serialization logic.
49-49: Model inheritance change looks good.The
Leaseclass now inherits fromSerializableBaseModelinstead ofBaseModel, which aligns with the overall pattern of using a shared serialization base class.
96-96: Model inheritance change looks good.The
ExporterListclass now inherits fromSerializableBaseModel, providing it with common serialization capabilities.
108-108: Model inheritance change looks good.The
LeaseListclass correctly inherits fromSerializableBaseModel, completing the pattern of having all model classes use the common base class.
9-108:Details
❓ Verification inconclusive
Verify that all code using these models has been updated.
The removal of specific serialization methods (
dump_json,dump_yaml) in favor of inheriting fromSerializableBaseModelis a good pattern that reduces duplication. However, we should ensure all calling code has been updated to use the new interface.
🏁 Script executed:
#!/bin/bash # Search for any remaining calls to dump_json or dump_yaml methods that might need to be updated echo "Searching for dump_json calls..." rg "\.dump_json\(" --type py echo "Searching for dump_yaml calls..." rg "\.dump_yaml\(" --type py echo "Searching for possible references to the affected classes..." rg -A 2 -B 2 "Exporter\(|Lease\(|ExporterList\(|LeaseList\(" --type pyLength of output: 37750
Action: Confirm serialization method consistency across the codebase
I verified with a repository-wide search that calls to
.dump_json()and.dump_yaml()are still present in various parts of our code—in tests (e.g. in jumpstarter-kubernetes, jumpstarter-cli-admin, etc.), CLI modules, and even within our common pydantic utilities. This indicates that the new inherited implementation fromSerializableBaseModelis being used in place of per-model serialization methods.A few notes for follow-up:
- Ensure that the inherited methods in
SerializableBaseModel(as used in packages/jumpstarter/jumpstarter/common/pydantic.py) provide serialization behavior fully compatible with previous outputs.- Double-check that downstream code (tests and CLI commands) correctly interprets the results from these inherited methods.
- If there’s any planned deprecation or renaming of these methods in the future, update the calling code accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/json.py (1)
16-17: Ensure error handling for missing attributesThe
dump_namemethod assumes the model has bothkindandmetadata.nameattributes. This could potentially cause runtime errors if used with models lacking these attributes.Consider adding validation or fallback handling:
def dump_name(self): + if not hasattr(self, 'kind') or not hasattr(self, 'metadata') or not hasattr(self.metadata, 'name'): + raise AttributeError("Model must have 'kind' and 'metadata.name' attributes to use dump_name") return "{}.jumpstarter.dev/{}".format(self.kind.lower(), self.metadata.name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py(2 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py(3 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/json.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.13)
- 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-utils Dockerfile.utils)
- 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/json.py (3)
3-3: Good improvement: Using common serialization utilitiesThe import of serialization-related types from a common module (
jumpstarter.common.pydantic) is a good step toward code consistency and reducing duplication across the codebase.
6-6: Nice refactoring: Inheriting from SerializableBaseModelChanging the base class from
BaseModeltoSerializableBaseModelaligns with the PR's objective of standardizing output handling and eliminating duplicate serialization logic.
9-14:Details
✅ Verification successful
Verify type consistency between OutputType and OutputMode
There's a potential type inconsistency between the parameter type annotation (
OutputType) and the match case usage (OutputMode.NAME). This suggestsOutputTypemight be a Union or superset type that includesOutputMode.
🏁 Script executed:
#!/bin/bash # Check the definitions of OutputType and OutputMode to understand their relationship cat $(fd -p 'jumpstarter/common/pydantic.py')Length of output: 831
Type Verification Confirmed: No Changes Needed
The type alias
OutputTypeis defined asOutputMode | Nonein thepackages/jumpstarter/jumpstarter/common/pydantic.pyfile. SinceOutputTypeexactly encapsulatesOutputMode(with the possibility of beingNone), the usage ofOutputMode.NAMEin the match case is consistent with the parameter annotation.
- Location Verified:
packages/jumpstarter/jumpstarter/common/pydantic.pyconfirms the correct type definition.- Key Point:
OutputTypebeing equivalent toOutputMode | Nonevalidates the implementation injumpstarter_kubernetes/json.py.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/common/pydantic.py (1)
15-27: Consider adding a return type annotation to the dump methodThe
dump_jsonanddump_yamlmethods have return type annotations, but thedumpmethod doesn't. Adding a return type annotation would improve consistency and type safety.- def dump(self, mode: OutputType = None): + def dump(self, mode: OutputType = None) -> str:packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (1)
60-84: Consider adding type annotations to opt_output_autoThe new function would benefit from type annotations for its parameter and return value.
-def opt_output_auto(cls): +def opt_output_auto(cls: type) -> click.Option:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py(5 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py(4 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/__init__.py(4 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py(2 hunks)packages/jumpstarter/jumpstarter/common/pydantic.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/init.py
🧰 Additional context used
🧠 Learnings (1)
packages/jumpstarter/jumpstarter/common/pydantic.py (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#386
File: packages/jumpstarter/jumpstarter/common/pydantic.py:15-24
Timestamp: 2025-03-31T17:45:06.483Z
Learning: In the Jumpstarter codebase, when using OutputType, the value None represents tabular output. The implementation for dumping tables is class-specific rather than being implemented in the SerializableBaseModel base class.
🧬 Code Definitions (2)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py (4)
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (1)
opt_output_auto(60-84)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (2)
ClientsV1Alpha1Api(64-137)V1Alpha1Client(26-53)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (2)
ExportersV1Alpha1Api(68-145)V1Alpha1Exporter(29-57)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (2)
LeasesV1Alpha1Api(88-103)V1Alpha1Lease(30-77)
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (1)
packages/jumpstarter/jumpstarter/common/pydantic.py (1)
OutputMode(5-9)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
🔇 Additional comments (8)
packages/jumpstarter/jumpstarter/common/pydantic.py (3)
26-27: Improve error message for None case to clarify expected implementationWhen
modeisNone, the error message doesn't explain that tabular output is expected to be implemented by derived classes.- raise NotImplementedError("unimplemented output mode: {}".format(mode)) + raise NotImplementedError("tabular output (mode={}) must be implemented by derived classes".format(mode))
29-30: LGTM: Clean JSON serialization implementationThe
dump_jsonmethod correctly uses Pydantic's built-in serialization with appropriate formatting options.
32-33: LGTM: Clean YAML serialization implementationThe
dump_yamlmethod correctly leverages PyYAML's safe_dump with appropriate formatting.packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py (2)
12-13: LGTM: Good transition to dynamic output optionsThe change from static output options to dynamic ones based on class capabilities improves code maintainability and flexibility.
Also applies to: 18-20, 47-48
71-72: LGTM: Consistent application of opt_output_auto across commandsThe pattern is consistently applied to all three commands (client, exporter, lease), which is excellent for maintainability.
Also applies to: 101-102
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (3)
5-7: LGTM: Clean import and export of typesThe explicit
__all__declaration makes it clear what is being exported from this module.
60-70: LGTM: Excellent dynamic option generation based on class capabilitiesThe function intelligently checks for the presence of specific methods to determine available output modes, which is a clean approach that avoids duplication.
71-84: LGTM: Helpful context-aware help textThe help text is customized based on available choices, which improves the user experience.
|
Looking good! @NickCao, let's also move the |
|
@NickCao maybe we want to attack the configs in a different PR, but I think we should handle them with the same logic with added capabilities for file I/O and ordering. |
But it's still something kubernetes specific? The grpc lists are of a different structure. |
Surely yes, but currently their |
I think it depends on how it's used. For example, sticking to the Kubernetes list type structure is nice for JSON/YAML output for the CLI tool as it's nice and consistent and makes it trivial to parse. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py(5 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py(0 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py(4 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py(0 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py(4 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/__init__.py(4 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/echo.py(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py(2 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/version.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/get.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(3 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/__init__.py(0 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(3 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(3 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/list.py(0 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py(1 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(4 hunks)packages/jumpstarter/jumpstarter/common/pydantic.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(2 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(2 hunks)
💤 Files with no reviewable changes (4)
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/init.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/list.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-cli-common/jumpstarter_cli_common/echo.py
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters_test.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/init.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients_test.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/version.py
- packages/jumpstarter/jumpstarter/config/exporter.py
- packages/jumpstarter-cli/jumpstarter_cli/update.py
- packages/jumpstarter-cli/jumpstarter_cli/create.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py
- packages/jumpstarter-cli/jumpstarter_cli/get.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/json.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py
- packages/jumpstarter/jumpstarter/client/grpc.py
- packages/jumpstarter/jumpstarter/config/client.py
🧰 Additional context used
🧠 Learnings (1)
packages/jumpstarter/jumpstarter/common/pydantic.py (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#386
File: packages/jumpstarter/jumpstarter/common/pydantic.py:15-24
Timestamp: 2025-03-31T17:45:06.483Z
Learning: In the Jumpstarter codebase, when using OutputType, the value None represents tabular output. The implementation for dumping tables is class-specific rather than being implemented in the SerializableBaseModel base class.
🧬 Code Graph Analysis (2)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (1)
packages/jumpstarter/jumpstarter/common/pydantic.py (1)
SerializableBaseModelList(40-49)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (1)
packages/jumpstarter/jumpstarter/common/pydantic.py (1)
SerializableBaseModelList(40-49)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
🔇 Additional comments (3)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (1)
12-12: LGTM! Refactoring to use the common SerializableBaseModelList.The change to inherit from
SerializableBaseModelListcentralizes the output serialization logic, reducing code duplication and improving maintainability across all list types.Also applies to: 56-56, 102-102
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (1)
11-11: LGTM! Consistent usage of SerializableBaseModelList.The change aligns with the project's effort to deduplicate output mode handling by using the common
SerializableBaseModelListbase class.Also applies to: 60-60, 71-71
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (1)
9-9: LGTM! Centralized serialization logic implementation.Good refactoring to use
SerializableBaseModelListconsistently with the other Kubernetes resource list types.Also applies to: 80-80, 91-91
| class SerializableBaseModel(BaseModel): | ||
| def dump(self, mode: OutputType = None): | ||
| match mode: | ||
| case OutputMode.JSON: | ||
| return self.dump_json() | ||
| case OutputMode.YAML: | ||
| return self.dump_yaml() | ||
| case OutputMode.NAME: | ||
| return self.dump_name() | ||
| case OutputMode.PATH: | ||
| return self.dump_path() | ||
| case None | _: | ||
| raise NotImplementedError("unimplemented output mode: {}".format(mode)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide default implementations for dump_name() and dump_path() in SerializableBaseModel.
The dump method dispatches to dump_name() and dump_path() methods when OutputMode.NAME or OutputMode.PATH is used, but there are no default implementations for these methods in the SerializableBaseModel class. This means subclasses that don't implement these methods will encounter obscure errors.
class SerializableBaseModel(BaseModel):
def dump(self, mode: OutputType = None):
match mode:
case OutputMode.JSON:
return self.dump_json()
case OutputMode.YAML:
return self.dump_yaml()
case OutputMode.NAME:
return self.dump_name()
case OutputMode.PATH:
return self.dump_path()
case None | _:
raise NotImplementedError("unimplemented output mode: {}".format(mode))
def dump_json(self) -> str:
return self.model_dump_json(indent=4, by_alias=True) + "\n"
def dump_yaml(self) -> str:
return yaml.safe_dump(self.model_dump(mode="json", by_alias=True), indent=2)
+ def dump_name(self) -> str:
+ raise NotImplementedError("dump_name() not implemented for this model")
+
+ def dump_path(self) -> str:
+ raise NotImplementedError("dump_path() not implemented for this model")📝 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.
| class SerializableBaseModel(BaseModel): | |
| def dump(self, mode: OutputType = None): | |
| match mode: | |
| case OutputMode.JSON: | |
| return self.dump_json() | |
| case OutputMode.YAML: | |
| return self.dump_yaml() | |
| case OutputMode.NAME: | |
| return self.dump_name() | |
| case OutputMode.PATH: | |
| return self.dump_path() | |
| case None | _: | |
| raise NotImplementedError("unimplemented output mode: {}".format(mode)) | |
| class SerializableBaseModel(BaseModel): | |
| def dump(self, mode: OutputType = None): | |
| match mode: | |
| case OutputMode.JSON: | |
| return self.dump_json() | |
| case OutputMode.YAML: | |
| return self.dump_yaml() | |
| case OutputMode.NAME: | |
| return self.dump_name() | |
| case OutputMode.PATH: | |
| return self.dump_path() | |
| case None | _: | |
| raise NotImplementedError("unimplemented output mode: {}".format(mode)) | |
| def dump_json(self) -> str: | |
| return self.model_dump_json(indent=4, by_alias=True) + "\n" | |
| def dump_yaml(self) -> str: | |
| return yaml.safe_dump(self.model_dump(mode="json", by_alias=True), indent=2) | |
| def dump_name(self) -> str: | |
| raise NotImplementedError("dump_name() not implemented for this model") | |
| def dump_path(self) -> str: | |
| raise NotImplementedError("dump_path() not implemented for this model") |
| class SerializableBaseModelList(SerializableBaseModel, Generic[T]): | ||
| api_version: Literal["jumpstarter.dev/v1alpha1"] = Field( | ||
| alias="apiVersion", | ||
| default="jumpstarter.dev/v1alpha1", | ||
| ) | ||
| kind: Literal["List"] = Field(default="List") | ||
| items: list[T] | ||
|
|
||
| def dump_name(self): | ||
| return "".join(item.dump_name() for item in self.items) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement dump_path() method in SerializableBaseModelList.
The SerializableBaseModelList class implements dump_name() but not dump_path(), even though the base class's dump method might dispatch to it. This could lead to unexpected errors if a call to dump(OutputMode.PATH) is made on a list instance.
class SerializableBaseModelList(SerializableBaseModel, Generic[T]):
api_version: Literal["jumpstarter.dev/v1alpha1"] = Field(
alias="apiVersion",
default="jumpstarter.dev/v1alpha1",
)
kind: Literal["List"] = Field(default="List")
items: list[T]
def dump_name(self):
return "".join(item.dump_name() for item in self.items)
+
+ def dump_path(self):
+ return "".join(item.dump_path() for item in self.items) if hasattr(self.items[0], "dump_path") else super().dump_path()📝 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.
| class SerializableBaseModelList(SerializableBaseModel, Generic[T]): | |
| api_version: Literal["jumpstarter.dev/v1alpha1"] = Field( | |
| alias="apiVersion", | |
| default="jumpstarter.dev/v1alpha1", | |
| ) | |
| kind: Literal["List"] = Field(default="List") | |
| items: list[T] | |
| def dump_name(self): | |
| return "".join(item.dump_name() for item in self.items) | |
| class SerializableBaseModelList(SerializableBaseModel, Generic[T]): | |
| api_version: Literal["jumpstarter.dev/v1alpha1"] = Field( | |
| alias="apiVersion", | |
| default="jumpstarter.dev/v1alpha1", | |
| ) | |
| kind: Literal["List"] = Field(default="List") | |
| items: list[T] | |
| def dump_name(self): | |
| return "".join(item.dump_name() for item in self.items) | |
| def dump_path(self): | |
| return "".join(item.dump_path() for item in self.items) if hasattr(self.items[0], "dump_path") else super().dump_path() |
github-project-automation
bot
moved this from Under Review
to Done
in Jumpstarter planning
Summary by CodeRabbit
New Features
Refactor
dumpmethod.echofunction to control output without automatic newlines, improving output formatting consistency.