Add must-gather command for diagnostic collection #115
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@CodeRabbit hi |
|
Tip For best results, initiate chat on the files or code changes.
How can I help you with PR |
Member
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a new top-level Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as "oadp CLI"
participant Validator as "Local Validator"
participant Oc as "oc adm must-gather"
User->>CLI: oadp must-gather --dest-dir ... --request-timeout ... [--skip-tls]
CLI->>CLI: Complete (resolve image, normalize dest)
CLI->>Validator: Validate (oc available, dest absolute)
Validator-->>CLI: ok / error
alt valid
CLI->>Oc: Execute built must-gather command (image, timeout, tls, gather-script)
Oc-->>CLI: Stream stdout/stderr
CLI-->>User: Display streamed output and exit status
else invalid
CLI-->>User: Return formatted validation error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
|
Note Docstrings generation - SUCCESS |
![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
🤖 Fix all issues with AI agents
In `@cmd/must-gather/must_gather.go`:
- Around line 128-140: The exec.ErrNotFound comparison in
MustGatherOptions.formatError is too strict and will miss wrapped errors; update
formatError to use errors.Is(err, exec.ErrNotFound) when checking for a missing
'oc' binary (and import "errors" if not present) so wrapped *exec.Error or
*os.PathError cases are handled; keep the existing exec.ExitError handling
(exitErr.ExitCode()) and the fallback fmt.Errorf("must-gather failed: %w", err).
🧹 Nitpick comments (2)
cmd/must-gather/must_gather.go (1)
55-61: Hardcoded image version may require maintenance.The default image
registry.redhat.io/oadp/oadp-mustgather-rhel9:v1.5is pinned to a specific version. As noted in the TODO comment, consider implementing version detection in the future to automatically select the appropriate image based on the installed OADP version.For now, this approach is acceptable for the initial implementation.
cmd/must-gather/must_gather_test.go (1)
27-39: Consider consolidating tests to reduce binary rebuild overhead.Each test function calls
testutil.BuildCLIBinary(t), which rebuilds the entire CLI binary. For faster test execution, consider consolidating these into a single test with subtests that share the built binary.♻️ Example consolidation pattern
func TestMustGatherHelp(t *testing.T) { binaryPath := testutil.BuildCLIBinary(t) t.Run("basic help", func(t *testing.T) { expectContains := []string{ "Collect diagnostic information", "--dest-dir", "--request-timeout", "--skip-tls", "Examples:", } testutil.TestHelpCommand(t, binaryPath, []string{"must-gather", "--help"}, expectContains) }) t.Run("help flags", func(t *testing.T) { for _, flag := range []string{"--help", "-h"} { t.Run(flag, func(t *testing.T) { output, err := testutil.RunCommand(t, binaryPath, "must-gather", flag) // ... existing assertions }) } }) // ... other subtests }
coderabbitai bot
added a commit
that referenced
this pull request
Feb 9, 2026
Docstrings generation was requested by @Joeavaikath. * #115 (comment) The following files were modified: * `cmd/must-gather/must_gather.go` * `cmd/root.go`
Contributor
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
Author
|
@coderabbitai can this be approved? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6663 🏁 Script executed: Length of output: 1409
⏳ CI Checks Still RunningSeveral CI checks are still in progress:
The following checks have passed successfully:
📋 Review StatusThe PR currently has a review decision of REVIEW_REQUIRED. I can see that you've addressed the error handling issue I previously flagged with your latest commit "Fix error handling in must-gather command", which is great! 🔍 RecommendationBefore approval, I recommend:
Once all CI checks pass and a maintainer has reviewed the implementation, the PR should be ready for approval. Would you like me to check back once the CI checks complete? |
Contributor
Author
|
@coderabbitai approve-able now? |
🧩 Analysis chain🏁 Script executed: Length of output: 10278
✅ All GreenCI Status: All checks have passed successfully
Code Quality:
Status: The PR is not in draft and is ready for merge. The implementation looks solid. The only remaining comments were minor nitpicks (hardcoded image version with a TODO, and test optimization suggestion), neither of which are blockers for this initial implementation. Approving now! 🚀 ✅ Actions performedComments resolved. |
kaovilai
reviewed
Feb 10, 2026
Comment on lines
+58
to
+59
| // TODO: Future enhancement - detect version and map to image | ||
| // o.effectiveImage = o.getDefaultImage() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the TODO follow-up
gh issue create \
--repo migtools/oadp-cli \
--title "Implement version detection for must-gather image selection" \
--body "## Description
Currently, the must-gather command uses a hardcoded default image when the \`--image\` flag is not specified:
\`\`\`go
o.effectiveImage = \"registry.redhat.io/oadp/oadp-mustgather-rhel9:v1.5\"
\`\`\`
As noted in the TODO comment in \`cmd/must-gather/must_gather.go\`, a future enhancement should automatically detect the installed OADP version and map it to the appropriate must-gather image.
## Proposed Enhancement
- Detect the OADP operator version installed in the cluster
- Maintain a mapping of OADP versions to their corresponding must-gather images
- Automatically select the correct must-gather image based on the detected version
- Fall back to a reasonable default if version detection fails
## References
- PR: https://github.com/migtools/oadp-cli/pull/115
- Comment: https://github.com/migtools/oadp-cli/pull/115#discussion_r2788814569
- Requested by: `@kaovilai`" \
--assignee kaovilai
Length of output: 47
@kaovilai I've created issue #117 to track the enhancement for automatic OADP version detection and must-gather image mapping. The issue has been assigned to you and includes references back to this PR and comment.
kaovilai
approved these changes
Feb 10, 2026
mpryc
approved these changes
Feb 10, 2026
Joeavaikath
force-pushed
the
add-must-gather
branch
from
1852e7a to
8770ccb
Compare
Adds a new must-gather subcommand to collect diagnostic information from OADP installations. Also updates Makefile references from kubectl oadp to oc oadp and adds /must-gather/ to .gitignore to exclude generated artifacts. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Joseph <jvaikath@redhat.com>
Use errors.Is() instead of direct comparison for exec.ErrNotFound to properly handle wrapped errors from exec.Command failures. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Joseph <jvaikath@redhat.com>
Joeavaikath
force-pushed
the
add-must-gather
branch
from
8770ccb to
dec4652
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why the changes were made
Adds a new must-gather subcommand to collect diagnostic information from OADP installations. Also updates Makefile references from kubectl oadp to oc oadp and adds /must-gather/ to .gitignore to exclude generated artifacts.
How to test the changes made
Run
oc oadp must-gatherRun
oc oadp must-gather --helpto see the available options
Summary by CodeRabbit
New Features
Documentation
Tests
Chores