feat(olminstall): run BVT in Konflux olminstall integration test - #382
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (37)
📝 WalkthroughWalkthroughAdds a unified Tekton Pipeline odh-olminstall-test, replaces the smoke-only pipeline, and introduces a tests-phase catalog plus CLI flags (--tests/--tests-config). Implements many helper CLIs and Python modules for Tekton result writing, in-cluster Tekton/Kubernetes access, PipelineRun summary collection/patching, KubeArchive auth handling, BVT image resolution and pytest execution (including PRODUCT=none placeholder), diagnostics collection, EaaS CTI provisioning, OCI artifact push, Slack notifications, runner/watch refactor, and updated docs/examples. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes |
![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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integration-tests/olminstall/helpers/runner.py (1)
642-643:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle KubeArchive auth failures in merged list path.
Line 642 calls
self.ka.get_json(path)without catchingKubeArchiveAuthError. If auth expires, list/watch flows can hard-fail instead of falling back to live data.Suggested fix
- data = self.ka.get_json(path) + try: + data = self.ka.get_json(path) + except KubeArchiveAuthError as exc: + print(f"WARN KubeArchive auth failed while listing archived PipelineRuns: {exc}", file=sys.stderr) + data = {} for item in data.get("items", []):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/runner.py` around lines 642 - 643, Wrap the call to self.ka.get_json(path) in a try/except that catches KubeArchiveAuthError and falls back to retrieving live data (e.g., call self.ka.get_live_json(path) or equivalent), so the merged list/watch path won’t hard-fail when archive auth expires; also log or warn when falling back and ensure KubeArchiveAuthError is imported/available in runner.py.
🧹 Nitpick comments (1)
integration-tests/olminstall/helpers/tests_config.py (1)
45-50: ⚡ Quick winAdd timeout to subprocess call to prevent indefinite hangs.
The
subprocess.runcall lacks a timeout parameter. Ifyqhangs or takes too long, the pipeline step will block indefinitely (or until the Tekton task timeout). Add an explicit timeout.⏱️ Suggested fix
proc = subprocess.run( [yq_bin, "e", "-o=json", ".", str(path)], capture_output=True, text=True, check=False, + timeout=30, )Wrap the call to handle
subprocess.TimeoutExpired:+ try: proc = subprocess.run( [yq_bin, "e", "-o=json", ".", str(path)], capture_output=True, text=True, check=False, + timeout=30, ) + except subprocess.TimeoutExpired as exc: + raise AppError(f"yq timed out reading {path}: {exc}", 2) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/tests_config.py` around lines 45 - 50, The subprocess.run invocation that sets proc (the call to subprocess.run([yq_bin, "e", "-o=json", ".", str(path)], ...)) needs an explicit timeout to avoid indefinite hangs; add a timeout parameter (e.g., timeout=30) to that call and wrap the call in a try/except that catches subprocess.TimeoutExpired to log/handle the timeout and fail gracefully (use the proc variable/result handling already present after the call). Ensure you reference subprocess.TimeoutExpired in the except block and provide a clear error path rather than letting the test hang.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration-tests/olminstall/helpers/bvt_product_none_placeholder_junit.py`:
- Around line 40-46: The script currently accepts an arbitrary CLI path into out
= Path(sys.argv[1]...) and writes files beneath it, which allows path traversal;
change this to validate and canonicalize the provided path before writing:
resolve the Path, reject absolute paths or any path whose resolved location is
not a subpath of the allowed base (e.g., the default "/artifacts") or falls
outside the repository/artifacts directory, and if invalid fall back to the safe
default; apply this check where out is created and before the loop that writes
(references: variable out, Path(...) construction, the for loop that writes
f"{prefix}.xml", and the _testsuite_xml/_SKIP usage) so that only permitted
artifact subpaths are created.
In `@integration-tests/olminstall/helpers/cli.py`:
- Around line 250-255: Before calling validate_and_normalize_tests_csv, capture
whether the --tests option was explicitly provided (e.g. set a boolean like
tests_explicit = args.tests is not None or args.tests != ""), then perform
cfg_arg/cfg_path/catalog load and normalize into args.tests as existing code
does; pass that explicit-presence flag into
_trigger_options_incompatible_with_query() (or use it inside that function) so
the incompatibility check uses explicit presence instead of comparing the
normalized args.tests value to args.tests_catalog_default_csv; apply the same
explicit-presence approach to the other compatibility checks currently around
the validate/normalize area.
In `@integration-tests/olminstall/helpers/collect_diagnostics.py`:
- Line 58: The call to _oc([...]) running "adm inspect" can fail silently;
modify the call in collect_diagnostics.py so the returned exit code and
stdout/stderr from _oc when invoking f"ns/{ns}" and
"--dest-dir={diag_dir}/inspect-ns-operator" are checked, persist the
stderr/stdout to a file (e.g. inspect-ns-operator/error.log or similar), and if
_oc indicates a non-zero exit status either raise/return a non-zero exit from
the diagnostics run or write an explicit failure marker file so the overall
diagnostics manifest is not reported as successful; locate the invocation of
_oc(...) and the variables ns and diag_dir to implement these checks and
persistence.
In `@integration-tests/olminstall/helpers/create_eaas_cluster.py`:
- Around line 118-121: After creating the CTI, validate that the extracted
cti_name (the result of run([... "-o=jsonpath={.metadata.name}"])) is non-empty;
if it is empty, immediately raise/exit with a clear error including the run()
captured stdout/stderr and the command context instead of calling write_result()
and continuing to poll. Update the logic around the run call and the
write_result invocation so you only call write_result(result_path, cti_name)
when cti_name is truthy, and on empty cti_name print/log the run output and exit
with a non-zero status to fail fast.
In `@integration-tests/olminstall/helpers/extract_fbcf_image.py`:
- Around line 91-95: The code is writing an unvalidated filesystem path
(result_path) which can enable path traversal or arbitrary file overwrite;
before calling Path(result_path).write_text in extract_fbcf_image (or the
surrounding helper), validate and normalize result_path: ensure it is not
absolute or contains upward traversal (e.g., reject paths with ".."), resolve it
to a canonical path and verify it is inside an allowed output directory (a
configured sanitized base directory or whitelist) and that its parent directory
exists and is writable; if validation fails, log an error and return non-zero
instead of writing.
In `@integration-tests/olminstall/helpers/install_and_verify.py`:
- Around line 389-396: The _apply_cr function currently logs apply failures and
continues, and the caller ignores wait_dsc_ready() result, allowing installs to
be marked successful when DSC isn't applied or Ready; update _apply_cr to raise
an exception or return a non-None/false failure indicator (e.g., raise
RuntimeError with the error message) when oc_run returns non-zero, and update
the code that calls wait_dsc_ready() (the caller at the location that currently
ignores its return) to check the result and abort/raise/exit on failure so the
overall install step fails immediately when DSC apply or readiness fails.
In `@integration-tests/olminstall/helpers/run_bvt_pytest.py`:
- Around line 55-70: The _run_with_tee function can hang indefinitely because it
loops over proc.stdout with no timeout; update _run_with_tee to support a
configurable timeout (e.g. read from env BVT_RUN_TIMEOUT_SECS) and enforce it by
calling proc.wait(timeout=timeout_seconds) and handling
subprocess.TimeoutExpired: on timeout call proc.terminate(), wait a short grace
period then proc.kill() if still alive, capture/flush any remaining output from
proc.stdout to both sys.stdout and log, and return a non-zero exit code to
indicate timeout; reference function name _run_with_tee and the subprocess.Popen
instance (proc) when making these changes.
- Around line 103-118: The fallback branch is never reached because ec is
initialized to 1 and only changed inside the uv+pyproject branch; change the
logic so ec is set to a non-zero infra-failure value when uv is missing or
pyproject.toml is not present (e.g., set ec = 2 before the if or explicitly set
ec = 2 in the else branch), then keep the existing uv path that calls
_run_with_tee([uv, "run", "pytest", *pytest_args], log, env=env) and its retry
logic; this ensures the later if ec >= 2 check will execute the fallback path
that uses pip --target + PYTHONPATH when uv is unavailable or produced an infra
error.
In `@integration-tests/olminstall/helpers/runner.py`:
- Around line 881-888: In _pick_newest_owned_pipelinerun, avoid calling
self.get_snapshot_owner(row[2]) when the SNAPSHOT field (row[2]) is a JSON
payload; detect JSON (e.g., startswith '{' or via json.loads) and only call
get_snapshot_owner when row[2] is a resource name, otherwise skip that branch
and rely solely on row[3] == self.run_owner; update the owned list comprehension
to perform this guarded check so oc get snapshot is never invoked with JSON
content.
In `@integration-tests/olminstall/helpers/secure_push_oci_artifacts.sh`:
- Around line 20-22: Capture and inspect stderr from the oras manifest fetch
call (the code setting MANIFESTS_ANNOTATIONS via "oras manifest fetch") instead
of redirecting all errors to /dev/null and swallowing them; if stderr contains a
"not found" style message treat it as missing artifact and proceed with create,
but for any other stderr content fail fast with a clear diagnostic and non-zero
exit; apply the same explicit error-checking pattern to the subsequent oras pull
invocation so authentication/network/registry errors are distinguished from an
actual missing artifact before choosing the create/update path.
- Around line 56-59: The script currently defaults ALWAYS_PASS to "true" which
masks failures by running main || true; change the behavior so failures
propagate by making ALWAYS_PASS default to "false" (or require an explicit
opt-in) and remove the "|| true" fallback; update the conditional that checks
ALWAYS_PASS so only when it is explicitly set to "true" the script will ignore
failures, otherwise call main and let non-zero exits fail the job (reference the
ALWAYS_PASS variable and the main invocation).
- Line 5: Replace the lone "set -e" invocation at the top of the script with
"set -euo pipefail" so the script treats unset variables as errors and surfaces
failures inside pipelines (preventing silent secret-scanning misses); ensure
this change is the first non-shebang line (replace the existing set -e
occurrence) and update any places that rely on undefined environment variables
to use explicit defaults or checks if needed.
In `@integration-tests/olminstall/helpers/send_notification.py`:
- Around line 79-85: The code currently posts to whatever value is in
webhook_url/SLACK_WEBHOOK_URL without validation; update the send logic in
send_notification.py (the webhook_url variable used when constructing Request)
to validate the URL before performing the POST: parse the URL, require scheme ==
"https", and restrict the host/path to an allowlist (for example ensure netloc
endswith "hooks.slack.com" and path starts with "/services/") or validate
against a configured set of approved webhook URLs; if validation fails,
raise/return an error and do not perform the Request.
In `@integration-tests/olminstall/helpers/tekton_util.py`:
- Line 64: The allowlist regex _RH_INTERNAL_HOSTS_RE currently uses an
unanchored pattern which lets substring matches bypass TLS verification; change
it to match whole hostnames (e.g. use anchors or a fullmatch-capable pattern
like ^(?:gitlab\.cee\.redhat\.com|git\.corp\.redhat\.com)$) and update any call
sites that use .search() to use re.fullmatch() (or keep .match() with the
anchored regex) so only exact hostnames enter the sslVerify=false path; ensure
you update both uses of _RH_INTERNAL_HOSTS_RE in the module.
In `@integration-tests/olminstall/helpers/write_pipeline_test_flags.py`:
- Around line 72-86: The loop writes to env-derived paths without validating
them; update the code that iterates over flags (variables path_var, p, and
Path(p)) to enforce that p is confined to the Tekton results directory:
determine the expected results root (read from an env var like RESULTS_DIR with
a default such as "/tekton/results"), resolve both Path(p).resolve() and
expected_root.resolve(), then reject and return error if the resolved path is
not inside the expected root (use Path.is_relative_to when available or compare
pathlib.Path(expected_root).resolve() with pathlib.Path(p).resolve() via
os.path.commonpath), and only after validation write the file (optionally ensure
parent directories exist with mkdir(parents=True, exist_ok=True)). Ensure the
error message references path_var and the offending path when rejecting.
In `@integration-tests/olminstall/olminstall-pipeline.yaml`:
- Around line 151-153: The pipeline-level result TEST_OUTPUT currently points to
$(tasks.install-operator.results.INSTALL_STATUS) which is missing when
PRODUCT=none; add a small always-run propagator task (e.g., task name
result-propagator or emit-test-output) that has a conditional step: if
install-operator produced INSTALL_STATUS use that, else use the
bvt-health-checks-no-eaas result (TEST_OUTPUT) and set a single propagator
result (e.g., PROPAGATED_TEST_OUTPUT); update spec.results.TEST_OUTPUT to
reference $(tasks.result-propagator.results.PROPAGATED_TEST_OUTPUT). Ensure the
propagator task is unconditionally included in pipeline tasks and reads both
task results conditionally so the pipeline result always exists regardless of
PRODUCT path.
In
`@pipelineruns/distributed-workloads/odh-training-cuda130-torch210-py312-openmpi41-ci-on-pull-request.yaml`:
- Around line 12-16: The CEL expression under
pipelinesascode.tekton.dev/on-cel-expression uses an invalid placeholder
"$$TARGET_BRANCH$$" so the pull_request trigger will never match; update the
expression in that block so the target_branch comparison uses a valid PaC
variable or a literal branch name (e.g., replace "$$TARGET_BRANCH$$" with the
PaC template variable {{target_branch}} or with a specific branch like "main")
so the condition involving target_branch in the on-cel-expression evaluates
correctly.
---
Outside diff comments:
In `@integration-tests/olminstall/helpers/runner.py`:
- Around line 642-643: Wrap the call to self.ka.get_json(path) in a try/except
that catches KubeArchiveAuthError and falls back to retrieving live data (e.g.,
call self.ka.get_live_json(path) or equivalent), so the merged list/watch path
won’t hard-fail when archive auth expires; also log or warn when falling back
and ensure KubeArchiveAuthError is imported/available in runner.py.
---
Nitpick comments:
In `@integration-tests/olminstall/helpers/tests_config.py`:
- Around line 45-50: The subprocess.run invocation that sets proc (the call to
subprocess.run([yq_bin, "e", "-o=json", ".", str(path)], ...)) needs an explicit
timeout to avoid indefinite hangs; add a timeout parameter (e.g., timeout=30) to
that call and wrap the call in a try/except that catches
subprocess.TimeoutExpired to log/handle the timeout and fail gracefully (use the
proc variable/result handling already present after the call). Ensure you
reference subprocess.TimeoutExpired in the except block and provide a clear
error path rather than letting the test hang.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 77f8dfe2-41fb-44da-9d5f-c827768299d2
📒 Files selected for processing (39)
config/component_repo_map.jsondoc/contributing-konflux-testing-rhoai.mdgitops/opendatahub-ci-components.yamlintegration-tests/kubeflow/Dockerfile.kubeflowintegration-tests/olminstall/README.mdintegration-tests/olminstall/helpers/bvt_product_none_placeholder_junit.pyintegration-tests/olminstall/helpers/cli.pyintegration-tests/olminstall/helpers/collect_diagnostics.pyintegration-tests/olminstall/helpers/constants.pyintegration-tests/olminstall/helpers/create_eaas_cluster.pyintegration-tests/olminstall/helpers/emit_test_output.pyintegration-tests/olminstall/helpers/extract_fbcf_image.pyintegration-tests/olminstall/helpers/install_and_verify.pyintegration-tests/olminstall/helpers/kubearchive.pyintegration-tests/olminstall/helpers/prune_stale_testops_its.pyintegration-tests/olminstall/helpers/resolve_ocp_prefix.pyintegration-tests/olminstall/helpers/resolve_opendatahub_tests_image.pyintegration-tests/olminstall/helpers/run_bvt_pytest.pyintegration-tests/olminstall/helpers/runner.pyintegration-tests/olminstall/helpers/secure_push_oci_artifacts.shintegration-tests/olminstall/helpers/send_notification.pyintegration-tests/olminstall/helpers/tekton_util.pyintegration-tests/olminstall/helpers/tests_config.pyintegration-tests/olminstall/helpers/tests_plan.pyintegration-tests/olminstall/helpers/write_pipeline_test_flags.pyintegration-tests/olminstall/its-olminstall-open-data-hub-tenant.yamlintegration-tests/olminstall/its-olminstall-rhoai-tenant.yamlintegration-tests/olminstall/olm_pipeline.pyintegration-tests/olminstall/olminstall-pipeline.yamlintegration-tests/olminstall/olminstall-smoke-pipeline.yamlintegration-tests/olminstall/olminstall-tests-config.yamlintegration-tests/olminstall/requirements.txtintegration-tests/olminstall/test-pipelinerun.yamlintegration-tests/olminstall/test-snapshot.yamlintegration-tests/olminstall/verify_cli_args.pypipelineruns/distributed-workloads/odh-training-cuda130-torch210-py312-openmpi41-ci-on-pull-request.yamlpipelineruns/distributed-workloads/odh-training-cuda130-torch210-py312-openmpi41-ci-on-push.yamlpipelineruns/llama-stack-distribution/odh-llama-stack-core-pull-request.yamlpipelineruns/llama-stack-distribution/odh-llama-stack-core-push.yaml
💤 Files with no reviewable changes (1)
- integration-tests/olminstall/olminstall-smoke-pipeline.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
integration-tests/olminstall/helpers/runner.py (2)
1576-1610:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
run_watch_modeswallows neitherKubeArchiveAuthErrornor 404s the wayget_pipelinerun_json_for_displaydoes.Direct
self.ka.get_json(...)at lines 1576 and 1594-1596 (and again at 1630) bypass_ka_get_json_warn_empty. If the KubeArchive token expires betweencheck_loginand the watch lookup — easy when users run--watchon a long-running PR — the user gets a stack trace instead of the friendly "auth failed → fall back to AppError" you implemented elsewhere. Same fix shape as my replay-path comment: route these through_ka_get_json_warn_empty(or aKubeArchiveAuthError-aware wrapper) and treat an empty result the same as "not in archive".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/runner.py` around lines 1576 - 1610, run_watch_mode directly calls self.ka.get_json in several places (the checks around selecting self.pr) and can raise KubeArchiveAuthError or propagate 404s; change those direct calls to use the existing helper _ka_get_json_warn_empty (or a small wrapper that catches KubeArchiveAuthError and returns {} / None) so a missing/expired KA token or a 404 is treated the same as “not in archive” and results in the same AppError/fallback behavior already implemented in get_pipelinerun_json_for_display; update the calls at the locations using self.ka.get_json(...) (the lookup after failing oc get and the earlier archive lookup) to call _ka_get_json_warn_empty(self, path) and treat an empty/None result as not found.
1755-1808:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
KubeArchiveAuthErrornot caught during archived replay; expired token = hard crash mid-summary.You added
_ka_get_json_warn_emptyprecisely to demote auth failures to warnings, butreplay_archived_logsstill callsself.ka.get_json(...)(lines 1757, 1782, 1790) andself.ka.get_text(...)(line 1803) directly. If the token expires between login and replay (long-running pipelines easily blow past token lifetimes), the user gets an uncaughtKubeArchiveAuthErrortraceback after the Summary header has already been printed — destroying the carefully constructed UX the rest of this PR is building.Use the warn-empty helper for JSON fetches and catch the auth error around
get_text:🔒 Suggested fix
def replay_archived_logs(self) -> None: assert self.ka is not None - prj = self.ka.get_json(f"/apis/tekton.dev/v1/namespaces/{quote(self.args.namespace)}/pipelineruns/{quote(self.pr)}") + prj = self._ka_get_json_warn_empty( + f"/apis/tekton.dev/v1/namespaces/{quote(self.args.namespace)}/pipelineruns/{quote(self.pr)}", + ctx="replay archived PipelineRun", + ) @@ - pods = self.ka.get_json( + pods = self._ka_get_json_warn_empty( f"/api/v1/namespaces/{quote(self.args.namespace)}/pods?labelSelector={quote(f'tekton.dev/taskRun={tr_name}')}" - ) + , ctx=f"list pods for TaskRun {tr_name}") items = pods.get("items", []) if not items: print(f"[{task_name}] (no pod found)") continue pod = items[0].get("metadata", {}).get("name", "") - pod_obj = self.ka.get_json(f"/api/v1/namespaces/{quote(self.args.namespace)}/pods/{quote(pod)}") + pod_obj = self._ka_get_json_warn_empty( + f"/api/v1/namespaces/{quote(self.args.namespace)}/pods/{quote(pod)}", + ctx=f"get pod {pod}", + ) @@ - log_text = _normalize_replayed_pod_log(self.ka.get_text(log_path)) + try: + raw_log = self.ka.get_text(log_path) + except KubeArchiveAuthError as exc: + print(f"WARN KubeArchive auth failed reading log {log_path}: {exc}", file=sys.stderr) + raw_log = "" + log_text = _normalize_replayed_pod_log(raw_log)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/runner.py` around lines 1755 - 1808, replay_archived_logs calls self.ka.get_json/get_text directly and will crash on KubeArchiveAuthError; replace the three self.ka.get_json(...) calls inside replay_archived_logs with the helper _ka_get_json_warn_empty(...) (use its return value and treat None/empty like missing objects) and wrap the self.ka.get_text(log_path) call in a try/except catching KubeArchiveAuthError to emit a clear warning (similar to _ka_get_json_warn_empty behavior) and skip replaying that container's logs so the summary continues; reference the replay_archived_logs function, _ka_get_json_warn_empty helper, and the get_text call for locating edits.
♻️ Duplicate comments (1)
integration-tests/olminstall/helpers/run_bvt_pytest.py (1)
55-70:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftAdd timeout to prevent indefinite hangs (CWE-835: infinite loop risk).
The
_run_with_teefunction streams output in an unbounded loop without timeout protection. If pytest hangs or produces infinite output, the Tekton step blocks indefinitely. Implement a configurable timeout withproc.wait(timeout=...)and terminate/kill on expiry.⏱️ Proposed fix with timeout enforcement
-def _run_with_tee(cmd: list[str], log_path: str, *, env: dict[str, str] | None = None) -> int: - """Run *cmd*, tee stdout+stderr to *log_path*, return exit code.""" +def _run_with_tee(cmd: list[str], log_path: str, *, env: dict[str, str] | None = None, timeout: int | None = None) -> int: + """Run *cmd*, tee stdout+stderr to *log_path*, return exit code.""" + if timeout is None: + timeout = int(os.environ.get("BVT_RUN_TIMEOUT_SECS", "3600")) with open(log_path, "w") as log: proc = subprocess.Popen( cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, env=env, ) assert proc.stdout is not None - for line in proc.stdout: - sys.stdout.write(line) - log.write(line) - proc.wait() + import time + deadline = time.time() + timeout + try: + for line in proc.stdout: + if time.time() > deadline: + proc.terminate() + time.sleep(5) + if proc.poll() is None: + proc.kill() + log.write(f"\n[TIMEOUT after {timeout}s]\n") + return 124 # timeout exit code + sys.stdout.write(line) + log.write(line) + proc.wait() + except Exception: + proc.terminate() + time.sleep(2) + if proc.poll() is None: + proc.kill() + raise return proc.returncodeThen pass timeout when calling:
ec = _run_with_tee([uv, "run", "pytest", *pytest_args], log, env=env, timeout=3600)As per coding guidelines, "REVIEW PRIORITIES: ... 3. Bug-prone patterns and error handling gaps".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/run_bvt_pytest.py` around lines 55 - 70, The _run_with_tee function currently blocks indefinitely; add a configurable timeout parameter (e.g., timeout: float | None = None) to its signature and use proc.wait(timeout=timeout) instead of unconditional proc.wait(); catch subprocess.TimeoutExpired, call proc.terminate(), wait a short grace period (e.g., proc.wait(timeout=some small value) inside the exception handler), and if it still hasn’t exited call proc.kill(); ensure you set and return proc.returncode and flush/close the log in all cases so the caller (e.g., callers invoking _run_with_tee for pytest) can pass a timeout like 3600s to prevent hangs.
🧹 Nitpick comments (6)
integration-tests/olminstall/helpers/runner.py (5)
1086-1131: 💤 Low valueSnapshot custom-columns parsing breaks on names containing whitespace and silently degrades to slow path.
parts = ln.split()oncustom-columns=NAME:.metadata.name,TS:.metadata.creationTimestampassumes neither field ever contains whitespace. K8s object names can't, so today that's fine — but the same loop assignsname = parts[0]andts = parts[-1], which means if oc ever emits a wider header row or any kind of WARN/NOTE line before the data (it sometimes does on auth-refresh), you'll mis-parse exactly one row. Two quick hardenings:
- Skip lines whose
parts[-1]does not look like an ISO timestamp (re.match(r"\d{4}-\d{2}-\d{2}T", ts)).- Print a one-line WARN when
proc.stdoutis non-empty butrowsis empty, so users understand why the function fell through to the "no match" return on line 1099 instead of attributing it to "no snapshots in this app".Low-priority — only matters when something unexpected lands on stdout — but a 5-line guard prevents a confusing "no FBCF found" failure mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/runner.py` around lines 1086 - 1131, The parsing loop that builds rows (variables ln, parts, name, ts) should skip any line whose last token doesn't look like an ISO timestamp and warn when proc.stdout is non-empty but no rows were parsed: add a guard using re.match(r"\d{4}-\d{2}-\d{2}T", ts) before appending and, after the loop, if (proc.stdout or "") and not rows print a one-line WARN (including app_name) to stderr so callers know parsing was skipped and the function fell back to the slow path; update the block around rows/parts/ts/name in runner.py and keep the rest of the snapshot scanning logic (scan, max_walk, run_cmd, pattern) unchanged.
1268-1273: ⚖️ Poor tradeoffHelpful but not fatal:
yqis treated as required when many environments only have stock Python.The hard
AppErroron missingyq(line 1271) is one of two places in this runner that turn a "soft prereq" into a blocker (tknis correctly degraded to oc polling at line 1888 — good). All the edits applied here are mechanical YAML mutations on a file under your control; consider either:
- documenting
yqinREADME.mdas a hard prereq for--trigger/olm_pipeline.py, or- degrading to a pure-Python
ruamel.yaml/PyYAMLpatch path so CI runners and air-gapped dev boxes don't have to ship a second YAML CLI.Not a blocker, but the asymmetric "tkn is optional, yq is mandatory" UX surprises users who hit
yqonly on first run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/runner.py` around lines 1268 - 1273, The current ensure_its_applied method raises AppError when yq is missing, making yq a hard prereq; change it to degrade gracefully by trying a pure-Python YAML patch path when shutil.which("yq") is False (use ruamel.yaml or PyYAML to load/modify/dump the ITS YAML) and only raise AppError if both the yq path and the Python YAML libraries are unavailable, or alternatively update README to document yq as a required dependency; modify the logic in ensure_its_applied and the AppError call so the code first attempts the Python fallback before failing.
1455-1518: ⚖️ Poor tradeoffInline spinner duplicates
spin_while; reuse the context manager so KeyboardInterrupt cleanup stays in one place.You already built
spin_whilewith proper TTY detection,BrokenPipeErrorhandling, and guaranteed stop onBaseException.wait_for_pipelinerunreimplements roughly half of that (lines 1458-1518) with its ownterm/use_spinner/spin_framesblock and manual\r\033[Kcleanup, then guards the cleanup again onBaseException. Two near-identical spinners means two places future bug fixes can drift (e.g., line 1462 misses theBrokenPipeErrorswallow that line 138 has).Worth folding this into
spin_while(...)with the polling loop inside itswithblock; the only thing that needs to stay outside is the success-vs-timeout messaging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/runner.py` around lines 1455 - 1518, wait_for_pipelinerun currently reimplements spinner logic duplicated from spin_while; replace the manual term/use_spinner/spin_frames/cleanup block with the existing spin_while context manager to centralize TTY detection, BrokenPipeError swallowing, and BaseException cleanup. Concretely, remove the local term/use_spinner/spin_frames and the manual writes that manage \r\033[K and exception cleanup, create wait_stream = _interactive_progress_stream() as now, then wrap the polling loop (the for attempt... loop and the non-success dot write) inside "with spin_while(wait_stream, msg_prefix):" and keep only the success (write ok/newline) and timeout messaging outside the context; preserve finished_ok/pr assignment logic and ensure any BrokenPipeError/KeyboardInterrupt behavior is handled by spin_while, not duplicated in wait_for_pipelinerun.
1759-1775: ⚡ Quick winRe-use
_is_resolver_couldnt_get_pipelineinstead of re-implementing the predicate.Line 1771 inlines
reason == "CouldntGetPipeline" or "couldntgetpipeline" in reason.lower() or "resolver failed" in message.lower(), which is a strict subset of_is_resolver_couldnt_get_pipeline(reason, message)defined at line 570. Two definitions of the same predicate guarantee they will drift (e.g., the helper also matches"file does not exist").- if reason == "CouldntGetPipeline" or "couldntgetpipeline" in reason.lower() or "resolver failed" in message.lower(): + if self._is_resolver_couldnt_get_pipeline(reason, message): self._warn_couldnt_get_pipeline_git_source()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/runner.py` around lines 1759 - 1775, The condition that detects resolver failures is re-implemented inline; replace the inline predicate at the TaskRun-missing branch with a call to the existing helper _is_resolver_couldnt_get_pipeline(reason, message) to avoid divergence. Locate the block that computes reason/message from prj.get("status", {}).get("conditions", []) and currently checks if reason == "CouldntGetPipeline" or "couldntgetpipeline" in reason.lower() or "resolver failed" in message.lower(), and instead call _is_resolver_couldnt_get_pipeline(reason, message) and, if true, call the same _warn_couldnt_get_pipeline_git_source() path.
51-56: ⚡ Quick win
_snapshot_param_is_resource_nameaccepts garbage that will later 404 againstoc get snapshot.The guard only rejects empty strings and inline JSON (
{/[prefix). Anything else — including non-DNS-1123 strings like"../foo","snap with space", multi-megabyte blobs, or accidentally-truncated JSON like"name\":\"foo"— passes through and reachesget_snapshot_owner, which then shells outoc get snapshot <garbage>. That's just noisy log spam today (no command injection becauserun_cmduses argv lists), but it makes ownership matching brittle in busy tenants and wastes oneoccall per bogus row.Add a cheap k8s-name sanity check so only plausible resource names hit the API:
♻️ Suggested fix
+_K8S_NAME_RE = re.compile(r"^[a-z0-9]([-a-z0-9]*[a-z0-9])?$") + + def _snapshot_param_is_resource_name(snap: str) -> bool: """True when SNAPSHOT param looks like a Kubernetes object name (not inline JSON).""" s = (snap or "").strip() if not s or s[0] in "{[": return False - return True + return len(s) <= 253 and bool(_K8S_NAME_RE.fullmatch(s))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/runner.py` around lines 51 - 56, The helper _snapshot_param_is_resource_name currently only rejects blank/JSON strings and lets arbitrary garbage through; update it to perform a cheap DNS-1123 label sanity check so only plausible k8s resource names call oc: validate that snap is a non-empty string, <=63 chars, matches the DNS-1123 label pattern (e.g. regex like ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ — no spaces, no slashes, no uppercase, no leading/trailing hyphens), and return False otherwise; add an import for re if needed and keep the function name _snapshot_param_is_resource_name unchanged so callers like get_snapshot_owner keep working.integration-tests/olminstall/helpers/write_pipeline_test_flags.py (1)
56-69: ⚡ Quick winBare
except Exceptionis too greedy; importyaml.YAMLErrorand let the rest crash.Sniffing
typ.__module__ in ("yaml", "_yaml")to identify YAML failures dodges importingyaml, but it also silently swallows every other unrelated exception (KeyError, TypeError, MemoryError, KeyboardInterrupt subclasses going through Exception, etc.) under a generic "failed" message that hides the real traceback in Tekton logs. Sinceload_tests_catalogalready depends on PyYAML, prefer:♻️ Suggested refactor
-from helpers.errors import AppError +import yaml + +from helpers.errors import AppError from helpers.tests_config import compute_pipeline_result_flags, load_tests_catalog from helpers.tests_plan import parse_tests_selection, validate_and_normalize_tests_csv @@ - except PermissionError as exc: + except PermissionError as exc: print( f"ERROR: permission denied reading tests config under REPO_ROOT={repo_root!r}: {exc}", file=sys.stderr, ) return 1 - except Exception as exc: - typ = type(exc) - if typ.__module__ in ("yaml", "_yaml") and typ.__name__.endswith("Error"): - print( - f"ERROR: invalid YAML in tests config ({cfg}): {exc}. Fix indentation/quoting in the file.", - file=sys.stderr, - ) - return 1 - print( - f"ERROR: load_tests_catalog / validate_and_normalize_tests_csv / parse_tests_selection / " - f"compute_pipeline_result_flags failed: {exc}", - file=sys.stderr, - ) - return 1 + except yaml.YAMLError as exc: + print( + f"ERROR: invalid YAML in tests config ({cfg}): {exc}. Fix indentation/quoting in the file.", + file=sys.stderr, + ) + return 1Letting unrelated exceptions propagate gives a real traceback for a debuggable Tekton step; programmer errors should not be papered over as "tests config failed".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/write_pipeline_test_flags.py` around lines 56 - 69, The current bare "except Exception as exc" swallows all errors; change it to specifically catch YAML errors by importing yaml and using "except yaml.YAMLError as exc" so only YAML parsing issues trigger the user-friendly message about invalid YAML (referencing cfg), print the same stderr message and return 1, and let all other exceptions (from load_tests_catalog / validate_and_normalize_tests_csv / parse_tests_selection / compute_pipeline_result_flags) propagate so they produce full tracebacks for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration-tests/olminstall/helpers/runner.py`:
- Around line 1289-1292: The drift check in ensure_its_applied uses the constant
ITS_TESTS_PARAM_DEFAULT as the baseline for deciding to strip TESTS, but
build_olminstall_context_annotations uses the catalog-effective default via
getattr(self.args, "tests_catalog_default_csv", ITS_TESTS_PARAM_DEFAULT); make
both use the same source of truth by computing tests_baseline =
getattr(self.args, "tests_catalog_default_csv", ITS_TESTS_PARAM_DEFAULT) and
replace the hard-compare against ITS_TESTS_PARAM_DEFAULT in the TESTS removal
logic (the del_names append and the similar block around the other
ensure_its_applied section) with a comparison to tests_baseline, and update the
display string that references the default to reflect the catalog-effective
default instead of the constant.
- Around line 1445-1453: snapshot_matches_trigger currently returns True for any
inline SNAPSHOT whose application matches and whose components contain
self.image, which can false-positive across concurrent snapshots; change the
logic in snapshot_matches_trigger so that if self.image is set and
self._trigger_snapshot_spec is present you require exact equality between the
inline param and self._trigger_snapshot_spec (compare application and
components), and only when self._trigger_snapshot_spec is missing fall back to
the existing image-only heuristic (checking components for containerImage ==
self.image); keep existing early application checks and note/retain use of
self._trigger_snapshot_created_ts elsewhere for timestamp safety.
In `@integration-tests/olminstall/helpers/tekton_util.py`:
- Around line 185-210: In parse_junit_summary, stop using
xml.etree.ElementTree.parse (vulnerable to XXE) and instead use defusedxml's
safe parser: import defusedxml.ElementTree as SafeET (or similar) and call
SafeET.parse(xml_path) inside the xml_path loop, and catch SafeET.ParseError as
before; update requirements to include defusedxml so the dependency is
available. Ensure references to ElementTree.getroot()/findall() are adjusted to
use the SafeET tree object returned by SafeET.parse and keep all downstream
logic (suites loop and _safe_junit_int calls) unchanged.
---
Outside diff comments:
In `@integration-tests/olminstall/helpers/runner.py`:
- Around line 1576-1610: run_watch_mode directly calls self.ka.get_json in
several places (the checks around selecting self.pr) and can raise
KubeArchiveAuthError or propagate 404s; change those direct calls to use the
existing helper _ka_get_json_warn_empty (or a small wrapper that catches
KubeArchiveAuthError and returns {} / None) so a missing/expired KA token or a
404 is treated the same as “not in archive” and results in the same
AppError/fallback behavior already implemented in
get_pipelinerun_json_for_display; update the calls at the locations using
self.ka.get_json(...) (the lookup after failing oc get and the earlier archive
lookup) to call _ka_get_json_warn_empty(self, path) and treat an empty/None
result as not found.
- Around line 1755-1808: replay_archived_logs calls self.ka.get_json/get_text
directly and will crash on KubeArchiveAuthError; replace the three
self.ka.get_json(...) calls inside replay_archived_logs with the helper
_ka_get_json_warn_empty(...) (use its return value and treat None/empty like
missing objects) and wrap the self.ka.get_text(log_path) call in a try/except
catching KubeArchiveAuthError to emit a clear warning (similar to
_ka_get_json_warn_empty behavior) and skip replaying that container's logs so
the summary continues; reference the replay_archived_logs function,
_ka_get_json_warn_empty helper, and the get_text call for locating edits.
---
Duplicate comments:
In `@integration-tests/olminstall/helpers/run_bvt_pytest.py`:
- Around line 55-70: The _run_with_tee function currently blocks indefinitely;
add a configurable timeout parameter (e.g., timeout: float | None = None) to its
signature and use proc.wait(timeout=timeout) instead of unconditional
proc.wait(); catch subprocess.TimeoutExpired, call proc.terminate(), wait a
short grace period (e.g., proc.wait(timeout=some small value) inside the
exception handler), and if it still hasn’t exited call proc.kill(); ensure you
set and return proc.returncode and flush/close the log in all cases so the
caller (e.g., callers invoking _run_with_tee for pytest) can pass a timeout like
3600s to prevent hangs.
---
Nitpick comments:
In `@integration-tests/olminstall/helpers/runner.py`:
- Around line 1086-1131: The parsing loop that builds rows (variables ln, parts,
name, ts) should skip any line whose last token doesn't look like an ISO
timestamp and warn when proc.stdout is non-empty but no rows were parsed: add a
guard using re.match(r"\d{4}-\d{2}-\d{2}T", ts) before appending and, after the
loop, if (proc.stdout or "") and not rows print a one-line WARN (including
app_name) to stderr so callers know parsing was skipped and the function fell
back to the slow path; update the block around rows/parts/ts/name in runner.py
and keep the rest of the snapshot scanning logic (scan, max_walk, run_cmd,
pattern) unchanged.
- Around line 1268-1273: The current ensure_its_applied method raises AppError
when yq is missing, making yq a hard prereq; change it to degrade gracefully by
trying a pure-Python YAML patch path when shutil.which("yq") is False (use
ruamel.yaml or PyYAML to load/modify/dump the ITS YAML) and only raise AppError
if both the yq path and the Python YAML libraries are unavailable, or
alternatively update README to document yq as a required dependency; modify the
logic in ensure_its_applied and the AppError call so the code first attempts the
Python fallback before failing.
- Around line 1455-1518: wait_for_pipelinerun currently reimplements spinner
logic duplicated from spin_while; replace the manual
term/use_spinner/spin_frames/cleanup block with the existing spin_while context
manager to centralize TTY detection, BrokenPipeError swallowing, and
BaseException cleanup. Concretely, remove the local term/use_spinner/spin_frames
and the manual writes that manage \r\033[K and exception cleanup, create
wait_stream = _interactive_progress_stream() as now, then wrap the polling loop
(the for attempt... loop and the non-success dot write) inside "with
spin_while(wait_stream, msg_prefix):" and keep only the success (write
ok/newline) and timeout messaging outside the context; preserve finished_ok/pr
assignment logic and ensure any BrokenPipeError/KeyboardInterrupt behavior is
handled by spin_while, not duplicated in wait_for_pipelinerun.
- Around line 1759-1775: The condition that detects resolver failures is
re-implemented inline; replace the inline predicate at the TaskRun-missing
branch with a call to the existing helper
_is_resolver_couldnt_get_pipeline(reason, message) to avoid divergence. Locate
the block that computes reason/message from prj.get("status",
{}).get("conditions", []) and currently checks if reason == "CouldntGetPipeline"
or "couldntgetpipeline" in reason.lower() or "resolver failed" in
message.lower(), and instead call _is_resolver_couldnt_get_pipeline(reason,
message) and, if true, call the same _warn_couldnt_get_pipeline_git_source()
path.
- Around line 51-56: The helper _snapshot_param_is_resource_name currently only
rejects blank/JSON strings and lets arbitrary garbage through; update it to
perform a cheap DNS-1123 label sanity check so only plausible k8s resource names
call oc: validate that snap is a non-empty string, <=63 chars, matches the
DNS-1123 label pattern (e.g. regex like ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ — no
spaces, no slashes, no uppercase, no leading/trailing hyphens), and return False
otherwise; add an import for re if needed and keep the function name
_snapshot_param_is_resource_name unchanged so callers like get_snapshot_owner
keep working.
In `@integration-tests/olminstall/helpers/write_pipeline_test_flags.py`:
- Around line 56-69: The current bare "except Exception as exc" swallows all
errors; change it to specifically catch YAML errors by importing yaml and using
"except yaml.YAMLError as exc" so only YAML parsing issues trigger the
user-friendly message about invalid YAML (referencing cfg), print the same
stderr message and return 1, and let all other exceptions (from
load_tests_catalog / validate_and_normalize_tests_csv / parse_tests_selection /
compute_pipeline_result_flags) propagate so they produce full tracebacks for
debugging.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ed144577-fae5-4dc2-9dfe-7122adf837d1
📒 Files selected for processing (24)
integration-tests/olminstall/README.mdintegration-tests/olminstall/helpers/bvt_product_none_placeholder_junit.pyintegration-tests/olminstall/helpers/cli.pyintegration-tests/olminstall/helpers/collect_diagnostics.pyintegration-tests/olminstall/helpers/constants.pyintegration-tests/olminstall/helpers/create_eaas_cluster.pyintegration-tests/olminstall/helpers/extract_fbcf_image.pyintegration-tests/olminstall/helpers/install_and_verify.pyintegration-tests/olminstall/helpers/propagate_pipeline_test_output.pyintegration-tests/olminstall/helpers/run_bvt_pytest.pyintegration-tests/olminstall/helpers/runner.pyintegration-tests/olminstall/helpers/secure_push_oci_artifacts.shintegration-tests/olminstall/helpers/send_notification.pyintegration-tests/olminstall/helpers/tekton_util.pyintegration-tests/olminstall/helpers/tests_config.pyintegration-tests/olminstall/helpers/write_pipeline_test_flags.pyintegration-tests/olminstall/its-olminstall-open-data-hub-tenant.yamlintegration-tests/olminstall/its-olminstall-rhoai-tenant.yamlintegration-tests/olminstall/olm_pipeline.pyintegration-tests/olminstall/olminstall-pipeline.yamlintegration-tests/olminstall/olminstall-tests-config.yamlintegration-tests/olminstall/test-pipelinerun.yamlintegration-tests/olminstall/verify_cli_args.pypipelineruns/distributed-workloads/odh-training-cuda130-torch210-py312-openmpi41-ci-on-pull-request.yaml
✅ Files skipped from review due to trivial changes (1)
- integration-tests/olminstall/olminstall-tests-config.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- integration-tests/olminstall/olm_pipeline.py
- integration-tests/olminstall/helpers/extract_fbcf_image.py
- integration-tests/olminstall/helpers/secure_push_oci_artifacts.sh
- integration-tests/olminstall/helpers/bvt_product_none_placeholder_junit.py
- integration-tests/olminstall/its-olminstall-open-data-hub-tenant.yaml
- integration-tests/olminstall/verify_cli_args.py
- pipelineruns/distributed-workloads/odh-training-cuda130-torch210-py312-openmpi41-ci-on-pull-request.yaml
- integration-tests/olminstall/helpers/collect_diagnostics.py
- integration-tests/olminstall/helpers/cli.py
- integration-tests/olminstall/helpers/constants.py
- integration-tests/olminstall/helpers/install_and_verify.py
- integration-tests/olminstall/olminstall-pipeline.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration-tests/olminstall/helpers/run_bvt_pytest.py`:
- Around line 34-38: Reject and sanitize path-bearing values for ARTIFACT_PREFIX
and TESTS_SUBDIR to prevent path traversal: ensure ARTIFACT_PREFIX contains no
path separators or parent segments (no '/' or '\\' or '..') and TESTS_SUBDIR is
a relative single-directory fragment (not absolute and no '..' in its parts).
Update validation where ARTIFACT_PREFIX and TESTS_SUBDIR are read and before
they are used in Path joins (affecting _locate_tests_root and the code that
constructs artifact paths around the code handling ARTIFACTS_DIR), using
Path(tests_subdir).is_absolute() and checking Path(tests_subdir).parts for '..'
and similarly rejecting ARTIFACT_PREFIX if it contains os.path.sep or
os.path.altsep or '..'; raise a clear error on invalid values so joins like
Path(d, tests_subdir) and Path(ARTIFACTS_DIR) / ARTIFACT_PREFIX cannot escape
the intended directories.
- Around line 142-147: The current logic treats any exit code ec >= 2 as an
infra error and retries, but _run_with_tee returns 124 for BVT_RUN_TIMEOUT_SECS
(timeout) and those should not be retried; update the branch that checks ec (the
block using _run_with_tee, ec, retry_env and UV_NO_SYNC) to special-case the
timeout value (124/BVT_RUN_TIMEOUT_SECS) and immediately propagate/return that
exit code without retrying or falling back; keep existing retry behavior only
for infra codes >=2 that are not the timeout value and apply the same fix to the
similar block later in the file (the repeated code around lines 155-196).
In `@integration-tests/olminstall/helpers/runner.py`:
- Around line 1024-1028: The archived-branch call to self.ka.get_json in
find_newest_olminstall_any_owner_for_watch can raise on KubeArchive auth/HTTP
failures and currently aborts the whole selection; wrap the
self.ka.get_json(...) call (inside the elif self.ka_available() branch) in a
try/except that catches the network/auth/HTTP error types your KubeArchive
client raises, log or warn the failure referencing ka.get_json and the
pipelinerun name, and then skip this archived row (allowing the loop to continue
and fall back to the existing non-archived path) instead of propagating the
exception.
- Around line 1044-1059: The method find_newest_olminstall_any_owner_for_watch
currently requests a fixed number of merged rows then filters for "olminstall",
which can miss recent olminstall runs; change the call to
self._merged_pipelinerun_rows to filter for olminstall up front (e.g. pass
name_substr="olminstall") so the scan applies to olminstall candidates only,
then preserve the existing smoke-only check (olminstall_smoke_only_pipelinerun),
resolver usability check (_pipelinerun_resolver_unusable_for_logs) and fallback
logic unchanged.
In `@integration-tests/olminstall/helpers/tekton_util.py`:
- Around line 21-24: The JUnit parser uses _parse_junit_xml but when defusedxml
is present its parse raises DefusedXmlException subclasses (e.g.,
EntitiesForbidden, ExternalReferenceForbidden) which are not caught; import
DefusedXmlException from defusedxml.common when available and then update the
parsing exception handler to catch both xml.etree.ElementTree.ParseError and the
DefusedXmlException (e.g., change the except block that currently catches
ParseError to except (ParseError, _DefusedXml_Exception): or similar), so a
forbidden-XML error from _parse_junit_xml is handled gracefully and the file is
skipped instead of failing the whole summary.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1bb34042-0ea0-441e-b2cc-df644eb960fc
📒 Files selected for processing (4)
integration-tests/olminstall/helpers/run_bvt_pytest.pyintegration-tests/olminstall/helpers/runner.pyintegration-tests/olminstall/helpers/secure_push_oci_artifacts.shintegration-tests/olminstall/helpers/tekton_util.py
🚧 Files skipped from review as they are similar to previous changes (1)
- integration-tests/olminstall/helpers/secure_push_oci_artifacts.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integration-tests/olminstall/helpers/runner.py (1)
1781-1829:⚠️ Potential issue | 🟠 Major | ⚡ Quick winArchived replay still hard-fails on KubeArchive auth loss.
This path goes back to raw
self.ka.get_json()/get_text()calls. If the KubeArchive token expires after--watchselects an archived run, replay raises and exits mid-stream instead of falling back to the existing UI/reattach guidance. Route these reads through auth-safe wrappers and degrade cleanly when the archive is unavailable.As per coding guidelines, "REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/runner.py` around lines 1781 - 1829, The code is directly calling self.ka.get_json() and self.ka.get_text() (inside the archived replay path that uses _archived_pipelinerun_task_refs, the pods/pod/<name> fetch and the log fetch) which will raise on KubeArchive auth loss; replace those calls with your auth-safe wrappers (e.g. a shared helper like _ka_get_json_safe and _ka_get_text_safe or the existing auth-aware helper used elsewhere) and handle failures by printing the existing UI/reattach guidance and returning (instead of letting exceptions bubble), so replay degrades cleanly when the archive token expires.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration-tests/olminstall/helpers/run_bvt_pytest.py`:
- Around line 139-156: The code assumes ARTIFACTS_DIR exists before creating
junit/log paths and calling _run_with_tee(), causing failures on fresh paths;
fix by ensuring artifacts_dir is created (e.g.,
Path(artifacts_dir).mkdir(parents=True, exist_ok=True)) before building
junit/log filenames or invoking _run_with_tee(), referencing the artifacts_dir
variable and artifact_prefix so artifact files can be opened safely.
In `@integration-tests/olminstall/helpers/tekton_util.py`:
- Around line 127-137: The code currently only calls
_git_clone_dest_allowed(dest) when dest.exists(), allowing creation of
disallowed paths before mkdir(); move the base-path allowlist check to run
immediately after dest = Path(dest).resolve() and before any existence check or
dest.mkdir() so any resolved dest that is not allowed causes an immediate
error/exit (use the same _git_clone_dest_allowed(dest) check and error
message/exit flow), and keep the existing removal logic (shutil.rmtree) for
allowed existing dirs unchanged; update the git_clone (or the function
containing this snippet) to validate dest first and only then proceed with
existence/removal and dest.mkdir().
In `@integration-tests/olminstall/README.md`:
- Line 265: The README incorrectly states that trigger mode prints a WARN when
--konflux-repo is omitted; update run_trigger_mode() to emit a warning when
konflux_repo is not provided (i.e., when the flag/variable for --konflux-repo is
missing/empty) so behavior matches the docs, or alternatively update the README
to reflect current behavior; to implement the code change, modify
run_trigger_mode() to check the konflux_repo variable and call the existing
logger.warn/error path (same style as the existing branch-related warning) when
konflux_repo is nil/empty while mentioning --konflux-repo in the message, and
keep the existing check that warns when --konflux-branch is set without
--konflux-repo.
---
Outside diff comments:
In `@integration-tests/olminstall/helpers/runner.py`:
- Around line 1781-1829: The code is directly calling self.ka.get_json() and
self.ka.get_text() (inside the archived replay path that uses
_archived_pipelinerun_task_refs, the pods/pod/<name> fetch and the log fetch)
which will raise on KubeArchive auth loss; replace those calls with your
auth-safe wrappers (e.g. a shared helper like _ka_get_json_safe and
_ka_get_text_safe or the existing auth-aware helper used elsewhere) and handle
failures by printing the existing UI/reattach guidance and returning (instead of
letting exceptions bubble), so replay degrades cleanly when the archive token
expires.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 74e00b7f-d32a-462c-b305-ef24705bae84
📒 Files selected for processing (6)
integration-tests/olminstall/README.mdintegration-tests/olminstall/helpers/collect_diagnostics.pyintegration-tests/olminstall/helpers/run_bvt_pytest.pyintegration-tests/olminstall/helpers/runner.pyintegration-tests/olminstall/helpers/tekton_util.pyintegration-tests/olminstall/olm_pipeline.py
🚧 Files skipped from review as they are similar to previous changes (2)
- integration-tests/olminstall/olm_pipeline.py
- integration-tests/olminstall/helpers/collect_diagnostics.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration-tests/olminstall/helpers/run_bvt_pytest.py`:
- Around line 152-171: The code currently uses ARTIFACTS_DIR (artifacts_dir ->
artifacts_path) without validation which allows arbitrary absolute or traversal
targets; modify the logic around artifacts_dir/artifacts_path to validate and
constrain the resolved path before creating or writing files: after reading
ARTIFACTS_DIR, normalize with Path(artifacts_dir).resolve(), ensure it is not
outside an allowed base directory (e.g., a configured safe base like
"/artifacts" or a repo/workspace root) and disallow paths containing symlink
escapes or parent traversal, fail fast with a clear error if the resolved path
is outside the allowed directory, and only then call mkdir() and construct
junit/log paths (artifact_prefix, junit, log) from that validated
artifacts_path.
In `@integration-tests/olminstall/helpers/runner.py`:
- Around line 1471-1482: The oc annotate calls for the snapshot (invoked via
run_cmd with arguments built from self.snapshot_name, self.args.namespace,
self.run_owner and self.olminstall_context_annotate_argv()) are currently
fire-and-forget; change them to detect failures and fail fast (or retry) instead
of silently ignoring them: capture the result/exception from run_cmd, and if the
annotate returns a non-zero exit or raises, log a clear error including the
resource name/namespace and the command output and then raise an exception (or
set check=True on run_cmd) so the caller can handle it; apply the same change to
the PipelineRun annotation block (the analogous code around the 1600-1611
region) so both annotations consistently surface errors rather than being
best-effort.
In `@integration-tests/olminstall/helpers/tekton_util.py`:
- Around line 150-191: The _apply_rh_tls_workaround function currently writes a
global git config via run(["git","config","--global",
f"http.{host_origin}.sslVerify","false"]) which downgrades TLS validation for
the whole container; stop mutating global state and instead scope the workaround
to the single git subprocess that needs it. Remove the global config call in
_apply_rh_tls_workaround and either (a) return a subprocess prefix (e.g. ["git",
"-c", f"http.{host_origin}.sslVerify=false"]) or (b) return an env override to
pass only to the specific run calls (e.g. GIT_SSL_NO_VERIFY or
GIT_CONFIG_PARAMETERS) so cloning/ls-remote uses the host-scoped disabled
verification for that single invocation; update callers that perform git
operations (the places that call run(["git", "ls-remote", ...] or clone) to
prepend the returned git -c prefix or pass the returned env only to that
subprocess so no global git config is mutated.
- Around line 42-44: The helper write_result currently allows arbitrary file
writes; restrict it so callers can only write inside the Tekton results
directory by resolving the provided path and verifying it is a descendant of a
configured TEKTON_RESULTS_DIR before writing; in write_result use
Path(path).resolve() and compare against TEKTON_RESULTS_DIR.resolve() (or use
Path.is_relative_to fallback) and raise an exception for any path outside that
directory (and handle symlink resolution), then call write_text only after the
check so no arbitrary-path writes are possible.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 49a7d0ea-b5f6-471b-8df7-4618c8fb73b8
📒 Files selected for processing (9)
integration-tests/olminstall/README.mdintegration-tests/olminstall/helpers/install_and_verify.pyintegration-tests/olminstall/helpers/run_bvt_pytest.pyintegration-tests/olminstall/helpers/runner.pyintegration-tests/olminstall/helpers/tekton_util.pyintegration-tests/olminstall/helpers/write_pipeline_test_flags.pyintegration-tests/olminstall/its-olminstall-open-data-hub-tenant.yamlintegration-tests/olminstall/requirements.txtintegration-tests/olminstall/test-pipelinerun.yaml
✅ Files skipped from review due to trivial changes (1)
- integration-tests/olminstall/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- integration-tests/olminstall/test-pipelinerun.yaml
- integration-tests/olminstall/its-olminstall-open-data-hub-tenant.yaml
manosnoam
changed the title
manosnoam
changed the title
manosnoam
force-pushed
the
olminstall-bvt
branch
2 times, most recently
from
May 17, 2026 12:15
e41b636 to
afb6fbf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
integration-tests/olminstall/helpers/bvt_artifacts.py (2)
83-83: 💤 Low valueNamespace not URL-encoded in API path.
Same issue as
propagate_pipeline_test_output.py: thenamespaceis interpolated directly withouturllib.parse.quote(), inconsistent withpipelinerun_summary.py.Proposed fix
- url = f"https://{host}:{port}/apis/tekton.dev/v1/namespaces/{namespace}/taskruns?labelSelector={sel}" + url = f"https://{host}:{port}/apis/tekton.dev/v1/namespaces/{urllib.parse.quote(namespace)}/taskruns?labelSelector={sel}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/bvt_artifacts.py` at line 83, The constructed URL in bvt_artifacts.py interpolates namespace directly into url (variable url) which can break for special characters; update the code to URL-encode namespace before interpolation using urllib.parse.quote (importing urllib.parse if necessary) and then build the url with the encoded namespace (keep host, port and sel as-is) so the path matches the approach used in pipelinerun_summary.py.
19-71: ⚖️ Poor tradeoffDuplicated private helpers.
These functions duplicate implementations in
pipelinerun_summary.py. The module already imports from helpers, so consolidating these into a shared internal module would reduce drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/bvt_artifacts.py` around lines 19 - 71, The six helper implementations (_pipeline_run_name_from_env, _namespace_from_env, _in_cluster_get, _task_name, _task_reason, _result_map) are duplicated here; remove these definitions from bvt_artifacts.py and instead import the canonical implementations from the existing shared helpers module (or pipelinerun_summary helper) that the module already imports, updating the top-level imports accordingly so the file uses the single source of truth for those functions.integration-tests/olminstall/helpers/propagate_pipeline_test_output.py (2)
75-75: 💤 Low valueNamespace not URL-encoded in API path.
The
nsvariable is interpolated directly into the URL path withouturllib.parse.quote(). This is inconsistent withpipelinerun_summary.py(lines 106-108) which encodes both namespace and pipeline run name. While Kubernetes namespace naming rules are restrictive, defensive encoding is best practice.Proposed fix
- url = f"{base}/apis/tekton.dev/v1/namespaces/{ns}/taskruns?labelSelector={sel}" + url = f"{base}/apis/tekton.dev/v1/namespaces/{urllib.parse.quote(ns)}/taskruns?labelSelector={sel}"And line 157:
- pr_url = f"{base}/apis/tekton.dev/v1/namespaces/{ns}/pipelineruns/{urllib.parse.quote(pr_name)}" + pr_url = f"{base}/apis/tekton.dev/v1/namespaces/{urllib.parse.quote(ns)}/pipelineruns/{urllib.parse.quote(pr_name)}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/propagate_pipeline_test_output.py` at line 75, The URL built in the assignment to the variable url interpolates ns directly into the path (url = f"{base}/apis/tekton.dev/v1/namespaces/{ns}/taskruns?labelSelector={sel}") without URL-encoding; update this to URL-encode the namespace using urllib.parse.quote() (the same approach used in pipelinerun_summary.py for namespace and name), i.e., import urllib.parse if needed and replace ns with urllib.parse.quote(ns, safe='') so the namespace is safely encoded in the API path while keeping sel as before.
27-109: ⚖️ Poor tradeoffDuplicated helpers across module boundaries.
Functions
_in_cluster_get,_pipeline_run_name,_namespace,task_name, andresult_mapduplicate implementations inpipelinerun_summary.pyandbvt_artifacts.py. Consider importing frompipelinerun_summary(which already exportspipeline_run_name_from_env,namespace_from_env,get_pipelinerun_json,task_result,_result_map).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/propagate_pipeline_test_output.py` around lines 27 - 109, Replace the duplicated helper implementations by importing and using the shared helpers from pipelinerun_summary: import pipeline_run_name_from_env, namespace_from_env, get_pipelinerun_json, task_result (or the equivalent task_name helper) and _result_map, then remove the local definitions of _in_cluster_get, _pipeline_run_name, _namespace, task_name and result_map and update _pick_output to call pipeline_run_name_from_env (instead of _pipeline_run_name), namespace_from_env (instead of _namespace), get_pipelinerun_json (instead of _in_cluster_get) and use task_result/_result_map (instead of task_name/result_map) so the file reuses pipelinerun_summary’s implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration-tests/olminstall/helpers/create_eaas_cluster.py`:
- Around line 96-99: The current code writes sensitive kubeconfig to a
predictable /tmp/kubeconfig which can be symlink-clobbered; replace the fixed
Path("/tmp/kubeconfig") usage by creating a secure, unique temp file (e.g., via
tempfile.NamedTemporaryFile(delete=False) or os.open with
os.O_TMPFILE/os.O_EXCL) and write kubeconfig_value to that file, set its
permissions to 0o600 using file descriptor operations (os.fchmod) to avoid
TOCTOU symlink races, and assign os.environ["KUBECONFIG"] to the temp file's
path (ensure the file is properly closed and cleaned up when no longer needed).
Ensure you update references to the kubeconfig variable (the variable currently
named kubeconfig and its write_text/chmod calls) to use the secure temp-file
approach.
In `@integration-tests/olminstall/helpers/propagate_pipeline_test_output.py`:
- Around line 165-166: The code writes RESULT_PATH contents directly to disk via
result_path and Path(...).write_text, enabling path traversal; replace this by
validating and confining the target: resolve result_path with
Path(result_path).resolve(), ensure it is a child of an allowed base directory
(e.g., a dedicated results dir) by comparing resolved paths (use Path.resolve()
and is_relative_to(base.resolve()) or equivalent check), and reject or adjust
any path that points outside the base; alternatively call the existing
tekton_util.write_result() used by pipelinerun_summary.py to reuse its safe
path-confinement and write semantics instead of directly using
Path(...).write_text.
In `@integration-tests/olminstall/helpers/run_bvt_pytest.py`:
- Line 126: The console log is being truncated because _run_with_tee() opens the
artifact console file with "w" (e.g., open(log_path, "w")), wiping prior
attempts; change the open mode to "a" (append) so retries and fallbacks preserve
earlier output, and precede each run's appended chunk with a short
header/separator indicating the attempt (timestamp and attempt type) to keep
logs readable; apply the same change to every other occurrence that uses
open(..., "w") for ARTIFACT_PREFIX/*.console.log in this module.
- Around line 103-111: The helper _bvt_timeout_seconds currently returns None
for malformed or non-positive BVT_RUN_TIMEOUT_SECS, which disables the timeout;
instead, validate the environment value and reject bad input by raising a clear
exception: when reading BVT_RUN_TIMEOUT_SECS in _bvt_timeout_seconds, if the
value is empty keep existing behavior (or explicitly return None if that is
intended), but if float(raw) raises ValueError or the parsed secs is <= 0 raise
a ValueError (or a custom exception) with a message that includes the raw env
value and that it must be a positive number so callers don't silently run
without a timeout.
In `@integration-tests/olminstall/helpers/send_notification.py`:
- Around line 98-106: The code currently calls urlopen(req, ...) which follows
redirects and can bypass the webhook_host check; modify the send logic around
urlopen (the Request usage and the with urlopen(...) as resp block) to validate
the final URL after redirects by calling resp.geturl(), parse it with
urllib.parse.urlparse, and confirm the hostname equals "hooks.slack.com" (or the
same allowlist logic used for webhook_url) before reading the response or
treating the request as successful; if the final host does not match, raise an
exception or treat it as an error and do not send the payload.
In `@integration-tests/olminstall/helpers/tests_config.py`:
- Around line 31-41: The PyYAML branch can raise raw OSError from path.read_text
and yaml parsing errors from pyyaml.safe_load which should be converted to
AppError(code=2); wrap the file read and parse calls in a try/except that
catches OSError and the YAML parse error (pyyaml.YAMLError or the general
Exception from safe_load) and re-raise AppError with the original exception
message/context, then continue to validate that loaded is a dict and raise
AppError(f"Tests config root must be a mapping: {path}", 2) if not; reference
the symbols path.read_text, pyyaml.safe_load, loaded, and AppError to locate
changes.
---
Nitpick comments:
In `@integration-tests/olminstall/helpers/bvt_artifacts.py`:
- Line 83: The constructed URL in bvt_artifacts.py interpolates namespace
directly into url (variable url) which can break for special characters; update
the code to URL-encode namespace before interpolation using urllib.parse.quote
(importing urllib.parse if necessary) and then build the url with the encoded
namespace (keep host, port and sel as-is) so the path matches the approach used
in pipelinerun_summary.py.
- Around line 19-71: The six helper implementations
(_pipeline_run_name_from_env, _namespace_from_env, _in_cluster_get, _task_name,
_task_reason, _result_map) are duplicated here; remove these definitions from
bvt_artifacts.py and instead import the canonical implementations from the
existing shared helpers module (or pipelinerun_summary helper) that the module
already imports, updating the top-level imports accordingly so the file uses the
single source of truth for those functions.
In `@integration-tests/olminstall/helpers/propagate_pipeline_test_output.py`:
- Line 75: The URL built in the assignment to the variable url interpolates ns
directly into the path (url =
f"{base}/apis/tekton.dev/v1/namespaces/{ns}/taskruns?labelSelector={sel}")
without URL-encoding; update this to URL-encode the namespace using
urllib.parse.quote() (the same approach used in pipelinerun_summary.py for
namespace and name), i.e., import urllib.parse if needed and replace ns with
urllib.parse.quote(ns, safe='') so the namespace is safely encoded in the API
path while keeping sel as before.
- Around line 27-109: Replace the duplicated helper implementations by importing
and using the shared helpers from pipelinerun_summary: import
pipeline_run_name_from_env, namespace_from_env, get_pipelinerun_json,
task_result (or the equivalent task_name helper) and _result_map, then remove
the local definitions of _in_cluster_get, _pipeline_run_name, _namespace,
task_name and result_map and update _pick_output to call
pipeline_run_name_from_env (instead of _pipeline_run_name), namespace_from_env
(instead of _namespace), get_pipelinerun_json (instead of _in_cluster_get) and
use task_result/_result_map (instead of task_name/result_map) so the file reuses
pipelinerun_summary’s implementations.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 42a3dd81-bd11-4d8d-bd53-5a6172032f42
📒 Files selected for processing (36)
doc/contributing-konflux-testing-rhoai.mdintegration-tests/olminstall/README.mdintegration-tests/olminstall/helpers/bvt_artifacts.pyintegration-tests/olminstall/helpers/bvt_product_none_placeholder_junit.pyintegration-tests/olminstall/helpers/cli.pyintegration-tests/olminstall/helpers/collect_diagnostics.pyintegration-tests/olminstall/helpers/constants.pyintegration-tests/olminstall/helpers/create_eaas_cluster.pyintegration-tests/olminstall/helpers/emit_test_output.pyintegration-tests/olminstall/helpers/extract_fbcf_image.pyintegration-tests/olminstall/helpers/install_and_verify.pyintegration-tests/olminstall/helpers/kubearchive.pyintegration-tests/olminstall/helpers/patch_pipelinerun_summary.pyintegration-tests/olminstall/helpers/pipelinerun_summary.pyintegration-tests/olminstall/helpers/propagate_pipeline_test_output.pyintegration-tests/olminstall/helpers/prune_stale_testops_its.pyintegration-tests/olminstall/helpers/resolve_ocp_prefix.pyintegration-tests/olminstall/helpers/resolve_opendatahub_tests_image.pyintegration-tests/olminstall/helpers/run_bvt_pytest.pyintegration-tests/olminstall/helpers/runner.pyintegration-tests/olminstall/helpers/secure_push_oci_artifacts.shintegration-tests/olminstall/helpers/send_notification.pyintegration-tests/olminstall/helpers/tekton_util.pyintegration-tests/olminstall/helpers/tests_config.pyintegration-tests/olminstall/helpers/tests_plan.pyintegration-tests/olminstall/helpers/write_pipeline_test_flags.pyintegration-tests/olminstall/its-olminstall-open-data-hub-tenant.yamlintegration-tests/olminstall/its-olminstall-rhoai-tenant.yamlintegration-tests/olminstall/olm_pipeline.pyintegration-tests/olminstall/olminstall-pipeline.yamlintegration-tests/olminstall/olminstall-smoke-pipeline.yamlintegration-tests/olminstall/olminstall-tests-config.yamlintegration-tests/olminstall/requirements.txtintegration-tests/olminstall/test-pipelinerun.yamlintegration-tests/olminstall/test-snapshot.yamlintegration-tests/olminstall/verify_cli_args.py
💤 Files with no reviewable changes (1)
- integration-tests/olminstall/olminstall-smoke-pipeline.yaml
✅ Files skipped from review due to trivial changes (2)
- integration-tests/olminstall/test-snapshot.yaml
- integration-tests/olminstall/olm_pipeline.py
🚧 Files skipped from review as they are similar to previous changes (18)
- integration-tests/olminstall/olminstall-tests-config.yaml
- doc/contributing-konflux-testing-rhoai.md
- integration-tests/olminstall/requirements.txt
- integration-tests/olminstall/test-pipelinerun.yaml
- integration-tests/olminstall/helpers/resolve_opendatahub_tests_image.py
- integration-tests/olminstall/helpers/prune_stale_testops_its.py
- integration-tests/olminstall/helpers/emit_test_output.py
- integration-tests/olminstall/its-olminstall-open-data-hub-tenant.yaml
- integration-tests/olminstall/helpers/tests_plan.py
- integration-tests/olminstall/its-olminstall-rhoai-tenant.yaml
- integration-tests/olminstall/helpers/secure_push_oci_artifacts.sh
- integration-tests/olminstall/helpers/extract_fbcf_image.py
- integration-tests/olminstall/helpers/cli.py
- integration-tests/olminstall/verify_cli_args.py
- integration-tests/olminstall/helpers/resolve_ocp_prefix.py
- integration-tests/olminstall/helpers/write_pipeline_test_flags.py
- integration-tests/olminstall/olminstall-pipeline.yaml
- integration-tests/olminstall/helpers/collect_diagnostics.py
aloganat
reviewed
May 19, 2026
| return data | ||
|
|
||
|
|
||
| def _task_name(tr: dict[str, Any]) -> str: |
Contributor
There was a problem hiding this comment.
These three modules (bvt_artifacts.py, pipelinerun_summary.py, propagate_pipeline_test_output.py) each independently implement the same K8s API access pattern, TaskRun parsing, and result extraction logic. Consider consolidating into a shared module (e.g., tekton_util.py already exists) to avoid drift across the three copies. For example, _task_name, _result_map, pipeline_run_name_from_env, namespace_from_env, and _in_cluster_get are nearly identical across files.
Contributor
Author
There was a problem hiding this comment.
Done: Added shared helpers/tekton_incluster.py, and the three modules import it now.
| run([kube, "get", "cti", cti_name, "-o", "yaml"], check=False) | ||
| return 1 | ||
| finally: | ||
| kubeconfig.unlink(missing_ok=True) |
Contributor
There was a problem hiding this comment.
In the finally block, kubeconfig.unlink(missing_ok=True) runs unconditionally — if a subprocess spawned by run() is still reading the kubeconfig (e.g., the diagnostic get cti -o yaml on timeout), the unlink could race. Consider either waiting for all subprocesses before cleanup, or documenting that this is safe because run() uses check=False and blocks until completion.
Contributor
Author
There was a problem hiding this comment.
All run() calls block are before cleanup (No race with current code) - I added a one-line comment.
aloganat
reviewed
May 19, 2026
| return raw | ||
|
|
||
|
|
||
| _ARTIFACTS_DIR_BASES = (Path("/artifacts"),) |
Contributor
There was a problem hiding this comment.
_ARTIFACTS_DIR_BASES is hardcoded to only (Path("/artifacts"),), making local testing impossible since /artifacts typically doesn't exist outside a container. Consider adding the working directory or a TEST_ARTIFACTS_DIR env override for local development/debugging
Contributor
Author
There was a problem hiding this comment.
Done: Use TEST_ARTIFACTS_DIR (e.g. ./artifacts) plus ARTIFACTS_DIR under that root for local runs.
| ) | ||
| # ``none``: skip rhoai/odh catalog auto-pick; use ``test-snapshot.yaml`` image unless ``--image`` is set. | ||
| # Use ``--product rhoai`` or ``odh`` for full installs. | ||
| DEFAULT_PRODUCT = "none" |
Contributor
There was a problem hiding this comment.
Why it is defaulted to none ? Can we set to rhoai by default ?
Contributor
Author
There was a problem hiding this comment.
It’s intentional - safe ITS default - no EaaS/install unless --product rhoai or odh
Contributor
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)
integration-tests/olminstall/helpers/pipelinerun_summary.py (1)
184-184:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard condition item types before
.get()access.A non-dict element in
status.conditionswill raise before fallback logic runs.Proposed fix
def pipeline_succeeded_status_label(prj: dict[str, Any]) -> str: - cond = next((c for c in (prj.get("status") or {}).get("conditions", []) if c.get("type") == "Succeeded"), {}) + conds = (prj.get("status") or {}).get("conditions", []) or [] + cond = next( + (c for c in conds if isinstance(c, dict) and c.get("type") == "Succeeded"), + {}, + )As per coding guidelines, "REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/pipelinerun_summary.py` at line 184, The current generator expression that computes cond from prj.get("status") .get("conditions") assumes each condition item is a dict and calls c.get("type"), which can raise if an element is non-dict; update the filter to ensure each item is a dict before accessing .get (e.g., check isinstance(c, dict) or similar) so cond safely falls back to {} when no suitable dict with type "Succeeded" exists; modify the expression that defines cond (the generator using prj, status, conditions, and c) to include that guard.
♻️ Duplicate comments (1)
integration-tests/olminstall/helpers/send_notification.py (1)
106-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock redirects before sending webhook payload (CWE-918).
urlopen()can follow redirects for this POST, so the body may be sent to a non-allowlisted host beforefinal_urlis checked.Proposed fix
-from urllib.request import Request, urlopen +from urllib.request import Request, urlopen, build_opener, HTTPRedirectHandler @@ +class _NoRedirect(HTTPRedirectHandler): + def redirect_request(self, req, fp, code, msg, headers, newurl): + return None @@ - try: - with urlopen(req, timeout=30) as resp: - final_url = resp.geturl() - if not _slack_incoming_webhook_ok(final_url): - raise URLError( - f"Slack webhook redirect left allowlisted host (final URL: {final_url})" - ) + try: + opener = build_opener(_NoRedirect()) + with opener.open(req, timeout=30) as resp: + final_url = resp.geturl() + if not _slack_incoming_webhook_ok(final_url): + raise URLError(f"Slack webhook URL not allowlisted (final URL: {final_url})") resp.read()#!/bin/bash # Verify redirect-following behavior is still in use and no redirect-blocking opener exists. rg -n -C2 'urlopen\(|build_opener|HTTPRedirectHandler|resp.geturl\(' integration-tests/olminstall/helpers/send_notification.pyAs per coding guidelines, "REVIEW PRIORITIES: 1. Security vulnerabilities (provide severity, exploit scenario, and remediation code)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/send_notification.py` around lines 106 - 112, The code currently calls urlopen(req, ...) which can follow redirects and thus may POST the payload to a non-allowlisted host before you check final_url; change the send logic to prevent automatic redirects: build and use an opener that disables redirects (using urllib.request.HTTPRedirectHandler or a small subclass) and call opener.open(req, timeout=30) instead of urlopen so the redirect is detected/blocked before sending the body, then call resp.geturl() and validate with _slack_incoming_webhook_ok(final_url) before reading resp.read() or returning success.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration-tests/olminstall/helpers/tekton_incluster.py`:
- Around line 20-25: The helpers currently treat an existing but empty metadata
file as present and return an empty string, bypassing required=True checks;
update the logic that reads Path p (using p.is_file() and
p.read_text(...).strip()) to treat empty content as missing: after reading, if
the stripped content is empty and required is True, raise SystemExit with the
same missing-file message; apply the same change to the other helper that reads
namespace (the similar block around lines 29-37) so both pipeline_run and
namespace validate empty files as missing when required=True.
---
Outside diff comments:
In `@integration-tests/olminstall/helpers/pipelinerun_summary.py`:
- Line 184: The current generator expression that computes cond from
prj.get("status") .get("conditions") assumes each condition item is a dict and
calls c.get("type"), which can raise if an element is non-dict; update the
filter to ensure each item is a dict before accessing .get (e.g., check
isinstance(c, dict) or similar) so cond safely falls back to {} when no suitable
dict with type "Succeeded" exists; modify the expression that defines cond (the
generator using prj, status, conditions, and c) to include that guard.
---
Duplicate comments:
In `@integration-tests/olminstall/helpers/send_notification.py`:
- Around line 106-112: The code currently calls urlopen(req, ...) which can
follow redirects and thus may POST the payload to a non-allowlisted host before
you check final_url; change the send logic to prevent automatic redirects: build
and use an opener that disables redirects (using
urllib.request.HTTPRedirectHandler or a small subclass) and call
opener.open(req, timeout=30) instead of urlopen so the redirect is
detected/blocked before sending the body, then call resp.geturl() and validate
with _slack_incoming_webhook_ok(final_url) before reading resp.read() or
returning success.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3b556949-5975-451a-ac81-63fd8018673b
📒 Files selected for processing (9)
integration-tests/olminstall/helpers/bvt_artifacts.pyintegration-tests/olminstall/helpers/create_eaas_cluster.pyintegration-tests/olminstall/helpers/pipelinerun_summary.pyintegration-tests/olminstall/helpers/propagate_pipeline_test_output.pyintegration-tests/olminstall/helpers/run_bvt_pytest.pyintegration-tests/olminstall/helpers/runner.pyintegration-tests/olminstall/helpers/send_notification.pyintegration-tests/olminstall/helpers/tekton_incluster.pyintegration-tests/olminstall/helpers/tests_config.py
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
integration-tests/olminstall/helpers/tekton_incluster.py (1)
20-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
required=Trueis bypassed when metadata files exist but are empty.Line 22 and Line 31 return
""for empty files, so the required-mode checks never execute. That can silently degrade to empty PipelineRun/namespace lookups and misleading downstream output.Proposed fix
def pipeline_run_name_from_env(*, required: bool = False) -> str: @@ p = Path("/etc/tekton/pipelineRunName") if p.is_file(): - return p.read_text(encoding="utf-8").strip() + file_value = p.read_text(encoding="utf-8").strip() + if file_value: + return file_value if required: raise SystemExit("PIPELINE_RUN_NAME missing (and no /etc/tekton/pipelineRunName)") return "" @@ def namespace_from_env(*, required: bool = False) -> str: p = Path("/var/run/secrets/kubernetes.io/serviceaccount/namespace") if p.is_file(): - return p.read_text(encoding="utf-8").strip() + file_value = p.read_text(encoding="utf-8").strip() + if file_value: + return file_value v = os.environ.get("NAMESPACE", "").strip() if v: return vAs per coding guidelines, "REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps".
Also applies to: 29-37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/tekton_incluster.py` around lines 20 - 25, The current read of metadata files using Path.read_text (e.g., the Path "p" in the code that reads "/etc/tekton/pipelineRunName") treats an empty file as valid and returns "", which bypasses the required check; change the logic so that after reading and stripping the file content you treat empty string as missing when required is True — i.e., call p.read_text(...).strip(), then if the value is non-empty return it, otherwise if required raise SystemExit("PIPELINE_RUN_NAME missing (and no /etc/tekton/pipelineRunName)") and if not required return "" (apply the same pattern to the corresponding namespace reader as well).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration-tests/olminstall/helpers/create_eaas_cluster.py`:
- Around line 34-43: The timeout parsing code that converts string inputs using
int(float(...)) (referencing variables lower, s and returning fallback) only
catches ValueError and can crash on OverflowError for huge inputs; update the
exception handler to catch both ValueError and OverflowError (e.g., except
(ValueError, OverflowError): return fallback) so any overflow from
int(float(...)) returns the fallback instead of propagating.
In `@integration-tests/olminstall/helpers/pipelinerun_summary.py`:
- Around line 88-99: The code currently constructs an authenticated request to
the Kubernetes API using KUBERNETES_SERVICE_HOST/KUBERNETES_SERVICE_PORT
directly (variables host/port) and will send the serviceaccount token to any
host those env vars point to; stop using unvalidated env values and validate the
API host before using the token. In pipelinerun_summary.py, before building the
URL or calling _k8s_request, resolve and validate host (the host variable) —
e.g., perform socket.gethostbyname(host) and ensure the resolved IP is a
private/cluster address (match RFC1918 ranges like 10.0.0.0/8, 172.16.0.0/12,
192.168.0.0/16) or the well-known in-cluster DNS name
("kubernetes.default.svc"); if validation fails, return {} or raise and do not
attach the token; alternatively replace env usage with the official in-cluster
discovery (use kube client/in-cluster config) to obtain a trusted API server URL
before calling _k8s_request.
In `@integration-tests/olminstall/helpers/resolve_ocp_prefix.py`:
- Around line 33-35: The code currently trims DEFAULT_MINOR into default_minor
via require_env("DEFAULT_MINOR").strip() but does not validate emptiness,
causing compute_prefix/ write_result to receive "."; update the flow to fail
fast: after assigning default_minor, check if default_minor is empty and if so
raise or log a clear error and exit (or raise a ValueError) before calling
compute_prefix; reference the DEFAULT_MINOR environment key, the default_minor
local variable, and the compute_prefix and write_result calls so you add the
validation immediately after the require_env(...) assignment.
In `@integration-tests/olminstall/helpers/tests_plan.py`:
- Around line 13-33: The code currently allows inputs like ",,," which normalize
to an empty seen set and thus can skip all phases; after the tokenization loop
add a guard that if seen is empty you raise an AppError (same exit code 2) with
a descriptive message (e.g. "TESTS selection is empty or normalizes to zero
phases") so inputs that contain only separators are rejected; update the block
that checks seen/missing (using variables s, seen, catalog, and the existing
AppError) to perform this non-empty check before returning frozenset(seen).
In `@integration-tests/olminstall/requirements.txt`:
- Around line 3-4: The code currently falls back to the stdlib XML parser when
importing defusedxml in helpers/tekton_util.py, which reintroduces XXE/Entity
Expansion risks; remove the fail-open fallback and make the import fail-closed
by requiring defusedxml: replace the try/except that catches import errors and
imports xml.etree.ElementTree with a direct import of defusedxml (or re-raise an
ImportError/RuntimeError with a clear message to install defusedxml), and ensure
any functions that parse JUnit XML (the module-level parsing logic in
helpers/tekton_util.py) use the defusedxml ElementTree API exclusively so
missing dependency stops startup rather than silently using the unsafe stdlib
parser.
---
Duplicate comments:
In `@integration-tests/olminstall/helpers/tekton_incluster.py`:
- Around line 20-25: The current read of metadata files using Path.read_text
(e.g., the Path "p" in the code that reads "/etc/tekton/pipelineRunName") treats
an empty file as valid and returns "", which bypasses the required check; change
the logic so that after reading and stripping the file content you treat empty
string as missing when required is True — i.e., call p.read_text(...).strip(),
then if the value is non-empty return it, otherwise if required raise
SystemExit("PIPELINE_RUN_NAME missing (and no /etc/tekton/pipelineRunName)") and
if not required return "" (apply the same pattern to the corresponding namespace
reader as well).
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: afc8552e-9cb9-4d6d-a055-4f907f097233
📒 Files selected for processing (37)
doc/contributing-konflux-testing-rhoai.mdintegration-tests/olminstall/README.mdintegration-tests/olminstall/helpers/bvt_artifacts.pyintegration-tests/olminstall/helpers/bvt_product_none_placeholder_junit.pyintegration-tests/olminstall/helpers/cli.pyintegration-tests/olminstall/helpers/collect_diagnostics.pyintegration-tests/olminstall/helpers/constants.pyintegration-tests/olminstall/helpers/create_eaas_cluster.pyintegration-tests/olminstall/helpers/emit_test_output.pyintegration-tests/olminstall/helpers/extract_fbcf_image.pyintegration-tests/olminstall/helpers/install_and_verify.pyintegration-tests/olminstall/helpers/kubearchive.pyintegration-tests/olminstall/helpers/patch_pipelinerun_summary.pyintegration-tests/olminstall/helpers/pipelinerun_summary.pyintegration-tests/olminstall/helpers/propagate_pipeline_test_output.pyintegration-tests/olminstall/helpers/prune_stale_testops_its.pyintegration-tests/olminstall/helpers/resolve_ocp_prefix.pyintegration-tests/olminstall/helpers/resolve_opendatahub_tests_image.pyintegration-tests/olminstall/helpers/run_bvt_pytest.pyintegration-tests/olminstall/helpers/runner.pyintegration-tests/olminstall/helpers/secure_push_oci_artifacts.shintegration-tests/olminstall/helpers/send_notification.pyintegration-tests/olminstall/helpers/tekton_incluster.pyintegration-tests/olminstall/helpers/tekton_util.pyintegration-tests/olminstall/helpers/tests_config.pyintegration-tests/olminstall/helpers/tests_plan.pyintegration-tests/olminstall/helpers/write_pipeline_test_flags.pyintegration-tests/olminstall/its-olminstall-open-data-hub-tenant.yamlintegration-tests/olminstall/its-olminstall-rhoai-tenant.yamlintegration-tests/olminstall/olm_pipeline.pyintegration-tests/olminstall/olminstall-pipeline.yamlintegration-tests/olminstall/olminstall-smoke-pipeline.yamlintegration-tests/olminstall/olminstall-tests-config.yamlintegration-tests/olminstall/requirements.txtintegration-tests/olminstall/test-pipelinerun.yamlintegration-tests/olminstall/test-snapshot.yamlintegration-tests/olminstall/verify_cli_args.py
💤 Files with no reviewable changes (1)
- integration-tests/olminstall/olminstall-smoke-pipeline.yaml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
integration-tests/olminstall/helpers/tekton_incluster.py (1)
133-134: 💤 Low valueError message is misleading for this code path.
The condition
os.environ.get("KUBERNETES_SERVICE_HOST", "").strip()being truthy means the env var IS set. The message says "missing or not an allowed" but "missing" is impossible here—only the "not allowed" case applies.Suggested fix
- error_out.append("ERROR: KUBERNETES_SERVICE_HOST is missing or not an allowed in-cluster API host") + error_out.append("ERROR: KUBERNETES_SERVICE_HOST is not an allowed in-cluster API host")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/helpers/tekton_incluster.py` around lines 133 - 134, The error text appended in the branch that checks `if error_out is not None and base is None and os.environ.get("KUBERNETES_SERVICE_HOST", "").strip():` is misleading because the environment variable is present; update the message appended via `error_out.append(...)` to remove "missing" and clearly state that the KUBERNETES_SERVICE_HOST value is set but not an allowed in-cluster API host (refer to the `error_out` append call in tekton_incluster.py and the surrounding conditional that checks `base is None` and `os.environ.get("KUBERNETES_SERVICE_HOST", "").strip()`).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@integration-tests/olminstall/helpers/tekton_incluster.py`:
- Around line 133-134: The error text appended in the branch that checks `if
error_out is not None and base is None and
os.environ.get("KUBERNETES_SERVICE_HOST", "").strip():` is misleading because
the environment variable is present; update the message appended via
`error_out.append(...)` to remove "missing" and clearly state that the
KUBERNETES_SERVICE_HOST value is set but not an allowed in-cluster API host
(refer to the `error_out` append call in tekton_incluster.py and the surrounding
conditional that checks `base is None` and
`os.environ.get("KUBERNETES_SERVICE_HOST", "").strip()`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f79ba208-291a-4d21-a907-7bbf713cc883
📒 Files selected for processing (8)
integration-tests/olminstall/helpers/create_eaas_cluster.pyintegration-tests/olminstall/helpers/pipelinerun_summary.pyintegration-tests/olminstall/helpers/propagate_pipeline_test_output.pyintegration-tests/olminstall/helpers/resolve_ocp_prefix.pyintegration-tests/olminstall/helpers/tekton_incluster.pyintegration-tests/olminstall/helpers/tekton_util.pyintegration-tests/olminstall/helpers/tests_plan.pyintegration-tests/olminstall/requirements.txt
✅ Files skipped from review due to trivial changes (1)
- integration-tests/olminstall/requirements.txt
aloganat
approved these changes
May 20, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integration-tests/olminstall/test-pipelinerun.yaml (1)
25-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
PRODUCT=odhto keep manual run behavior aligned with ITS.This manifest now targets ODH-specific operator values but omits
PRODUCT; ITS includes it. That can exercise a different code path than the real ITS flow.Proposed fix
params: + - name: PRODUCT + value: odh - name: SNAPSHOT value: | {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/olminstall/test-pipelinerun.yaml` around lines 25 - 48, The pipeline params are missing PRODUCT=odh which causes divergence from ITS; add a new params entry with name PRODUCT and value odh alongside the existing params (e.g., after QUAY_PULL_SECRET_NAME or near TESTS) so the manifest uses the same product code path as ITS; update the params block that includes SNAPSHOT, FBCF_COMPONENT_NAME, UPDATE_CHANNEL, OPERATOR_NAMESPACE, OPERATOR_NAME, OLMINSTALL_REPO_URL, QUAY_PULL_SECRET_NAME and TESTS to include: - name: PRODUCT value: odh.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration-tests/olminstall/helpers/bvt_product_none_placeholder_junit.py`:
- Line 43: The test uses ET.indent() which requires Python 3.9+, so either
enforce that requirement or avoid the API: add a Python requirement declaration
(e.g., python_requires=">=3.9" in your packaging config such as
setup.cfg/pyproject.toml) and/or add a pipeline/container comment documenting
the image's Python version used by the tests, or replace ET.indent usage in
bvt_product_none_placeholder_junit.py with a small compatibility helper that
indents when ET.indent is unavailable; reference the ET.indent call in
bvt_product_none_placeholder_junit.py to locate where to apply the change.
In `@integration-tests/olminstall/helpers/propagate_pipeline_test_output.py`:
- Around line 80-81: Wrap the service-account file reads (the
Path(...).read_text call that assigns token and the Path(...) construct that
assigns ca) in a try/except that catches OSError and FileNotFoundError, log an
error with the exception details, and exit with a non-zero status (e.g.,
sys.exit(1) or raise SystemExit(1)) so failures are handled consistently with
the surrounding API-fetch error path; ensure you include the exception text in
the log message for debugging.
In `@integration-tests/olminstall/test-snapshot.yaml`:
- Around line 22-23: The comment examples use the placeholder token "…" making
the commands non-executable; update the comment lines that contain "python3
…/olm_pipeline.py" and "python3 …/helpers/prune_stale_testops_its.py" to use
actual repo-relative paths (for example the real relative path to
olm_pipeline.py and helpers/prune_stale_testops_its.py from the repo root or the
integration-tests directory) and confirm they are runnable with python3, so
anyone copying the examples can execute them directly.
---
Outside diff comments:
In `@integration-tests/olminstall/test-pipelinerun.yaml`:
- Around line 25-48: The pipeline params are missing PRODUCT=odh which causes
divergence from ITS; add a new params entry with name PRODUCT and value odh
alongside the existing params (e.g., after QUAY_PULL_SECRET_NAME or near TESTS)
so the manifest uses the same product code path as ITS; update the params block
that includes SNAPSHOT, FBCF_COMPONENT_NAME, UPDATE_CHANNEL, OPERATOR_NAMESPACE,
OPERATOR_NAME, OLMINSTALL_REPO_URL, QUAY_PULL_SECRET_NAME and TESTS to include:
- name: PRODUCT value: odh.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 547a621b-4bf6-4aa2-9c7e-dc7693762c06
📒 Files selected for processing (37)
doc/contributing-konflux-testing-rhoai.mdintegration-tests/olminstall/README.mdintegration-tests/olminstall/helpers/bvt_artifacts.pyintegration-tests/olminstall/helpers/bvt_product_none_placeholder_junit.pyintegration-tests/olminstall/helpers/cli.pyintegration-tests/olminstall/helpers/collect_diagnostics.pyintegration-tests/olminstall/helpers/constants.pyintegration-tests/olminstall/helpers/create_eaas_cluster.pyintegration-tests/olminstall/helpers/emit_test_output.pyintegration-tests/olminstall/helpers/extract_fbcf_image.pyintegration-tests/olminstall/helpers/install_and_verify.pyintegration-tests/olminstall/helpers/kubearchive.pyintegration-tests/olminstall/helpers/patch_pipelinerun_summary.pyintegration-tests/olminstall/helpers/pipelinerun_summary.pyintegration-tests/olminstall/helpers/propagate_pipeline_test_output.pyintegration-tests/olminstall/helpers/prune_stale_testops_its.pyintegration-tests/olminstall/helpers/resolve_ocp_prefix.pyintegration-tests/olminstall/helpers/resolve_opendatahub_tests_image.pyintegration-tests/olminstall/helpers/run_bvt_pytest.pyintegration-tests/olminstall/helpers/runner.pyintegration-tests/olminstall/helpers/secure_push_oci_artifacts.shintegration-tests/olminstall/helpers/send_notification.pyintegration-tests/olminstall/helpers/tekton_incluster.pyintegration-tests/olminstall/helpers/tekton_util.pyintegration-tests/olminstall/helpers/tests_config.pyintegration-tests/olminstall/helpers/tests_plan.pyintegration-tests/olminstall/helpers/write_pipeline_test_flags.pyintegration-tests/olminstall/its-olminstall-open-data-hub-tenant.yamlintegration-tests/olminstall/its-olminstall-rhoai-tenant.yamlintegration-tests/olminstall/olm_pipeline.pyintegration-tests/olminstall/olminstall-pipeline.yamlintegration-tests/olminstall/olminstall-smoke-pipeline.yamlintegration-tests/olminstall/olminstall-tests-config.yamlintegration-tests/olminstall/requirements.txtintegration-tests/olminstall/test-pipelinerun.yamlintegration-tests/olminstall/test-snapshot.yamlintegration-tests/olminstall/verify_cli_args.py
💤 Files with no reviewable changes (1)
- integration-tests/olminstall/olminstall-smoke-pipeline.yaml
✅ Files skipped from review due to trivial changes (1)
- doc/contributing-konflux-testing-rhoai.md
RHOAIENG-57716 - Unified olminstall Tekton pipeline (bvt/smoke/tier1) from --tests and olminstall-tests-config.yaml; replace smoke-only pipeline - olm_pipeline.py + helpers: trigger, watch, BVT pytest, KubeArchive replay, diagnostics, Konflux run summaries - Push BVT JUnit/logs to quay.io/opendatahub/odh-ci-artifacts; publish ARTIFACTS_URL for the QE artifact browser; CLI Test Results when uploaded - ODH odh-stable: rhods-operator install aligned with Jenkins - Review hardening and CLI UX (detach, summaries, artifact URL when BVT ran) - Shared Tekton in-cluster helpers; lazy/pip defusedxml for JUnit in konflux-test - Doc: contributing-konflux-testing-rhoai.md BVT and --watch cross-links
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
2 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.
RHOAIENG-57716
Implements Build Verification Tests (BVT) in the existing olminstall Konflux integration test: install ODH/RHOAI from a Snapshot on an ephemeral cluster, then run post-install checks and surface results in Konflux.
--testsand tenant config.opendatahub-testshealth checks after a successful install.olm_pipeline.py) can trigger runs, watch progress, and show Konflux / artifact links.odh-stableinstall path aligned with existing Jenkins behavior (rhods-operatormanifest).integration-tests/olminstall/README.mdanddoc/contributing-konflux-testing-rhoai.md(usage, BVT, watching runs).See integration-tests/olminstall/README.md for trigger options and examples.
Output example for running:
python ./olminstall/olm_pipeline.py --konflux-repo https://github.com/manosnoam/odh-konflux-central.git --konflux-branch add-olminstall-its --channel beta --product rhoai --version 3.5 --tests bvtIn Konflux UI:
Summary by CodeRabbit
New Features
Documentation
Improvements