Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis update introduces a centralized Changes
Sequence Diagram(s)sequenceDiagram
participant CLI_Command
participant Model
participant model_print
participant RichConsole
CLI_Command->>Model: Retrieve resource(s)
CLI_Command->>model_print: Call with (model, output_mode, kwargs)
alt output_mode is JSON
model_print->>Model: model_dump_json()
model_print->>RichConsole: Print JSON
else output_mode is YAML
model_print->>Model: model_dump()
model_print->>RichConsole: Print YAML
else output_mode is NAME or PATH
model_print->>Model: rich_add_names/paths()
model_print->>RichConsole: Print names/paths
else
model_print->>Model: rich_add_columns(table)
model_print->>Model: rich_add_rows(table)
model_print->>RichConsole: Print table or "No resources found"
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![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
🧹 Nitpick comments (8)
packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py (1)
9-70: Excellent unified printing implementation that eliminates duplication across the codebase.This
model_printfunction elegantly centralizes output formatting by leveraging rich display methods defined on model classes. It handles all output formats (JSON, YAML, name, path, and table) in a consistent way while delegating the specifics of what to display to the models themselves.A few observations:
- The match statement is used correctly to handle different output modes
- Error handling is appropriate, raising NotImplementedError with the original AttributeError as context
- Table formatting is clean with sensible defaults
Consider addressing the TODO comment on line 68 to also print the namespace when no resources are found, which would provide more context to users.
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (1)
56-70: Good implementation of rich display methods for individual client objects.These methods properly implement the interface expected by
model_printfor formatting client details into a table or list of names.Consider implementing the commented "AGE" column at line 60 to show resource age, which would be useful for users tracking when clients were created.
packages/jumpstarter/jumpstarter/config/client.py (1)
257-284: Well-structured refactor to return a ClientConfigListV1Alpha1 objectThe
listmethod has been updated to return aClientConfigListV1Alpha1instance instead of a plain list, which aligns with the unified output format approach. Good addition of thecurrent_configfield to track the currently active client.Consider adding error handling for the case where
UserConfigV1Alpha1.load()might fail due to malformed but existing files.packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (1)
74-96: Comprehensive row formatting implementationThe implementation correctly handles both device-level details and summary views. Good handling of optional fields and label formatting.
One suggestion for consistency: in line 94, you check both
self.status and self.status.devices, but in line 76 you only checkself.status. Consider using consistent null-checking patterns throughout the method.packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (2)
92-108: Well-structured reason mappingThe
get_reasonmethod intelligently maps condition reasons and statuses to user-friendly strings, improving readability in the CLI output.For robustness, consider adding a null check for
self.status.conditionsbefore accessing it, in case it's None.
109-124: Thorough row formatting with selector handlingGood implementation for formatting lease data into table rows, including nice handling of selector labels.
Consider adding a null check for
self.spec.selector.match_labelsbefore iterating over it, to prevent potential errors if it's None.selectors = [] - for label in self.spec.selector.match_labels: + if self.spec.selector and self.spec.selector.match_labels: + for label in self.spec.selector.match_labels: + selectors.append(f"{label}:{str(self.spec.selector.match_labels[label])}") - selectors.append(f"{label}:{str(self.spec.selector.match_labels[label])}")packages/jumpstarter/jumpstarter/client/grpc.py (2)
52-57: Consider more readable label formattingThe current approach to formatting labels joins them with commas, which works but could become difficult to read with many labels.
- ",".join(("{}={}".format(i[0], i[1]) for i in self.labels.items())), + ", ".join("{}={}".format(k, v) for k, v in self.labels.items()),This small change adds spaces after commas and uses more descriptive variable names for better readability.
116-124: Consider adding condition status informationThe
rich_add_rowsimplementation displays the core lease properties but doesn't show any condition status information, which might be important for understanding lease state. Consider adding a column for lease status based on conditions.@classmethod def rich_add_columns(cls, table): table.add_column("NAME") table.add_column("SELECTOR") table.add_column("DURATION") table.add_column("CLIENT") table.add_column("EXPORTER") + table.add_column("STATUS") def rich_add_rows(self, table): + status = "Unknown" + for condition in self.conditions: + if condition.type == "Ready" and condition.status: + status = condition.status + break table.add_row( self.name, self.selector, str(self.duration), self.client, self.exporter, + status, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py(3 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py(4 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py(4 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.py(1 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py(0 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_client.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/get.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py(2 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(4 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(2 hunks)
💤 Files with no reviewable changes (1)
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py
🧰 Additional context used
🧬 Code Graph Analysis (7)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.py (1)
packages/jumpstarter/jumpstarter/config/client.py (1)
ClientConfigV1Alpha1(54-293)
packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py (2)
packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py (1)
model_print(9-70)packages/jumpstarter/jumpstarter/config/exporter.py (1)
ExporterConfigV1Alpha1(72-175)
packages/jumpstarter-cli/jumpstarter_cli/config_client.py (2)
packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py (1)
model_print(9-70)packages/jumpstarter/jumpstarter/config/client.py (2)
ClientConfigV1Alpha1(54-293)ClientConfigV1Alpha1Drivers(49-51)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (4)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (6)
rich_add_columns(57-60)rich_add_columns(80-81)rich_add_rows(62-66)rich_add_rows(83-85)rich_add_names(68-69)rich_add_names(87-89)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (6)
rich_add_columns(80-90)rich_add_columns(138-139)rich_add_rows(109-124)rich_add_rows(141-143)rich_add_names(126-127)rich_add_names(145-147)packages/jumpstarter/jumpstarter/config/exporter.py (3)
rich_add_columns(186-188)rich_add_rows(190-195)rich_add_names(197-199)packages/jumpstarter/jumpstarter/config/client.py (3)
rich_add_columns(305-309)rich_add_rows(311-318)rich_add_names(320-322)
packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py (4)
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (1)
OutputMode(41-45)packages/jumpstarter/jumpstarter/config/exporter.py (3)
rich_add_names(197-199)rich_add_columns(186-188)rich_add_rows(190-195)packages/jumpstarter/jumpstarter/config/client.py (3)
rich_add_names(320-322)rich_add_columns(305-309)rich_add_rows(311-318)packages/jumpstarter/jumpstarter/client/grpc.py (12)
rich_add_names(58-59)rich_add_names(125-126)rich_add_names(148-150)rich_add_names(172-174)rich_add_columns(48-50)rich_add_columns(109-114)rich_add_columns(141-142)rich_add_columns(165-166)rich_add_rows(52-56)rich_add_rows(116-123)rich_add_rows(144-146)rich_add_rows(168-170)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (2)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (6)
rich_add_columns(61-72)rich_add_columns(110-111)rich_add_rows(74-96)rich_add_rows(113-115)rich_add_names(98-99)rich_add_names(117-119)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (6)
rich_add_columns(57-60)rich_add_columns(80-81)rich_add_rows(62-66)rich_add_rows(83-85)rich_add_names(68-69)rich_add_names(87-89)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
packages/jumpstarter/jumpstarter/config/client.py (4)
rich_add_columns(305-309)rich_add_rows(311-318)rich_add_names(320-322)lease(81-89)packages/jumpstarter/jumpstarter/common/metadata.py (1)
name(13-14)packages/jumpstarter/jumpstarter/client/lease.py (1)
Lease(31-211)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
🔇 Additional comments (55)
packages/jumpstarter/jumpstarter/config/client_config_test.py (3)
328-328: Update to access.itemscorrectly.This change reflects the new structure where
ClientConfigV1Alpha1.list()now returns a structured object with anitemsattribute containing the list of configurations, rather than returning a plain list directly.
336-336: Consistent use of.itemsfor accessing client configs.Properly updated to access the list items through the
.itemsattribute, maintaining test compatibility with the new return type.
342-342: Updated empty list check for new return type.The test now correctly verifies an empty list using the
.itemsattribute on the returned object, aligned with the new function return structure.packages/jumpstarter-cli/jumpstarter_cli/create.py (2)
6-7: Updated imports to support the new output handling approach.Correctly importing the
OutputTypefrom the common package and the newmodel_printfunction that centralizes output formatting.
53-53: Simplified output handling using central model_print function.This change elegantly replaces the previous custom output formatting code with a call to the centralized
model_printfunction, improving code maintainability and ensuring consistent output handling across the CLI.packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.py (1)
81-81: Updated to access client configs through.itemsattribute.This correctly adapts the condition to the new
ClientConfigV1Alpha1.list()return type, which now returns an object with anitemsattribute instead of a direct list.packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py (4)
270-271: Updated test expectation for "no resources found" case.Changed the expected exit code from 1 to 0, reflecting the new standardized behavior where "no resources found" is treated as a successful execution with a specific message rather than an error condition.
639-640: Consistent exit code for "no exporters found" case.Test now expects exit code 0 for the "no resources found" case, maintaining consistency with the updated output handling approach across the CLI.
803-804: Aligned exit code expectations for empty exporter devices list.Updated to expect exit code 0 when no exporter devices are found, consistent with the new standardized behavior.
1196-1197: Standardized exit code for empty lease listing.Test now consistently expects exit code 0 for "no leases found", completing the standardization of "no resources found" handling across all resource types.
packages/jumpstarter-cli/jumpstarter_cli/update.py (2)
6-7: Imports updated to use new output formatting utility.The imports have been updated to use the new
model_printfunction fromjumpstarter_cli_common.printinstead of the previous output formatting utilities.
32-32: Output formatting delegated to unified model_print function.The code now uses the unified
model_printfunction instead of explicit conditional logic for different output modes. This simplifies the code and ensures consistent output formatting across the CLI.packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py (7)
13-13: Import added for unified model_print function.The import of
model_printfromjumpstarter_cli_common.printsupports the new unified output formatting approach.
48-48: Replaced client-specific printing with model_print.The code now delegates client output formatting to the unified
model_printfunction, improving consistency and reducing duplication across commands.
51-51: Replaced clients list printing with model_print.Multiple clients output formatting is now handled by the unified
model_printfunction.
79-79: Replaced exporter-specific printing with model_print.The code now passes both the exporter and the devices flag to
model_print, preserving the behavior of showing device information when requested.
82-82: Replaced exporters list printing with model_print.Multiple exporters output formatting is now handled by the unified
model_printfunction, with preserved devices display functionality.
104-104: Replaced lease-specific printing with model_print.The code now delegates lease output formatting to the unified
model_printfunction.
107-107: Replaced leases list printing with model_print.Multiple leases output formatting is now handled by the unified
model_printfunction.packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py (3)
9-9: Import added for unified model_print function.The import of
model_printfromjumpstarter_cli_common.printsupports the new unified output formatting approach.
11-11: Simplified imports by removing ExporterConfigListV1Alpha1.The
ExporterConfigListV1Alpha1import has been removed since it's not directly referenced in this file. According to the provided context,ExporterConfigV1Alpha1.list()already returns an instance ofExporterConfigListV1Alpha1.
82-82: Replaced exporter config list printing with model_print.The code now delegates output formatting to the unified
model_printfunction, removing the need for conditional logic based on output mode. This simplifies the code and ensures consistent output formatting.packages/jumpstarter-cli/jumpstarter_cli/get.py (3)
4-5: Imports updated to use new output formatting utility.The imports have been updated to use the new
model_printfunction fromjumpstarter_cli_common.printinstead of the previous output formatting utilities.
29-29: Replaced exporters list printing with model_print.Output formatting for the list of exporters is now delegated to the unified
model_printfunction, simplifying the code and ensuring consistent output.
44-44: Replaced leases list printing with model_print.Output formatting for the list of leases is now delegated to the unified
model_printfunction, simplifying the code and ensuring consistent output.packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (1)
79-90: Clean implementation of list display methods delegating to individual items.The list implementation correctly delegates to the individual client methods, maintaining the separation of concerns and avoiding code duplication.
packages/jumpstarter-cli/jumpstarter_cli/config_client.py (4)
12-12: Good replacement of table-specific import with generic model_print.
96-96: Updated list access pattern to match new structure.Correctly updated to access the items attribute on the list result, which aligns with the changes in list return types.
112-112: Consistent use of items attribute access.
139-139: Simplified output with unified model_print function.Eliminated manual output formatting logic, making the code cleaner and easier to maintain.
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py (4)
15-15: Good import of the unified model_print function.
104-104: Consistent update to access items attribute.Correctly updated to access the items attribute on the list result, matching the pattern in config_client.py.
110-110: Replaced custom print function with model_print.Simplified output formatting by using the unified model_print function, improving consistency across commands.
166-166: Consistent use of model_print for exporter output.Applied the same pattern for exporter output, ensuring consistency across different resource types.
packages/jumpstarter/jumpstarter/config/client.py (2)
304-319: Well-implemented rich display supportThe
rich_add_columnsandrich_add_rowsmethods provide a clean implementation for table-based display. The approach of marking the current config with "*" is intuitive for CLI users.
320-322: Good name collection implementationThe
rich_add_namesmethod correctly appends all client aliases to the provided list, supporting the name-only output mode.packages/jumpstarter/jumpstarter/config/exporter.py (4)
112-117: Appropriate refactor of the list methodThis update to return a list model rather than a raw list aligns with the new output formatting approach and follows the same pattern implemented across other resources.
183-183: Useful model_config additionThe
model_configaddition enables proper handling of aliases and arbitrary types, which supports the serialization requirements for rich output formatting.
186-195: Well-structured rich table columns and rowsThese methods appropriately define and populate the table columns and rows for exporter configurations, providing a consistent display mechanism.
197-199: Proper name collection implementationThe
rich_add_namesmethod correctly implements the name-only output mode pattern used by the centralized printing utility.packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (2)
61-72: Flexible column definition with device mode supportThe conditional column setup based on the
devicesflag provides a nice flexible output format. This allows both summary views and detailed device views using the same underlying code.
98-99: Clear resource naming formatThe
rich_add_namesmethod correctly formats exporter names with their resource type prefix, which is helpful for CLI users.packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (3)
80-90: Comprehensive lease table columnsThe column definitions cover all important lease attributes, providing a complete view of lease information in table output mode.
126-127: Consistent resource naming patternThe format matches the pattern used in other resource types, providing a consistent experience across different resource kinds.
138-147: Well-implemented list class methodsThese methods correctly delegate to the instance methods of each contained lease, following the pattern used across other resource list types.
packages/jumpstarter/jumpstarter/client/grpc.py (10)
47-51: Appropriate column definition for exportersThe implementation of
rich_add_columnsprovides a clean structure for displaying exporter information. The column headers are concise and descriptive.
58-60: Simple and effective name collection methodThe
rich_add_namesimplementation is clean and efficient for collecting exporter names.
108-115: Complete column structure for lease displayThe column headers cover all important lease attributes, facilitating effective data presentation.
125-127: Consistent name collection for leasesThe implementation follows the same pattern as other entities, maintaining consistency across the codebase.
140-143: Efficient reuse of Exporter column definitionGood delegation to the Exporter class for column definitions, promoting code reuse and ensuring consistency.
144-147: Clean implementation for handling multiple exportersThe method efficiently iterates through exporters and delegates row addition to each instance.
148-151: Consistent name collection for exporter listsFollows the established pattern for name collection, maintaining consistency across the codebase.
164-167: Efficient reuse of Lease column definitionGood delegation to the Lease class for column definitions, promoting code reuse and ensuring consistency.
168-171: Clean implementation for handling multiple leasesThe method efficiently iterates through leases and delegates row addition to each instance.
172-175: Consistent name collection for lease listsFollows the established pattern for name collection, maintaining consistency across the codebase.
NickCao
force-pushed
the
output-mode-new
branch
4 times, most recently
from
25560aa to
de586fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py (1)
9-75: 🛠️ Refactor suggestionConsider refactoring to reduce complexity and improve error handling.
The function works well but has some areas for improvement:
- The
# noqa: C901comment indicates high cyclomatic complexity. Consider extracting each output mode handler into separate methods.- Using
AttributeErrorto detect missing methods is fragile. Consider usinghasattr()checks instead.- The error messages could be more helpful by specifying which methods are required for each output mode.
Apply this refactor to improve error handling and clarity:
- try: - model.rich_add_names(names) - except AttributeError as err: - raise NotImplementedError from err + if not hasattr(model, 'rich_add_names'): + raise NotImplementedError( + f"Model {model.__class__.__name__} must implement rich_add_names() for NAME output mode" + ) + model.rich_add_names(names)Similar changes should be applied to the PATH and table output modes.
🧹 Nitpick comments (3)
packages/jumpstarter/jumpstarter/config/exporter.py (1)
190-195: Handle potential None value for exporter path.The
str(exporter.path)on line 194 will display "None" if an exporter doesn't have a path set. Consider displaying an empty string or a placeholder instead.Apply this improvement:
def rich_add_rows(self, table): for exporter in self.items: table.add_row( exporter.alias, - str(exporter.path), + str(exporter.path) if exporter.path else "", )packages/jumpstarter/jumpstarter/config/client.py (1)
311-318: Handle potential None value for client path.Similar to the exporter config,
str(client.path)will display "None" if a client doesn't have a path set.Apply this improvement:
def rich_add_rows(self, table): for client in self.items: table.add_row( "*" if self.current_config == client.alias else "", client.alias, client.endpoint, - str(client.path), + str(client.path) if client.path else "", )packages/jumpstarter/jumpstarter/client/grpc.py (1)
108-115: Consider column consistency with Kubernetes lease implementation.The column order and selection differs from the Kubernetes lease implementation in the other file. While this may be intentional due to different data availability, consider if consistency would improve user experience.
The Kubernetes lease has columns: NAME, CLIENT, SELECTOR, EXPORTER, DURATION, STATUS, REASON, BEGIN, END, AGE
This gRPC lease has: NAME, SELECTOR, DURATION, CLIENT, EXPORTERConsider if STATUS, REASON, and AGE columns would be valuable here as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py(3 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py(4 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py(4 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.py(1 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py(0 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/table.py(0 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/table_test.py(0 hunks)packages/jumpstarter-cli-driver/jumpstarter_cli_driver/driver.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_client.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/get.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py(3 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py(3 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(4 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(2 hunks)
💤 Files with no reviewable changes (3)
- packages/jumpstarter-cli-common/jumpstarter_cli_common/table.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/table_test.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- 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)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
🔇 Additional comments (37)
packages/jumpstarter/jumpstarter/config/client_config_test.py (1)
328-328: LGTM!The test updates correctly adapt to the new API where
ClientConfigV1Alpha1.list()returns an object with anitemsattribute rather than returning the list directly. These changes maintain the test logic while aligning with the refactored list method implementation.Also applies to: 336-336, 342-342
packages/jumpstarter-cli/jumpstarter_cli/create.py (1)
6-7: Clean refactoring to centralize output handling!The replacement of the explicit output mode handling with
model_print(lease, output)aligns perfectly with the PR objective. This change eliminates code duplication and centralizes all output formatting logic, making the codebase more maintainable.Also applies to: 53-53
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.py (1)
87-87: LGTM!The update correctly adapts to the new API where
ClientConfigV1Alpha1.list()returns an object with anitemsattribute. The logic for setting the first imported client as default remains intact.packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py (2)
13-13: Good refactoring to centralized output handling.The import of
model_printaligns with the PR's objective to deduplicate output mode handling.
48-51: Consistent implementation of centralized output handling.All calls to
model_printcorrectly:
- Pass the model object as the first argument
- Pass the output format as the second argument
- Include
namespace=namespaceas a keyword argument- For exporters, include the
devicesflag when neededThe refactoring successfully removes duplicate output logic and centralizes it in the
model_printfunction.Also applies to: 79-82, 104-107
packages/jumpstarter-cli/jumpstarter_cli/get.py (2)
4-5: Clean import refactoring.The removal of
OutputModeandmake_tableimports, replaced bymodel_print, demonstrates the successful consolidation of output handling logic.
29-29: Excellent simplification of output handling.The replacement of complex match statements with simple
model_printcalls significantly improves code maintainability and readability. The centralized output handling ensures consistency across all commands.Also applies to: 44-44
packages/jumpstarter-cli-driver/jumpstarter_cli_driver/driver.py (1)
9-9: Why was the function changed from async to sync?The change from
async deftodefis not explained in the PR description. Was this necessary for the new output handling design?packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py (1)
17-17: Good refactoring to centralize output handling!The migration to use the centralized
model_printfunction improves modularity and follows the DRY principle. The change to access.itemsis consistent with the updated return type ofClientConfigV1Alpha1.list().Also applies to: 110-110, 116-116, 176-176
packages/jumpstarter/jumpstarter/config/client.py (1)
259-259: Address circular import pattern.Importing
UserConfigV1Alpha1inside the method suggests a circular dependency issue. While this works, it indicates a potential architectural problem that should be addressed.Consider refactoring the module structure to avoid circular imports, or document why this pattern is necessary here.
packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
6-7: Clean refactoring to centralize output handling!The replacement of the match statement with
model_printsimplifies the code and aligns with the PR's objective of deduplicating output mode handling.Also applies to: 32-32
packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py (1)
9-9:Details
✅ Verification successful
Verify that the list() method returns a model with rich display methods
The refactoring looks good. The removal of
ExporterConfigListV1Alpha1import suggests thatExporterConfigV1Alpha1.list()now returns a model with built-in rich display capabilities.Also applies to: 11-11, 82-82
🏁 Script executed:
#!/bin/bash # Description: Verify that ExporterConfigV1Alpha1.list() returns a model with the required rich display methods # Check the return type and implementation of list() method ast-grep --pattern 'class ExporterConfigV1Alpha1 { $$$ list($$$) { $$$ } $$$ }' # Also search for the list method implementation rg -A 10 "def list.*ExporterConfig" --type pyLength of output: 1162
🏁 Script executed:
#!/bin/bash # Locate and inspect the class definition for ExporterConfigListV1Alpha1 rg -n "class ExporterConfigListV1Alpha1" packages/jumpstarter/jumpstarter/config/exporter.py rg -A 20 "class ExporterConfigListV1Alpha1" packages/jumpstarter/jumpstarter/config/exporter.pyLength of output: 984
ExporterConfigV1Alpha1.list() return type validated
The
list()method still returns anExporterConfigListV1Alpha1, which definesrich_add_columns,rich_add_rows, andrich_add_namesfor rich display. No further changes are needed here.packages/jumpstarter-cli/jumpstarter_cli/config_client.py (4)
12-14: Import changes look good!The migration from
make_tabletomodel_printand the removal of unused import align well with the centralized output handling approach.
96-96: Consistent with the new list API design.The change correctly accesses the
.itemsproperty of the list object returned by the refactoredlist()method.
112-112: Iteration correctly updated for the new API.The loop properly iterates over the
.itemsproperty of the list object.
137-139: Excellent simplification of output handling!The centralized
model_printfunction elegantly handles all output formats, removing the need for conditional logic and improving maintainability.packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (3)
56-61: Well-implemented rich display column definition.The class method correctly defines the table columns for client display.
62-70: Rich display methods properly implemented.Both
rich_add_rowsandrich_add_namesare well-implemented with appropriate null safety for the status field.
79-90: List class methods correctly delegate to item methods.The implementation properly iterates over items and delegates to the corresponding methods.
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (4)
8-8: Required import for time display.The
time_sinceimport is properly added to support the AGE column in the rich display.
61-74: Flexible column definition with device mode support.The method correctly handles two display modes with appropriate columns for each view.
75-98: Comprehensive row generation with proper null safety.The method correctly handles both display modes with appropriate null checks and label formatting.
118-121: Methods correctly implemented with past issue resolved.The
rich_add_namesparameter naming issue from the previous review has been fixed - the parameter is now correctly namednamesinstead oftable.packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (5)
80-92: LGTM! Well-structured table column definitions.The column headers are comprehensive and appropriate for lease resource display.
93-109: LGTM! Safe condition handling with clear status mapping.The method properly checks for empty conditions before accessing the last element and provides clear status interpretation logic.
110-125: LGTM! Good null safety and data formatting.The method handles potential None values appropriately and formats complex data (selectors) clearly for display.
127-129: LGTM! Simple and correct name formatting.The method properly formats the lease name with its resource type prefix.
138-149: LGTM! Proper delegation pattern for list operations.The list class methods correctly delegate to individual item methods, following good OOP patterns.
packages/jumpstarter/jumpstarter/client/grpc.py (9)
47-51: LGTM! Clear column definitions for exporter display.The columns are appropriate for displaying exporter information.
52-57: LGTM! Proper label formatting.The method correctly formats labels as key=value pairs and handles empty labels gracefully.
58-60: LGTM! Simple name appending.The method correctly appends the exporter name.
116-124: LGTM! Clean data formatting for table display.The method properly converts duration to string and handles the data appropriately.
125-127: LGTM! Simple name appending.The method correctly appends the lease name.
140-147: LGTM! Proper delegation pattern for list operations.The list class methods correctly delegate to individual item methods.
148-151: LGTM! Consistent delegation pattern.The method follows the same delegation pattern as other list methods.
164-171: LGTM! Proper delegation pattern for list operations.The list class methods correctly delegate to individual item methods.
172-175: LGTM! Consistent delegation pattern.The method follows the same delegation pattern as other list methods.
NickCao
force-pushed
the
output-mode-new
branch
3 times, most recently
from
d507ba0 to
e76e7f7
Compare
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
This is good, sorry for taking so long, now there is at least a conflict :-/
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (1)
6-6:time_sincerobustness already flagged earlier
Previous review noted thattime_sincemust accept bothdatetimeobjects and ISO-8601 strings (with/without fractional seconds). Make sure the updated implementation landed before relying on it here.
🧹 Nitpick comments (3)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (3)
80-92: Column metadata looks good – consider widths/no-wrap
The column set matches the row data. If very long selector strings are expected, consider settingno_wrap=Trueor amax_widthon the “SELECTOR” column to keep tables readable, but that’s optional.
93-109: Remove redundantelseblocks & simplify logic
Tworeturnstatements make the trailingelseunnecessary and slightly deepen the nesting. A small refactor improves readability:def get_reason(self): condition = self.status.conditions[-1] if self.status.conditions else None - reason = condition.reason if condition is not None else "Unknown" - status = condition.status if condition is not None else "False" - if reason == "Ready": - if status == "True": - return "Ready" - else: - return "Waiting" - elif reason == "Expired": - if status == "True": - return "Expired" - else: - return "Complete" - else: - return reason + reason = condition.reason if condition else "Unknown" + status = condition.status if condition else "False" + + if reason == "Ready": + return "Ready" if status == "True" else "Waiting" + if reason == "Expired": + return "Expired" if status == "True" else "Complete" + return reason
112-114: Use dictionary iteration for clarity
Minor readability nit – iterate directly overdict.items()instead of indexing by key.- selectors = [] - for label in self.spec.selector.match_labels: - selectors.append(f"{label}:{str(self.spec.selector.match_labels[label])}") + selectors = [ + f"{key}:{value}" + for key, value in self.spec.selector.match_labels.items() + ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py(3 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py(4 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py(4 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.py(1 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py(0 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/table.py(0 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/table_test.py(0 hunks)packages/jumpstarter-cli-driver/jumpstarter_cli_driver/driver.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_client.py(4 hunks)packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/get.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py(3 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py(3 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(5 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(2 hunks)
💤 Files with no reviewable changes (3)
- packages/jumpstarter-cli-common/jumpstarter_cli_common/table_test.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/table.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/jumpstarter-cli/jumpstarter_cli/get.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.py
- packages/jumpstarter/jumpstarter/config/client_config_test.py
- packages/jumpstarter-cli/jumpstarter_cli/update.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.py
- packages/jumpstarter-cli/jumpstarter_cli/create.py
- packages/jumpstarter-cli/jumpstarter_cli/config_exporter.py
- packages/jumpstarter-cli/jumpstarter_cli/config_client.py
- packages/jumpstarter-cli-driver/jumpstarter_cli_driver/driver.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py
- packages/jumpstarter/jumpstarter/config/exporter.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py
- packages/jumpstarter/jumpstarter/config/client.py
- packages/jumpstarter/jumpstarter/client/grpc.py
🧰 Additional context used
🪛 Pylint (3.3.7)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py
[refactor] 98-101: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 103-106: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: e2e
| def rich_add_rows(self, table): | ||
| selectors = [] | ||
| for label in self.spec.selector.match_labels: | ||
| selectors.append(f"{label}:{str(self.spec.selector.match_labels[label])}") | ||
| table.add_row( | ||
| self.metadata.name, | ||
| self.spec.client.name if self.spec.client is not None else "", | ||
| ",".join(selectors) if len(selectors) > 0 else "*", | ||
| self.status.exporter.name if self.status.exporter is not None else "", | ||
| self.spec.duration, | ||
| "Ended" if self.status.ended else "InProgress", | ||
| self.get_reason(), | ||
| self.status.begin_time if self.status.begin_time is not None else "", | ||
| self.status.end_time if self.status.end_time is not None else "", | ||
| time_since(self.metadata.creation_timestamp), | ||
| ) | ||
|
|
There was a problem hiding this comment.
None values will crash Rich – coerce to empty strings
rich.table.Table.add_row() expects only str / RichRenderable instances.
self.spec.duration can be None, causing TypeError: unsupported operand type(s) for +: 'NoneType' and 'str' when Rich tries to concatenate. Apply the same safeguard used for begin_time/end_time.
- self.spec.duration,
+ self.spec.duration or "",While you’re here, the selector list can be built more idiomatically:
selectors = [f"{k}:{v}" for k, v in self.spec.selector.match_labels.items()]🤖 Prompt for AI Agents
In packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py around lines
110 to 126, the call to table.add_row passes self.spec.duration which can be
None, causing a TypeError in Rich. Fix this by coercing self.spec.duration to an
empty string if it is None, similar to how begin_time and end_time are handled.
Also, refactor the selectors list construction to use a list comprehension:
replace the for loop with selectors = [f"{k}:{v}" for k, v in
self.spec.selector.match_labels.items()].
2 participants
Summary by CodeRabbit
model_printfor consistent output handling.model_print, removing manual format logic.dump_json,dump_yaml) in favor ofmodel_print.rich.rich_add_columns,rich_add_rows,rich_add_names) to resource classes for enhanced table visualization.itemsattribute, ensuring accurate default setting logic.