Fix tekton test image extraction to use Konflux mirrors#3984
Open
geetikakay wants to merge 1 commit intoRedHatQE:mainfrom
Open
Fix tekton test image extraction to use Konflux mirrors#3984geetikakay wants to merge 1 commit intoRedHatQE:mainfrom
geetikakay wants to merge 1 commit intoRedHatQE:mainfrom
Conversation
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Contributor
📝 WalkthroughWalkthroughTest fixture infrastructure refactored to decompose image reference resolution. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Reasoning: Single file with interconnected fixture logic changes. Requires verification that: (1) CSV annotation parsing correctly extracts image digest, (2) ImageDigestMirrorSet lookup properly resolves Konflux base path, (3) fixture composition produces valid image references, and (4) dependent fixture integration works as intended. Moderate logic density with fixture dependency chains demands careful validation but limited scope. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Contributor
Author
|
/verified |
Contributor
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Your deliverable: Test Execution Plan
Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL: Before setting "Run smoke tests: True", you MUST use shell scripts to: |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 46 🏁 Script executed: Length of output: 2947 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 281 🏁 Script executed: Length of output: 3062 🏁 Script executed: Length of output: 2658 🏁 Script executed: Length of output: 118 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 278 🏁 Script executed: Length of output: 1735 🧠 Learnings used |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/infrastructure/tekton/conftest.py`:
- Around line 119-123: Guard access to the CSV annotation before splitting:
instead of directly indexing
csv_scope_session.instance.metadata.annotations["test-images-nvrs"], fetch the
annotations dict and use .get("test-images-nvrs") (or check for presence) and
raise a clear ValueError if it's missing or empty so tests fail with a helpful
message; then split and iterate the returned string as before and match against
KUBEVIRT_TEKTON_AVAILABLE_TASKS_TEST. Ensure you update the logic near
csv_scope_session and the loop that references
KUBEVIRT_TEKTON_AVAILABLE_TASKS_TEST so an absent or empty annotation produces a
deterministic, descriptive error rather than a KeyError.
- Around line 127-132: The current loop in ImageDigestMirrorSet.get(...) returns
the first mirror containing "konflux-builds", which is nondeterministic;
instead, gather all candidate mirrors from idms.instance.spec.imageDigestMirrors
that contain "konflux-builds", derive their base paths (using the same
mirror.rsplit("/", 1)[0] logic), deduplicate them, sort the list to ensure a
deterministic order, and then select the first entry from that sorted list (or
raise the existing ValueError if empty). Update the code around
ImageDigestMirrorSet.get, the mirror extraction logic, and the return to use
this deterministic selection.
- Around line 118-137: Add concise Google-style docstrings to the three new
fixtures tekton_test_image_name_and_digest, konflux_base_path, and
tekton_test_image: for each fixture include a one-line summary, an Args section
describing any fixture parameters (e.g., csv_scope_session, admin_client) and
what input the fixture reads, and a Returns section describing the exact return
format (e.g., "str: image name and digest in 'name@sha256:...' format", "str:
base mirror path", "str: full pullspec
'<registry>/<path>/<image>@sha256:<digest>'"). Place the docstrings immediately
above each fixture definition and keep them in Google docstring style so
downstream tests/users understand the contract and return formats.
Comment on lines
+118
to
+137
| def tekton_test_image_name_and_digest(csv_scope_session): | ||
| for image in csv_scope_session.instance.metadata.annotations["test-images-nvrs"].split(","): | ||
| if KUBEVIRT_TEKTON_AVAILABLE_TASKS_TEST in image: | ||
| return f"{BREW_REGISTERY_SOURCE}/rh-osbs/container-native-virtualization-{image.strip()}" | ||
| raise ValueError("Tekton test image not found in CSV annotations.") | ||
| return image.strip() | ||
| raise ValueError(f"{KUBEVIRT_TEKTON_AVAILABLE_TASKS_TEST} not found in CSV 'test-images-nvrs' annotation.") | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session") | ||
| def konflux_base_path(admin_client): | ||
| for idms in ImageDigestMirrorSet.get(client=admin_client): | ||
| for mirror_entry in idms.instance.spec.imageDigestMirrors: | ||
| for mirror in mirror_entry.get("mirrors", []): | ||
| if "konflux-builds" in mirror: | ||
| return mirror.rsplit("/", 1)[0] | ||
| raise ValueError("Konflux mirror not found in ImageDigestMirrorSet.") | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session") | ||
| def tekton_test_image(tekton_test_image_name_and_digest, konflux_base_path): | ||
| return f"{konflux_base_path}/{tekton_test_image_name_and_digest}" |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
MEDIUM: Add Google-style docstrings for new fixtures
These fixtures introduce non-obvious contracts (name@digest, discovered mirror base path, and composed pullspec). Short Google-format docstrings help prevent misuse in downstream fixtures/tests.
Example pattern
`@pytest.fixture`(scope="session")
def tekton_test_image(...):
"""Compose the full Konflux pullspec for Tekton test resources.
Args:
...
Returns:
str: Full pullspec in `<registry>/<path>/<image>@sha256:<digest>` format.
"""As per coding guidelines, "Google-format docstrings REQUIRED for all public functions with non-obvious return values OR side effects".
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 122-122: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 132-132: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/infrastructure/tekton/conftest.py` around lines 118 - 137, Add concise
Google-style docstrings to the three new fixtures
tekton_test_image_name_and_digest, konflux_base_path, and tekton_test_image: for
each fixture include a one-line summary, an Args section describing any fixture
parameters (e.g., csv_scope_session, admin_client) and what input the fixture
reads, and a Returns section describing the exact return format (e.g., "str:
image name and digest in 'name@sha256:...' format", "str: base mirror path",
"str: full pullspec '<registry>/<path>/<image>@sha256:<digest>'"). Place the
docstrings immediately above each fixture definition and keep them in Google
docstring style so downstream tests/users understand the contract and return
formats.
Comment on lines
+119
to
+123
| for image in csv_scope_session.instance.metadata.annotations["test-images-nvrs"].split(","): | ||
| if KUBEVIRT_TEKTON_AVAILABLE_TASKS_TEST in image: | ||
| return f"{BREW_REGISTERY_SOURCE}/rh-osbs/container-native-virtualization-{image.strip()}" | ||
| raise ValueError("Tekton test image not found in CSV annotations.") | ||
| return image.strip() | ||
| raise ValueError(f"{KUBEVIRT_TEKTON_AVAILABLE_TASKS_TEST} not found in CSV 'test-images-nvrs' annotation.") | ||
|
|
Contributor
There was a problem hiding this comment.
HIGH: Guard CSV annotation access before splitting
Line 119 dereferences ["test-images-nvrs"] directly. If the annotation is missing/empty, this fails as KeyError instead of a clear setup error, which makes CI triage harder.
Suggested fix
`@pytest.fixture`(scope="session")
def tekton_test_image_name_and_digest(csv_scope_session):
- for image in csv_scope_session.instance.metadata.annotations["test-images-nvrs"].split(","):
- if KUBEVIRT_TEKTON_AVAILABLE_TASKS_TEST in image:
- return image.strip()
+ annotations = csv_scope_session.instance.metadata.annotations or {}
+ test_images_nvrs = annotations.get("test-images-nvrs")
+ if not test_images_nvrs:
+ raise ValueError("CSV annotation 'test-images-nvrs' is missing or empty.")
+
+ for test_image in test_images_nvrs.split(","):
+ if KUBEVIRT_TEKTON_AVAILABLE_TASKS_TEST in test_image:
+ return test_image.strip()
raise ValueError(f"{KUBEVIRT_TEKTON_AVAILABLE_TASKS_TEST} not found in CSV 'test-images-nvrs' annotation.")🧰 Tools
🪛 Ruff (0.15.2)
[warning] 122-122: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/infrastructure/tekton/conftest.py` around lines 119 - 123, Guard access
to the CSV annotation before splitting: instead of directly indexing
csv_scope_session.instance.metadata.annotations["test-images-nvrs"], fetch the
annotations dict and use .get("test-images-nvrs") (or check for presence) and
raise a clear ValueError if it's missing or empty so tests fail with a helpful
message; then split and iterate the returned string as before and match against
KUBEVIRT_TEKTON_AVAILABLE_TASKS_TEST. Ensure you update the logic near
csv_scope_session and the loop that references
KUBEVIRT_TEKTON_AVAILABLE_TASKS_TEST so an absent or empty annotation produces a
deterministic, descriptive error rather than a KeyError.
Comment on lines
+127
to
+132
| for idms in ImageDigestMirrorSet.get(client=admin_client): | ||
| for mirror_entry in idms.instance.spec.imageDigestMirrors: | ||
| for mirror in mirror_entry.get("mirrors", []): | ||
| if "konflux-builds" in mirror: | ||
| return mirror.rsplit("/", 1)[0] | ||
| raise ValueError("Konflux mirror not found in ImageDigestMirrorSet.") |
Contributor
There was a problem hiding this comment.
HIGH: Make Konflux mirror selection deterministic
Line 131 returns the first konflux-builds mirror found. If multiple mirror entries exist, ordering can pick the wrong base path and break the final pullspec composed on Line 137 (and extraction on Line 153).
Suggested fix
`@pytest.fixture`(scope="session")
def konflux_base_path(admin_client):
- for idms in ImageDigestMirrorSet.get(client=admin_client):
- for mirror_entry in idms.instance.spec.imageDigestMirrors:
+ konflux_base_paths = set()
+ for image_digest_mirror_set in ImageDigestMirrorSet.get(client=admin_client):
+ for mirror_entry in image_digest_mirror_set.instance.spec.imageDigestMirrors or []:
for mirror in mirror_entry.get("mirrors", []):
if "konflux-builds" in mirror:
- return mirror.rsplit("/", 1)[0]
- raise ValueError("Konflux mirror not found in ImageDigestMirrorSet.")
+ konflux_base_paths.add(mirror.rsplit("/", 1)[0])
+
+ if len(konflux_base_paths) != 1:
+ raise ValueError(
+ f"Expected exactly one Konflux mirror base path, found: {sorted(konflux_base_paths)}"
+ )
+ return next(iter(konflux_base_paths))🧰 Tools
🪛 Ruff (0.15.2)
[warning] 132-132: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/infrastructure/tekton/conftest.py` around lines 127 - 132, The current
loop in ImageDigestMirrorSet.get(...) returns the first mirror containing
"konflux-builds", which is nondeterministic; instead, gather all candidate
mirrors from idms.instance.spec.imageDigestMirrors that contain
"konflux-builds", derive their base paths (using the same mirror.rsplit("/",
1)[0] logic), deduplicate them, sort the list to ensure a deterministic order,
and then select the first entry from that sorted list (or raise the existing
ValueError if empty). Update the code around ImageDigestMirrorSet.get, the
mirror extraction logic, and the return to use this deterministic selection.
openshift-virtualization-qe-bot
added
lgtm-coderabbitai[bot]
tox:verify-tc-requirement-polarion:passed
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.
Short description:
tekton test image kubevirt-tekton-tasks-tests was previously pulled from Brew. With the move to Konflux builds, the Brew path no longer hosts these images causing Tekton pipeline tests to fail during image extraction. These image carry pipeline definitions.
More details:
we don't ship test-nvrs so they can never be part of external registry. Accessing them through konflux is only solution as discussed by CI team.
What this PR does / why we need it:
small PR change to point to right konflux builds dynamically from ImageDigestMirrorSet resources on the cluster
Which issue(s) this PR fixes:
Special notes for reviewer:
this will fix all tier3 lanes
jira-ticket:
Summary by CodeRabbit
Release Notes