add determine-if-pipeline-should-skip task to create-custom-snapshot …#19892
Closed
tommartensen wants to merge 8 commits intomasterfrom
Closed
add determine-if-pipeline-should-skip task to create-custom-snapshot …#19892tommartensen wants to merge 8 commits intomasterfrom
tommartensen wants to merge 8 commits intomasterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19892 +/- ##
==========================================
+ Coverage 49.58% 49.60% +0.02%
==========================================
Files 2766 2765 -1
Lines 208523 208541 +18
==========================================
+ Hits 103387 103455 +68
+ Misses 97460 97408 -52
- Partials 7676 7678 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
🚀 Build Images ReadyImages are ready for commit 765dc5b. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-601-g765dc5b02b |
Contributor
|
Caution There are some errors in your PipelineRun template.
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Only
clone-repositoryis currently gated bySHOULD_SKIP_PIPELINE; consider wrapping the rest of the heavy/side‑effecting tasks in similarwhenconditions so that a skipped pipeline truly avoids unnecessary work. - The
sleep 60in thedetermine-if-pipeline-should-skipscript unconditionally adds latency to all PR runs; consider making this conditional (e.g., only when the label is not yet present) or shorter/configurable to reduce pipeline runtime impact. - The
has_konflux_build_labelfunction doesn’t handle failures fromgh/jq(e.g., rate limits, auth issues), which could inadvertently cause the pipeline to run or skip incorrectly; consider adding explicit error handling and logging around that call.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Only `clone-repository` is currently gated by `SHOULD_SKIP_PIPELINE`; consider wrapping the rest of the heavy/side‑effecting tasks in similar `when` conditions so that a skipped pipeline truly avoids unnecessary work.
- The `sleep 60` in the `determine-if-pipeline-should-skip` script unconditionally adds latency to all PR runs; consider making this conditional (e.g., only when the label is not yet present) or shorter/configurable to reduce pipeline runtime impact.
- The `has_konflux_build_label` function doesn’t handle failures from `gh`/`jq` (e.g., rate limits, auth issues), which could inadvertently cause the pipeline to run or skip incorrectly; consider adding explicit error handling and logging around that call.
## Individual Comments
### Comment 1
<location path=".tekton/create-custom-snapshot.yaml" line_range="186-190" />
<code_context>
+ fi
+ }
+
+ has_konflux_build_label() {
+ local pull_request_number=$1
+ local label_name="konflux-build"
+
+ gh pr view --json labels "${pull_request_number}" \
+ | jq -r '.labels[] | select(.name == "'"${label_name}"'") | .name' | grep -q "${label_name}"
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The `has_konflux_build_label` function can terminate the whole script due to `set -euo pipefail` when the label is absent.
With `set -euo pipefail` enabled, a non‑zero exit from the `gh | jq | grep -q` pipeline will terminate the entire script instead of just making `has_konflux_build_label` return 1 when the label is missing. Please explicitly handle the non‑zero case, e.g. by wrapping the pipeline in an `if ...; then return 0; else return 1; fi` or by using `|| return 1` so the function can safely signal “label not found” without aborting the script.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
📝 WalkthroughWalkthroughUpdates to CI/CD configuration: extended labeler rule in Changes
Sequence DiagramsequenceDiagram
participant Git as Git Event<br/>(push/PR)
participant CEL as CEL Expression<br/>(Trigger)
participant SkipTask as determine-if-pipeline-should-skip<br/>Task
participant CloneTask as clone-repository<br/>Task
participant Downstream as Downstream Tasks
Git->>CEL: push to master/release/* or PR (not ready_for_review)
CEL->>SkipTask: Trigger pipeline
activate SkipTask
SkipTask->>SkipTask: Check: Is PR?<br/>Is release branch?<br/>Source branch match?<br/>Has konflux-build label?<br/>(wait 60s if checking label)
SkipTask-->>SkipTask: Compute SHOULD_SKIP_PIPELINE<br/>(RUN or SKIP)
deactivate SkipTask
SkipTask->>CloneTask: Provide SHOULD_SKIP_PIPELINE result
alt SHOULD_SKIP_PIPELINE == RUN
CloneTask->>Downstream: Execute clone & continue
else SHOULD_SKIP_PIPELINE == SKIP
CloneTask->>CloneTask: Task skipped
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.tekton/create-custom-snapshot.yaml:
- Around line 217-220: The pipeline currently only gates clone-repository with
the SHOULD_SKIP_PIPELINE==RUN condition but leaves dependent tasks running;
update the snapshot-producing chain so the same conditional is applied to all
downstream tasks (e.g., determine-image-tag, wait-for-bundle-image,
create-acs-style-snapshot) by adding the when block (input:
$(tasks.determine-if-pipeline-should-skip.results.SHOULD_SKIP_PIPELINE),
operator: in, values: [RUN]) to each of those Task/step specs so the entire
snapshot flow is short-circuited when SKIP is chosen.
- Around line 13-19: The current annotations cause duplicate runs because
pipelinesascode.tekton.dev/on-cel-expression triggers non-draft PRs while
pipelinesascode.tekton.dev/on-label also retriggers on label changes; fix by
restricting or removing the label-based trigger: modify
pipelinesascode.tekton.dev/on-label so it only triggers when a PR is labeled
while still a draft (e.g., conditionally allow label events only when
body.pull_request.draft == true and body.action == "labeled"), or remove the
on-label annotation entirely and rely solely on
pipelinesascode.tekton.dev/on-cel-expression; update the annotations near
pipelinesascode.tekton.dev/on-cel-expression and
pipelinesascode.tekton.dev/on-label accordingly to prevent duplicate snapshot
runs.
- Around line 125-140: The gh CLI calls in the determine-if-pipeline-should-skip
task cannot infer repo without a checkout; add explicit repo context by
exporting GH_REPO and/or passing --repo to gh pr view. Concretely, add an env
entry named GH_REPO (value "stackrox/stackrox") alongside GH_TOKEN,
SOURCE_BRANCH, TARGET_BRANCH, PULL_REQUEST_NUMBER in the task spec, and update
any gh pr view invocations in the determine-if-pipeline-should-skip task to
include --repo "$GH_REPO" (or rely on GH_REPO env) so label lookups succeed when
running before clone-repository.
🪄 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: ee7eb1a1-8bd8-4eda-a48a-6e2c512f1461
📒 Files selected for processing (2)
.github/labeler.yml.tekton/create-custom-snapshot.yaml
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
1 participant
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.
…pipeline
Description
For https://redhat.atlassian.net/browse/ROX-34007, we need to have a job that tells us whether Konflux feels happy about every PR. This is a suggestion how to achieve this.
If accepted,
create-custom-snapshotpipeline would become a required check.How it works:
What I don't like about this:
create-custom-snapshotnow does two things and they're not clear from the name. Alternatively, one could thing about adding a custom Konflux pipeline that does the check and wait for the snapshot to appear.User-facing documentation
Testing and quality
Automated testing
How I validated my change
empty commit, should not trigger any Konflux pipelines 2380225
empty commit, after PR opens should not trigger any Konflux pipelines execept create-custom-snapshot, which skips: 8005ed1
https://konflux-ui.apps.stone-prd-rh01.pg1f.p1.openshiftapps.com/ns/rh-acs-tenant/applications/acs/pipelineruns/create-custom-snapshot-pjvzk
if targetting release branch, should trigger all pipelines:
if source branch has magic string, should trigger all pipelines
if has konflux-build label, should trigger all pipelines:
pushing to release branch should trigger all pipelines