Archive the must-gather collected#605
Merged
dbasunag merged 2 commits intoopendatahub-io:mainfrom Sep 12, 2025
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe must-gather collection was refactored: image resolution now reads relatedImages from the installed CSV; collection output is archived into a ZIP and moved to the target directory. The API changed to accept Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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. Comment |
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/lgtm', '/wip', '/cherry-pick', '/hold', '/verified'} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
conftest.py (1)
450-456: Make archive name collision-proof and ensure target dir exists.If DB lookup fails, multiple failures across tests could all use mg-0. Also, rely on oc to create dest dir is brittle. Pre-create the directory and make the archive name unique.
- collect_rhoai_must_gather( - base_file_name=f"mg-{test_start_time}", - since=calculate_must_gather_timer(test_start_time=test_start_time), - target_dir=os.path.join(get_must_gather_collector_dir(), "pytest_exception_interact"), - ) + target_dir = os.path.join(get_must_gather_collector_dir(), "pytest_exception_interact") + os.makedirs(target_dir, exist_ok=True) + base_file_name = f"mg-{test_start_time}-{shortuuid.uuid()[:8]}" + collect_rhoai_must_gather( + base_file_name=base_file_name, + since=calculate_must_gather_timer(test_start_time=test_start_time), + target_dir=target_dir, + )utilities/must_gather_collector.py (3)
152-156: Sync docstring with new API (removed architecture, no return value).Current docstring mentions architecture and a string return that no longer exist.
def collect_rhoai_must_gather( base_file_name: str, target_dir: str, since: int, save_collection_output: bool = True, ) -> None: @@ - Args: - base_file_name: (str): Base file name for must-gather compressed file + Args: + base_file_name (str): Base file name (without extension) for the archive target_dir (str): Directory to store the must-gather output since (int): Time in seconds to collect logs from save_collection_output (bool, optional): Whether to save must-gather command output. Defaults to True. - architecture (str, optional): Target architecture for must-gather image. Defaults to "linux/amd64". - - Returns: - str: Path to the must-gather output directory, or empty string if collection is skipped + Returns: + NoneAlso applies to: 161-169
188-188: Clarify error message.Minor grammar and clarity tweak.
- LOGGER.error("No must-gather image is found from the csv. Must-gather collection would be skipped.") + LOGGER.error("No must-gather image found in the installed CSV; skipping must-gather collection.")
110-113: Quote component_name to avoid shell injection if ever user-sourced.Low risk today, but quoting is cheap and safer.
- if component_name: - must_gather_command += f" -- 'export COMPONENT={component_name}; /usr/bin/gather' " + if component_name: + must_gather_command += f" -- 'export COMPONENT={shlex.quote(component_name)}; /usr/bin/gather' "
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
conftest.py(1 hunks)utilities/infra.py(0 hunks)utilities/must_gather_collector.py(3 hunks)
💤 Files with no reviewable changes (1)
- utilities/infra.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-13T17:27:26.289Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#240
File: conftest.py:328-335
Timestamp: 2025-05-13T17:27:26.289Z
Learning: In the opendatahub-tests codebase, py_config["must_gather_collector"]["must_gather_base_directory"] is never empty as it's set during pytest_sessionstart() by combining get_base_dir() with BASE_DIRECTORY_NAME="must-gather-collected", making it safe to use in directory operations.
Applied to files:
conftest.pyutilities/must_gather_collector.py
📚 Learning: 2025-05-13T17:27:26.289Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#240
File: conftest.py:328-335
Timestamp: 2025-05-13T17:27:26.289Z
Learning: In the opendatahub-tests codebase, py_config["must_gather_collector"]["must_gather_base_directory"] is never empty as it's set during pytest_sessionstart() by combining get_base_dir() with BASE_DIRECTORY_NAME, making it safe to use in directory operations.
Applied to files:
conftest.pyutilities/must_gather_collector.py
🧬 Code graph analysis (1)
utilities/must_gather_collector.py (1)
utilities/infra.py (1)
get_rhods_operator_installed_csv(1016-1022)
fege
reviewed
Sep 11, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
utilities/must_gather_collector.py (2)
136-148: Harden CSV parsing and fix misleading/typo log.Avoid AttributeError when relatedImages is missing; log when CSV is absent; fix “RHAOI” → “RHOAI”; safely read image keys.
def get_must_gather_image_info() -> str: - csv_object = get_rhods_operator_installed_csv() - if not csv_object: - return "" - must_gather_image = [ - image["image"] for image in csv_object.instance.spec.relatedImages if "odh-must-gather" in image["image"] - ] - if not must_gather_image: - LOGGER.warning( - "No RHAOI CSV found. Potentially ODH cluster and must-gather collection is not relevant for this cluster" - ) - return "" - return must_gather_image[0] + csv_object = get_rhods_operator_installed_csv() + if not csv_object: + LOGGER.warning("No installed RHOAI/ODH CSV found; skipping must-gather image resolution.") + return "" + try: + related_images = getattr(csv_object.instance.spec, "relatedImages", []) or [] + except Exception: + related_images = [] + images = [img.get("image", "") for img in related_images if "odh-must-gather" in img.get("image", "")] + if not images: + LOGGER.warning("No odh-must-gather image found in installed CSV relatedImages; skipping collection.") + return "" + return images[0]
169-186: Write archive directly into target_dir; make toggle case-insensitive; use root_dir to avoid path issues.Pre-create target_dir, emit archive in-place, remove extra copy/unlink, and accept common truthy values.
- must_gather_image = get_must_gather_image_info() + must_gather_image = get_must_gather_image_info() if must_gather_image: - output = run_must_gather(image_url=must_gather_image, target_dir=target_dir, since=f"{since}s") + os.makedirs(target_dir, exist_ok=True) + output = run_must_gather(image_url=must_gather_image, target_dir=target_dir, since=f"{since}s") if save_collection_output: with open(os.path.join(target_dir, "output.log"), "w") as _file: _file.write(output) - # get must gather directory to archive - path = get_must_gather_output_dir(must_gather_path=target_dir) - if os.getenv("ARCHIVE_MUST_GATHER", "true") == "true": - # archive the folder and get the zip file's name - file_name = shutil.make_archive(base_name=base_file_name, format="zip", base_dir=path) - # remove the folder that was archived - shutil.rmtree(path=path, ignore_errors=True) - # copy back the archived file to the same path - dest_file = os.path.join(target_dir, file_name) - shutil.copy(src=file_name, dst=dest_file) - LOGGER.info(f"{dest_file} is collected successfully") - os.unlink(file_name) + path = get_must_gather_output_dir(must_gather_path=target_dir) + archive_enabled = os.getenv("ARCHIVE_MUST_GATHER", "true").strip().lower() in {"1", "true", "yes", "y"} + if archive_enabled: + archive_base = os.path.join(target_dir, base_file_name) + archive_path = shutil.make_archive(base_name=archive_base, format="zip", root_dir=path) + shutil.rmtree(path=path, ignore_errors=True) + LOGGER.info(f"{archive_path} is collected successfully")
🧹 Nitpick comments (3)
docs/GETTING_STARTED.md (1)
44-58: Clarify toggle semantics and hyphenate “must-gather”.
- Make env-var behavior explicit (case-insensitive truthy/falsey) to match intended UX.
- Use consistent “must-gather” spelling.
-## Must gather +## Must-gather @@ -By default, the collected must-gather would be archived. To skip archiving, please set environment variable -ARCHIVE_MUST_GATHER to any value other than "true". e.g. +By default, the collected must-gather is archived. To skip archiving, set the +ARCHIVE_MUST_GATHER environment variable to a falsey value (e.g., "false", "0", "no"; case-insensitive). For example: @@ -export ARCHIVE_MUST_GATHER="false" +export ARCHIVE_MUST_GATHER=falseconftest.py (1)
452-455: Make archive name collision-proof when test_start_time is 0 or reused.Add a short unique suffix (and include phase) to avoid overwriting across retries/multiple failures.
- base_file_name=f"mg-{test_start_time}", + base_file_name=f"mg-{test_start_time}-{report.when}-{shortuuid.uuid()[:6]}",utilities/must_gather_collector.py (1)
166-168: Docstring return type is stale (function returns None).Update Returns section to avoid confusion.
- Returns: - str: Path to the must-gather output directory, or empty string if collection is skipped + Returns: + None
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
conftest.py(1 hunks)docs/GETTING_STARTED.md(1 hunks)utilities/infra.py(0 hunks)utilities/must_gather_collector.py(2 hunks)
💤 Files with no reviewable changes (1)
- utilities/infra.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-13T17:27:26.289Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#240
File: conftest.py:328-335
Timestamp: 2025-05-13T17:27:26.289Z
Learning: In the opendatahub-tests codebase, py_config["must_gather_collector"]["must_gather_base_directory"] is never empty as it's set during pytest_sessionstart() by combining get_base_dir() with BASE_DIRECTORY_NAME="must-gather-collected", making it safe to use in directory operations.
Applied to files:
conftest.pyutilities/must_gather_collector.py
📚 Learning: 2025-05-13T17:27:26.289Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#240
File: conftest.py:328-335
Timestamp: 2025-05-13T17:27:26.289Z
Learning: In the opendatahub-tests codebase, py_config["must_gather_collector"]["must_gather_base_directory"] is never empty as it's set during pytest_sessionstart() by combining get_base_dir() with BASE_DIRECTORY_NAME, making it safe to use in directory operations.
Applied to files:
conftest.pyutilities/must_gather_collector.py
🧬 Code graph analysis (1)
utilities/must_gather_collector.py (1)
utilities/infra.py (1)
get_rhods_operator_installed_csv(1016-1022)
Collaborator
Author
|
/build-push-pr-image |
|
Status of building tag pr-605: success. |
|
Status of building tag latest: success. |
Collaborator
Author
|
/cherry-pcik 2.24 |
Collaborator
Author
|
/cherry-pick 2.24 |
rhods-ci-bot
pushed a commit
that referenced
this pull request
Sep 12, 2025
Contributor
|
Cherry pick action created PR #607 successfully 🎉! |
dbasunag
added a commit
that referenced
this pull request
Sep 12, 2025
mwaykole
pushed a commit
to mwaykole/opendatahub-tests
that referenced
this pull request
Jan 23, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
New Features
Refactor
Documentation