Add artifact collection and finally block to MaaS pipeline#213
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change updates a Tekton PipelineRun for integration-tests/models-as-a-service. It adds PipelineRun parameters Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Security and design concerns
Only actionable issues are listed. Address these before enabling artifact uploads and automatic PR updates. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
integration-tests/models-as-a-service/pr-test-pipelinerun.yaml (1)
284-284: Redundant|| truewithonError: continue.The
|| truesuffix is unnecessary sinceonError: continuealready prevents step failure from halting the pipeline. Not harmful, but adds noise.Suggested simplification
- oc adm must-gather --dest-dir "${ARTIFACT_DIR}/gather-openshift" || true + oc adm must-gather --dest-dir "${ARTIFACT_DIR}/gather-openshift"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml` at line 284, Remove the redundant shell fallback "|| true" appended to the oc adm must-gather command since the pipeline step already specifies onError: continue; edit the step containing the oc adm must-gather invocation to delete the "|| true" suffix so the command remains "oc adm must-gather --dest-dir \"${ARTIFACT_DIR}/gather-openshift\"" and rely on the existing onError: continue behavior to avoid failing the pipeline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml`:
- Around line 83-112: The finally task share-status-on-pull-request is running
even when PR metadata is empty and causing GitHub API errors; update the task
definition to add a when condition that checks
tasks.check-prerequisites.results.pull-request-number is not an empty string
(i.e., add a when clause using input
"$(tasks.check-prerequisites.results.pull-request-number)" operator notin values
[""]). This change ensures the share-status-on-pull-request task only runs in
actual PR contexts and prevents empty fieldRef results from being written/passed
through.
---
Nitpick comments:
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml`:
- Line 284: Remove the redundant shell fallback "|| true" appended to the oc adm
must-gather command since the pipeline step already specifies onError: continue;
edit the step containing the oc adm must-gather invocation to delete the "||
true" suffix so the command remains "oc adm must-gather --dest-dir
\"${ARTIFACT_DIR}/gather-openshift\"" and rely on the existing onError: continue
behavior to avoid failing the pipeline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 68e194cb-2ad3-4bce-a096-913d1fbfad15
📒 Files selected for processing (1)
integration-tests/models-as-a-service/pr-test-pipelinerun.yaml
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml`:
- Around line 322-399: Merge the two finally tasks into a single finally task so
PR status is updated only after artifact push completes: replace the separate
finally tasks named push-ci-artifacts and update-pr-status with one finally task
(e.g., push-ci-artifacts-and-update-pr) that includes the existing steps from
push-ci-artifacts (pull-ci-artifacts, secure-push-oci,
cleanup-artifacts-from-git) in order, then add the update-pr-status step at the
end; ensure the task retains the oci-artifacts-repo param and uses
$(context.pipelineRun.name) for paths and $(tasks.status) (or the appropriate
result) when posting the PR status, and remove the standalone update-pr-status
finally task.
- Around line 395-405: The update-pr-status Task's taskSpec is missing the
artifact-browser-url param referenced by the step share-status-on-pull-request,
causing a runtime undefined parameter error; add a params entry named
artifact-browser-url (type string, with an appropriate description) to the
update-pr-status taskSpec so $(params.artifact-browser-url) is defined when the
step runs (look for taskSpec: params: and the task name/update-pr-status and the
step share-status-on-pull-request to place the new param).
- Around line 285-316: The git-push-artifacts Task step can abort the TaskRun
and hide the original e2e failure; add onError: continue to the
git-push-artifacts entry (the step named "git-push-artifacts") so artifact
staging doesn't short-circuit the pipeline, and update the
"check-test-exit-code" script to read and check both the e2e test exit code
(from steps.e2e-maas-api-openshift.exitCode.path) and the artifact staging exit
code (from steps.git-push-artifacts.exitCode.path), then fail with a clear
message and non-zero exit if either exit code is non-zero.
- Line 191: The task-level timeout (timeout: 1h) on the e2e-maas-api-openshift
Task can cancel remaining steps (including must-gather, git-push-artifacts,
check-test-exit-code) if the e2e test nears the deadline; to fix, relocate the
steps named must-gather, git-push-artifacts and check-test-exit-code out of the
main steps list and into a Task-level finally block so they always run after the
test, and give that finally block its own timeout (or alternatively increase the
Task's timeout to cover test SLA plus cleanup), ensuring onError: continue is
preserved where needed and referencing the e2e-maas-api-openshift Task and those
step names when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bd47f6e3-0ca6-4593-9999-0136e28dfc87
📒 Files selected for processing (1)
integration-tests/models-as-a-service/pr-test-pipelinerun.yaml
chaitanya1731
force-pushed
the
ck-maas-must-gather
branch
from
6f0fb68 to
b0b42a3
Compare
|
keeping this open for now. But eventually this will be covered in - #241 where we update our pipeline run definition with group testing. |
- Add oci-artifacts-repo and artifact-browser-url params - Expand check-prerequisites to emit PR metadata for PR commenting - Add onError: continue to e2e step so artifacts are collected on failure - Add ARTIFACT_DIR env var and 1h timeout to e2e task - Add MaaS-specific must-gather (quay.io/chkulkar/must-gather:maas-test-v3) and openshift must-gather collection - Add git-push-artifacts to stage artifacts and check-test-exit-code to preserve test failure - Add finally block: pull artifacts -> push OCI -> PR comment -> cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chaitanya1731
force-pushed
the
ck-maas-must-gather
branch
from
8b78972 to
72db2cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
integration-tests/models-as-a-service/pr-test-pipelinerun.yaml (2)
362-379:⚠️ Potential issue | 🔴 Critical
artifact-browser-urlis undefined in the merged finally task.Line 447 expands
$(params.artifact-browser-url), butpush-ci-artifacts-and-update-pronly passes and declaresoci-artifacts-repoandpipeline-status. Tekton will fail variable resolution before the PR comment step runs.Suggested fix
- name: push-ci-artifacts-and-update-pr params: - name: oci-artifacts-repo value: $(params.oci-artifacts-repo) + - name: artifact-browser-url + value: $(params.artifact-browser-url) - name: pipeline-status value: $(tasks.status) taskSpec: params: - name: oci-artifacts-repo description: oci-artifacts-repo + - name: artifact-browser-url + type: string + description: artifact-browser-url - name: pipeline-status type: stringAlso applies to: 429-447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml` around lines 362 - 379, The merged finally step fails because $(params.artifact-browser-url) is not declared or passed into the finally/merged task; update the pipeline to declare a pipeline-level param named artifact-browser-url (or ensure an existing param is propagated) and add that param to the taskSpec params for the final/merged task and to the task invocation for push-ci-artifacts-and-update-pr so the value flows in (similar to how oci-artifacts-repo and pipeline-status are declared/passed). Specifically, add a param entry for artifact-browser-url and pass value: $(params.artifact-browser-url) into the push-ci-artifacts-and-update-pr task invocation and its taskSpec params so Tekton can resolve the variable at runtime.
323-354:⚠️ Potential issue | 🟠 Major
git-push-artifactscan still mask the original e2e result.If artifact staging exits non-zero, Tekton stops the task before
check-test-exit-coderuns. The task then reports the staging error instead of the preserved e2e exit code, which breaks the new failure-reporting contract.Suggested fix
- name: git-push-artifacts + onError: continue ref: resolver: git params: @@ - name: check-test-exit-code image: quay.io/konflux-ci/konflux-test:stable script: | #!/bin/bash - EXIT_CODE=$(cat $(steps.e2e-maas-api-openshift.exitCode.path)) - if [ "${EXIT_CODE}" != "0" ]; then - echo "Tests failed with exit code ${EXIT_CODE}" - exit "${EXIT_CODE}" + E2E_EXIT_CODE="$(cat "$(steps.e2e-maas-api-openshift.exitCode.path)")" + if [ "${E2E_EXIT_CODE}" != "0" ]; then + echo "Tests failed with exit code ${E2E_EXIT_CODE}" + exit "${E2E_EXIT_CODE}" + fi + GIT_PUSH_EXIT_CODE="$(cat "$(steps.git-push-artifacts.exitCode.path)")" + if [ "${GIT_PUSH_EXIT_CODE}" != "0" ]; then + echo "Artifact staging failed with exit code ${GIT_PUSH_EXIT_CODE}" + exit "${GIT_PUSH_EXIT_CODE}" fiAs per coding guidelines, "REVIEW PRIORITIES: ... Bug-prone patterns and error handling gaps".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml` around lines 323 - 354, The git-push-artifacts task can fail and stop the pipeline before check-test-exit-code reads the preserved e2e exit code, masking the original test result; fix by moving or converting the git-push-artifacts step into a PipelineRun final task (Tekton finally) or otherwise running it after test-result reporting so it cannot block check-test-exit-code, e.g., relocate the git-push-artifacts definition out of the main task sequence into the PipelineRun.finally section (or ensure check-test-exit-code runs as a finally task that reads steps.e2e-maas-api-openshift.exitCode.path) so the preserved EXIT_CODE is always read and returned even if artifact staging fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml`:
- Around line 402-464: The finally block steps secure-push-oci and
share-status-on-pull-request can fail and prevent cleanup-artifacts-from-git
from running; add onError: continue to those external-call steps
(secure-push-oci and share-status-on-pull-request) so failures don’t
short-circuit finally, record their exit codes into task results (e.g.,
results.securePushExit / results.shareStatusExit) or a dedicated finalizer task
input, then add an unconditional final step that always runs (the existing
cleanup-artifacts-from-git) and, after cleanup, rethrow or fail the pipeline if
any recorded exit code indicates an error so cleanup runs but original failures
are propagated.
- Around line 324-332: The pipeline uses git resolver refs with params (url,
revision, pathInRepo) pointing to the main branch; replace the mutable revision
values with immutable commit SHAs for each git-resolved stepaction reference
(the ref blocks that contain resolver: git and params name: revision) so the
pipeline always fetches a specific commit for stepactions like secure-git-push,
git-clone, secure-push-oci and pull-request-comment; update the revision param
values in all occurrences (lines shown around the ref blocks) to the
corresponding commit SHA(s) from the rhoai-konflux-tasks repository.
---
Duplicate comments:
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml`:
- Around line 362-379: The merged finally step fails because
$(params.artifact-browser-url) is not declared or passed into the finally/merged
task; update the pipeline to declare a pipeline-level param named
artifact-browser-url (or ensure an existing param is propagated) and add that
param to the taskSpec params for the final/merged task and to the task
invocation for push-ci-artifacts-and-update-pr so the value flows in (similar to
how oci-artifacts-repo and pipeline-status are declared/passed). Specifically,
add a param entry for artifact-browser-url and pass value:
$(params.artifact-browser-url) into the push-ci-artifacts-and-update-pr task
invocation and its taskSpec params so Tekton can resolve the variable at
runtime.
- Around line 323-354: The git-push-artifacts task can fail and stop the
pipeline before check-test-exit-code reads the preserved e2e exit code, masking
the original test result; fix by moving or converting the git-push-artifacts
step into a PipelineRun final task (Tekton finally) or otherwise running it
after test-result reporting so it cannot block check-test-exit-code, e.g.,
relocate the git-push-artifacts definition out of the main task sequence into
the PipelineRun.finally section (or ensure check-test-exit-code runs as a
finally task that reads steps.e2e-maas-api-openshift.exitCode.path) so the
preserved EXIT_CODE is always read and returned even if artifact staging fails.
🪄 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: Pro
Run ID: ccf5d0f5-5aa5-4dc6-96a1-4fbedb885598
📒 Files selected for processing (1)
integration-tests/models-as-a-service/pr-test-pipelinerun.yaml
| ref: | ||
| resolver: git | ||
| params: | ||
| - name: url | ||
| value: https://github.com/red-hat-data-services/rhoai-konflux-tasks.git | ||
| - name: revision | ||
| value: main | ||
| - name: pathInRepo | ||
| value: stepactions/secure-git-push/0.1/secure-git-push.yaml |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the relevant sections
cd integration-tests/models-as-a-service || cd .
ls -la pr-test-pipelinerun.yaml 2>/dev/null || echo "File not found, searching for it..."
find . -name "pr-test-pipelinerun.yaml" -type fRepository: opendatahub-io/odh-konflux-central
Length of output: 179
🏁 Script executed:
# Examine the specific line ranges mentioned in the review
sed -n '324,332p' pr-test-pipelinerun.yaml
echo "---"
sed -n '382,411p' pr-test-pipelinerun.yaml
echo "---"
sed -n '420,428p' pr-test-pipelinerun.yamlRepository: opendatahub-io/odh-konflux-central
Length of output: 306
🏁 Script executed:
# Find the exact path of the file
find . -name "pr-test-pipelinerun.yaml" -type fRepository: opendatahub-io/odh-konflux-central
Length of output: 274
🏁 Script executed:
# Examine the specific line ranges from the correct file
echo "=== Lines 324-332 ==="
sed -n '324,332p' ./integration-tests/models-as-a-service/pr-test-pipelinerun.yaml
echo ""
echo "=== Lines 382-411 ==="
sed -n '382,411p' ./integration-tests/models-as-a-service/pr-test-pipelinerun.yaml
echo ""
echo "=== Lines 420-428 ==="
sed -n '420,428p' ./integration-tests/models-as-a-service/pr-test-pipelinerun.yamlRepository: opendatahub-io/odh-konflux-central
Length of output: 2121
🏁 Script executed:
# Search for all git resolver references with 'main' branch in this file
echo "=== All git resolver + main occurrences ==="
grep -n "resolver: git" ./integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | head -20
echo ""
echo "=== All 'main' branch references ==="
grep -n "value: main" ./integration-tests/models-as-a-service/pr-test-pipelinerun.yamlRepository: opendatahub-io/odh-konflux-central
Length of output: 887
🏁 Script executed:
# Let's get broader context around each mentioned range to understand what these stepactions do
echo "=== Context for lines 320-335 (around 324-332) ==="
sed -n '320,335p' ./integration-tests/models-as-a-service/pr-test-pipelinerun.yaml
echo ""
echo "=== Task name for first instance ==="
sed -n '310,335p' ./integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | grep -A5 "name:"Repository: opendatahub-io/odh-konflux-central
Length of output: 1652
🏁 Script executed:
# Check what credentials/volumes are mounted for these tasks
echo "=== git-push-artifacts task (lines 324-350) ==="
sed -n '324,350p' ./integration-tests/models-as-a-service/pr-test-pipelinerun.yaml
echo ""
echo "=== Check all stepactions in rhoai-konflux-tasks repo being referenced ==="
grep -B2 "stepactions/" ./integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | grep -E "value:|pathInRepo"Repository: opendatahub-io/odh-konflux-central
Length of output: 2914
🏁 Script executed:
# Check all git resolver instances from rhoai-konflux-tasks to understand full scope
echo "=== All instances of rhoai-konflux-tasks with main branch ==="
grep -B10 "rhoai-konflux-tasks" ./integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | grep -E "name:|revision|pathInRepo|value: https"Repository: opendatahub-io/odh-konflux-central
Length of output: 1305
🏁 Script executed:
# Get a comprehensive view of all git resolver + main pairs in this file
echo "=== Complete mapping of git resolver instances with rhoai-konflux-tasks and main ==="
rg -A8 "resolver: git" ./integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | rg -B5 "rhoai-konflux-tasks|main" | head -100Repository: opendatahub-io/odh-konflux-central
Length of output: 2785
Pin these git-resolved stepactions to immutable commit SHAs instead of main.
The pipeline fetches stepaction definitions from rhoai-konflux-tasks.git at main branch runtime. Since these execute secure-git-push, git-clone, secure-push-oci, and pull-request-comment tasks handling cluster credentials, registry credentials, and PR metadata, an upstream compromise or branch modification executes unreviewed code without detection. CWE-494 applies here.
Suggested fix
- value: main
+ value: <trusted-commit-sha>Affects lines 324–332, 382–411, and 420–428.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml` around lines
324 - 332, The pipeline uses git resolver refs with params (url, revision,
pathInRepo) pointing to the main branch; replace the mutable revision values
with immutable commit SHAs for each git-resolved stepaction reference (the ref
blocks that contain resolver: git and params name: revision) so the pipeline
always fetches a specific commit for stepactions like secure-git-push,
git-clone, secure-push-oci and pull-request-comment; update the revision param
values in all occurrences (lines shown around the ref blocks) to the
corresponding commit SHA(s) from the rhoai-konflux-tasks repository.
There was a problem hiding this comment.
consistent with the template given
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for template or similar pipeline files to understand the pattern
find . -name "*.yaml" -path "*/pipeline*" | head -20
echo "==="
find . -name "*template*" -name "*.yaml" | head -20
echo "==="
# Check other pipeline files to see if they also use 'main' for rhoai-konflux-tasks
grep -r "rhoai-konflux-tasks" --include="*.yaml" -l
Length of output: 2230
🏁 Script executed:
#!/bin/bash
# Check the template files for rhoai-konflux-tasks references
echo "=== Template: pr-its-pipelinerun.yaml ==="
grep -n "rhoai-konflux-tasks\|revision:\|value: main" ./integration-tests/template/pr-its-pipelinerun.yaml | head -40
echo ""
echo "=== Template: pr-group-testing-pipeline.yaml ==="
grep -n "rhoai-konflux-tasks\|revision:\|value: main" ./integration-tests/template/pr-group-testing-pipeline.yaml | head -40
echo ""
echo "=== Template: group-test-plr-template.yaml ==="
grep -n "rhoai-konflux-tasks\|revision:\|value: main" ./integration-tests/template/group-test-plr-template.yaml | head -40
echo ""
# Also check the kserve and odh-model-controller pipelines as close neighbors
echo "=== odh-model-controller pr-test-pipelinerun.yaml ==="
grep -n "rhoai-konflux-tasks\|revision:\|value: main" ./integration-tests/odh-model-controller/pr-test-pipelinerun.yaml | head -40
Length of output: 3113
@chaitanya1731, confirmed — the templates (integration-tests/template/pr-its-pipelinerun.yaml, integration-tests/template/pr-group-testing-pipeline.yaml) and sibling pipelines (e.g., integration-tests/odh-model-controller/pr-test-pipelinerun.yaml) all consistently use revision: main for rhoai-konflux-tasks references. This PR follows that established pattern.
The CWE-494 concern is a repo-wide one; if pinning to commit SHAs is ever desired, it would need to be addressed in the shared templates first. Withdrawing this finding for this PR.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| - name: secure-push-oci | ||
| ref: | ||
| resolver: git | ||
| params: | ||
| - name: url | ||
| value: https://github.com/red-hat-data-services/rhoai-konflux-tasks.git | ||
| - name: revision | ||
| value: main | ||
| - name: pathInRepo | ||
| value: stepactions/secure-push-oci/0.1/secure-push-oci.yaml | ||
| params: | ||
| - name: workdir-path | ||
| value: '/workspace/odh-ci-artifacts/test-artifacts/$(context.pipelineRun.name)' | ||
| - name: oci-ref | ||
| value: $(params.oci-artifacts-repo):$(context.pipelineRun.name) | ||
| - name: credentials-volume-name | ||
| value: odh-registry-secret-volume | ||
| - name: share-status-on-pull-request | ||
| ref: | ||
| resolver: git | ||
| params: | ||
| - name: url | ||
| value: https://github.com/red-hat-data-services/rhoai-konflux-tasks.git | ||
| - name: revision | ||
| value: main | ||
| - name: pathInRepo | ||
| value: stepactions/pull-request-comment/0.1/pull-request-comment.yaml | ||
| params: | ||
| - name: test-name | ||
| value: $(context.pipelineRun.name) | ||
| - name: oci-container | ||
| value: $(params.oci-artifacts-repo):$(context.pipelineRun.name) | ||
| - name: pipeline-aggregate-status | ||
| value: $(params.pipeline-status) | ||
| - name: pull-request-author | ||
| value: $(tasks.check-prerequisites.results.pull-request-author) | ||
| - name: pull-request-number | ||
| value: $(tasks.check-prerequisites.results.pull-request-number) | ||
| - name: git-repo | ||
| value: $(tasks.check-prerequisites.results.git-repo) | ||
| - name: git-org | ||
| value: $(tasks.check-prerequisites.results.git-org) | ||
| - name: git-revision | ||
| value: $(tasks.check-prerequisites.results.git-revision) | ||
| - name: artifact-browser-url | ||
| value: $(params.artifact-browser-url) | ||
| - name: cleanup-artifacts-from-git | ||
| ref: | ||
| resolver: git | ||
| params: | ||
| - name: url | ||
| value: https://github.com/red-hat-data-services/rhoai-konflux-tasks.git | ||
| - name: revision | ||
| value: main | ||
| - name: pathInRepo | ||
| value: stepactions/cleanup-git-repo/0.1/cleanup-git-repo.yaml | ||
| params: | ||
| - name: work-dir | ||
| value: '/workspace/odh-ci-artifacts' | ||
| - name: artifacts-dir-path | ||
| value: 'test-artifacts' | ||
| - name: dir-to-be-deleted | ||
| value: '$(context.pipelineRun.name)' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get the full context around lines 402-464 to see the parent structure
sed -n '380,475p' integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | cat -nRepository: opendatahub-io/odh-konflux-central
Length of output: 4373
🏁 Script executed:
# Get broader context to see the parent Task structure and any finally blocks
sed -n '340,500p' integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | cat -nRepository: opendatahub-io/odh-konflux-central
Length of output: 6114
🏁 Script executed:
# Check if there are any onError configurations anywhere in the file
rg 'onError' integration-tests/models-as-a-service/pr-test-pipelinerun.yamlRepository: opendatahub-io/odh-konflux-central
Length of output: 141
🌐 Web query:
Tekton PipelineRun finally block step failure behavior sequential execution
💡 Result:
In Tekton PipelineRuns, the finally block consists of tasks that execute in parallel after all tasks in the main tasks section have completed (succeeded, failed, skipped, or cancelled), regardless of their status. This execution happens sequentially from the perspective of the main pipeline flow but the finally tasks themselves run concurrently with each other—no runAfter dependencies or sequential ordering can be specified within finally. If a step within a finally task fails, that specific step fails (following standard Task step failure behavior: subsequent steps in that TaskRun are skipped unless onError: continue is set), the TaskRun fails, and since finally task failures contribute to the overall PipelineRun status, the PipelineRun will be marked as Failed if any finally task fails (even if main tasks succeeded). Finally tasks always run even if main tasks timeout (with separate timeouts.finally configurable), and results from failed main tasks may not be available in finally tasks, potentially causing skips if referenced. No sequential execution within finally block; it's parallel by design. For cleanup regardless of failures, use finally tasks with appropriate when expressions or defaults for missing results.
Citations:
- 1: https://tekton.dev/docs/pipelines/pipelines/
- 2: https://tekton.dev/docs/pipelines/pipelines
- 3: https://tekton.dev/vault/pipelines-main/pipelines/
- 4: https://github.com/tektoncd/pipeline/blob/master/docs/pipelines.md
🏁 Script executed:
# Find where onError: continue is used and show context
rg -B 5 'onError: continue' integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | head -40Repository: opendatahub-io/odh-konflux-central
Length of output: 501
Cleanup step is not guaranteed to run if upstream finally steps fail.
secure-push-oci and share-status-on-pull-request lack onError: continue in the finally block. When either fails, cleanup-artifacts-from-git is skipped, leaving /workspace/odh-ci-artifacts/test-artifacts/$(context.pipelineRun.name) stranded. Add onError: continue to external-call steps and capture their exit codes in a final step that runs unconditionally, then rethrow after cleanup executes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml` around lines
402 - 464, The finally block steps secure-push-oci and
share-status-on-pull-request can fail and prevent cleanup-artifacts-from-git
from running; add onError: continue to those external-call steps
(secure-push-oci and share-status-on-pull-request) so failures don’t
short-circuit finally, record their exit codes into task results (e.g.,
results.securePushExit / results.shareStatusExit) or a dedicated finalizer task
input, then add an unconditional final step that always runs (the existing
cleanup-artifacts-from-git) and, after cleanup, rethrow or fail the pipeline if
any recorded exit code indicates an error so cleanup runs but original failures
are propagated.
There was a problem hiding this comment.
consistent with template.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find pipeline template files in the repo
fd -e yaml -e yml template integration-tests/ --exec echo {} | head -20
# Also search for pipeline templates more broadly
fd -e yaml -e yml 'pipeline' --exec echo {} | head -20
Length of output: 1770
🏁 Script executed:
#!/bin/bash
# Check the template's finally block structure
grep -n 'finally\|onError\|secure-push-oci\|share-status\|cleanup-artifacts\|pull-request-comment' integration-tests/template/group-test-plr-template.yaml | head -40
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the full template file to understand its finally block
cat integration-tests/template/group-test-plr-template.yaml | grep -n 'finally\|onError\|push-oci\|push-ci\|cleanup\|update-pr' | head -40
# Also check the kserve group testing pipeline as another reference
grep -n 'finally\|onError\|secure-push-oci\|share-status\|cleanup-artifacts\|pull-request-comment' integration-tests/kserve/pr-group-testing-pipeline.yaml | head -40
Length of output: 584
🏁 Script executed:
#!/bin/bash
# Check the finally block in the template - look for onError around those step names
sed -n '1620,1740p' integration-tests/template/group-test-plr-template.yaml
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the line count and content of the template file
wc -l integration-tests/template/group-test-plr-template.yaml
tail -200 integration-tests/template/group-test-plr-template.yaml
Length of output: 1731
🏁 Script executed:
#!/bin/bash
# Check the kserve group testing pipeline finally block in detail
wc -l integration-tests/kserve/pr-group-testing-pipeline.yaml
grep -n 'finally\|onError\|secure-push-oci\|share-status\|cleanup-artifacts' integration-tests/kserve/pr-group-testing-pipeline.yaml
Length of output: 554
🏁 Script executed:
#!/bin/bash
# Check the finally block details in kserve pipeline
sed -n '1621,1737p' integration-tests/kserve/pr-group-testing-pipeline.yaml
Length of output: 4316
@chaitanya1731, confirmed — integration-tests/kserve/pr-group-testing-pipeline.yaml uses the exact same push-ci-artifacts-and-update-pr finally structure (pull-ci-artifacts → secure-push-oci → share-status-on-pull-request → cleanup-artifacts-from-git) without onError: continue on any of those steps. The MaaS pipeline is consistent with that established pattern. Fair enough, I'll drop the concern.
(´• ω •`) ʕ•ᴥ•ʔ
📝 Learnings were identified and not saved (knowledge base disabled). Enable
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
integration-tests/models-as-a-service/pr-test-pipelinerun.yaml (3)
402-447:⚠️ Potential issue | 🟠 MajorFinally block steps lack
onError: continue- cleanup unreliable.
secure-push-ociorshare-status-on-pull-requestfailures skipcleanup-artifacts-from-git, leaving orphaned directories inodh-build-metadatarepo. AddonError: continueto external-call steps and handle their exit codes in a final verification step.Suggested fix
- name: secure-push-oci + onError: continue ref: resolver: git ... - name: share-status-on-pull-request + onError: continue ref: resolver: gitThen add exit code checks after cleanup or accept that cleanup always runs regardless of upstream failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml` around lines 402 - 447, The final-stage steps `secure-push-oci` and `share-status-on-pull-request` can fail and prevent `cleanup-artifacts-from-git` from running; update those step specs to include `onError: continue` so cleanup always executes, and add a final verification step (e.g., `verify-exit-codes` or similar) that inspects the recorded exit codes/results of `secure-push-oci` and `share-status-on-pull-request` to decide whether to fail the PipelineRun or report errors; ensure you reference the task result fields (tasks.secure-push-oci.results / tasks.share-status-on-pull-request.results) when implementing the final check.
362-379:⚠️ Potential issue | 🔴 CriticalMissing
artifact-browser-urlparameter in taskSpec causes runtime failure.
share-status-on-pull-requestreferences$(params.artifact-browser-url)(line 447), buttaskSpec.params(lines 375-379) only declaresoci-artifacts-repoandpipeline-status. Tekton will fail to resolve this parameter at runtime.Suggested fix
- name: push-ci-artifacts-and-update-pr params: - name: oci-artifacts-repo value: $(params.oci-artifacts-repo) + - name: artifact-browser-url + value: $(params.artifact-browser-url) - name: pipeline-status value: $(tasks.status) taskSpec: ... params: - name: oci-artifacts-repo description: oci-artifacts-repo + - name: artifact-browser-url + description: URL for browsing test artifacts - name: pipeline-status type: stringAlso applies to: 446-447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml` around lines 362 - 379, The TaskSpec for share-status-on-pull-request is missing the artifact-browser-url parameter referenced by $(params.artifact-browser-url); add a new param named artifact-browser-url (type: string, with a brief description) to taskSpec.params and ensure the top-level params mapping for the TaskRun/PipelineRun includes a value entry for artifact-browser-url (so it is passed into the task), updating the same param name used by share-status-on-pull-request to avoid the runtime resolution failure.
324-332:⚠️ Potential issue | 🟠 MajorPin git-resolved stepactions to immutable commit SHAs (CWE-494).
All stepactions from
rhoai-konflux-tasksuserevision: main. These handle cluster credentials, registry secrets, and PR metadata. An upstream compromise or force-push executes unreviewed code in CI with elevated privileges.Replace each occurrence:
- value: main + value: <trusted-commit-sha>Affected stepactions:
secure-git-push,git-clone,secure-push-oci,pull-request-comment,cleanup-git-repo.Also applies to: 381-390, 402-411, 419-428, 448-457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml` around lines 324 - 332, The pipeline references git-resolver stepactions with params.revision set to "main"; replace those with immutable commit SHAs to prevent execution of unreviewed changes. For each stepaction reference (e.g., pathInRepo "stepactions/secure-git-push/0.1/secure-git-push.yaml" and the other affected stepactions: secure-git-push, git-clone, secure-push-oci, pull-request-comment, cleanup-git-repo) update the params entry named "revision" to the exact commit SHA from the rhoai-konflux-tasks repository (do this for every occurrence noted), and ensure the same SHA is used consistently for identical stepaction versions so the resolver pulls a pinned, immutable commit instead of "main".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml`:
- Around line 317-318: The must-gather invocation uses a personal registry image
quay.io/chkulkar/must-gather:maas-test-v3 which is a supply-chain risk; replace
that image reference in the oc adm must-gather command with a vetted
organization-owned image (e.g., mirror it to quay.io/opendatahub/ or
quay.io/rhoai/ and use that tag), update the YAML entry where the command
contains oc adm must-gather and the image string, ensure the new org image is
pushed with appropriate access controls and vulnerability scanning, and update
any CI documentation or image-pinning policy to reflect the organization-hosted
image.
- Around line 299-301: Remove the extraneous standalone "bash" invocation that
occurs between "set -euxo pipefail" and the
"./test/e2e/scripts/prow_run_smoke_test.sh" call; specifically delete the
solitary "bash" line so the script runs the smoke test directly (the lines to
look for are the surrounding "set -euxo pipefail" and
"./test/e2e/scripts/prow_run_smoke_test.sh").
- Around line 323-345: The git-push-artifacts task step (name:
git-push-artifacts) must not abort the pipeline on failure because that hides
upstream e2e failures; add onError: continue to the git-push-artifacts step so
the pipeline proceeds, and then modify the check-test-exit-code step to also
read and consider $(steps.git-push-artifacts.exitCode.path) when
determining/reporting test pass/fail status so artifact staging failures are
reported separately from the e2e result.
- Around line 360-464: The finally task "push-ci-artifacts-and-update-pr" runs
unconditionally and passes empty PR metadata from "check-prerequisites" into the
"share-status-on-pull-request" step in non-PR/manual runs; add the same
when/condition used elsewhere (e.g., the "provision-eaas-space" pattern) so the
finally task only executes for PR events — check and update the taskSpec/when
for "push-ci-artifacts-and-update-pr" to guard on $(params.event-type) (or
equivalent pipeline param) equaling the pull-request event value so
"share-status-on-pull-request" and other steps are skipped for non-PR runs.
---
Duplicate comments:
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml`:
- Around line 402-447: The final-stage steps `secure-push-oci` and
`share-status-on-pull-request` can fail and prevent `cleanup-artifacts-from-git`
from running; update those step specs to include `onError: continue` so cleanup
always executes, and add a final verification step (e.g., `verify-exit-codes` or
similar) that inspects the recorded exit codes/results of `secure-push-oci` and
`share-status-on-pull-request` to decide whether to fail the PipelineRun or
report errors; ensure you reference the task result fields
(tasks.secure-push-oci.results / tasks.share-status-on-pull-request.results)
when implementing the final check.
- Around line 362-379: The TaskSpec for share-status-on-pull-request is missing
the artifact-browser-url parameter referenced by $(params.artifact-browser-url);
add a new param named artifact-browser-url (type: string, with a brief
description) to taskSpec.params and ensure the top-level params mapping for the
TaskRun/PipelineRun includes a value entry for artifact-browser-url (so it is
passed into the task), updating the same param name used by
share-status-on-pull-request to avoid the runtime resolution failure.
- Around line 324-332: The pipeline references git-resolver stepactions with
params.revision set to "main"; replace those with immutable commit SHAs to
prevent execution of unreviewed changes. For each stepaction reference (e.g.,
pathInRepo "stepactions/secure-git-push/0.1/secure-git-push.yaml" and the other
affected stepactions: secure-git-push, git-clone, secure-push-oci,
pull-request-comment, cleanup-git-repo) update the params entry named "revision"
to the exact commit SHA from the rhoai-konflux-tasks repository (do this for
every occurrence noted), and ensure the same SHA is used consistently for
identical stepaction versions so the resolver pulls a pinned, immutable commit
instead of "main".
🪄 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: Pro
Run ID: b539b01b-c13d-4cd2-9580-ae4c506b47a1
📒 Files selected for processing (1)
integration-tests/models-as-a-service/pr-test-pipelinerun.yaml
| set -euxo pipefail | ||
| bash | ||
| ./test/e2e/scripts/prow_run_smoke_test.sh |
There was a problem hiding this comment.
Extraneous bash command in script.
Line 300 invokes bash with no arguments, which spawns an interactive shell that immediately exits (no stdin). This is a no-op that appears to be a debug artifact or copy-paste error.
script: |
set -euxo pipefail
- bash
./test/e2e/scripts/prow_run_smoke_test.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml` around lines
299 - 301, Remove the extraneous standalone "bash" invocation that occurs
between "set -euxo pipefail" and the "./test/e2e/scripts/prow_run_smoke_test.sh"
call; specifically delete the solitary "bash" line so the script runs the smoke
test directly (the lines to look for are the surrounding "set -euxo pipefail"
and "./test/e2e/scripts/prow_run_smoke_test.sh").
| mkdir -p "${ARTIFACT_DIR}/gather-models-as-a-service" | ||
| oc adm must-gather --image=quay.io/chkulkar/must-gather:maas-test-v3 --dest-dir "${ARTIFACT_DIR}/gather-models-as-a-service" -- "export COMPONENT=maas; /usr/bin/gather" |
There was a problem hiding this comment.
Supply chain risk: must-gather image from personal account (CWE-829).
quay.io/chkulkar/must-gather:maas-test-v3 is from a personal Quay account, not an organization-controlled registry. Compromised personal credentials could inject malicious code into CI. Move this image to an organization registry (quay.io/opendatahub/ or quay.io/rhoai/) with proper access controls and vulnerability scanning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml` around lines
317 - 318, The must-gather invocation uses a personal registry image
quay.io/chkulkar/must-gather:maas-test-v3 which is a supply-chain risk; replace
that image reference in the oc adm must-gather command with a vetted
organization-owned image (e.g., mirror it to quay.io/opendatahub/ or
quay.io/rhoai/ and use that tag), update the YAML entry where the command
contains oc adm must-gather and the image string, ensure the new org image is
pushed with appropriate access controls and vulnerability scanning, and update
any CI documentation or image-pinning policy to reflect the organization-hosted
image.
There was a problem hiding this comment.
that is the latest build of must-gather. we are only verifying to test this works
There was a problem hiding this comment.
@chaitanya1731, understood — this is a temporary image for validating the pipeline flow. That said, the risk still applies before this merges to main: a personal account image in a pipeline on the default branch is a persistent supply-chain vector regardless of intent (CWE-829). Even for the test phase, consider:
- Pin by digest (
quay.io/chkulkar/must-gather@sha256:<digest>) to at least prevent tag mutation attacks while the image lives in the personal repo. - Before merging, mirror the image to
quay.io/opendatahub/orquay.io/rhoai/and update the reference. Per PR#241's scope, this should be done as part of that broader pipeline update at the latest.
(´･ω･`)
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| - name: git-push-artifacts | ||
| ref: | ||
| resolver: git | ||
| params: | ||
| - name: url | ||
| value: https://github.com/red-hat-data-services/rhoai-konflux-tasks.git | ||
| - name: revision | ||
| value: main | ||
| - name: pathInRepo | ||
| value: stepactions/secure-git-push/0.1/secure-git-push.yaml | ||
| params: | ||
| - name: source-path | ||
| value: /workspace/artifacts-dir | ||
| - name: repo-path | ||
| value: 'opendatahub-io/odh-build-metadata' | ||
| - name: repo-branch | ||
| value: 'ci-artifacts' | ||
| - name: sparse-file-path | ||
| value: 'test-artifacts/docs' | ||
| - name: dest-path | ||
| value: 'test-artifacts/$(context.pipelineRun.name)' | ||
| - name: pipelinerun-name | ||
| value: $(context.pipelineRun.name) |
There was a problem hiding this comment.
git-push-artifacts lacks onError: continue - can hide original e2e failure.
If git-push-artifacts fails, Tekton halts before check-test-exit-code executes. The test failure becomes unreported; the pipeline failure is attributed to artifact staging instead.
Suggested fix
- name: git-push-artifacts
+ onError: continue
ref:
resolver: gitThen update check-test-exit-code to also check $(steps.git-push-artifacts.exitCode.path) after reporting the e2e result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml` around lines
323 - 345, The git-push-artifacts task step (name: git-push-artifacts) must not
abort the pipeline on failure because that hides upstream e2e failures; add
onError: continue to the git-push-artifacts step so the pipeline proceeds, and
then modify the check-test-exit-code step to also read and consider
$(steps.git-push-artifacts.exitCode.path) when determining/reporting test
pass/fail status so artifact staging failures are reported separately from the
e2e result.
| finally: | ||
| - name: push-ci-artifacts-and-update-pr | ||
| params: | ||
| - name: oci-artifacts-repo | ||
| value: $(params.oci-artifacts-repo) | ||
| - name: pipeline-status | ||
| value: $(tasks.status) | ||
| taskSpec: | ||
| volumes: | ||
| - name: odh-registry-secret-volume | ||
| secret: | ||
| secretName: odh-registry-secret | ||
| items: | ||
| - key: .dockerconfigjson | ||
| path: oci-storage-dockerconfigjson | ||
| params: | ||
| - name: oci-artifacts-repo | ||
| description: oci-artifacts-repo | ||
| - name: pipeline-status | ||
| type: string | ||
| steps: | ||
| - name: pull-ci-artifacts | ||
| ref: | ||
| resolver: git | ||
| params: | ||
| - name: url | ||
| value: https://github.com/red-hat-data-services/rhoai-konflux-tasks.git | ||
| - name: revision | ||
| value: main | ||
| - name: pathInRepo | ||
| value: stepactions/git-clone/0.1/git-clone.yaml | ||
| params: | ||
| - name: repo-path | ||
| value: 'opendatahub-io/odh-build-metadata' | ||
| - name: repo-branch | ||
| value: 'ci-artifacts' | ||
| - name: sparse-file-path | ||
| value: 'test-artifacts/$(context.pipelineRun.name)' | ||
| - name: dest-path | ||
| value: /workspace/odh-ci-artifacts | ||
| - name: artifacts-dir-path | ||
| value: 'test-artifacts/$(context.pipelineRun.name)' | ||
| - name: secure-push-oci | ||
| ref: | ||
| resolver: git | ||
| params: | ||
| - name: url | ||
| value: https://github.com/red-hat-data-services/rhoai-konflux-tasks.git | ||
| - name: revision | ||
| value: main | ||
| - name: pathInRepo | ||
| value: stepactions/secure-push-oci/0.1/secure-push-oci.yaml | ||
| params: | ||
| - name: workdir-path | ||
| value: '/workspace/odh-ci-artifacts/test-artifacts/$(context.pipelineRun.name)' | ||
| - name: oci-ref | ||
| value: $(params.oci-artifacts-repo):$(context.pipelineRun.name) | ||
| - name: credentials-volume-name | ||
| value: odh-registry-secret-volume | ||
| - name: share-status-on-pull-request | ||
| ref: | ||
| resolver: git | ||
| params: | ||
| - name: url | ||
| value: https://github.com/red-hat-data-services/rhoai-konflux-tasks.git | ||
| - name: revision | ||
| value: main | ||
| - name: pathInRepo | ||
| value: stepactions/pull-request-comment/0.1/pull-request-comment.yaml | ||
| params: | ||
| - name: test-name | ||
| value: $(context.pipelineRun.name) | ||
| - name: oci-container | ||
| value: $(params.oci-artifacts-repo):$(context.pipelineRun.name) | ||
| - name: pipeline-aggregate-status | ||
| value: $(params.pipeline-status) | ||
| - name: pull-request-author | ||
| value: $(tasks.check-prerequisites.results.pull-request-author) | ||
| - name: pull-request-number | ||
| value: $(tasks.check-prerequisites.results.pull-request-number) | ||
| - name: git-repo | ||
| value: $(tasks.check-prerequisites.results.git-repo) | ||
| - name: git-org | ||
| value: $(tasks.check-prerequisites.results.git-org) | ||
| - name: git-revision | ||
| value: $(tasks.check-prerequisites.results.git-revision) | ||
| - name: artifact-browser-url | ||
| value: $(params.artifact-browser-url) | ||
| - name: cleanup-artifacts-from-git | ||
| ref: | ||
| resolver: git | ||
| params: | ||
| - name: url | ||
| value: https://github.com/red-hat-data-services/rhoai-konflux-tasks.git | ||
| - name: revision | ||
| value: main | ||
| - name: pathInRepo | ||
| value: stepactions/cleanup-git-repo/0.1/cleanup-git-repo.yaml | ||
| params: | ||
| - name: work-dir | ||
| value: '/workspace/odh-ci-artifacts' | ||
| - name: artifacts-dir-path | ||
| value: 'test-artifacts' | ||
| - name: dir-to-be-deleted | ||
| value: '$(context.pipelineRun.name)' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if pull-request-comment stepaction handles empty PR metadata
rg -A 30 "name: pull-request-comment" --type yaml | head -60Repository: opendatahub-io/odh-konflux-central
Length of output: 60
🏁 Script executed:
# Find the referenced template file
fd -type f "pr-its-pipelinerun.yaml" .Repository: opendatahub-io/odh-konflux-central
Length of output: 248
🏁 Script executed:
# Read the full context around the finally task to check for when conditions
cat -n integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | sed -n '350,470p'Repository: opendatahub-io/odh-konflux-central
Length of output: 5605
🏁 Script executed:
# Find and check the check-prerequisites task definition
rg -B 5 "name: check-prerequisites" integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | head -50Repository: opendatahub-io/odh-konflux-central
Length of output: 441
🏁 Script executed:
# Find the full check-prerequisites task definition
rg -A 100 "- name: check-prerequisites" integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | head -120Repository: opendatahub-io/odh-konflux-central
Length of output: 482
🏁 Script executed:
# Search for the referenced pr-its-pipelinerun.yaml file
find . -name "pr-its-pipelinerun.yaml" -type fRepository: opendatahub-io/odh-konflux-central
Length of output: 130
🏁 Script executed:
# Look for other finally blocks in the repository to see how they handle PR context
rg -B 2 "^ finally:" integration-tests/ --type yaml -A 5 | head -80Repository: opendatahub-io/odh-konflux-central
Length of output: 2912
🏁 Script executed:
# Read the pr-its-pipelinerun.yaml to check for when conditions on finally task
grep -n "finally:" -A 100 ./integration-tests/template/pr-its-pipelinerun.yaml | head -120Repository: opendatahub-io/odh-konflux-central
Length of output: 4636
🏁 Script executed:
# Search for check-prerequisites task definition in the file
grep -n "name: check-prerequisites" -A 80 integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | head -100Repository: opendatahub-io/odh-konflux-central
Length of output: 3543
🏁 Script executed:
# Check if check-prerequisites returns results conditionally
grep -B 5 -A 5 "results:" integration-tests/models-as-a-service/pr-test-pipelinerun.yaml | head -80Repository: opendatahub-io/odh-konflux-central
Length of output: 1211
🏁 Script executed:
# Check if odh-model-controller has a when condition on its finally task
grep -n "finally:" -A 50 integration-tests/odh-model-controller/pr-test-pipelinerun.yaml | head -60Repository: opendatahub-io/odh-konflux-central
Length of output: 2108
🏁 Script executed:
# Check trainer pipeline finally task
grep -n "finally:" -A 50 integration-tests/trainer/pr-testing-pipeline.yaml | head -60Repository: opendatahub-io/odh-konflux-central
Length of output: 2108
🏁 Script executed:
# Check if there's any documentation or stepaction definition for pull-request-comment in the repo
find . -name "*pull-request-comment*" -type fRepository: opendatahub-io/odh-konflux-central
Length of output: 60
🏁 Script executed:
# Check if any finally task in the repo has when conditions
grep -B 2 "finally:" -A 20 integration-tests/ --include="*.yaml" | grep -A 15 "finally:" | grep "when:" -A 3Repository: opendatahub-io/odh-konflux-central
Length of output: 118
🏁 Script executed:
# Check if any finally task in the repo has when conditions
rg "finally:" -A 25 integration-tests/ | grep -A 25 "finally:" | grep "when:"Repository: opendatahub-io/odh-konflux-central
Length of output: 60
🏁 Script executed:
# Look at the actual structure - check if finally tasks in any pipelinerun have when conditions
rg "^\s+when:" integration-tests/ -B 10 | grep -B 10 "finally"Repository: opendatahub-io/odh-konflux-central
Length of output: 60
🏁 Script executed:
# Check the actual check-prerequisites task in odh-model-controller to see if it has different handling
grep -n "name: check-prerequisites" -A 80 integration-tests/odh-model-controller/pr-test-pipelinerun.yaml | head -100Repository: opendatahub-io/odh-konflux-central
Length of output: 3552
Add when condition to finally task to skip in non-PR contexts.
The finally task executes unconditionally and will pass empty PR metadata to share-status-on-pull-request during manual triggers or non-PAC webhook events. The check-prerequisites task populates results (lines 437-445) from PAC labels/annotations that are absent in non-PR contexts, resulting in empty strings. Other tasks in the pipeline conditionally skip based on event-type (e.g., provision-eaas-space); apply the same pattern to the finally task to avoid unnecessary API calls or failures with missing PR context. The same pattern exists in integration-tests/template/pr-its-pipelinerun.yaml.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration-tests/models-as-a-service/pr-test-pipelinerun.yaml` around lines
360 - 464, The finally task "push-ci-artifacts-and-update-pr" runs
unconditionally and passes empty PR metadata from "check-prerequisites" into the
"share-status-on-pull-request" step in non-PR/manual runs; add the same
when/condition used elsewhere (e.g., the "provision-eaas-space" pattern) so the
finally task only executes for PR events — check and update the taskSpec/when
for "push-ci-artifacts-and-update-pr" to guard on $(params.event-type) (or
equivalent pipeline param) equaling the pull-request event value so
"share-status-on-pull-request" and other steps are skipped for non-PR runs.
2 participants
summary
Add artifact collection,
must-gatherto the MaaS PR pipeline via afinallyblock.description
oci-artifacts-repo,artifact-browser-url.check-prerequisitesnow emits PR metadata (author, number, repo, org, revision) for commenting.ARTIFACT_DIR; e2e +must-gatheruseonError: continueso artifacts run on failure;git-push-artifactsstages toci-artifacts;check-test-exit-codepreserves e2e failure.finally: pull staged artifacts → push OCI → PR comment with links → cleanup git staging.e2e-maas-api-openshifttask timeout set to1h.Defaults keep parameter behavior compatible; failed runs still get artifacts and
finally, while the task fails on non-zero e2e.how it was tested
must-gather+finally.Merge criteria:
Summary by CodeRabbit
New Features
Chores