Print resource yaml #20
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
Supported /retest check runs
Supported labels
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
README.md (2)
31-32: Excellent addition of YAML printing instructions!The new section clearly explains how to use the YAML printing feature, which is a key objective of this PR. This addition significantly enhances the utility of the tool for users.
Consider adding a brief explanation of why a user might want to print the resource YAML. For example:
- To print the resource YAML (useful for viewing detailed configuration or troubleshooting), add `yaml` to the `get` command, for example:This additional context could help users understand when and why they might use this feature.
33-34: Well-structured examples for YAML printing!These examples effectively demonstrate how to use the new YAML printing feature, both with and without specifying a namespace. They are clear, concise, and consistent with the format of previous examples in the document.
For consistency with the previous examples, consider adding a comment after each command explaining what it does. For example:
get pvc hpp yaml # To print YAMLs of all PVCs when name starts from 'hpp' get pvc -n openshift-storage hpp yaml # To print YAMLs of all PVCs in 'openshift-storage' namespace when name starts from 'hpp'This would maintain the style established in the earlier examples and further clarify the purpose of each command.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- README.md (1 hunks)
- app/main.py (4 hunks)
🧰 Additional context used
🔇 Additional comments (3)
README.md (2)
28-29: Great improvements to the documentation!The changes in these lines enhance the clarity and completeness of the usage instructions:
- The grammatical correction from "name starts form" to "name starts from" improves readability.
- The new example demonstrating the use of the 'get' command with a specified namespace is a valuable addition, providing users with a clear understanding of how to query resources in specific namespaces.
These updates contribute to a more user-friendly and comprehensive README.
28-34: Overall excellent improvements to the README!The changes to this file successfully document the new YAML printing feature, aligning perfectly with the PR objectives. The updated examples and new instructions are clear, concise, and will greatly assist users in understanding and utilizing the new functionality.
The minor suggestions provided in the previous comments (adding context to the YAML printing explanation and maintaining consistency in example comments) would further enhance the already improved documentation. Great work on making the Must Gather Explorer tool more user-friendly and powerful!
app/main.py (1)
90-91: Good addition to handle empty user commandsThe check for an empty
user_commandensures the prompt continues if the user presses 'Enter' without typing a command, improving the user experience.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- README.md (1 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md
42-42: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
44-44: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
43-43: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
43-43: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
🔇 Additional comments (2)
README.md (2)
40-41: LGTM: Clear and helpful examples addedThe new examples for using the
getcommand with PersistentVolumeClaims are clear and provide good coverage of different use cases. They demonstrate filtering by name and namespace, which will be helpful for users.
42-42: No changes requiredThis line is part of the existing content and hasn't been modified. It fits well within the context of the examples provided.
🧰 Tools
🪛 Markdownlint
42-42: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
README.md (1)
40-46: Summary: Documentation improvements with suggestions for further enhancementsThe additions to the README.md file effectively document the new YAML printing feature of the Must-gather explorer tool. The examples provided are clear and helpful for users. To further improve the documentation:
- Consider adopting the Kubernetes-style
-o yamlformat for consistency with widely-used conventions.- Improve markdown formatting by adding blank lines around list items and code blocks.
- Ensure consistency in the YAML output format across all examples.
These changes will enhance readability and align the tool's usage with familiar patterns, improving the overall user experience.
Would you like assistance in implementing these suggested changes throughout the README.md file?
🧰 Tools
🪛 Markdownlint
42-42: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
44-44: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
43-43: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
43-43: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
app/main.py (2)
135-142: Consider supporting the standard '-o yaml' option formatTo align with common CLI conventions (e.g.,
kubectl), consider parsing the output format flag as-o yamlinstead of-oyaml. This would improve user experience by matching expected command formats.Apply this diff to adjust the parsing:
print_yaml = False -yaml_flag = "-oyaml" +output_flag = "-o" +output_format = "" -if yaml_flag in commands_list: +if output_flag in commands_list: + idx = commands_list.index(output_flag) + if idx + 1 < len(commands_list): + output_format = commands_list[idx + 1] + if output_format == 'yaml': + print_yaml = True + commands_list.pop(idx) # Remove '-o' + commands_list.pop(idx) # Remove 'yaml' + else: + CONSOLE.print(f"[red]Unsupported output format '{output_format}'[/red]") + continue + else: + CONSOLE.print("[red]Output format not specified after '-o' flag[/red]") + continue +if print_yaml and action_name != GET_ACTION: if action_name != GET_ACTION: - CONSOLE.print(f"'{yaml_flag}' is only supported with '{GET_ACTION}' action") + CONSOLE.print(f"'--output yaml' is only supported with '{GET_ACTION}' action") continue - print_yaml = True - commands_list.remove(yaml_flag)
183-191: Note: Empty action functionsThe
get_logs,get_describe, andprint_helpfunctions are currently empty. If they are placeholders for future implementation, consider adding a TODO comment to indicate that they need to be completed.Would you like assistance in implementing these functions or creating a GitHub issue to track their completion?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- README.md (1 hunks)
- app/main.py (3 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md
42-42: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
44-44: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
43-43: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
43-43: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
🔇 Additional comments (5)
README.md (1)
40-41: LGTM: Clear and helpful examples addedThe new examples for using the
getcommand with PersistentVolumeClaims are clear and provide valuable information for users. They demonstrate how to filter resources by name prefix and how to specify a namespace, which enhances the usability of the tool.app/main.py (4)
81-82: [Duplicate Comment] 'yaml' should not be inactions_dictThis issue was previously raised and is still applicable. Please refer to the prior feedback regarding not including
'yaml'as an action since it's an option for the'get'action, not a separate action.
92-93: LGTM: Added check for empty user inputThe addition of this check improves the user experience by preventing unexpected termination when the prompt is left empty.
157-167: LGTM: Updated 'get_resources' to handle YAML outputThe modifications allow the
get_resourcesfunction to print resource YAML whenprint_yamlisTrue, enhancing functionality.
170-180: LGTM: Added exception handling and dynamic separatorThe function now handles exceptions when opening files and uses a dynamic separator based on terminal width, improving robustness and readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/main.py (4)
91-92: LGTM: Improved user experience for empty inputThe addition of the condition to continue the loop on empty user input enhances the user experience by preventing unexpected termination. This aligns well with the PR objective.
Consider adding a user-friendly message when empty input is received:
if not user_command: + CONSOLE.print("Empty input received. Please enter a command or type 'exit' to quit.") continue
132-143: LGTM: Added YAML output functionalityThe implementation of the "-oyaml" flag successfully adds the ability to print YAML representations of resources, aligning with the PR objectives. The code correctly checks for the appropriate action and sets a boolean flag to control the output format.
For consistency with other constants in the file, consider defining the YAML flag as a constant:
+YAML_FLAG = "-oyaml" # ... (earlier in the file) -yaml_flag = "-oyaml" +if YAML_FLAG in commands_list: if action_name != GET_ACTION: - CONSOLE.print(f"'{yaml_flag}' is only supported with '{GET_ACTION}' action") + CONSOLE.print(f"'{YAML_FLAG}' is only supported with '{GET_ACTION}' action") continue print_yaml = True - commands_list.remove(yaml_flag) + commands_list.remove(YAML_FLAG)
162-173: LGTM: Updated get_resources function to handle YAML outputThe
get_resourcesfunction has been successfully updated to handle both YAML and table output formats based on theprint_yamlflag. This change aligns with the PR objectives and maintains existing functionality.Consider extracting the table printing logic into a separate function for better code organization:
def get_resources(resources_raw_data: List[Dict[str, Any]], print_yaml: bool = False, **kwargs: Dict[Any, Any]) -> None: if print_yaml: print_resource_yaml(resources_raw_data=resources_raw_data) else: - # Print table of Namespace, Name - table = Table() - table.add_column("NAMESPACE") - table.add_column("NAME") - for raw_data in resources_raw_data: - table.add_row(raw_data["namespace"], raw_data["name"]) - CONSOLE.print(table) + print_resource_table(resources_raw_data) + +def print_resource_table(resources_raw_data: List[Dict[str, Any]]) -> None: + table = Table() + table.add_column("NAMESPACE") + table.add_column("NAME") + for raw_data in resources_raw_data: + table.add_row(raw_data["namespace"], raw_data["name"]) + CONSOLE.print(table)
175-185: LGTM: New print_resource_yaml function addedThe
print_resource_yamlfunction successfully implements the YAML output functionality, including proper error handling for file operations. The use of a terminal-width separator between resources improves readability.Consider the following minor improvements:
- Use a context manager for file operations to ensure proper resource cleanup:
- try: - with open(raw_data["yaml_file"]) as fd: - resource_yaml_content = fd.read() - except (FileNotFoundError, IOError) as e: + try: + with open(raw_data["yaml_file"], "r") as fd: + resource_yaml_content = fd.read() + except OSError as e:
- Consider using
yaml.safe_dump()to ensure consistent YAML formatting:import yaml # ... - CONSOLE.print(resource_yaml_content) + yaml_dict = yaml.safe_load(resource_yaml_content) + CONSOLE.print(yaml.safe_dump(yaml_dict, default_flow_style=False))
- Extract the separator line to a constant for consistency:
+SEPARATOR = "-" * os.get_terminal_size().columns # ... - CONSOLE.print("-" * os.get_terminal_size().columns) + CONSOLE.print(SEPARATOR)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- README.md (1 hunks)
- app/main.py (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🔇 Additional comments (4)
app/main.py (4)
78-81: LGTM: Improved code maintainabilityThe introduction of the
GET_ACTIONconstant and its usage inactions_dictenhances code readability and maintainability. This change aligns with the DRY principle and reduces the risk of typos when referring to the "get" action.
106-108: LGTM: Improved error message formattingThe reformatted error message for unsupported actions enhances readability and follows Python best practices for string formatting. The use of f-strings and multi-line formatting improves code clarity.
Line range hint
1-265: Overall assessment: Approved with minor suggestionsThe changes in this PR successfully implement the ability to print YAML representations of resources and improve the user experience of the Must Gather Explorer tool. The code is well-structured and aligns with the PR objectives.
Key improvements:
- Added YAML output functionality
- Enhanced error handling and user input processing
- Improved code readability and maintainability
Minor suggestions have been made throughout the review to further enhance code quality and consistency. These include:
- Extracting constants for better maintainability
- Improving error handling in file operations
- Refactoring for better code organization
Overall, this PR represents a significant improvement to the tool's functionality and user experience.
159-159:⚠️ Potential issuePotential issue with function argument mismatch
The current implementation passes the
print_yamlparameter to all action functions, which may cause errors for functions that don't expect this argument.Consider modifying the code to handle different function signatures:
-actions_dict[action_name](resources_raw_data, print_yaml) +if action_name == GET_ACTION: + actions_dict[action_name](resources_raw_data, print_yaml) +else: + actions_dict[action_name](resources_raw_data)Alternatively, you could update all action functions to accept
**kwargsto handle additional parameters gracefully.Likely invalid or redundant comment.
|
/lgtm |
|
/verified |
getpattern, print yamls for all of themSummary by CodeRabbit
getcommand.-oyamlflag in the command-line interface.