Print specified yaml fields, like .spec.storageClassName #41
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested labels
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
|
jpeimer
changed the title
jpeimer
changed the title
jpeimer
changed the title
|
/verified |
![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 (4)
README.md (2)
53-59: Documentation looks good, consider adding more contextThe new section clearly documents the YAML field extraction feature with good examples. Consider adding:
- A brief explanation of the dot notation syntax (e.g., "Use dot notation to traverse the YAML structure")
- A note about error handling when specified fields don't exist
- To print specific YAML fields, add `.<key>.<key>.<key>...` after `-oyaml`: + +> Note: Use dot notation to traverse the YAML structure. If a specified field doesn't exist, +> the command will return an empty result. + ```bash get node <node-name> -oyaml .status.nodeInfo get pvc <pvc-name-start-with> -oyaml .spec.storageClassName # To print .spec.storageClassName of all PVCs when name starts from <pvc-name-start-with>
55-59: Improve code block formattingThe command examples could be more readable with consistent indentation and line breaks between different examples.
```bash get node <node-name> -oyaml .status.nodeInfo + get pvc <pvc-name-start-with> -oyaml .spec.storageClassName # To print .spec.storageClassName of all PVCs when name starts from <pvc-name-start-with> + get pvc -n <namespace> <pvc-name-start-with> -oyaml .spec.accessModes # To print .spec.accessModes of all PVCs in 'namespace' when name starts from <pvc-name-start-with></blockquote></details> <details> <summary>must_gather_explorer/main.py (2)</summary><blockquote> `157-163`: **Enhance command parsing for robustness** The current parsing of command-line arguments may not handle all edge cases and could become complex as more options are added. Consider using `click`'s advanced features or the `argparse` library to parse arguments more robustly and maintainably. --- `269-274`: **Use `NotImplementedError` for unimplemented functions** In the `get_logs` and `get_describe` functions, instead of printing "Not implemented yet", raise a `NotImplementedError`. This is a Python convention to indicate that the function is intentionally not implemented. Apply this diff to update the functions: ```diff def get_logs(**kwargs: Dict[Any, Any]) -> None: - CONSOLE.print("Not implemented yet") - pass + raise NotImplementedError("get_logs function is not implemented yet") def get_describe(**kwargs: Dict[Any, Any]) -> None: - CONSOLE.print("Not implemented yet, please use 'get <resource_kind> ... -oyaml'") - pass + raise NotImplementedError("get_describe function is not implemented yet")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
README.md(1 hunks)must_gather_explorer/main.py(6 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🔇 Additional comments (1)
must_gather_explorer/main.py (1)
252-265: Good implementation of YAML field extraction
The print_specific_yaml_fields function effectively extracts and prints specific fields from the YAML content based on user input, handling nested fields appropriately.
must_gather_explorer/main.py
Outdated
| def parse_all_files_fill_dictionaries_by_file_type( | ||
| must_gather_path: str, | ||
| ) -> Tuple[Dict[str, List[str]], Dict[str, List[str]]]: | ||
| # Fill dictionaries for all files kinds | ||
| all_yaml_files: Dict[str, List[str]] = {} | ||
| all_log_files: Dict[str, List[str]] = {} | ||
| for root, dirs, _ in Path(must_gather_path).walk(): | ||
| for _dir in dirs: | ||
| _dir_path = Path(root, _dir) | ||
| for _file in _dir_path.iterdir(): | ||
| if _file.is_dir(): | ||
| continue | ||
|
|
||
| file_extention = _file.suffix | ||
| if not file_extention: | ||
| LOGGER.debug(f"Skipping file {_file}, file has no extension") | ||
| continue | ||
|
|
||
| if file_extention in (".yaml", ".yml"): | ||
| all_yaml_files.setdefault(str(_dir_path), []).append(_file.name) | ||
|
|
||
| # Pod may have several containers, the path to the log is: | ||
| # <path>/namespaces/openshift-ovn-kubernetes/pods/ovnkube-node-6vrt6/kubecfg-setup/kubecfg-setup/logs | ||
| # <path>/namespaces/openshift-ovn-kubernetes/pods/ovnkube-node-6vrt6/kube-rbac-proxy-node/kube-rbac-proxy-node/logs | ||
| elif file_extention == ".log": | ||
| all_log_files.setdefault(str(_dir_path), []).append(_file.name) | ||
|
|
||
| return all_yaml_files, all_log_files | ||
|
|
There was a problem hiding this comment.
Fix the usage of Path.walk(): Replace with os.walk()
The function parse_all_files_fill_dictionaries_by_file_type incorrectly uses Path.walk(), which does not exist in the pathlib.Path class. This will raise an AttributeError at runtime. Replace Path.walk() with os.walk().
Apply this diff to fix the issue:
def parse_all_files_fill_dictionaries_by_file_type(
must_gather_path: str,
) -> Tuple[Dict[str, List[str]], Dict[str, List[str]]]:
# Fill dictionaries for all files kinds
all_yaml_files: Dict[str, List[str]] = {}
all_log_files: Dict[str, List[str]] = {}
- for root, dirs, _ in Path(must_gather_path).walk():
- for _dir in dirs:
- _dir_path = Path(root, _dir)
- for _file in _dir_path.iterdir():
- if _file.is_dir():
- continue
-
- file_extention = _file.suffix
- if not file_extention:
- LOGGER.debug(f"Skipping file {_file}, file has no extension")
- continue
-
- if file_extention in (".yaml", ".yml"):
- all_yaml_files.setdefault(str(_dir_path), []).append(_file.name)
-
- elif file_extention == ".log":
- all_log_files.setdefault(str(_dir_path), []).append(_file.name)
+ for root, dirs, files in os.walk(must_gather_path):
+ for file_name in files:
+ file_path = Path(root, file_name)
+ file_extension = file_path.suffix
+ if not file_extension:
+ LOGGER.debug(f"Skipping file {file_path}, file has no extension")
+ continue
+ if file_extension in (".yaml", ".yml"):
+ all_yaml_files.setdefault(root, []).append(file_name)
+ elif file_extension == ".log":
+ all_log_files.setdefault(root, []).append(file_name)
return all_yaml_files, all_log_files📝 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.
| def parse_all_files_fill_dictionaries_by_file_type( | |
| must_gather_path: str, | |
| ) -> Tuple[Dict[str, List[str]], Dict[str, List[str]]]: | |
| # Fill dictionaries for all files kinds | |
| all_yaml_files: Dict[str, List[str]] = {} | |
| all_log_files: Dict[str, List[str]] = {} | |
| for root, dirs, _ in Path(must_gather_path).walk(): | |
| for _dir in dirs: | |
| _dir_path = Path(root, _dir) | |
| for _file in _dir_path.iterdir(): | |
| if _file.is_dir(): | |
| continue | |
| file_extention = _file.suffix | |
| if not file_extention: | |
| LOGGER.debug(f"Skipping file {_file}, file has no extension") | |
| continue | |
| if file_extention in (".yaml", ".yml"): | |
| all_yaml_files.setdefault(str(_dir_path), []).append(_file.name) | |
| # Pod may have several containers, the path to the log is: | |
| # <path>/namespaces/openshift-ovn-kubernetes/pods/ovnkube-node-6vrt6/kubecfg-setup/kubecfg-setup/logs | |
| # <path>/namespaces/openshift-ovn-kubernetes/pods/ovnkube-node-6vrt6/kube-rbac-proxy-node/kube-rbac-proxy-node/logs | |
| elif file_extention == ".log": | |
| all_log_files.setdefault(str(_dir_path), []).append(_file.name) | |
| return all_yaml_files, all_log_files | |
| def parse_all_files_fill_dictionaries_by_file_type( | |
| must_gather_path: str, | |
| ) -> Tuple[Dict[str, List[str]], Dict[str, List[str]]]: | |
| # Fill dictionaries for all files kinds | |
| all_yaml_files: Dict[str, List[str]] = {} | |
| all_log_files: Dict[str, List[str]] = {} | |
| for root, dirs, files in os.walk(must_gather_path): | |
| for file_name in files: | |
| file_path = Path(root, file_name) | |
| file_extension = file_path.suffix | |
| if not file_extension: | |
| LOGGER.debug(f"Skipping file {file_path}, file has no extension") | |
| continue | |
| if file_extension in (".yaml", ".yml"): | |
| all_yaml_files.setdefault(root, []).append(file_name) | |
| elif file_extension == ".log": | |
| all_log_files.setdefault(root, []).append(file_name) | |
| return all_yaml_files, all_log_files |
There was a problem hiding this comment.
Path.walk() exists in Python 3.12 and above: https://docs.python.org/3.12/library/pathlib.html#pathlib.Path.walk
There was a problem hiding this comment.
@jpeimer, thank you for the clarification. You're correct that Path.walk() exists in Python 3.12 and above. If your project targets Python 3.12 or later, using Path.walk() is appropriate.
✏️ Learnings added
Learnt from: jpeimer
PR: RedHatQE/must-gather-explorer#41
File: must_gather_explorer/main.py:185-213
Timestamp: 2024-11-28T16:03:15.736Z
Learning: In `must_gather_explorer/main.py`, the function `parse_all_files_fill_dictionaries_by_file_type` correctly uses `Path.walk()`, which is available in Python 3.12 and later versions of Python.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
can't we set the project to use >= 3.12?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I think it's set: https://github.com/RedHatQE/must-gather-explorer/blob/main/pyproject.toml#L30
Is there any other place we should set it?
must_gather_explorer/main.py
Outdated
| def parse_all_files_fill_dictionaries_by_file_type( | ||
| must_gather_path: str, | ||
| ) -> Tuple[Dict[str, List[str]], Dict[str, List[str]]]: | ||
| # Fill dictionaries for all files kinds | ||
| all_yaml_files: Dict[str, List[str]] = {} | ||
| all_log_files: Dict[str, List[str]] = {} | ||
| for root, dirs, _ in Path(must_gather_path).walk(): | ||
| for _dir in dirs: | ||
| _dir_path = Path(root, _dir) | ||
| for _file in _dir_path.iterdir(): | ||
| if _file.is_dir(): | ||
| continue | ||
|
|
||
| file_extention = _file.suffix | ||
| if not file_extention: | ||
| LOGGER.debug(f"Skipping file {_file}, file has no extension") | ||
| continue | ||
|
|
||
| if file_extention in (".yaml", ".yml"): | ||
| all_yaml_files.setdefault(str(_dir_path), []).append(_file.name) | ||
|
|
||
| # Pod may have several containers, the path to the log is: | ||
| # <path>/namespaces/openshift-ovn-kubernetes/pods/ovnkube-node-6vrt6/kubecfg-setup/kubecfg-setup/logs | ||
| # <path>/namespaces/openshift-ovn-kubernetes/pods/ovnkube-node-6vrt6/kube-rbac-proxy-node/kube-rbac-proxy-node/logs | ||
| elif file_extention == ".log": | ||
| all_log_files.setdefault(str(_dir_path), []).append(_file.name) | ||
|
|
||
| return all_yaml_files, all_log_files | ||
|
|
There was a problem hiding this comment.
can't we set the project to use >= 3.12?
|
/verified |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
must_gather_explorer/main.py (4)
149-163: Consider adding validation for YAML field pathsWhile the implementation correctly handles YAML field paths, consider adding validation to ensure the path format is valid (e.g., only contains allowed characters, proper dot notation).
yaml_fields_to_get = "" if yaml_flag in commands_list: if action_name != get_action: CONSOLE.print(f"'{yaml_flag}' is only supported with '{get_action}' action") continue print_yaml = True commands_list.remove(yaml_flag) if commands_list: commands_list_last_value = commands_list[-1] if commands_list_last_value.startswith("."): + # Validate YAML field path format + if not all(part.isidentifier() for part in commands_list_last_value[1:].split(".")): + CONSOLE.print(f"Invalid YAML field path format: {commands_list_last_value}") + continue yaml_fields_to_get = commands_list_last_value commands_list.remove(yaml_fields_to_get)
185-213: Add error handling for file operationsWhile the function is well-structured, consider adding error handling for file operations to gracefully handle potential issues.
def parse_all_files_fill_dictionaries_by_file_type( must_gather_path: str, ) -> tuple[dict[str, list[str]], dict[str, list[str]]]: all_yaml_files: dict[str, list[str]] = {} all_log_files: dict[str, list[str]] = {} - for root, dirs, _ in Path(must_gather_path).walk(): - for _dir in dirs: - _dir_path = Path(root, _dir) - for _file in _dir_path.iterdir(): + try: + for root, dirs, _ in Path(must_gather_path).walk(): + for _dir in dirs: + _dir_path = Path(root, _dir) + try: + for _file in _dir_path.iterdir():
253-272: LGTM! Well-implemented YAML field extractionThe function handles missing fields gracefully and provides clear error messages. Consider adding a return type hint for clarity.
-def print_specific_yaml_fields(resource_yaml_content: str, yaml_fields_to_get: str) -> None: +def print_specific_yaml_fields(resource_yaml_content: str, yaml_fields_to_get: str) -> None | bool: # ... existing code ... - if yaml_fields_dict_to_print: - CONSOLE.print(yaml.dump(yaml_fields_dict_to_print)) + if yaml_fields_dict_to_print: + CONSOLE.print(yaml.dump(yaml_fields_dict_to_print)) + return True + return False
274-282: Mark unimplemented functions with TODO commentsConsider adding proper TODO comments to track these unimplemented functions.
-def get_logs(**kwargs: dict[Any, Any]) -> None: +# TODO: Implement log retrieval functionality +def get_logs(**kwargs: dict[Any, Any]) -> None: CONSOLE.print("Not implemented yet") pass -def get_describe(**kwargs: dict[Any, Any]) -> None: +# TODO: Implement resource description functionality +def get_describe(**kwargs: dict[Any, Any]) -> None: CONSOLE.print("Not implemented yet, please use 'get <resource_kind> ... -oyaml'") pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
must_gather_explorer/main.py(8 hunks)must_gather_explorer/resource_aliases.py(2 hunks)
🧰 Additional context used
📓 Learnings (1)
must_gather_explorer/main.py (1)
Learnt from: jpeimer
PR: RedHatQE/must-gather-explorer#41
File: must_gather_explorer/main.py:185-213
Timestamp: 2024-11-28T16:03:16.140Z
Learning: In `must_gather_explorer/main.py`, the function `parse_all_files_fill_dictionaries_by_file_type` correctly uses `Path.walk()`, which is available in Python 3.12 and later versions of Python.
🔇 Additional comments (3)
must_gather_explorer/resource_aliases.py (1)
24-24: LGTM! Type hint modernization
The changes to use lowercase dict and list instead of Dict and List align with PEP 585 and modern Python type hints.
Also applies to: 34-34
must_gather_explorer/main.py (2)
47-51: LGTM! Good refactoring
Extracting the file parsing logic into a separate function improves code organization and maintainability. The type annotation update to use dict aligns with the modern Python type hints.
289-291: LGTM! Type hint modernization
The changes to use lowercase dict and list instead of Dict and List align with PEP 585 and modern Python type hints.
Print specified yaml fields, i.e.
get node <node-name> -oyaml .status.nodeInfoExtract
parse_all_files_fill_dictionaries_by_file_typeto a separate functionSummary by CodeRabbit
New Features
-oyamlflag.Bug Fixes
Documentation
Chores