Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a new info-collection test target and module that gathers cluster and component metadata, refactored metadata utilities into static helpers, introduced a Jinja2 template rendering utility and template, and exposed kubeconfig path via the Kubernetes client. Changes
Sequence Diagram(s)mermaid Runner->>Collector: invoke tests with COLLECTOR_ENABLE=true Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
silvi-t
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
Makefile (1)
83-86: Addcollectto the.PHONYdeclaration.The
collecttarget doesn't produce a file and should be declared as phony to prevent issues if a file namedcollectis ever created in the repository root.♻️ Proposed fix
Update line 1 to include
collect:-.PHONY: commit-acceptance pylint mypy black reformat test authorino poetry poetry-no-dev mgc container-image polish-junit reportportal authorino-standalone limitador kuadrant kuadrant-only disruptive kuadrantctl multicluster ui playwright-install +.PHONY: commit-acceptance pylint mypy black reformat test authorino poetry poetry-no-dev mgc container-image polish-junit reportportal authorino-standalone limitador kuadrant kuadrant-only disruptive kuadrantctl multicluster ui playwright-install collect🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 83 - 86, Add the collect target to the .PHONY declaration so make treats it as a phony target; update the existing .PHONY line to include the symbol "collect" alongside the other phony targets, ensuring the Makefile's collect rule (the target named collect that runs poetry-no-dev and pytest) is declared phony to avoid conflicts with any filesystem entry named "collect".testsuite/tests/conftest.py (1)
141-148: Minor inconsistency: use tuple instead of list foruser_properties.Line 144 correctly uses a tuple
("issue", issue), but line 148 uses a list["__rp_case_description", ...]. For consistency and sinceuser_propertiesis a list of name-value pairs, prefer tuples throughout.♻️ Proposed fix
## extracting test's docstring for RP if item.function.__doc__: - item.user_properties.append(["__rp_case_description", item.function.__doc__]) + item.user_properties.append(("__rp_case_description", item.function.__doc__))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/conftest.py` around lines 141 - 148, Replace the list used when appending the docstring property with a tuple for consistency: in the loop that inspects pytest items (the block referencing item.iter_markers and item.function.__doc__), change the append call that currently uses ["__rp_case_description", item.function.__doc__] to append a tuple ("__rp_case_description", item.function.__doc__) so all entries added to item.user_properties use the same (name, value) tuple format.testsuite/component_metadata.py (1)
135-139: Remove debugLine 135 has a debug
logger.debug()instead. Additionally,rsplit(":", 1)on line 139 will raiseValueErrorif an image has no tag (no colon after the image name), though this is rare for container images.♻️ Proposed fix
seen.add(image) - print(f"{image=}") + logger.debug("Found image: %s", image) if "@sha256" in image: continue image_name = image.split("/")[-1] - name, tag = image_name.rsplit(":", 1) - images.append((name, tag, image)) + if ":" in image_name: + name, tag = image_name.rsplit(":", 1) + images.append((name, tag, image)) + else: + logger.debug("Skipping image without tag: %s", image)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/component_metadata.py` around lines 135 - 139, Replace the debug print with a logger.debug call and make the tag-splitting robust: instead of print(f"{image=}"), call logger.debug(...) and when extracting name and tag from image_name (the code using image_name.rsplit(":", 1")), handle the case where no ":" exists by assigning a sensible default tag (e.g., "latest" or None) or skipping such images to avoid ValueError; update the logic around the image variable, image_name, and the rsplit usage to perform a conditional split or fallback before unpacking.testsuite/templates/reporting/launch_description.txt.j2 (1)
6-7: Static list numbering may not render as intended.The template uses
1.as a literal for each cluster in the loop. While some Markdown renderers auto-increment numbered lists, this produces repeated "1." prefixes in the raw output. Consider usingloop.indexfor explicit numbering if the output is consumed as plain text or by renderers that don't auto-correct.♻️ Optional: Use loop.index for explicit numbering
{% for key, cluster in clusters.items() -%} -1. {{ cluster.console_url }} ({{ key }}): +{{ loop.index }}. {{ cluster.console_url }} ({{ key }}): {% if cluster.ocp_version is defined %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/templates/reporting/launch_description.txt.j2` around lines 6 - 7, The numbered list in the Jinja loop uses a literal "1." for every cluster which yields repeated "1." prefixes in raw/plain output; update the template loop line that currently reads "1. {{ cluster.console_url }} ({{ key }}):" to use Jinja's loop.index (or loop.index0+1) for explicit numbering so each item is numbered correctly (refer to the for loop over clusters.items and the cluster.console_url / key expressions to locate the line to change).testsuite/tests/info_collector.py (1)
100-108: Consider recordingistiod_imageas well.The function retrieves both
istio_versionandistiod_imagefromget_istio_metadata(), but onlyistio_versionis recorded as a property. If the image information is useful for debugging or reporting, consider recording it too.♻️ Optional: Record additional Istio metadata
for key, value in istio_metadata.items(): print(f"{key}: {value}") - if key == "istio_version": - record_testsuite_property(key, value) + record_testsuite_property(key, value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/info_collector.py` around lines 100 - 108, The test_istio_properties function records only istio_version from the istio_metadata returned by ReportPortalMetadataCollector.get_istio_metadata(); update the logic to also record the istiod_image property by calling record_testsuite_property("istiod_image", value) when key == "istiod_image" (or explicitly read istio_metadata["istiod_image"] and record it), ensuring both istio_version and istiod_image are persisted for debugging/reporting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Around line 83-86: Add the collect target to the .PHONY declaration so make
treats it as a phony target; update the existing .PHONY line to include the
symbol "collect" alongside the other phony targets, ensuring the Makefile's
collect rule (the target named collect that runs poetry-no-dev and pytest) is
declared phony to avoid conflicts with any filesystem entry named "collect".
In `@testsuite/component_metadata.py`:
- Around line 135-139: Replace the debug print with a logger.debug call and make
the tag-splitting robust: instead of print(f"{image=}"), call logger.debug(...)
and when extracting name and tag from image_name (the code using
image_name.rsplit(":", 1")), handle the case where no ":" exists by assigning a
sensible default tag (e.g., "latest" or None) or skipping such images to avoid
ValueError; update the logic around the image variable, image_name, and the
rsplit usage to perform a conditional split or fallback before unpacking.
In `@testsuite/templates/reporting/launch_description.txt.j2`:
- Around line 6-7: The numbered list in the Jinja loop uses a literal "1." for
every cluster which yields repeated "1." prefixes in raw/plain output; update
the template loop line that currently reads "1. {{ cluster.console_url }} ({{
key }}):" to use Jinja's loop.index (or loop.index0+1) for explicit numbering so
each item is numbered correctly (refer to the for loop over clusters.items and
the cluster.console_url / key expressions to locate the line to change).
In `@testsuite/tests/conftest.py`:
- Around line 141-148: Replace the list used when appending the docstring
property with a tuple for consistency: in the loop that inspects pytest items
(the block referencing item.iter_markers and item.function.__doc__), change the
append call that currently uses ["__rp_case_description", item.function.__doc__]
to append a tuple ("__rp_case_description", item.function.__doc__) so all
entries added to item.user_properties use the same (name, value) tuple format.
In `@testsuite/tests/info_collector.py`:
- Around line 100-108: The test_istio_properties function records only
istio_version from the istio_metadata returned by
ReportPortalMetadataCollector.get_istio_metadata(); update the logic to also
record the istiod_image property by calling
record_testsuite_property("istiod_image", value) when key == "istiod_image" (or
explicitly read istio_metadata["istiod_image"] and record it), ensuring both
istio_version and istiod_image are persisted for debugging/reporting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d50ff169-0532-4c4f-9835-ddc79c4c27af
📒 Files selected for processing (7)
Makefiletestsuite/component_metadata.pytestsuite/kubernetes/client.pytestsuite/template_utils.pytestsuite/templates/reporting/launch_description.txt.j2testsuite/tests/conftest.pytestsuite/tests/info_collector.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
testsuite/tests/conftest.py (2)
141-148: Consider guarding against markers used without arguments.The
marker.args[0]access assumes theissuemarker is always used with an argument. If someone mistakenly uses@pytest.mark.issuewithout providing a URL, this would raise anIndexError. Thepytest_runtest_makereporthook (line 78) has the same pattern, so this is consistent, but adding a guard would improve robustness.♻️ Optional: Add defensive guard
for item in items: for marker in item.iter_markers(name="issue"): - issue = marker.args[0] - item.user_properties.append(("issue", issue)) + if marker.args: + issue = marker.args[0] + item.user_properties.append(("issue", issue)) ## extracting test's docstring for RP if item.function.__doc__: item.user_properties.append(("__rp_case_description", item.function.__doc__))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/conftest.py` around lines 141 - 148, The loop that extracts the "issue" marker assumes marker.args[0] exists and will raise IndexError if the marker is used without arguments; update the marker handling in the for item in items loop (and mirror the same change in pytest_runtest_makereport) to check that marker.args is non-empty before using marker.args[0], and only append ("issue", issue) to item.user_properties when an argument is present (optionally log or ignore empty markers).
99-110: XML parsing vulnerability flagged by static analysis.The static analysis tool (Ruff S318) flags
xml.dom.minidom.parseas vulnerable to XML attacks (XXE, billion laughs, etc.). However, in this context, the XML file being parsed is the JUnit XML output generated by pytest itself during the same test session, not untrusted external input. This significantly reduces the risk.If you want to eliminate the warning and add defence in depth, consider using
defusedxml. The broad exception catch is acceptable here given the explicit comment and the goal of not failing test runs due to formatting issues.♻️ Optional: Use defusedxml for defence in depth
-import xml.dom.minidom +import defusedxml.minidom def pytest_sessionfinish(session, exitstatus): # pylint: disable=unused-argument """Pretty-print JUnit XML files after test session""" junit_xml = session.config.option.xmlpath if junit_xml and os.path.exists(junit_xml): try: - dom = xml.dom.minidom.parse(junit_xml) + dom = defusedxml.minidom.parse(junit_xml) pretty_xml = dom.toprettyxml(indent=" ")Note: This would require adding
defusedxmlto your dependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/conftest.py` around lines 99 - 110, Replace the vulnerable xml.dom.minidom.parse call in pytest_sessionfinish with a secure parser from defusedxml: add an import for defusedxml.minidom (or defusedxml.ElementTree) and call its parse function to read the JUnit XML before calling toprettyxml; keep the existing try/except behavior and the broad-exception comment so formatting failures don't fail the test run, and ensure you add defusedxml to test dependencies if not already present.testsuite/component_metadata.py (1)
103-118: Inconsistent exception handling compared to sibling methods.This method only catches
OpenShiftPythonException, butjson.loadscan raisejson.JSONDecodeErrorif the output is malformed. Other methods in this class (e.g.,get_ocp_version,get_component_images) catch a broader set of exceptions includingValueError(whichJSONDecodeErrorinherits from).♻️ Proposed fix for consistent exception handling
except oc.OpenShiftPythonException as e: logger.warning("Failed to get Kubernetes version: %s", e) + except (json.JSONDecodeError, AttributeError, KeyError) as e: + logger.warning("Failed to parse Kubernetes version output: %s", e) return NoneAlternatively, align with other methods by catching
ValueError(parent ofJSONDecodeError):- except oc.OpenShiftPythonException as e: + except (oc.OpenShiftPythonException, ValueError, AttributeError, KeyError) as e: logger.warning("Failed to get Kubernetes version: %s", e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/component_metadata.py` around lines 103 - 118, The get_kubernetes_version static method currently only catches oc.OpenShiftPythonException but can also raise json.JSONDecodeError (a subclass of ValueError) when parsing version_result; update the exception handling in get_kubernetes_version to mirror sibling methods by catching ValueError (or both ValueError and oc.OpenShiftPythonException) so JSON parsing errors are handled and logged consistently, and ensure the logger message remains informative by including the caught exception variable in the log call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testsuite/component_metadata.py`:
- Line 135: Remove the stray debug print statement print(f"{image=}") in
testsuite/component_metadata.py; replace it with a logger.debug call (e.g.,
logger.debug("image=%r", image)) or remove it entirely if not needed, and ensure
a module logger exists (create one via logging.getLogger(__name__) if absent) so
no debug output leaks to stdout in production.
---
Nitpick comments:
In `@testsuite/component_metadata.py`:
- Around line 103-118: The get_kubernetes_version static method currently only
catches oc.OpenShiftPythonException but can also raise json.JSONDecodeError (a
subclass of ValueError) when parsing version_result; update the exception
handling in get_kubernetes_version to mirror sibling methods by catching
ValueError (or both ValueError and oc.OpenShiftPythonException) so JSON parsing
errors are handled and logged consistently, and ensure the logger message
remains informative by including the caught exception variable in the log call.
In `@testsuite/tests/conftest.py`:
- Around line 141-148: The loop that extracts the "issue" marker assumes
marker.args[0] exists and will raise IndexError if the marker is used without
arguments; update the marker handling in the for item in items loop (and mirror
the same change in pytest_runtest_makereport) to check that marker.args is
non-empty before using marker.args[0], and only append ("issue", issue) to
item.user_properties when an argument is present (optionally log or ignore empty
markers).
- Around line 99-110: Replace the vulnerable xml.dom.minidom.parse call in
pytest_sessionfinish with a secure parser from defusedxml: add an import for
defusedxml.minidom (or defusedxml.ElementTree) and call its parse function to
read the JUnit XML before calling toprettyxml; keep the existing try/except
behavior and the broad-exception comment so formatting failures don't fail the
test run, and ensure you add defusedxml to test dependencies if not already
present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1d33266-8ece-4fc1-a0a8-fbc70f09c2a6
📒 Files selected for processing (4)
Makefiletestsuite/component_metadata.pytestsuite/templates/reporting/launch_description.txt.j2testsuite/tests/conftest.py
✅ Files skipped from review due to trivial changes (1)
- testsuite/templates/reporting/launch_description.txt.j2
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
There was a problem hiding this comment.
🧹 Nitpick comments (2)
testsuite/tests/conftest.py (2)
99-110: Consider usingdefusedxmlfor XML parsing.Static analysis flags
xml.dom.minidom.parseas potentially vulnerable to XML attacks (S318). While the risk is low here since you're parsing pytest-generated JUnit XML, usingdefusedxmlwould be a safer practice and silence the warning.The broad exception catch on line 108 is appropriate given you don't want formatting failures to break the test run.
🛡️ Optional fix using defusedxml
-import xml.dom.minidom +import defusedxml.minidom ... - dom = xml.dom.minidom.parse(junit_xml) + dom = defusedxml.minidom.parse(junit_xml)This would require adding
defusedxmlto your dependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/conftest.py` around lines 99 - 110, Replace the use of xml.dom.minidom.parse in pytest_sessionfinish with the safe defusedxml parser: import defusedxml.minidom and call defusedxml.minidom.parse (instead of xml.dom.minidom.parse) when loading junit_xml, keeping the existing dom.toprettyxml and file-write logic; also add defusedxml to test dependencies so the secure parser is available.
146-148: Guard against items without afunctionattribute.Not all pytest items have a
functionattribute (e.g., doctests or custom item types). Accessingitem.function.__doc__directly could raiseAttributeErrorin edge cases.🛡️ Proposed defensive fix
## extracting test's docstring for RP - if item.function.__doc__: - item.user_properties.append(("__rp_case_description", item.function.__doc__)) + func = getattr(item, "function", None) + if func and func.__doc__: + item.user_properties.append(("__rp_case_description", func.__doc__))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/conftest.py` around lines 146 - 148, The code unconditionally reads item.function.__doc__, which can raise AttributeError for items that lack a function (doctests/custom items); change the logic around the block that sets ("__rp_case_description", ...) to first check that getattr(item, "function", None) is not None and that item.function.__doc__ is truthy (e.g., use func = getattr(item, "function", None); if func and getattr(func, "__doc__", None): item.user_properties.append(("__rp_case_description", func.__doc__))); this safely guards the item access and only appends when a docstring exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@testsuite/tests/conftest.py`:
- Around line 99-110: Replace the use of xml.dom.minidom.parse in
pytest_sessionfinish with the safe defusedxml parser: import defusedxml.minidom
and call defusedxml.minidom.parse (instead of xml.dom.minidom.parse) when
loading junit_xml, keeping the existing dom.toprettyxml and file-write logic;
also add defusedxml to test dependencies so the secure parser is available.
- Around line 146-148: The code unconditionally reads item.function.__doc__,
which can raise AttributeError for items that lack a function (doctests/custom
items); change the logic around the block that sets ("__rp_case_description",
...) to first check that getattr(item, "function", None) is not None and that
item.function.__doc__ is truthy (e.g., use func = getattr(item, "function",
None); if func and getattr(func, "__doc__", None):
item.user_properties.append(("__rp_case_description", func.__doc__))); this
safely guards the item access and only appends when a docstring exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae88bdec-64b4-4833-bd7f-25dbde5bdca9
📒 Files selected for processing (2)
testsuite/component_metadata.pytestsuite/tests/conftest.py
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 `@testsuite/component_metadata.py`:
- Around line 131-141: The current loop in testsuite/component_metadata.py drops
digest-qualified refs because it checks "@sha256" before parsing tags; change
the logic in the block that uses image, seen, images and image_name to first
strip any digest suffix (remove the "@sha256:..." part) into a normalized_ref,
dedupe using that normalized_ref (use seen.add(normalized_ref)), and then parse
the tag from the normalized_ref (split to get image_name then rsplit(":", 1) to
extract name and tag) so tag information is preserved for entries like
"name:tag@sha256:...".
- Around line 63-71: The current code picks pods[0] which is non-deterministic;
instead iterate over the pods list returned by oc.selector(...) and for each pod
inspect pod.model.status.containerStatuses to find the container whose name
matches container_name (or "manager" when looking up istiod_image), preferably
checking pod.status.phase == "Running" and containerStatus.ready == True to
prefer active workloads; when found extract and normalize status.image (strip
digest after "@"), set metadata["kuadrant_image"] and break out of the loops.
In `@testsuite/tests/conftest.py`:
- Around line 99-110: The pytest_sessionfinish hook may run before pytest's
junitxml plugin writes the XML, so add the `@pytest.hookimpl`(trylast=True)
decorator above the pytest_sessionfinish function to ensure your prettification
runs after junitxml finalizes the file; update the decorator import/usage if
necessary so pytest_sessionfinish (in testsuite/tests/conftest.py) is executed
last and can safely read and rewrite the completed JUnit XML file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1099cab0-f5cc-462e-9411-4a9390a390cf
📒 Files selected for processing (2)
testsuite/component_metadata.pytestsuite/tests/conftest.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
testsuite/component_metadata.py (1)
157-160:⚠️ Potential issue | 🟠 MajorAvoid choosing Istio metadata from
pods[0].This still depends on pod-list order and the first container in that pod, so rollouts or multiple matching pods can make
istiod_imagedrift between runs. Select a Running/Ready pod deterministically before reading the image.Does Kubernetes guarantee the order of PodList.items returned by a LIST with a label selector, or should clients sort/filter explicitly before choosing a pod?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/component_metadata.py` around lines 157 - 160, The code currently picks pods[0] and containers[0] which is nondeterministic; instead filter the list returned by oc.selector("pods", labels={"app": "istiod"}).objects() to only include pods whose model.status.phase == "Running" and whose conditions contain type "Ready" with status "True", sort that filtered list deterministically (e.g., by model.metadata.name), pick the first pod, and then select the istiod image by matching a container name (if known) or falling back to containers[0]; update the image assignment logic around pods and containers to use this ready-and-sorted selection so istiod_image no longer drifts between runs.
🧹 Nitpick comments (1)
testsuite/tests/conftest.py (1)
132-145: Sync the hook docstring with the properties actually emitted.The docstring says this adds both
issueandissue-id, but the implementation only appends("issue", marker.args[0]). Either emit the extra property or trim the docstring to match the current behaviour.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/conftest.py` around lines 132 - 145, The docstring claims both "issue" and "issue-id" are added but the loop only appends ("issue", ...); update the implementation to emit both properties to match the docstring by appending an additional ("issue-id", marker.args[0]) for the same marker inside the loop (locate the loop using items, item.iter_markers(name="issue") and item.user_properties.append(("issue", ...))). Ensure both properties are appended for each marker so the docstring and behavior are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testsuite/tests/conftest.py`:
- Around line 104-109: The current prettify block writes directly to junit_xml
which truncates the original file before the new content is durable; instead,
write the prettified content to a temporary file in the same directory (e.g.,
using tempfile.NamedTemporaryFile or tempfile.mkstemp with
dir=os.path.dirname(junit_xml) and encoding='utf-8'), flush and fsync the temp
file, close it, then atomically replace the original with os.replace(temp_path,
junit_xml); do this in the same try/except around the
xml.dom.minidom.parse/prettify logic and reference the existing variable
junit_xml so the final rename is atomic and avoids leaving a truncated JUnit
file on write failure.
---
Duplicate comments:
In `@testsuite/component_metadata.py`:
- Around line 157-160: The code currently picks pods[0] and containers[0] which
is nondeterministic; instead filter the list returned by oc.selector("pods",
labels={"app": "istiod"}).objects() to only include pods whose
model.status.phase == "Running" and whose conditions contain type "Ready" with
status "True", sort that filtered list deterministically (e.g., by
model.metadata.name), pick the first pod, and then select the istiod image by
matching a container name (if known) or falling back to containers[0]; update
the image assignment logic around pods and containers to use this
ready-and-sorted selection so istiod_image no longer drifts between runs.
---
Nitpick comments:
In `@testsuite/tests/conftest.py`:
- Around line 132-145: The docstring claims both "issue" and "issue-id" are
added but the loop only appends ("issue", ...); update the implementation to
emit both properties to match the docstring by appending an additional
("issue-id", marker.args[0]) for the same marker inside the loop (locate the
loop using items, item.iter_markers(name="issue") and
item.user_properties.append(("issue", ...))). Ensure both properties are
appended for each marker so the docstring and behavior are consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b75fa587-44ad-4f82-8d4d-b10727b5dc63
📒 Files selected for processing (2)
testsuite/component_metadata.pytestsuite/tests/conftest.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
testsuite/tests/conftest.py (1)
104-109:⚠️ Potential issue | 🟠 MajorWrite the prettified JUnit file atomically to avoid artefact loss.
Line 107 still truncates the original XML before replacement content is durable. If this write fails, the JUnit artefact can be left broken/empty.
💡 Suggested patch
import os import signal +import tempfile import xml.dom.minidom @@ try: dom = xml.dom.minidom.parse(junit_xml) pretty_xml = dom.toprettyxml(indent=" ") - with open(junit_xml, "w", encoding="utf-8") as f: - f.write(pretty_xml) + target_dir = os.path.dirname(junit_xml) or "." + with tempfile.NamedTemporaryFile("w", encoding="utf-8", dir=target_dir, delete=False) as f: + f.write(pretty_xml) + f.flush() + os.fsync(f.fileno()) + tmp_path = f.name + os.replace(tmp_path, junit_xml) except Exception as e: # pylint: disable=broad-exception-caught # Don't fail the test run if formatting fails print(f"Warning: Failed to format JUnit XML: {e}")#!/bin/bash # Verify the current non-atomic overwrite pattern is still present. rg -n -C2 'pytest_sessionfinish|open\(junit_xml,\s*"w"|NamedTemporaryFile|os\.replace' testsuite/tests/conftest.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/conftest.py` around lines 104 - 109, The current prettify block in pytest_sessionfinish that parses junit_xml and writes with open(junit_xml, "w", ...) can truncate the original file if the write fails; change it to write the pretty_xml to a temporary file in the same directory (e.g., using tempfile.NamedTemporaryFile or tempfile.mkstemp) with the same encoding, flush and fsync the temp file, then atomically replace the original by os.replace(temp_path, junit_xml); update any exception handling around the dom.toprettyxml/temporary write so the original junit_xml is only replaced on success and the temp file is cleaned up on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@testsuite/tests/conftest.py`:
- Around line 104-109: The current prettify block in pytest_sessionfinish that
parses junit_xml and writes with open(junit_xml, "w", ...) can truncate the
original file if the write fails; change it to write the pretty_xml to a
temporary file in the same directory (e.g., using tempfile.NamedTemporaryFile or
tempfile.mkstemp) with the same encoding, flush and fsync the temp file, then
atomically replace the original by os.replace(temp_path, junit_xml); update any
exception handling around the dom.toprettyxml/temporary write so the original
junit_xml is only replaced on success and the temp file is cleaned up on
failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 827f5a07-cded-4920-a5f5-1d81a0e10efe
📒 Files selected for processing (2)
testsuite/component_metadata.pytestsuite/tests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- testsuite/component_metadata.py
|
After co-author review, we decided to remove Pretty XML format to simplify, and reduce amount of breakage points. Incompatibility with current reporting: Before we switch to rptool we loose on launch description, and basic version properties we had. No actual test results should be affected. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testsuite/tests/conftest.py`:
- Around line 118-119: The module docstring claiming both `issue` and `issue-id`
are emitted is inconsistent with the implementation that only appends the
`issue` property for pytest.mark.issue; either update the docstring to state
only `issue` is emitted, or modify the junit property-emission logic (the hook
that appends the `issue` property for pytest.mark.issue in conftest.py) to also
append an `issue-id` property using the same value/logic as `issue` so the
docstring and behavior match. Ensure you update the docstring text near the top
of the file to exactly reflect which junit properties are produced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17331d22-63d9-46c0-bf5b-183f73450246
📒 Files selected for processing (1)
testsuite/tests/conftest.py
zkraus
requested review from
averevki,
azgabur,
crstrn13,
emmaaroche and
trepel
azgabur
left a comment
azgabur
left a comment
There was a problem hiding this comment.
I see most properties are collected from the "first" cluster, which is okay as almost all tests run on it as well as the cluster pairs are deployed with the same configuration.
Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
- Add format_cluster_info() method to render cluster metadata as markdown - Use formatted output in info_collector launch description - Move info-collector suite name to Makefile instead of pytest_configure Signed-off-by: Silvia Tarabova <starabov@redhat.com>
…of subprocess Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
Signed-off-by: Zdenek Kraus <zkraus@redhat.com>
|
Rebase to the latest main to resolve merge conflicts. Accepted/merged 2 new property methods from both branches. No additional code changes. |
emmaaroche
left a comment
emmaaroche
left a comment
There was a problem hiding this comment.
LGTM, nice work! Tested the workflow locally and all works as expected. The launch description looks great with all the cluster/component info 👏
4 participants
Important
This PR modifies shared/core testsuite code that could potentially affect multiple test areas. 2 reviewers should review this PR to ensure adequate coverage.
Warning
Incompatibility with current reporting: Before we switch to
rptoolwe loose on launch description, and basic version properties we had. No actual test results should be affected.Description
Testsuite adaptation for a new results reporting enabled by
rptool.conftest.pyand into a dedicatedinfo_collector.pymodule that runs as a separate pre-suite steptemplate_utils.py) for rendering ReportPortal launch descriptions from external template filesComponentMetadataCollectorintoReportPortalMetadataCollector, adding new static methods for Kubernetes version, OCP version, and component image collectionpytest_sessionfinishand issue marker support inpytest_collection_modifyitemsChanges
Refactoring
ComponentMetadataCollectorclass intoReportPortalMetadataCollectorincomponent_metadata.py, converting methods to@staticmethodwhere possibleget_kubernetes_version(),get_component_images(),get_cluster_metadata()add_properties_to_items()and_build_property_value()fromReportPortalMetadataCollectorkubeconfig_pathproperty onKubernetesClientNew Modules
testsuite/tests/info_collector.py— standalone collector pseudo-tests (test_launch_description,test_cluster_properties,test_kube_context,test_kuadrant_properties,test_istio_properties) gated byCOLLECTOR_ENABLEenv vartestsuite/template_utils.py— Jinja2 template loading/rendering utilities with CLI entrypointtestsuite/templates/reporting/launch_description.txt.j2— Jinja2 template for ReportPortal launch descriptionVerification steps
Note: Example results can be found in the test-info-collector launch in Report Portal TESTSUITE project.
Co-authored by @silvi-t
Summary by CodeRabbit
Tests
New Features
Chores