ci: create calendar version tags on push#118
Conversation
📝 WalkthroughWalkthroughA new Bash script Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security Considerations
No other exported/public API changes were introduced. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.tekton/odh-midstream-cuda-base-12-9-push.yaml (1)
10-17:⚠️ Potential issue | 🟡 MinorMissing path filter for
scripts/generate-tag.sh.Same issue as other pipeline files. Add
'scripts/generate-tag.sh'.pathChanged()to the CEL expression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tekton/odh-midstream-cuda-base-12-9-push.yaml around lines 10 - 17, The pipelinesascode.tekton.dev/on-cel-expression CEL filter is missing the scripts/generate-tag.sh path check; update the CEL expression string (value of pipelinesascode.tekton.dev/on-cel-expression) to include the additional predicate 'scripts/generate-tag.sh'.pathChanged() alongside the existing checks so pushes touching that script will trigger the pipeline..tekton/odh-midstream-cuda-base-12-8-push.yaml (1)
10-17:⚠️ Potential issue | 🟡 MinorMissing path filter for
scripts/generate-tag.sh.The
on-cel-expressiontriggers this pipeline when various paths change, butscripts/generate-tag.shis not included. If the tag generation script is modified, this pipeline won't rebuild, potentially causing inconsistent tagging behavior across images.Proposed fix
event == "push" && target_branch == "main" && ( 'cuda/12.8/**'.pathChanged() || 'requirements-build.txt'.pathChanged() || 'scripts/fix-permissions'.pathChanged() || + 'scripts/generate-tag.sh'.pathChanged() || '.dockerignore'.pathChanged() || '.tekton/odh-midstream-cuda-base-12-8-push.yaml'.pathChanged() )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tekton/odh-midstream-cuda-base-12-8-push.yaml around lines 10 - 17, The on-cel-expression filter currently lists specific changed paths but omits the tag generation script; update the pipeline trigger expression (the pipelinesascode.tekton.dev/on-cel-expression block) to include 'scripts/generate-tag.sh'. Modify the logical OR group where paths are checked (the array that contains 'cuda/12.8/**'.pathChanged(), 'requirements-build.txt'.pathChanged(), etc.) to add 'scripts/generate-tag.sh'.pathChanged() so changes to the tag generator will also trigger this pipeline..tekton/odh-midstream-python-base-3-12-push.yaml (1)
10-17:⚠️ Potential issue | 🟡 MinorMissing path filter for
scripts/generate-tag.sh.Same issue as other pipeline files. Add
'scripts/generate-tag.sh'.pathChanged()to the CEL expression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tekton/odh-midstream-python-base-3-12-push.yaml around lines 10 - 17, The pipelines-as-code CEL expression in the pipelinesascode.tekton.dev/on-cel-expression block is missing the path filter for scripts/generate-tag.sh; update the expression used in the multiline string (the CEL condition containing event == "push" && target_branch == "main" && (...)) to include the clause 'scripts/generate-tag.sh'.pathChanged() alongside the other .pathChanged() checks so changes to that script will trigger the pipeline.
🧹 Nitpick comments (2)
scripts/generate-tag.sh (2)
15-15: Silently suppressing skopeo errors may mask real failures.Redirecting stderr to
/dev/nullhides authentication failures, network issues, or invalid repository URLs. The fallback to{"Tags":[]}is reasonable for new repositories with no tags, but other failures should be surfaced.Proposed fix
-TAGS_JSON=$(skopeo list-tags "docker://${REPO_URL}" 2>/dev/null || echo '{"Tags":[]}') +TAGS_JSON=$(skopeo list-tags "docker://${REPO_URL}" 2>&1) || { + if [[ "${TAGS_JSON}" == *"manifest unknown"* ]] || [[ "${TAGS_JSON}" == *"not found"* ]]; then + TAGS_JSON='{"Tags":[]}' + else + echo "Error fetching tags: ${TAGS_JSON}" >&2 + exit 1 + fi +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-tag.sh` at line 15, The current assignment to TAGS_JSON hides skopeo errors by redirecting stderr to /dev/null; change the logic around the skopeo list-tags invocation so you capture both its stdout and exit status (call out the skopeo list-tags invocation and the TAGS_JSON assignment), do not blanket-redirect stderr away, and only fallback to '{"Tags":[]}' when the command succeeds but returns an empty tag list or when the repository truly has no tags; for other non-zero exit codes (authentication, network, invalid repo) write the skopeo stderr to stderr (or log it) and exit with a non-zero status so failures are surfaced.
18-18: Regex may match unintended tag patterns.The pattern
1\.${TODAY}\.[0-9]+matches tags like1.20260305.0abcif such malformed tags exist (the+is greedy andgrep -oextracts the matching portion). This is low risk but could be tightened.Proposed fix using word boundaries
-MATCHING=$(grep -oE "1\.${TODAY}\.[0-9]+" <<< "${TAGS_JSON}" || true) +MATCHING=$(grep -oE "\b1\.${TODAY}\.[0-9]+\b" <<< "${TAGS_JSON}" || true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-tag.sh` at line 18, The current regex assigned to MATCHING (using TAGS_JSON and TODAY) is too permissive and can capture trailing letters; tighten it by requiring a word/end boundary after the numeric suffix so only tags like 1.${TODAY}.<digits> match. Update the pattern used in the grep invocation that sets MATCHING to include an appropriate boundary token (for example a word boundary or an anchor) after the final [0-9]+ so malformed tags like 1.${TODAY}.0abc are excluded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/generate-tag.sh`:
- Around line 15-24: The script generate-tag.sh currently computes NEXT_BUILD
from TAGS_JSON/MATCHING and can produce duplicate tags under concurrent
pipelines; change generate-tag.sh to implement compare-and-swap retry semantics:
wrap the NEXT_BUILD computation and tag publish in a loop that re-fetches
TAGS_JSON (using skopeo list-tags) then computes MATCHING and NEXT_BUILD, checks
whether the candidate tag already exists, and only proceeds to push/apply the
tag if absent; if it finds the tag exists, recompute NEXT_BUILD and retry with
an exponential backoff and a retry limit; reference the existing variables and
steps TAGS_JSON, MATCHING, MAX_BUILD and NEXT_BUILD and add the existence check
+ retry logic to avoid race conditions.
---
Outside diff comments:
In @.tekton/odh-midstream-cuda-base-12-8-push.yaml:
- Around line 10-17: The on-cel-expression filter currently lists specific
changed paths but omits the tag generation script; update the pipeline trigger
expression (the pipelinesascode.tekton.dev/on-cel-expression block) to include
'scripts/generate-tag.sh'. Modify the logical OR group where paths are checked
(the array that contains 'cuda/12.8/**'.pathChanged(),
'requirements-build.txt'.pathChanged(), etc.) to add
'scripts/generate-tag.sh'.pathChanged() so changes to the tag generator will
also trigger this pipeline.
In @.tekton/odh-midstream-cuda-base-12-9-push.yaml:
- Around line 10-17: The pipelinesascode.tekton.dev/on-cel-expression CEL filter
is missing the scripts/generate-tag.sh path check; update the CEL expression
string (value of pipelinesascode.tekton.dev/on-cel-expression) to include the
additional predicate 'scripts/generate-tag.sh'.pathChanged() alongside the
existing checks so pushes touching that script will trigger the pipeline.
In @.tekton/odh-midstream-python-base-3-12-push.yaml:
- Around line 10-17: The pipelines-as-code CEL expression in the
pipelinesascode.tekton.dev/on-cel-expression block is missing the path filter
for scripts/generate-tag.sh; update the expression used in the multiline string
(the CEL condition containing event == "push" && target_branch == "main" &&
(...)) to include the clause 'scripts/generate-tag.sh'.pathChanged() alongside
the other .pathChanged() checks so changes to that script will trigger the
pipeline.
---
Nitpick comments:
In `@scripts/generate-tag.sh`:
- Line 15: The current assignment to TAGS_JSON hides skopeo errors by
redirecting stderr to /dev/null; change the logic around the skopeo list-tags
invocation so you capture both its stdout and exit status (call out the skopeo
list-tags invocation and the TAGS_JSON assignment), do not blanket-redirect
stderr away, and only fallback to '{"Tags":[]}' when the command succeeds but
returns an empty tag list or when the repository truly has no tags; for other
non-zero exit codes (authentication, network, invalid repo) write the skopeo
stderr to stderr (or log it) and exit with a non-zero status so failures are
surfaced.
- Line 18: The current regex assigned to MATCHING (using TAGS_JSON and TODAY) is
too permissive and can capture trailing letters; tighten it by requiring a
word/end boundary after the numeric suffix so only tags like 1.${TODAY}.<digits>
match. Update the pattern used in the grep invocation that sets MATCHING to
include an appropriate boundary token (for example a word boundary or an anchor)
after the final [0-9]+ so malformed tags like 1.${TODAY}.0abc are excluded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8074d095-f7c6-4f52-8e01-9852efc4cb09
📒 Files selected for processing (6)
.tekton/odh-midstream-cuda-base-12-8-push.yaml.tekton/odh-midstream-cuda-base-12-9-push.yaml.tekton/odh-midstream-cuda-base-13-0-push.yaml.tekton/odh-midstream-cuda-base-13-1-push.yaml.tekton/odh-midstream-python-base-3-12-push.yamlscripts/generate-tag.sh
| TODAY=$(date -u +%Y%m%d) | ||
| MAJOR=1 | ||
|
|
||
| TAGS_JSON=$(skopeo list-tags "docker://${REPO_URL}" 2>/dev/null || echo '{"Tags":[]}') |
There was a problem hiding this comment.
The 2>/dev/null here treats all skopeo failures the same as "repo has no tags yet", if there's a network issue, the script would silently fall back to {"Tags":[]} and start tagging from .0 again instead of failing the pipeline.
Maybe we can handle the common failures like 404, 500 and then do a fallback. Just a nit-pick
There was a problem hiding this comment.
Agree with @smoparth , there are several reasons why skopeo might fail to list tags. Here is a possible fix
if ! TAGS_JSON=$(skopeo list-tags "docker://${REPO_URL}" 2>&1); then
# skopeo failed — check why
case "${TAGS_JSON}" in
*"NAME_UNKNOWN"*|*"not found"*|*"manifest unknown"*|*"repository name not known"*)
# Repo genuinely doesn't exist yet — no tags, start fresh
echo "INFO: Repository not found, starting with build 0" >&2
TAGS_JSON='{"Tags":[]}'
;;
*)
# Anything else is unexpected — fail the pipeline
echo "ERROR: skopeo list-tags failed for ${REPO_URL}:" >&2
echo "${TAGS_JSON}" >&2
exit 1
;;
esac
fi
There was a problem hiding this comment.
this is absolutely good feedback and I think if I keep Skopeo I'll implement these error code handling you're 100% right here.
However, I was thinking taking a slightly different approach, and instead of using Skopeo to check the tag and bumping it, using the git history (which is linear) to generate the tag instead.
This is because I'm thinking on how to better deal with the race condition, since the on-push pipelines don't cancel each other (two or more commits can be built simoultaneously)
So each commit will already have their own tag calculated out of it's own creation date, for example:
│ Commit A (09:00 UTC 2026-03-10) → BUILD_NUM=0 → 1.20260310.0
│ Commit B (10:00 UTC 2026-03-10) → BUILD_NUM=1 → 1.20260310.1 (always, even if built before A finishes) │
│ Commit C (11:00 UTC 2026-03-10) → BUILD_NUM=2 → 1.20260310.2
that eliminates the race condition altogether, as well as the Skopeo dependency.
one tradeoff though is, that this would leave gaps on un-built commits. so if we have 3 commits today, we assume all three will build, but if for some reason one won't, that would void that commit's tag creation.
@smoparth @LalatenduMohanty please can you share your thoughts on this approach instead?
Happy to hear about some other feedback / suggestions.
I also considered driving this through git tags. so each commit would be tagged, with the tag it needs, then a pipeline triggered on git-tags would do the tagging in Quay, but seems like an unnecessary 2 step process to me.
There was a problem hiding this comment.
that eliminates the race condition altogether, as well as the Skopeo dependency.
This sounds good to me . However I am worried that if some reason the konflux fails to build a commit and a team needs image from that certain commit but not the next commit because of some reason what should we do.
There was a problem hiding this comment.
I think we should be able to rebuild that specific commit, no ? just re-trigger the push pipeline for that specific commit, that should create the missing image and tag, no ?
| TODAY=$(date -u +%Y%m%d) | ||
| MAJOR=1 | ||
|
|
||
| TAGS_JSON=$(skopeo list-tags "docker://${REPO_URL}" 2>/dev/null || echo '{"Tags":[]}') |
There was a problem hiding this comment.
Agree with @smoparth , there are several reasons why skopeo might fail to list tags. Here is a possible fix
if ! TAGS_JSON=$(skopeo list-tags "docker://${REPO_URL}" 2>&1); then
# skopeo failed — check why
case "${TAGS_JSON}" in
*"NAME_UNKNOWN"*|*"not found"*|*"manifest unknown"*|*"repository name not known"*)
# Repo genuinely doesn't exist yet — no tags, start fresh
echo "INFO: Repository not found, starting with build 0" >&2
TAGS_JSON='{"Tags":[]}'
;;
*)
# Anything else is unexpected — fail the pipeline
echo "ERROR: skopeo list-tags failed for ${REPO_URL}:" >&2
echo "${TAGS_JSON}" >&2
exit 1
;;
esac
fi
| 'requirements-build.txt'.pathChanged() || | ||
| 'scripts/fix-permissions'.pathChanged() || | ||
| '.dockerignore'.pathChanged() || | ||
| '.tekton/odh-midstream-cuda-base-12-8-push.yaml'.pathChanged() |
There was a problem hiding this comment.
scripts/generate-tag.sh should be part of the script path to all 5 pipeline CEL expressions: Otherwise any change to the script will not trigger new builds.
There was a problem hiding this comment.
please, let me try to ask the question here if whether we actually want that ? why does a change to the script that generates tags need to rebuild the components ? all of them ?
I'm not pushing back, I'm pausing and asking if this is the outcome we want, if we do, I'm happy to update the trigger expression to include it
There was a problem hiding this comment.
If generate-tag.sh has a bug, there's currently no way to re-trigger the pipelines to apply correct tags to existing images without also making a dummy change to one of the watched paths. The tagging logic silently becomes stale.
There was a problem hiding this comment.
May be we hit the desired outcome in a follow up task
-
Separate the tagging into its own lightweight pipeline
- Create a single pipeline that only runs generate-tag.sh + apply-tags (no image build)
- Trigger it on 'scripts/generate-tag.sh'.pathChanged()
- The push pipelines stay as-is, focused on building
Victoremepunto
left a comment
Victoremepunto
left a comment
There was a problem hiding this comment.
Thanks for the review, I left some responses but the gist of it is two fold:
- First, I think the "centralized pipeline definition" is out of scope of this PR, but we def. want to tackle it. I'll create an issue for it.
- Second, the current implementation has a nasty race condition we need to solve, I provided an option that relies on git history but need ACK from the team
| 'requirements-build.txt'.pathChanged() || | ||
| 'scripts/fix-permissions'.pathChanged() || | ||
| '.dockerignore'.pathChanged() || | ||
| '.tekton/odh-midstream-cuda-base-12-8-push.yaml'.pathChanged() |
There was a problem hiding this comment.
please, let me try to ask the question here if whether we actually want that ? why does a change to the script that generates tags need to rebuild the components ? all of them ?
I'm not pushing back, I'm pausing and asking if this is the outcome we want, if we do, I'm happy to update the trigger expression to include it
| TODAY=$(date -u +%Y%m%d) | ||
| MAJOR=1 | ||
|
|
||
| TAGS_JSON=$(skopeo list-tags "docker://${REPO_URL}" 2>/dev/null || echo '{"Tags":[]}') |
There was a problem hiding this comment.
this is absolutely good feedback and I think if I keep Skopeo I'll implement these error code handling you're 100% right here.
However, I was thinking taking a slightly different approach, and instead of using Skopeo to check the tag and bumping it, using the git history (which is linear) to generate the tag instead.
This is because I'm thinking on how to better deal with the race condition, since the on-push pipelines don't cancel each other (two or more commits can be built simoultaneously)
So each commit will already have their own tag calculated out of it's own creation date, for example:
│ Commit A (09:00 UTC 2026-03-10) → BUILD_NUM=0 → 1.20260310.0
│ Commit B (10:00 UTC 2026-03-10) → BUILD_NUM=1 → 1.20260310.1 (always, even if built before A finishes) │
│ Commit C (11:00 UTC 2026-03-10) → BUILD_NUM=2 → 1.20260310.2
that eliminates the race condition altogether, as well as the Skopeo dependency.
one tradeoff though is, that this would leave gaps on un-built commits. so if we have 3 commits today, we assume all three will build, but if for some reason one won't, that would void that commit's tag creation.
@smoparth @LalatenduMohanty please can you share your thoughts on this approach instead?
Happy to hear about some other feedback / suggestions.
I also considered driving this through git tags. so each commit would be tagged, with the tag it needs, then a pipeline triggered on git-tags would do the tagging in Quay, but seems like an unnecessary 2 step process to me.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/generate-tag.sh (1)
10-10:IMAGE_URLis accepted but unused.The variable is assigned but never referenced. This appears intentional since the script was refactored from skopeo-based registry lookup to git-based tag generation. If kept for forward compatibility (e.g., future registry validation), add a comment. Otherwise, remove the parameter requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-tag.sh` at line 10, The IMAGE_URL variable in generate-tag.sh is assigned but never used; either remove the unused parameter by deleting the IMAGE_URL="$1" assignment and update any usage/USAGE text or argument parsing so the script no longer requires that positional argument, or explicitly keep it by adding an inline comment above IMAGE_URL (e.g., "intentionally unused; reserved for future registry validation") and ensure any help/usage text marks the parameter optional; reference IMAGE_URL in your change so reviewers can verify the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/generate-tag.sh`:
- Line 10: The IMAGE_URL variable in generate-tag.sh is assigned but never used;
either remove the unused parameter by deleting the IMAGE_URL="$1" assignment and
update any usage/USAGE text or argument parsing so the script no longer requires
that positional argument, or explicitly keep it by adding an inline comment
above IMAGE_URL (e.g., "intentionally unused; reserved for future registry
validation") and ensure any help/usage text marks the parameter optional;
reference IMAGE_URL in your change so reviewers can verify the intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9b47ef1b-90da-433a-bc53-fe9eb11001a2
📒 Files selected for processing (13)
.tekton/odh-midstream-cuda-base-12-8-pull-request.yaml.tekton/odh-midstream-cuda-base-12-8-push.yaml.tekton/odh-midstream-cuda-base-12-9-pull-request.yaml.tekton/odh-midstream-cuda-base-12-9-push.yaml.tekton/odh-midstream-cuda-base-13-0-pull-request.yaml.tekton/odh-midstream-cuda-base-13-0-push.yaml.tekton/odh-midstream-cuda-base-13-1-pull-request.yaml.tekton/odh-midstream-cuda-base-13-1-push.yaml.tekton/odh-midstream-python-base-3-12-pull-request.yaml.tekton/odh-midstream-python-base-3-12-push.yaml.tekton/pipelines/pull-request.yaml.tekton/pipelines/push.yamlscripts/generate-tag.sh
|
The pipeline refactoring has been extracted to #147 to avoid scope creep. |
Victoremepunto
added
help wanted
| COMMIT_DATE=$(git -C "${SOURCE_DIR}" log -1 --format="%ad" --date=format:"%Y%m%d" HEAD) | ||
| # Count commits on the same day that are ancestors of HEAD (i.e., came before it). | ||
| # This gives a deterministic, sequential build number from git history — no registry access needed. | ||
| BUILD_NUM=$(git -C "${SOURCE_DIR}" log HEAD^ --format="%ad" --date=format:"%Y%m%d" 2>/dev/null \ |
There was a problem hiding this comment.
(Not a blocker, nit) Since this only runs on main (which is protected and always has history), HEAD^ will always exist I assume, so the 2>/dev/null isn't needed for that case. But it also suppresses real failures with || true, the script silently produces BUILD_NUM=0 instead of failing the pipeline.
Given these tags are consumed by downstream teams via Renovate, a silently wrong tag is worse than a failed build.
There was a problem hiding this comment.
thank you ! I actually think this is really important - and while testing the BUILD_NUM more thoroughly I realized the build nums were "unexpected" due to not following the linear history - now that should be fixed too
thanks for the review !
|
Hey @LalatenduMohanty — I'd like to align on the direction before reworking this PR. There are two approaches on the table, and they're not mutually exclusive — but they serve different workflows, so I want to make sure we pick the right one: Option 1: Automatic tagging on every build (per #99 consensus) The concern about failed builds leaving gaps is addressed by re-running the pipeline for that commit — the git-based tag generation is deterministic, so it produces the same tag on re-run. And Option 2: Selective tagging via a lightweight pipeline This approach has some implications worth noting:
Both approaches are valid. I'm leaning towards option 1 since it matches the original consensus and is simpler to implement, but happy to go with option 2 if the team prefers selective versioning. What do you think? |
- Remove unused IMAGE_URL parameter - Normalize dates to UTC (TZ=UTC + --date=format-local) - Remove silent error suppression (fail loud > wrong tag) - Update pipeline task to match new script interface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Victoremepunto
force-pushed
the
version-tags
branch
from
6779a92 to
0f02da3
Compare
- Flip param order to [commit] [source-dir] for easier local use - Normalize dates to UTC (TZ=UTC + --date=format-local) - Use --first-parent to count only main-line merges, avoiding gaps - Remove silent error suppression (fail loud > wrong tag) - Remove unused IMAGE_URL parameter - Update pipeline task to match new script interface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 @.tekton/pipelines/push.yaml:
- Around line 541-547: The generate-image-tag task lacks the conditional guard
and may run when builds are skipped; modify the taskSpec for the task named
generate-image-tag (which currently sets param SOURCE_ARTIFACT and runAfter:
build-image-index) to include a when clause that checks the build result (e.g.,
guard on $(tasks.init.results.build) like other post-build tasks) so it only
executes if a build actually occurred.
- Around line 571-576: The pipeline performs a shallow clone by default which
breaks generate-tag.sh (it runs git log --first-parent "${COMMIT}^"), so update
the git clone step that invokes the git-clone-oci-ta task to pass a depth param
enabling full history: add a params entry with - name: depth and value: "0" (or
another sufficient integer) to the task invocation so the repository is cloned
with full history before generate-tag.sh runs.
In `@scripts/generate-tag.sh`:
- Around line 12-13: The BUILD_NUM assignment fails when grep -c returns exit
code 1 on zero matches; update the command that sets BUILD_NUM (the line using
BUILD_NUM, SOURCE_DIR, COMMIT, COMMIT_DATE and grep -c) to avoid a non-zero exit
on zero matches — either by appending a no-op fallback (e.g., use "|| :" after
grep -c) or by capturing grep output into a variable and treating empty output
as 0 so the script doesn't exit under set -eo pipefail; ensure the final value
of BUILD_NUM remains the numeric count (0 when no matches).
- Line 12: The BUILD_NUM computation uses git -C "${SOURCE_DIR}" log
--first-parent "${COMMIT}^" which fails for the repository's initial commit;
update the script around BUILD_NUM to first test whether the parent commit
exists (e.g., use git -C "${SOURCE_DIR}" rev-parse --verify "${COMMIT}^"
>/dev/null 2>&1) and, if it does, run the existing log/grep pipeline (keeping
the grep ... || true fallback) to compute BUILD_NUM, otherwise set BUILD_NUM=0
so the script won’t exit with an error when COMMIT is the first commit;
reference BUILD_NUM, COMMIT and SOURCE_DIR when locating where to add this
conditional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: eb2cd864-8d81-4ecf-9a1b-9b0d98a8c019
📒 Files selected for processing (2)
.tekton/pipelines/push.yamlscripts/generate-tag.sh
| - name: generate-image-tag | ||
| params: | ||
| - name: SOURCE_ARTIFACT | ||
| value: $(tasks.prefetch-dependencies.results.SOURCE_ARTIFACT) | ||
| runAfter: | ||
| - build-image-index | ||
| taskSpec: |
There was a problem hiding this comment.
Missing when condition; task may run when build is skipped.
Other post-build tasks (e.g., deprecated-base-image-check, clair-scan) guard execution with a when clause checking $(tasks.init.results.build). Without this, generate-image-tag could attempt to run even when no image was built, potentially causing confusing failures or unnecessary work.
Proposed fix
- name: generate-image-tag
params:
- name: SOURCE_ARTIFACT
value: $(tasks.prefetch-dependencies.results.SOURCE_ARTIFACT)
runAfter:
- build-image-index
+ when:
+ - input: $(tasks.init.results.build)
+ operator: in
+ values:
+ - "true"
taskSpec:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.tekton/pipelines/push.yaml around lines 541 - 547, The generate-image-tag
task lacks the conditional guard and may run when builds are skipped; modify the
taskSpec for the task named generate-image-tag (which currently sets param
SOURCE_ARTIFACT and runAfter: build-image-index) to include a when clause that
checks the build result (e.g., guard on $(tasks.init.results.build) like other
post-build tasks) so it only executes if a build actually occurred.
| script: | | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| VERSION_TAG=$(bash /var/workdir/source/scripts/generate-tag.sh HEAD /var/workdir/source) | ||
| echo -n "${VERSION_TAG}" > /tekton/results/VERSION_TAG | ||
| echo "Generated version tag: ${VERSION_TAG}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if git-clone-oci-ta performs shallow clone by default
# Look for depth-related parameters in task definition references and documentation
# Search for shallow/depth mentions in .tekton files
rg -n -i "depth|shallow" .tekton/ || echo "No depth/shallow references found in .tekton"
# Check if clone-repository task has any depth param configured
rg -n -B2 -A10 "name: clone-repository" .tekton/pipelines/push.yaml | head -40Repository: opendatahub-io/base-containers
Length of output: 509
🏁 Script executed:
# Find and examine the generate-tag.sh script
find . -name "generate-tag.sh" -type f
# Also check the full pipeline file to understand task references
cat -n .tekton/pipelines/push.yaml | sed -n '100,150p'Repository: opendatahub-io/base-containers
Length of output: 1877
🏁 Script executed:
# Look for task definitions that might be git-clone-oci-ta
rg -n "git-clone-oci-ta|clone.*git" .tekton/
# Check if clone-repository is a resolver reference to a bundled task
rg -n -B5 -A15 "resolver: bundles" .tekton/pipelines/push.yaml | head -50Repository: opendatahub-io/base-containers
Length of output: 2309
🏁 Script executed:
# Search for BUILD_NUM usage in scripts
rg -rn "BUILD_NUM" scripts/ || echo "No BUILD_NUM found in scripts"Repository: opendatahub-io/base-containers
Length of output: 389
🏁 Script executed:
# Check for any documentation about the git-clone-oci-ta task
find . -name "*.md" -o -name "*.txt" | xargs grep -l "git-clone-oci-ta" 2>/dev/null || echo "No docs found"
# Look for any task definitions or configuration files locally
find . -name "*git-clone*" -type f
# Check the exact git-clone-oci-ta task parameters to see what's available
rg -n "git-clone-oci-ta" -B10 -A20 .tekton/pipelines/push.yaml | head -80Repository: opendatahub-io/base-containers
Length of output: 1186
🏁 Script executed:
# Check if there are any environment variables or settings that control clone depth
rg -n "clone.*depth|shallow" . --type yaml --type sh || echo "No clone depth settings found"
# Examine the generate-tag.sh more carefully for what n represents
cat -n scripts/generate-tag.sh | head -50Repository: opendatahub-io/base-containers
Length of output: 777
🌐 Web query:
git-clone-oci-ta Tekton task shallow clone depth quay.io konflux-ci
💡 Result:
The git-clone-oci-ta is a Konflux CI Tekton Task that performs a git clone operation and produces a Trusted Artifact (OCI artifact) for secure sharing between pipeline tasks without PVCs. It is hosted at quay.io/konflux-ci/tekton-catalog/task-git-clone-oci-ta and defined in the konflux-ci/build-definitions repository under task/git-clone-oci-ta/. It supports shallow cloning via the standard 'depth' parameter, which controls the --depth flag in git fetch. Default is typically "1" for shallow clone (most recent commit). Set to "0" for full clone or higher values (e.g., "10") for deeper history. Example usage in a PipelineRun (from Konflux patterns): tasks: - name: clone-repository-oci-ta taskRef: name: git-clone-oci-ta bundle: quay.io/konflux-ci/tekton-catalog/task-git-clone-oci-ta:0.1@sha256:... # Use specific digest params: - name: url value:
Citations:
- 1: https://github.com/konflux-ci/build-definitions
- 2: https://konflux-ci.dev/docs/building/using-trusted-artifacts/
- 3: https://github.com/konflux-ci/build-definitions/blob/main/pipelines/docker-build-oci-ta/README.md
- 4: https://konflux-ci.dev/docs/end-to-end/building-tekton-tasks/
- 5: https://github.com/konflux-ci/build-trusted-artifacts
- 6: chore(deps): update konflux references (konflux-quay-test) quay/quay#3474
🏁 Script executed:
# Also check if there's any git command that would indicate shallow cloning
rg -n "shallow|--depth|--single-branch" . --type yaml --type sh || echo "No shallow-related params found"
# Check the exact parameters the clone-repository task passes
rg -A30 "name: clone-repository" .tekton/pipelines/push.yamlRepository: opendatahub-io/base-containers
Length of output: 928
Pass depth parameter to clone-repository task to enable full git history.
The git-clone-oci-ta task defaults to shallow clone (depth=1), which provides only the most recent commit. However, generate-tag.sh at line 574 requires git history to run git log --first-parent "${COMMIT}^" for BUILD_NUM calculation. With a shallow clone, this command will produce incorrect results (likely 0 or failure). Add - name: depth with value "0" (full clone) or a sufficient value to the clone-repository task params.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.tekton/pipelines/push.yaml around lines 571 - 576, The pipeline performs a
shallow clone by default which breaks generate-tag.sh (it runs git log
--first-parent "${COMMIT}^"), so update the git clone step that invokes the
git-clone-oci-ta task to pass a depth param enabling full history: add a params
entry with - name: depth and value: "0" (or another sufficient integer) to the
task invocation so the repository is cloned with full history before
generate-tag.sh runs.
| BUILD_NUM=$(TZ=UTC git -C "${SOURCE_DIR}" log --first-parent "${COMMIT}^" --format="%ad" --date=format-local:"%Y%m%d" \ | ||
| | grep -c "^${COMMIT_DATE}$") |
There was a problem hiding this comment.
grep -c exits 1 when count is 0, failing the pipeline on first commit of any day.
With set -eo pipefail, grep -c returning zero matches produces exit code 1, terminating the script. This breaks every first-of-day build where BUILD_NUM should legitimately be 0.
Proposed fix
-BUILD_NUM=$(TZ=UTC git -C "${SOURCE_DIR}" log --first-parent "${COMMIT}^" --format="%ad" --date=format-local:"%Y%m%d" \
- | grep -c "^${COMMIT_DATE}$")
+BUILD_NUM=$(TZ=UTC git -C "${SOURCE_DIR}" log --first-parent "${COMMIT}^" --format="%ad" --date=format-local:"%Y%m%d" \
+ | grep -c "^${COMMIT_DATE}$" || true)Alternatively, use grep -c ... || : or capture output and handle explicitly if you need to distinguish "no matches" from actual grep errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate-tag.sh` around lines 12 - 13, The BUILD_NUM assignment fails
when grep -c returns exit code 1 on zero matches; update the command that sets
BUILD_NUM (the line using BUILD_NUM, SOURCE_DIR, COMMIT, COMMIT_DATE and grep
-c) to avoid a non-zero exit on zero matches — either by appending a no-op
fallback (e.g., use "|| :" after grep -c) or by capturing grep output into a
variable and treating empty output as 0 so the script doesn't exit under set -eo
pipefail; ensure the final value of BUILD_NUM remains the numeric count (0 when
no matches).
| COMMIT_DATE=$(TZ=UTC git -C "${SOURCE_DIR}" log -1 --format="%ad" --date=format-local:"%Y%m%d" "${COMMIT}") | ||
| # Count first-parent commits on the same day that came before COMMIT. | ||
| # --first-parent follows only the main line, so BUILD_NUM reflects merge count, not branch activity. | ||
| BUILD_NUM=$(TZ=UTC git -C "${SOURCE_DIR}" log --first-parent "${COMMIT}^" --format="%ad" --date=format-local:"%Y%m%d" \ |
There was a problem hiding this comment.
${COMMIT}^ fails on initial commit (no parent).
If COMMIT is the repository's first commit, git log "${COMMIT}^" errors out. While unlikely on a protected main branch with history, this edge case would crash the script with set -e.
Proposed defensive fix
-BUILD_NUM=$(TZ=UTC git -C "${SOURCE_DIR}" log --first-parent "${COMMIT}^" --format="%ad" --date=format-local:"%Y%m%d" \
+BUILD_NUM=$(TZ=UTC git -C "${SOURCE_DIR}" log --first-parent "${COMMIT}^" --format="%ad" --date=format-local:"%Y%m%d" 2>/dev/null \
| grep -c "^${COMMIT_DATE}$" || true)Or check parent existence first:
if git -C "${SOURCE_DIR}" rev-parse --verify "${COMMIT}^" >/dev/null 2>&1; then
BUILD_NUM=$(TZ=UTC git -C "${SOURCE_DIR}" log --first-parent "${COMMIT}^" --format="%ad" --date=format-local:"%Y%m%d" \
| grep -c "^${COMMIT_DATE}$" || true)
else
BUILD_NUM=0
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate-tag.sh` at line 12, The BUILD_NUM computation uses git -C
"${SOURCE_DIR}" log --first-parent "${COMMIT}^" which fails for the repository's
initial commit; update the script around BUILD_NUM to first test whether the
parent commit exists (e.g., use git -C "${SOURCE_DIR}" rev-parse --verify
"${COMMIT}^" >/dev/null 2>&1) and, if it does, run the existing log/grep
pipeline (keeping the grep ... || true fallback) to compute BUILD_NUM, otherwise
set BUILD_NUM=0 so the script won’t exit with an error when COMMIT is the first
commit; reference BUILD_NUM, COMMIT and SOURCE_DIR when locating where to add
this conditional.
3 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit