Conversation
Signed-off-by: Jooho Lee <jlee@redhat.com>
WalkthroughThe changes introduce helper functions and conditional logic to avoid redundant CLI tool downloads in shell scripts, enhance the CLI run command with options to list and inspect available scripts, and improve logging with customizable colors. Script headers and error handling were refined, and test timeouts and command flags were updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant RunCommand
User->>CLI: run --list
CLI->>RunCommand: list_scripts()
RunCommand-->>CLI: Print available scripts
User->>CLI: run --info <command>
CLI->>RunCommand: show_script_info(command)
RunCommand-->>CLI: Print script info
User->>CLI: run <command> [params]
CLI->>RunCommand: run(command, params)
RunCommand-->>CLI: Execute script or print error
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: 2
🧹 Nitpick comments (2)
hacks/download-cli-for-ci.sh (1)
95-103: Consider improving the condition logic for OC & KUBECTL.The current condition requires BOTH binaries to be missing before downloading. This could lead to unnecessary downloads if only one binary is missing.
Consider this improvement:
-if ! check_binary_exists "${root_directory}/bin/oc" || ! check_binary_exists "${root_directory}/bin/kubectl"; then +oc_missing=false +kubectl_missing=false +if ! check_binary_exists "${root_directory}/bin/oc"; then + oc_missing=true +fi +if ! check_binary_exists "${root_directory}/bin/kubectl"; then + kubectl_missing=true +fi +if [[ "$oc_missing" == true ]] || [[ "$kubectl_missing" == true ]]; thenThis would provide more granular control and only download when actually needed.
src/cli/commands/run.py (1)
7-7: Remove unused import identified by static analysis.The
infofunction is imported but never used in the code.-from commons.python.py_utils import error, die, info, custom_log +from commons.python.py_utils import error, die, custom_log🧰 Tools
🪛 Ruff (0.11.9)
7-7:
commons.python.py_utils.infoimported but unusedRemove unused import:
commons.python.py_utils.info(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
hacks/download-cli-for-ci.sh(1 hunks)hacks/download-cli.sh(1 hunks)src/cli/cli.py(2 hunks)src/cli/commands/run.py(4 hunks)src/commons/python/py_utils.py(2 hunks)src/dev/scripts/debug-pod.sh(2 hunks)tests/conftest.py(1 hunks)tests/e2e/roles/cluster-tests/operator_install_delete/test_e2e_operator.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/roles/cluster-tests/operator_install_delete/test_e2e_operator.py
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/cli/cli.py (1)
src/cli/commands/run.py (2)
run(119-148)run(192-220)
hacks/download-cli.sh (1)
hacks/download-cli-for-ci.sh (3)
get_latest_release(36-39)check_binary_exists(49-56)delete_if_exists(41-47)
hacks/download-cli-for-ci.sh (1)
hacks/download-cli.sh (3)
get_latest_release(36-39)check_binary_exists(49-56)delete_if_exists(41-47)
src/cli/commands/run.py (1)
src/commons/python/py_utils.py (4)
error(76-77)die(55-57)info(65-69)custom_log(48-52)
🪛 Ruff (0.11.9)
src/cli/commands/run.py
7-7: commons.python.py_utils.info imported but unused
Remove unused import: commons.python.py_utils.info
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cluster-test (3.12)
- GitHub Check: non-cluster-test (3.12)
🔇 Additional comments (13)
tests/conftest.py (1)
17-17: LGTM! Reasonable timeout reduction for improved test efficiency.The timeout reduction from 120 to 60 seconds aligns with the PR's efficiency improvements and should help speed up test execution.
src/dev/scripts/debug-pod.sh (2)
3-10: Excellent documentation enhancement!The detailed script header with description and parameter documentation significantly improves maintainability and usability. This makes it much easier for developers to understand the script's purpose and available options.
32-32: Good error handling refinement.Replacing
--ignore-not-found=truewith2>/dev/nullis a good approach. This change ensures that actual errors aren't silently ignored while still suppressing expected "not found" error messages during cleanup.src/commons/python/py_utils.py (2)
48-53: Well-implemented logging enhancement!The new
custom_logfunction provides flexible color customization while maintaining backward compatibility. The implementation correctly handles the optional color parameter and properly resets colors after printing.
65-69: Good backward-compatible enhancement to info function.The updated
infofunction maintains full backward compatibility while adding color customization support. The logic correctly prioritizes the custom color when provided and falls back to the default info color.src/cli/cli.py (2)
6-6: Consistent import update for refactored run command.The import change from
run_commandtorunaligns with the enhanced CLI functionality that now supports--listand--infooptions for script management.
54-54: Proper CLI command registration update.The command registration correctly uses the renamed
runfunction, maintaining consistency with the refactored import and enhanced command functionality.hacks/download-cli-for-ci.sh (2)
49-56: LGTM! Well-implemented binary existence check.The function correctly checks for binary existence and provides clear feedback. The return values (0 for exists, 1 for not exists) follow shell convention.
62-69: Excellent idempotency improvement for YQ installation.The conditional check prevents unnecessary downloads when the binary already exists, improving script efficiency and reliability.
hacks/download-cli.sh (1)
49-56: LGTM! Consistent binary existence check implementation.The function is identical to the CI version, ensuring consistent behavior across scripts.
src/cli/commands/run.py (3)
18-41: Well-implemented script listing functionality.The method correctly handles file discovery, deduplication, and error handling. Using a set to avoid duplicates when both .sh and .py versions exist is a good approach.
76-118: Robust script information extraction with good error handling.The regex patterns for extracting description and parameters from script comments are well-designed. The formatting with colored output enhances user experience.
185-220: Well-structured CLI option handling with clear logic flow.The branching logic correctly handles the different command modes (list, info, direct execution) and provides appropriate error messages when no command is specified.
Signed-off-by: Jooho Lee <jlee@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/cli/commands/run.py (1)
66-69:⚠️ Potential issueFix unreachable code after die() call.
This is identical to a previous review comment. The
die()function callssys.exit(1), making the subsequent code unreachable. The past review suggestion should be applied.- die(user_msg) + die(user_msg)Note: The return statement and exception handling after
die()are unreachable and should be removed.
🧹 Nitpick comments (2)
src/cli/commands/run.py (2)
7-7: Remove unused import.The
infofunction is imported but never used in this file.-from commons.python.py_utils import error, die, info, custom_log +from commons.python.py_utils import error, die, custom_log🧰 Tools
🪛 Ruff (0.11.9)
7-7:
commons.python.py_utils.infoimported but unusedRemove unused import:
commons.python.py_utils.info(F401)
82-99: Consider improving regex patterns for better robustness.The regex patterns for extracting description and parameters could be more robust. The current patterns may not handle edge cases well, such as descriptions spanning multiple comment blocks or nested parameter structures.
Consider these improvements:
# Extract description - description_match = re.search( - r"#\s*Description:\s*(.*?)(?:\n#|\n\n|\n$)", content, re.DOTALL - ) + description_match = re.search( + r"#\s*Description:\s*((?:#.*\n?)*)", content, re.MULTILINE + ) # Extract parameters - params_match = re.search( - r"#\s*Available Parameters:(.*?)(?:\n\n|\n$)", content, re.DOTALL - ) + params_match = re.search( + r"#\s*Available Parameters:\s*((?:#.*\n?)*)", content, re.MULTILINE + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hacks/download-cli.sh(1 hunks)src/cli/commands/run.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hacks/download-cli.sh
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/cli/commands/run.py (1)
src/commons/python/py_utils.py (4)
error(76-77)die(55-57)info(65-69)custom_log(48-52)
🪛 Ruff (0.11.9)
src/cli/commands/run.py
7-7: commons.python.py_utils.info imported but unused
Remove unused import: commons.python.py_utils.info
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: non-cluster-test (3.12)
- GitHub Check: cluster-test (3.12)
🔇 Additional comments (4)
src/cli/commands/run.py (4)
18-41: Well-implemented script listing functionality.The
list_scriptsmethod correctly handles script discovery and deduplication. The use of a set to avoid duplicate command names when both.shand.pyversions exist is a good approach.
105-113: Good parameter formatting logic.The parameter formatting correctly handles the extraction and cleaning of comment-based documentation, providing user-friendly bullet-point output.
144-146: Excellent improvement to user feedback.Adding the user-facing error message via
error()provides better user experience by showing errors directly to the user in addition to logging.
184-219: Well-structured CLI option implementation.The CLI function properly handles the new
--listand--infooptions with appropriate validation and error messages. The logic flow is clear and the optionalcommand_nameargument works well with the new flags.
|
/lgtm |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests