fix(KFLUXSPRT-8173): use taskGitRevision SHA for pipeline metadata by seanconroy2021 · Pull Request #2314 · konflux-ci/release-service-catalog

seanconroy2021 · 2026-06-23T10:28:10Z

Describe your changes

The operator already resolves the pipeline ref revision to a SHA via ResolveBranchToSHA and passes it as taskGitRevision. Use that directly instead of calling the GitHub API. Fail back to curl-with-retry if taskGitRevision is not a SHA. The curl will not fail the task since it is only metadata.

Relevant Jira

https://redhat.atlassian.net/browse/KFLUXSPRT-8173
https://redhat-internal.slack.com/archives/C04PZ7H0VA8/p1781612224952519?thread_ts=1781610891.776909&cid=C04PZ7H0VA8

Checklist before requesting a review

I have marked as draft or added do not merge label if there's a dependency PR
- If you want reviews on your draft PR, you can add reviewers or add the release-service-maintainers handle if you are unsure who to tag
My commit message includes Signed-off-by: My name <email>
I read CONTRIBUTING.MD and commit formatting
I have run the README.md generator script in .github/scripts/readme_generator.sh and verified the results using .github/scripts/check_readme.sh
If an AI agent was used, I marked that via a commit footer like Assisted-By: Cursor

The operator already resolves the pipeline ref revision to a SHA via ResolveBranchToSHA and passes it as taskGitRevision. Use that directly instead of calling the GitHub API. Fail back to curl-with-retry if taskGitRevision is not a SHA. The curl will not fail the task since it is only metadata. Signed-off-by: Sean Conroy <sconroy@redhat.com>

seanconroy2021 · 2026-06-23T10:41:23Z

/ok-to-test

qodo-app-for-konflux-ci · 2026-06-23T10:42:33Z

PR Reviewer Guide 🔍

Warning

/review is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Robustness Validate the SHA detection and fallback behavior. The regex only matches 40 lowercase hex characters; if upstream ever provides uppercase SHA, short SHA, or includes whitespace/newlines, the logic will incorrectly call the GitHub API. Also consider explicitly handling an empty/invalid API response so `sha` deterministically becomes `unknown`. # taskGitRevision is resolved to a SHA by the release-service operator, # fall back to the revision from the pipeline ref if it is not a SHA sha="$(params.taskGitRevision)" if [[ ! "${sha}" =~ ^[0-9a-f]{40}$ ]]; then sha=$(curl-with-retry -s "https://api.github.com/repos/${org}/${repo}/commits/${revision}" \ \| jq -r '.sha // ""' \|\| true) fi Test Reliability The mock uses a pattern match on the full argument string to simulate rate-limit failures. This could accidentally match other URLs/args and make tests flaky; consider matching more precisely on the URL host/path or a dedicated flag argument. curl-with-retry() { echo "Mock curl-with-retry called with:" "$@" >&2 echo "$@" >> "$(params.dataDir)/mock_curl.txt" if [[ "$*" =~ fail-catalog ]]; then echo "API rate limit exceeded" >&2 return 1 fi
📚 Focus areas based on broader codebase context Metadata Mismatch The fallback GitHub API lookup uses `revision` extracted from the `pipelineRef` (`commits/${revision}`), but the rest of the system treats `$(params.taskGitRevision)` as the authoritative git `revision` value to resolve tasks. If `pipelineRef.params.revision` and `params.taskGitRevision` ever diverge, the recorded `sha` metadata may not correspond to the revision actually used to resolve task content. Consider using `$(params.taskGitRevision)` in the API URL when it is not already a SHA. (Ref 2, Ref 1) # taskGitRevision is resolved to a SHA by the release-service operator, # fall back to the revision from the pipeline ref if it is not a SHA sha="$(params.taskGitRevision)" if [[ ! "${sha}" =~ ^[0-9a-f]{40}$ ]]; then sha=$(curl-with-retry -s "https://api.github.com/repos/${org}/${repo}/commits/${revision}" \ \| jq -r '.sha // ""' \|\| true) fi Reference reasoning: Existing pipelines consistently pass `$(params.taskGitRevision)` as the `revision` parameter to the `git` resolver, and the parameter is documented as the revision used for `taskGitUrl`. That implies `taskGitRevision` is the canonical revision input, so any SHA resolution/metadata should be derived from it for consistency.
📄 References konflux-ci/release-service-catalog/pipelines/internal/process-file-updates/process-file-updates.yaml [39-41] konflux-ci/release-service-catalog/pipelines/managed/release-to-github/release-to-github.yaml [635-643] konflux-ci/release-service-catalog/pipelines/managed/release-to-github/release-to-github.yaml [340-351] konflux-ci/release-service-catalog/pipelines/managed/release-to-github/release-to-github.yaml [408-416] konflux-ci/release-service-catalog/pipelines/managed/release-to-github/release-to-github.yaml [604-612] konflux-ci/release-service-catalog/pipelines/managed/rhtap-service-push/rhtap-service-push.yaml [494-502] konflux-ci/release-service-catalog/pipelines/managed/rh-push-to-external-registry/rh-push-to-external-registry.yaml [603-611] konflux-ci/release-service-catalog/pipelines/managed/rhtap-service-push/rhtap-service-push.yaml [542-543]

qodo-app-for-konflux-ci · 2026-06-23T11:16:32Z

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (4) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: KFLUXSPRT-8173

✅ Compliance rules (platform): 25 rules

1. set -eux missing pipefail 📘 Rule violation ☼ Reliability

Description

Multiple Tekton script: blocks in the newly added pipeline tests start with set -eux and do not
enable pipefail, violating the required strict-mode pattern. Without pipefail, failures in piped
commands can be masked, reducing reliability of task execution.

Code

tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[R51-52]
+              #!/usr/bin/env bash
+              set -eux

Relevance

⭐⭐ Medium
Pipefail often suggested for new Tekton scripts (PR#2118/#2191), but other reviews caution against
enforcing it universally.
PR-#2118
PR-#2191

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 1268 requires Tekton step scripts to begin with set -eo pipefail (or stronger
including -e and pipefail). The new scripts begin with set -eux, omitting pipefail.
Rule 1268: Shell scripts in Tekton tasks must start with set -eo pipefail
tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[51-52]
tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[211-213]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tekton `script:` blocks must start with `set -eo pipefail` (or stronger). The new pipeline test tasks use `set -eux` which omits `pipefail`.

## Issue Context
This affects the inline `taskSpec` steps in the newly added pipeline test YAMLs.

## Fix Focus Areas
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[50-56]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[210-218]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[238-246]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[50-56]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[210-220]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[240-248]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Steps missing computeResources 📘 Rule violation ☼ Reliability

Description

The newly added inline Tekton taskSpec steps in the pipeline test YAMLs do not specify per-step
compute resources (requests/limits for CPU and memory) nor a shared template. Missing resource
constraints can lead to noisy-neighbor issues and unstable scheduling.

Code

tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[R47-52]

+        steps:
+          - name: create-crs
+            image: quay.io/konflux-ci/release-service-utils@sha256:440c27f13adab931460a9371df82d3757b77f7ccc1b7604d737e0c78760a95c9
+            script: |
+              #!/usr/bin/env bash
+              set -eux

Relevance

⭐⭐ Medium
Seen bot suggestion to add computeResources in test taskSpecs (PR#2118), but no confirmed
acceptance/rejection pattern.
PR-#2118

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 1265 requires compute resources to be set for Tekton task steps. The new step
definitions show image: and script: but no resources/computeResources blocks and no shared
template applying such constraints.
Rule 1265: Specify compute resources in all Tekton tasks
tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[47-52]
tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[205-213]
tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[235-241]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tekton tasks should specify compute resources (CPU/memory requests and limits) for each step, or via a shared template.

## Issue Context
The new pipeline test YAMLs define inline `taskSpec` steps (e.g., `create-crs`, `check-result`, and `delete-crs`) without any compute resource constraints.

## Fix Focus Areas
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[47-53]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[205-213]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[235-241]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[47-53]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[205-213]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[237-243]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Inconsistent SHA metadata 🐞 Bug ≡ Correctness

Description

collect-data now sets releasePipelineMetadata.sha from params.taskGitRevision (when it looks
like a SHA) while org/repo/revision are derived from
ReleasePlanAdmission.spec.pipeline.pipelineRef; if taskGitUrl/taskGitRevision are not the same
repo as pipelineRef.url, the metadata can claim one repo but include a commit SHA from another.
This breaks the implicit invariant that org/repo/revision/sha/pathinrepo all describe the same
pipeline source and can mislead downstream consumers/auditing.

Code

tasks/managed/collect-data/collect-data.yaml[R289-295]

+          # taskGitRevision is resolved to a SHA by the release-service operator,
+          # fall back to the revision from the pipeline ref if it is not a SHA
+          sha="$(params.taskGitRevision)"
+          if [[ ! "${sha}" =~ ^[0-9a-f]{40}$ ]]; then
+            sha=$(curl-with-retry -s "https://api.github.com/repos/${org}/${repo}/commits/${revision}" \
+              | jq -r '.sha // ""' || true)
+          fi

Relevance

⭐⭐ Medium
No similar historical review on pipelineRef vs taskGitRevision SHA consistency; correctness concern
plausible but unproven here.
PR-#2210

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The task documents taskGitRevision as belonging to taskGitUrl, but uses it to populate the sha
field of metadata whose org/repo/revision are derived from pipelineRef. The added SHA test
explicitly supplies a pipelineRef.url different from taskGitUrl, demonstrating the code accepts
divergent repos and can therefore emit inconsistent metadata.
tasks/managed/collect-data/collect-data.yaml[66-72]
tasks/managed/collect-data/collect-data.yaml[281-295]
tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[91-100]
tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[154-157]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`releasePipelineMetadata` is constructed from `pipelineRef` fields (`org/repo/revision/pathInRepo`) but `sha` is sourced from `params.taskGitRevision` whenever it matches a 40-hex SHA. If `taskGitRevision` refers to a different repository than `pipelineRef.url`, the task can emit a mismatched `{org, repo, sha}` tuple.

### Issue Context
- `taskGitRevision` is documented as the revision for `taskGitUrl` (task/stepaction catalog), but the pipeline identity comes from `ReleasePlanAdmission.spec.pipeline.pipelineRef`.
- The newly added test demonstrates these can diverge (`taskGitUrl=http://localhost` while `pipelineRef.url=https://github.com/some-catalog/...`) and still expects `sha` to come from `taskGitRevision`.

### Fix Focus Areas
- tasks/managed/collect-data/collect-data.yaml[281-295]
- tasks/managed/collect-data/collect-data.yaml[66-72]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[91-100]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[154-157]

### Suggested remediation options (pick one)
1) **Guarded reuse:** Only trust `params.taskGitRevision` as `sha` if `pipelineRef.url` matches `params.taskGitUrl` after normalization (strip `.git`, handle trailing slashes, handle `https://github.com/org/repo` vs `https://github.com/org/repo.git`). Otherwise, resolve SHA from `pipelineRef` (API) or set `sha=unknown`.
2) **Separate fields:** Emit both `pipeline_sha` (resolved from `pipelineRef`) and `catalog_sha` (from `taskGitRevision`) so consumers don’t assume they refer to the same repo.
3) **Dedicated param:** Introduce a new param like `pipelineRefResolvedSha` (set by the operator) and use that for metadata; keep `taskGitRevision` semantics intact.

Also update the task README/param descriptions (and test descriptions) to reflect whichever contract you choose.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

4. check-result exits nonzero 📘 Rule violation ≡ Correctness

Description

The newly added pipeline test tasks use exit 1 for business-logic assertions inside Tekton step
scripts. This violates the requirement that Tekton task scripts must always exit 0 and report status
via results, not process exit codes.

Code

tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[R214-218]

+              if [ "$(wc -l < "$(params.dataDir)/mock_curl.txt")" != 1 ]; then
+                echo Error: curl-with-retry was expected to be called 1 time. Actual calls:
+                cat "$(params.dataDir)/mock_curl.txt"
+                exit 1
+              fi

Relevance

⭐ Low
Repo test pipelines commonly use exit 1 assertions; no evidence team enforces “always exit 0, use
results”.
PR-#2118
PR-#2189

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 1256 forbids using non-zero exits for business logic in Tekton scripts and requires
using results instead. The new check-result scripts explicitly call exit 1 on assertion
failures.
Rule 1256: Tekton task scripts must always exit 0 and use results for status
tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[214-218]
tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[216-220]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tekton task scripts in the new pipeline tests use `exit 1` to signal assertion failure. Per policy, scripts must always `exit 0` and communicate success/failure via Tekton results.

## Issue Context
The failing exits are in the `check-result` steps of the newly added pipeline test YAMLs.

## Fix Focus Areas
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[214-231]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[214-233]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

5. image: line exceeds 120 📘 Rule violation ⚙ Maintainability

Description

The new YAML test pipeline files include long image: lines with full sha256 digests that exceed
120 characters. This violates the YAML line-length limit and makes diffs/reviews harder.

Code

tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[49]
+            image: quay.io/konflux-ci/release-service-utils@sha256:440c27f13adab931460a9371df82d3757b77f7ccc1b7604d737e0c78760a95c9

Relevance

⭐ Low
Line-length/image wrapping suggestions were explicitly rejected in similar Tekton test YAMLs
(PR#2197).
PR-#2197

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 1252 limits YAML lines to 120 characters. The image: lines embed the entire
sha256 digest on a single indented line, exceeding the limit.
Rule 1252: Limit YAML line length to 120 characters
tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[49-49]
tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[49-49]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
YAML lines must be limited to 120 characters. The new pipeline test YAMLs include long `image:` values on a single line.

## Issue Context
These are Tekton Pipeline test definitions under `tasks/managed/collect-data/tests/`.

## Fix Focus Areas
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml[48-52]
- tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-sha.yaml[48-52]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

seanconroy2021 · 2026-06-23T14:57:13Z

/retest

seanconroy2021 marked this pull request as ready for review June 23, 2026 10:40

seanconroy2021 requested review from arewm, gbenhaim, ralphbean and twaugh as code owners June 23, 2026 10:40

release-service-bot assigned happybhati and theflockers Jun 23, 2026

qodo-app-for-konflux-ci Bot added the Review effort 3/5 label Jun 23, 2026

qodo-app-for-konflux-ci Bot reviewed Jun 23, 2026

View reviewed changes

Comment thread tasks/managed/collect-data/tests/test-collect-data-print-pipeline-ref-api-fail.yaml

happybhati approved these changes Jun 24, 2026

View reviewed changes

seanconroy2021 marked this pull request as draft June 25, 2026 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(KFLUXSPRT-8173): use taskGitRevision SHA for pipeline metadata#2314

fix(KFLUXSPRT-8173): use taskGitRevision SHA for pipeline metadata#2314
seanconroy2021 wants to merge 1 commit into
konflux-ci:developmentfrom
seanconroy2021:KFLUXSPRT-8173

seanconroy2021 commented Jun 23, 2026

Uh oh!

seanconroy2021 commented Jun 23, 2026

Uh oh!

qodo-app-for-konflux-ci Bot commented Jun 23, 2026

Uh oh!

qodo-app-for-konflux-ci Bot commented Jun 23, 2026

Uh oh!

Uh oh!

seanconroy2021 commented Jun 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

seanconroy2021 commented Jun 23, 2026

Describe your changes

Relevant Jira

Checklist before requesting a review

Uh oh!

seanconroy2021 commented Jun 23, 2026

Uh oh!

qodo-app-for-konflux-ci Bot commented Jun 23, 2026

PR Reviewer Guide 🔍

Uh oh!

qodo-app-for-konflux-ci Bot commented Jun 23, 2026

Code Review by Qodo

Uh oh!

Uh oh!

seanconroy2021 commented Jun 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants