refactor(e2e): migrate to external oadp-must-gather container image #2007
Conversation
WalkthroughRemoves the local must-gather implementation and build artifacts, deletes must-gather docs/scripts, and updates E2E tests and Makefile to run must-gather via a configurable container image (MUST_GATHER_IMAGE) with glob-based summary discovery. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Assessment against linked issues
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/lib/apps.go (1)
399-410: Add bounds checking before slicing ClusterID.Line 409 will panic if
clusterVersion.Spec.ClusterIDis shorter than 8 characters. While cluster IDs are typically UUIDs (36 characters), defensive programming requires bounds checking.Apply this diff to add bounds checking:
clusterVersion := &clusterVersionList.Items[0] - clusterID := string(clusterVersion.Spec.ClusterID[:8]) + clusterIDStr := string(clusterVersion.Spec.ClusterID) + if len(clusterIDStr) < 8 { + return fmt.Errorf("cluster ID is too short: %s", clusterIDStr) + } + clusterID := clusterIDStr[:8]
🧹 Nitpick comments (1)
tests/e2e/lib/apps.go (1)
422-432: Consider logging which summary file is being read.When multiple matches exist (e.g., from previous test runs), line 423 silently uses the first match. Adding a log statement would help with debugging.
// Read and validate the summary + log.Printf("Reading must-gather summary from: %s", matches[0]) mustGatherSummaryContent, err := os.ReadFile(matches[0])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
must-gather/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
Makefile(1 hunks)docs/developer/testing/TESTING.md(1 hunks)must-gather/Dockerfile(0 hunks)must-gather/README.md(0 hunks)must-gather/cmd/main.go(0 hunks)must-gather/deprecated/gather_1h(0 hunks)must-gather/deprecated/gather_1h_essential(0 hunks)must-gather/deprecated/gather_24h(0 hunks)must-gather/deprecated/gather_24h_essential(0 hunks)must-gather/deprecated/gather_6h(0 hunks)must-gather/deprecated/gather_6h_essential(0 hunks)must-gather/deprecated/gather_72h(0 hunks)must-gather/deprecated/gather_72h_essential(0 hunks)must-gather/deprecated/gather_all(0 hunks)must-gather/deprecated/gather_all_essential(0 hunks)must-gather/deprecated/gather_metrics_dump(0 hunks)must-gather/deprecated/gather_with_timeout(0 hunks)must-gather/deprecated/gather_without_tls(0 hunks)must-gather/go.mod(0 hunks)must-gather/pkg/cli.go(0 hunks)must-gather/pkg/gather/gather.go(0 hunks)must-gather/pkg/gvk/gvk.go(0 hunks)must-gather/pkg/templates/summary.go(0 hunks)tests/e2e/lib/apps.go(2 hunks)
💤 Files with no reviewable changes (21)
- must-gather/deprecated/gather_metrics_dump
- must-gather/deprecated/gather_1h
- must-gather/deprecated/gather_72h
- must-gather/go.mod
- must-gather/deprecated/gather_72h_essential
- must-gather/deprecated/gather_24h_essential
- must-gather/deprecated/gather_all_essential
- must-gather/deprecated/gather_with_timeout
- must-gather/pkg/gather/gather.go
- must-gather/cmd/main.go
- must-gather/README.md
- must-gather/deprecated/gather_without_tls
- must-gather/deprecated/gather_6h
- must-gather/deprecated/gather_1h_essential
- must-gather/pkg/cli.go
- must-gather/deprecated/gather_all
- must-gather/deprecated/gather_6h_essential
- must-gather/Dockerfile
- must-gather/pkg/gvk/gvk.go
- must-gather/pkg/templates/summary.go
- must-gather/deprecated/gather_24h
🔇 Additional comments (4)
Makefile (1)
792-792: LGTM! Dependency simplified correctly.The removal of the
build-must-gatherdependency aligns with the PR objective to use an external container image instead of building locally.docs/developer/testing/TESTING.md (1)
28-29: LGTM! Documentation clearly describes the new environment variables.The documentation accurately reflects the new must-gather behavior with environment-driven image selection.
tests/e2e/lib/apps.go (2)
381-387: LGTM! Environment variable handling is correct.The implementation properly reads the
MUST_GATHER_IMAGEenvironment variable with a sensible default value, and includes helpful comments about version-specific usage.
389-397: LGTM! Command execution and error handling look good.The
oc adm must-gathercommand is constructed correctly with appropriate flags, and errors are captured with output for debugging.
| // Find the must-gather output directory | ||
| // oc adm must-gather creates: <artifact_dir>/quay-io-konveyor-oadp-must-gather-*/clusters/<cluster-id>/ | ||
| pattern := filepath.Join(artifact_dir, "quay-io-konveyor-oadp-must-gather-*", "clusters", clusterID, "oadp-must-gather-summary.md") | ||
| matches, err := filepath.Glob(pattern) | ||
| if err != nil { | ||
| return fmt.Errorf("error finding must-gather summary: %w", err) | ||
| } | ||
| if len(matches) == 0 { | ||
| return fmt.Errorf("no must-gather summary found at pattern: %s", pattern) | ||
| } |
There was a problem hiding this comment.
Hardcoded directory pattern breaks custom image support.
The glob pattern on line 413 assumes the directory name will always be quay-io-konveyor-oadp-must-gather-*, but this won't work when users set MUST_GATHER_IMAGE to a different registry or repository (e.g., myregistry.com/my-must-gather:latest). The directory name created by oc adm must-gather is derived from the image URL, so the pattern must be dynamic.
Consider deriving the pattern from the actual image name or using a more flexible glob:
- // Find the must-gather output directory
- // oc adm must-gather creates: <artifact_dir>/quay-io-konveyor-oadp-must-gather-*/clusters/<cluster-id>/
- pattern := filepath.Join(artifact_dir, "quay-io-konveyor-oadp-must-gather-*", "clusters", clusterID, "oadp-must-gather-summary.md")
+ // Find the must-gather output directory
+ // oc adm must-gather creates: <artifact_dir>/<image-based-dir>/clusters/<cluster-id>/
+ // Use a wildcard pattern to match any must-gather output directory
+ pattern := filepath.Join(artifact_dir, "*", "clusters", clusterID, "oadp-must-gather-summary.md")Additionally, consider logging what was found when the pattern fails to match:
matches, err := filepath.Glob(pattern)
if err != nil {
return fmt.Errorf("error finding must-gather summary: %w", err)
}
if len(matches) == 0 {
- return fmt.Errorf("no must-gather summary found at pattern: %s", pattern)
+ // List what directories exist to help debug
+ dirs, _ := filepath.Glob(filepath.Join(artifact_dir, "*"))
+ return fmt.Errorf("no must-gather summary found at pattern: %s\nDirectories in artifact_dir: %v", pattern, dirs)
}📝 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.
| // Find the must-gather output directory | |
| // oc adm must-gather creates: <artifact_dir>/quay-io-konveyor-oadp-must-gather-*/clusters/<cluster-id>/ | |
| pattern := filepath.Join(artifact_dir, "quay-io-konveyor-oadp-must-gather-*", "clusters", clusterID, "oadp-must-gather-summary.md") | |
| matches, err := filepath.Glob(pattern) | |
| if err != nil { | |
| return fmt.Errorf("error finding must-gather summary: %w", err) | |
| } | |
| if len(matches) == 0 { | |
| return fmt.Errorf("no must-gather summary found at pattern: %s", pattern) | |
| } | |
| // Find the must-gather output directory | |
| // oc adm must-gather creates: <artifact_dir>/<image-based-dir>/clusters/<cluster-id>/ | |
| // Use a wildcard pattern to match any must-gather output directory | |
| pattern := filepath.Join(artifact_dir, "*", "clusters", clusterID, "oadp-must-gather-summary.md") | |
| matches, err := filepath.Glob(pattern) | |
| if err != nil { | |
| return fmt.Errorf("error finding must-gather summary: %w", err) | |
| } | |
| if len(matches) == 0 { | |
| // List what directories exist to help debug | |
| dirs, _ := filepath.Glob(filepath.Join(artifact_dir, "*")) | |
| return fmt.Errorf("no must-gather summary found at pattern: %s\nDirectories in artifact_dir: %v", pattern, dirs) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/lib/apps.go (1)
411-425: Consider using a wildcard pattern for better robustness.The current pattern derivation logic (lines 415-418) attempts to mirror how
oc adm must-gathernames directories, but this could fail if the actual naming logic differs or if edge cases exist (e.g., registries with multiple dots, special characters in tags). The past review suggested using a simpler wildcard pattern, which would be more resilient.Apply this diff to use a wildcard pattern and add debugging information:
// Find the must-gather output directory - // oc adm must-gather creates a directory based on the image name with registry separators - // replaced by hyphens. E.g., quay.io/konveyor/oadp-must-gather:latest -> quay-io-konveyor-oadp-must-gather-* - // We need to derive the pattern from the actual image being used - imagePattern := strings.ReplaceAll(mustGatherImage, ":", "-") - imagePattern = strings.ReplaceAll(imagePattern, "/", "-") - imagePattern = strings.ReplaceAll(imagePattern, ".", "-") - pattern := filepath.Join(artifact_dir, imagePattern+"-*", "clusters", clusterID, "oadp-must-gather-summary.md") + // oc adm must-gather creates: <artifact_dir>/<image-based-dir>/clusters/<cluster-id>/ + // Use a wildcard pattern to match any must-gather output directory + pattern := filepath.Join(artifact_dir, "*", "clusters", clusterID, "oadp-must-gather-summary.md") matches, err := filepath.Glob(pattern) if err != nil { return fmt.Errorf("error finding must-gather summary: %w", err) } if len(matches) == 0 { - return fmt.Errorf("no must-gather summary found at pattern: %s", pattern) + // List what directories exist to help debug + dirs, _ := filepath.Glob(filepath.Join(artifact_dir, "*")) + return fmt.Errorf("no must-gather summary found at pattern: %s\nDirectories in artifact_dir: %v", pattern, dirs) }This approach is simpler, more robust, and provides better debugging information when the pattern fails to match.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
tests/e2e/lib/apps.go(2 hunks)
🔇 Additional comments (3)
tests/e2e/lib/apps.go (3)
381-387: LGTM! Clean environment variable handling.The environment variable handling is well-implemented with a sensible default and helpful inline documentation for users who need version-specific images.
389-397: LGTM! Proper command execution and error handling.The command execution correctly uses
CombinedOutputand wraps errors with the command output for better debugging.
427-441: LGTM! Validation logic is sound.The summary file reading and validation correctly checks for the expected success message. The TODO comment appropriately flags future enhancement opportunities.
| } | ||
| clusterVersion := &clusterVersionList.Items[0] | ||
| mustGatherPath := fmt.Sprintf("%s/must-gather/clusters/%s/", artifact_dir, string(clusterVersion.Spec.ClusterID[:8])) | ||
| clusterID := string(clusterVersion.Spec.ClusterID[:8]) |
There was a problem hiding this comment.
Potential panic: validate ClusterID length before slicing.
The slice operation [:8] assumes the ClusterID is at least 8 characters long. If it's shorter, this will panic with a runtime error.
Apply this diff to safely handle shorter ClusterIDs:
- clusterID := string(clusterVersion.Spec.ClusterID[:8])
+ clusterID := string(clusterVersion.Spec.ClusterID)
+ if len(clusterID) > 8 {
+ clusterID = clusterID[:8]
+ }📝 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.
| clusterID := string(clusterVersion.Spec.ClusterID[:8]) | |
| clusterID := string(clusterVersion.Spec.ClusterID) | |
| if len(clusterID) > 8 { | |
| clusterID = clusterID[:8] | |
| } |
🤖 Prompt for AI Agents
In tests/e2e/lib/apps.go around line 409, the code unconditionally slices
clusterVersion.Spec.ClusterID[:8] which can panic if ClusterID is shorter than 8
characters; validate the length first and only slice up to min(8,
len(ClusterID)). If ClusterID is empty or nil handle that case (e.g., use the
whole string, a safe default, or return an error) and ensure you do not index
past the string length before converting to string.
|
/retest ai-retester: The OADP E2E test failed because the "Mongo application CSI" test timed out while waiting for the The The |
Good, https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_oadp-operator/2007/pull-ci-openshift-oadp-operator-oadp-dev-4.19-e2e-test-aws/1985812568559587328/artifacts/e2e-test-aws/e2e/artifacts/Mongo%20application%20CSI/ still populated, which means this PR works. |
openshift-merge-robot
added
the
needs-rebase
Remove local must-gather directory and build process in favor of using the external quay.io/konveyor/oadp-must-gather:latest image via oc adm must-gather. This eliminates architecture mismatch issues and keeps must-gather code in its dedicated repository. Changes: - Updated RunMustGather() in tests/e2e/lib/apps.go to use oc adm must-gather - Added MUST_GATHER_IMAGE env var (defaults to quay.io/konveyor/oadp-must-gather:latest) - Removed build-must-gather target from Makefile - Removed entire must-gather/ directory (3,174 lines deleted) - Updated documentation in TESTING.md The SKIP_MUST_GATHER flag is preserved for skipping must-gather collection. Version-specific images can be used by setting MUST_GATHER_IMAGE env var. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The directory pattern was hardcoded to 'quay-io-konveyor-oadp-must-gather-*' which breaks when using custom images via MUST_GATHER_IMAGE env var. Now dynamically derives the pattern from the actual image name by replacing registry separators (. / :) with hyphens to match oc adm must-gather's directory naming convention. Examples: - quay.io/konveyor/oadp-must-gather:latest -> quay-io-konveyor-oadp-must-gather-latest-* - docker.io/myuser/custom:v1 -> docker-io-myuser-custom-v1-* 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
kaovilai
force-pushed
the
no-mor-mustgather
branch
from
ac8bcc6 to
2ae6d45
Compare
|
New changes are detected. LGTM label has been removed. |
openshift-merge-robot
removed
the
needs-rebase
|
updated after rebase |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/lib/apps.go (1)
409-409: Potential panic: validate ClusterID length before slicing.This is the same critical issue flagged in the previous review. The slice operation
[:8]will panic at runtime ifClusterIDis shorter than 8 characters.Apply this diff to safely handle shorter ClusterIDs:
- clusterID := string(clusterVersion.Spec.ClusterID[:8]) + clusterID := string(clusterVersion.Spec.ClusterID) + if len(clusterID) > 8 { + clusterID = clusterID[:8] + }
🧹 Nitpick comments (1)
tests/e2e/lib/apps.go (1)
423-425: Enhance error message with available directories for debugging.When the pattern fails to match, listing the actual directories in
artifact_dirwould help diagnose whether the issue is pattern mismatch or missing must-gather output.Apply this diff:
if len(matches) == 0 { - return fmt.Errorf("no must-gather summary found at pattern: %s", pattern) + // List what directories exist to help debug + dirs, _ := filepath.Glob(filepath.Join(artifact_dir, "*")) + return fmt.Errorf("no must-gather summary found at pattern: %s\nDirectories in artifact_dir: %v", pattern, dirs) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
must-gather/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
Makefile(1 hunks)docs/developer/testing/TESTING.md(1 hunks)must-gather/Dockerfile(0 hunks)must-gather/README.md(0 hunks)must-gather/cmd/main.go(0 hunks)must-gather/deprecated/gather_1h(0 hunks)must-gather/deprecated/gather_1h_essential(0 hunks)must-gather/deprecated/gather_24h(0 hunks)must-gather/deprecated/gather_24h_essential(0 hunks)must-gather/deprecated/gather_6h(0 hunks)must-gather/deprecated/gather_6h_essential(0 hunks)must-gather/deprecated/gather_72h(0 hunks)must-gather/deprecated/gather_72h_essential(0 hunks)must-gather/deprecated/gather_all(0 hunks)must-gather/deprecated/gather_all_essential(0 hunks)must-gather/deprecated/gather_metrics_dump(0 hunks)must-gather/deprecated/gather_with_timeout(0 hunks)must-gather/deprecated/gather_without_tls(0 hunks)must-gather/go.mod(0 hunks)must-gather/pkg/cli.go(0 hunks)must-gather/pkg/gather/gather.go(0 hunks)must-gather/pkg/gvk/gvk.go(0 hunks)must-gather/pkg/templates/summary.go(0 hunks)tests/e2e/lib/apps.go(2 hunks)
💤 Files with no reviewable changes (21)
- must-gather/deprecated/gather_all_essential
- must-gather/deprecated/gather_metrics_dump
- must-gather/Dockerfile
- must-gather/deprecated/gather_72h
- must-gather/deprecated/gather_24h_essential
- must-gather/deprecated/gather_1h
- must-gather/deprecated/gather_6h
- must-gather/deprecated/gather_1h_essential
- must-gather/go.mod
- must-gather/deprecated/gather_all
- must-gather/pkg/cli.go
- must-gather/deprecated/gather_24h
- must-gather/deprecated/gather_without_tls
- must-gather/deprecated/gather_with_timeout
- must-gather/cmd/main.go
- must-gather/pkg/gather/gather.go
- must-gather/README.md
- must-gather/pkg/gvk/gvk.go
- must-gather/deprecated/gather_72h_essential
- must-gather/deprecated/gather_6h_essential
- must-gather/pkg/templates/summary.go
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/developer/testing/TESTING.md
- Makefile
🔇 Additional comments (1)
tests/e2e/lib/apps.go (1)
381-397: LGTM: Environment-driven must-gather execution.The environment variable setup and command execution are well-implemented:
- Sensible default image with clear upgrade path via
MUST_GATHER_IMAGE- Proper use of
oc adm must-gatherwith explicit flags- Comprehensive error handling with command output included
| imagePattern := strings.ReplaceAll(mustGatherImage, ":", "-") | ||
| imagePattern = strings.ReplaceAll(imagePattern, "/", "-") | ||
| imagePattern = strings.ReplaceAll(imagePattern, ".", "-") | ||
| pattern := filepath.Join(artifact_dir, imagePattern+"-*", "clusters", clusterID, "oadp-must-gather-summary.md") |
There was a problem hiding this comment.
Simplify pattern to avoid brittle image-name transformation.
The current approach attempts to replicate oc adm must-gather's directory-naming logic by replacing characters in the image name. This introduces several risks:
- The transformation may not match
oc's actual behavior (e.g.,ocmight handle edge cases like consecutive dots or special characters differently) - The order of replacements could affect the result (e.g., replacing
.before:might interact unexpectedly) - Future changes to
oc adm must-gathercould break this logic
Consider using a simpler wildcard pattern that doesn't depend on replicating oc's transformation:
- // Find the must-gather output directory
- // oc adm must-gather creates a directory based on the image name with registry separators
- // replaced by hyphens. E.g., quay.io/konveyor/oadp-must-gather:latest -> quay-io-konveyor-oadp-must-gather-*
- // We need to derive the pattern from the actual image being used
- imagePattern := strings.ReplaceAll(mustGatherImage, ":", "-")
- imagePattern = strings.ReplaceAll(imagePattern, "/", "-")
- imagePattern = strings.ReplaceAll(imagePattern, ".", "-")
- pattern := filepath.Join(artifact_dir, imagePattern+"-*", "clusters", clusterID, "oadp-must-gather-summary.md")
+ // Find the must-gather output directory
+ // oc adm must-gather creates: <artifact_dir>/<image-based-dir>/clusters/<cluster-id>/
+ // Use a wildcard to match any must-gather output directory
+ pattern := filepath.Join(artifact_dir, "*", "clusters", clusterID, "oadp-must-gather-summary.md")This approach is more robust and doesn't require maintaining knowledge of oc's internal naming conventions.
📝 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.
| imagePattern := strings.ReplaceAll(mustGatherImage, ":", "-") | |
| imagePattern = strings.ReplaceAll(imagePattern, "/", "-") | |
| imagePattern = strings.ReplaceAll(imagePattern, ".", "-") | |
| pattern := filepath.Join(artifact_dir, imagePattern+"-*", "clusters", clusterID, "oadp-must-gather-summary.md") | |
| // Find the must-gather output directory | |
| // oc adm must-gather creates: <artifact_dir>/<image-based-dir>/clusters/<cluster-id>/ | |
| // Use a wildcard to match any must-gather output directory | |
| pattern := filepath.Join(artifact_dir, "*", "clusters", clusterID, "oadp-must-gather-summary.md") |
🤖 Prompt for AI Agents
In tests/e2e/lib/apps.go around lines 415 to 418, remove the brittle manual
image-name character replacements and instead use a simple wildcard segment when
building the path to the must-gather summary; replace the transformed
imagePattern/path construction with a pattern that uses a wildcard (e.g., a
single "*" or a "*must-gather*" segment) under artifact_dir so we match the
must-gather directory regardless of how oc transformed the image name, then join
that wildcard segment with "clusters", clusterID and
"oadp-must-gather-summary.md".
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how artifact_dir is used in tests and if multiple must-gather runs could occur
echo "=== Checking artifact_dir usage patterns in e2e tests ==="
rg -n "artifact_dir" --type=go -C2
echo -e "\n=== Checking for multiple RunMustGather calls or patterns ==="
rg -n "RunMustGather" --type=go -C3
echo -e "\n=== Checking if tests clean up artifact directories between runs ==="
rg -n "RemoveAll|MkdirAll.*artifact" --type=go -C2
echo -e "\n=== Looking for any existing glob patterns or directory discovery logic ==="
rg -n "filepath\.Glob|filepath\.Join.*\*" --type=go tests/e2e/
Length of output: 9417
@kaovilai Thanks for being open to discussion! I analyzed the test codebase to understand the context better.
Key findings:
artifact_diris shared across all test suites and defaults to/tmp- Multiple
RunMustGather()calls can accumulate in the same directory (success cases, failure cases, different test suites) oc adm must-gathercreates timestamped directories likequay-io-konveyor-oadp-must-gather-20250105123456- Your current code uses
matches[0](first glob match)
Trade-off analysis:
Current approach (image-based transformations):
- ✅ More specific to the actual image used
- ❌ Brittle: order-dependent replacements (
"."before":"matters) - ❌ Assumes knowledge of
oc's internal logic - ❌ Could break with future
occhanges
Simple wildcard (*):
- ✅ Robust and maintainable
- ❌ May match unrelated directories in artifact_dir
- ❌ With multiple must-gather runs, matches[0] gives first alphabetically, not newest
Recommendation - middle ground:
Use a more specific wildcard pattern with explicit sorting:
// Find the most recent must-gather output directory
// oc adm must-gather creates timestamped directories under artifact_dir
pattern := filepath.Join(artifact_dir, "*oadp-must-gather*", "clusters", clusterID, "oadp-must-gather-summary.md")
matches, err := filepath.Glob(pattern)
if err != nil {
return fmt.Errorf("error finding must-gather summary: %w", err)
}
if len(matches) == 0 {
return fmt.Errorf("no must-gather summary found at pattern: %s", pattern)
}
// Sort to get the most recent (timestamps sort lexicographically)
sort.Strings(matches)
summaryPath := matches[len(matches)-1]
mustGatherSummaryContent, err := os.ReadFile(summaryPath)This approach:
- Avoids replicating
oc's transformation logic - Still filters to OADP-specific directories
- Explicitly picks the most recent run when multiple exist
- Documents the behavior clearly
Alternative: If you want to keep the transformation approach for specificity, at least add a comment documenting the risk and consider validating the pattern works as expected (maybe log the matched path).
What do you think?
🧬 Code Graph Analysis Results
<no_relevant_code_snippets>
|
/retest ai-retester: The end-to-end (e2e) test Why the Mongo test blew up The entire CI run was green until we hit the Mongo application CSI test. The test had just finished installing the Mongo database and the In short, the todolist container could not start (most likely the image Short failure summary
So, the single failure occurs when the Mongo T‑odo‑list pod failed to start (likely a race or resource issue), and the must‑gather step also failed to locate its summary file. |
|
/retest |
1 similar comment
|
/retest |
|
ugh.. AI reviews :) If this not running the must-gather atm in ci? I'll look |
|
Feedback is to build from source of a must-gather repo clone/local-dir so that must-gather can be developed on and tested in e2e w/o having to push an image to registry. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
5 participants
Summary
Remove local must-gather directory and build process in favor of using the external
quay.io/konveyor/oadp-must-gather:latestimage viaoc adm must-gather. This eliminates architecture mismatch issues and keeps must-gather code in its dedicated repository at https://github.com/openshift/oadp-must-gather.Changes
RunMustGather()intests/e2e/lib/apps.goto useoc adm must-gathercommandMUST_GATHER_IMAGEenvironment variable (defaults toquay.io/konveyor/oadp-must-gather:latest)build-must-gathertarget from Makefilemust-gather/directory (3,174 lines deleted)docs/developer/testing/TESTING.mdBenefits
MUST_GATHER_IMAGEenv varSKIP_MUST_GATHERflag preserved for skipping collectionUsage
Default behavior (uses latest):
With custom image:
Skip must-gather:
Fixes #2005
Testing
Note
Responses generated with Claude