Fix must-gather scripts directory discovery #164
Conversation
Signed-off-by: chiragkyal <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chiragkyal The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughTwo documentation files are updated to replace hardcoded script paths with dynamic discovery mechanisms. A new preliminary step is introduced to locate analysis scripts by searching the directory structure, with fallback error messaging. Subsequent steps use discovered path variables instead of static paths. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
/cc @swghosh |
|
@chiragkyal: GitHub didn't allow me to request PR reviews from the following users: swghosh. Note that only openshift-eng members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![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 (1)
plugins/must-gather/commands/ovn-dbs.md (1)
75-97: Verify that SCRIPTS_DIR is used consistently in subsequent steps.The step numbering is correct after adding the preliminary discovery step. However, the documentation for steps 2–5 describes what to do but doesn't explicitly show the command invocation using
$SCRIPTS_DIR/analyze_ovn_dbs.py. While the steps appear logically sound, consider adding a brief note showing how SCRIPTS_DIR is used when executing the script in step 4 for clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
plugins/must-gather/commands/analyze.md(4 hunks)plugins/must-gather/commands/ovn-dbs.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/must-gather/commands/analyze.md
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
148-148: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (6)
plugins/must-gather/commands/ovn-dbs.md (2)
49-56: Clear explanation of plugin root and automatic discovery.The updated documentation effectively clarifies where scripts are located and that automatic discovery is independent of the working directory. This is helpful for end-users.
62-73: Review comment is incorrect; the code block already has the bash language identifier.The actual file content shows the fenced code block at lines 62-73 already includes the
bashlanguage specification (\``bash`). The review claim that "the bash script block is missing a language identifier" is contradicted by the file itself. The provided diff makes no meaningful changes—it only adjusts whitespace while the language identifier remains present.The secondary note about find command performance (searching from home directory) is a reasonable observation but is not a critical or mandatory fix.
Likely an incorrect or invalid review comment.
plugins/must-gather/commands/analyze.md (4)
49-63: Clear and consistent documentation of plugin installation location.The explanation of
<plugin-root>and typical installation path aligns well with the similar update in ovn-dbs.md and provides helpful context for users.
139-151: Execution approach is clear and demonstrates correct SCRIPTS_DIR usage.The new "Locate Plugin Scripts" step and updated "Execute Analysis Scripts" section properly show how to use the dynamically discovered
$SCRIPTS_DIRvariable. The example at lines 150–151 effectively demonstrates the pattern.
49-99: Overall assessment: Dynamic script discovery is well-structured and documented.The changes successfully introduce a robust script discovery mechanism to replace hardcoded paths. The bash patterns are consistent across both files, error handling is in place, and the documentation clearly explains the
<plugin-root>concept. The main action items are addressing the markdown linting issues (missing language specifications) and verifying the directory structure assumption.
62-93: Path patterns are correct; no changes needed.The verification confirms that the
findcommand patterns in the documentation correctly match the actual installation structure. All analysis scripts are present atplugins/must-gather/skills/must-gather-analyzer/scripts/as expected by the documented patterns.
| 1. Locate the scripts directory by searching for a known script: | ||
| ```bash | ||
| SCRIPT_PATH=$(find ~ -name "analyze_clusteroperators.py" -path "*/must-gather/skills/must-gather-analyzer/scripts/*" 2>/dev/null | head -1) | ||
|
|
||
| if [ -z "$SCRIPT_PATH" ]; then | ||
| echo "ERROR: Must-gather analysis scripts not found." | ||
| echo "Please ensure the must-gather plugin from ai-helpers is properly installed." | ||
| exit 1 | ||
| fi | ||
|
|
||
| # All scripts are in the same directory, so just get the directory | ||
| SCRIPTS_DIR=$(dirname "$SCRIPT_PATH") | ||
| ``` |
There was a problem hiding this comment.
Add bash language specification to fenced code block.
The script discovery block is missing a language identifier on the opening fence. This should match the approach in ovn-dbs.md for consistency.
Apply this diff:
1. Locate the scripts directory by searching for a known script:
- ```
+ ```bash
SCRIPT_PATH=$(find ~ -name "analyze_clusteroperators.py" -path "*/must-gather/skills/must-gather-analyzer/scripts/*" 2>/dev/null | head -1)
if [ -z "$SCRIPT_PATH" ]; then
echo "ERROR: Must-gather analysis scripts not found."
echo "Please ensure the must-gather plugin from ai-helpers is properly installed."
exit 1
fi
# All scripts are in the same directory, so just get the directory
SCRIPTS_DIR=$(dirname "$SCRIPT_PATH")
```🤖 Prompt for AI Agents
plugins/must-gather/commands/analyze.md around lines 81 to 93: the fenced code
block containing the script discovery snippet is missing the language
identifier; update the opening fence to use "```bash" (matching ovn-dbs.md) so
the block begins with ```bash and leave the rest unchanged.
| python3 "$SCRIPTS_DIR/<script>.py" <must-gather-path> | ||
| ``` | ||
|
|
||
| Example: | ||
| ```bash | ||
| python3 plugins/must-gather/skills/must-gather-analyzer/scripts/<script>.py <must-gather-path> | ||
| python3 "$SCRIPTS_DIR/analyze_clusteroperators.py" ./must-gather.local.123/quay-io-... | ||
| ``` |
There was a problem hiding this comment.
Add bash language specification to example code block.
The execution example is missing a language identifier, consistent with the issue at lines 81–93.
Apply this diff:
3. **Locate Plugin Scripts**:
- Use the script availability check from the Error Handling section to find the plugin root
- Store the scripts directory path in `$SCRIPTS_DIR`
4. **Execute Analysis Scripts**:
- ```
+ ```bash
python3 "$SCRIPTS_DIR/<script>.py" <must-gather-path>
```
Example:
```bash
python3 "$SCRIPTS_DIR/analyze_clusteroperators.py" ./must-gather.local.123/quay-io-...
```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
148-148: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In plugins/must-gather/commands/analyze.md around lines 145 to 151, the fenced
code blocks for the execution example lack a language identifier; update the
opening fences to use "```bash" for the execution snippet and the Example
snippet (i.e., change the opening "```" to "```bash" for both blocks) and leave
the contained commands unchanged.
|
/cc @Prashanth684 |
is this with the plugin locally installed or from the marketplace? |
| ``` | ||
| 1. Locate the scripts directory by searching for a known script: | ||
| ```bash | ||
| SCRIPT_PATH=$(find ~ -name "analyze_clusteroperators.py" -path "*/must-gather/skills/must-gather-analyzer/scripts/*" 2>/dev/null | head -1) |
There was a problem hiding this comment.
is there a better way to do this that just grep for a script with the same name? i thought for a local plugin you could specify the github repo in your filesystem
There was a problem hiding this comment.
I feel this approach is more robust since it doesn't depend on from which directory we are starting the Claude CLI. I am open to suggestions as well!
I have faced this issue in both the cases, while installing locally or from the Marketplace as a git repo |
3 participants
What this PR does / why we need it:
This PR updated the must-gather command to dynamically discover script locations using
findand absolute paths.Which issue(s) this PR fixes:
/must-gather:analyzeand/must-gather:ovn-dbsfailed when run outside the ai-helpers repo directory because they used relative paths to find analysis scripts.Summary by CodeRabbit
Documentation