MGMT-24125: pin OSAC component images by SHA tag - #80
Conversation
|
@omer-vishlitzky: This pull request references MGMT-24125 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 52 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds a GitHub Actions workflow that checks image tags on pull requests, a Bash script ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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 (3)
.github/workflows/check-image-tags.yaml (1)
10-18: Add an explicit least-privilegepermissions:block.This job only needs to read the repo + submodules; it doesn't push, comment, or write checks. Setting
permissions: contents: readat the workflow (or job) level makes that intent explicit and protects the job against future org-wide default-permission changes. A smalltimeout-minutesis also worth adding so a stuckgit submoduledoesn't burn the 6-hour default.♻️ Suggested change
on: pull_request: +permissions: + contents: read + concurrency: group: "check-image-tags-${{ github.head_ref || github.run_id }}" cancel-in-progress: true jobs: check-image-tags: runs-on: ubuntu-latest + timeout-minutes: 5 steps: - uses: actions/checkout@v6 with: submodules: recursive🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/check-image-tags.yaml around lines 10 - 18, Add an explicit least-privilege permissions block and a per-job timeout to the check-image-tags job: update the job "check-image-tags" (which runs the "Verify image tags match submodule commits" step that runs scripts/sync-image-tags.sh) to include permissions: contents: read and a timeout-minutes value (e.g., 10) at the job level so the job only has read access to the repo/submodules and cannot run indefinitely.base/kustomization.yaml (1)
27-35: Consider pinning bydigest:for stronger immutability guarantees.SHA-prefixed tags like
sha-f2cd619are still mutable references in an OCI registry — the publisher can move them at any time. The hypershift2 overlay already usesdigest: sha256:…for the osac-operator image, demonstrating the pattern kustomize recommends for reproducibility and drift prevention. If your component pipelines emitsha256:digests alongside these tags, switching todigest:here would eliminate the remaining mutable reference window. Updatescripts/sync-image-tags.shto readdigestfrom your registry or published manifests if adopting this approach.Additionally, verify that
overlays/development/kustomization.yaml(and overlays/vmaas-ci, overlays/hypershift2, overlays/caas-ci) intentionally keepghcr.io/osac-project/osac-aap:latestforAAP_EE_IMAGE. If the goal of MGMT-24125 is to fully eliminate runtime drift against submodule CRDs, this env var may also need pinning to match the base image tag.Not blocking — pinning by tag is an improvement over
:latest— but worth tracking as a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/kustomization.yaml` around lines 27 - 35, Replace mutable tag pins under the kustomize images (e.g., fulfillment-service, osac-aap, osac-operator) with immutable digest pins (use the registry-provided sha256:... value via the digest: field instead of newTag:) and update the image sync tool (scripts/sync-image-tags.sh) to fetch and write the digest values from the registry or published manifests; also review overlays/development/kustomization.yaml (and overlays/vmaas-ci, overlays/hypershift2, overlays/caas-ci) to confirm whether the AAP_EE_IMAGE env var must be changed from ghcr.io/osac-project/osac-aap:latest to a pinned digest to avoid runtime drift.scripts/sync-image-tags.sh (1)
36-46: Consider usingyqinstead ofgrep -A2+sedfor more robust YAML parsing.While the current code works because the YAML structure is consistent, the
grep -A2+sedapproach is architecturally fragile. It assumes a fixed 3-line layout and lacks semantic understanding of YAML structure. Reordering thenewNameandnewTagfields, adding comments, or future schema changes would silently break the detection or mis-target the update. Sinceyqis preinstalled onubuntu-latestrunners, using it provides structural queries immune to formatting changes:♻️ Example with yq
# read current tag current_tag=$(yq ".images[] | select(.name == \"${image}\") | .newTag" "${KUSTOMIZATION}") # fix in place yq -i "(.images[] | select(.name == \"${image}\") | .newTag) = \"${tag}\"" "${KUSTOMIZATION}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sync-image-tags.sh` around lines 36 - 46, Replace the fragile grep/awk/sed YAML handling around current_tag and the sed fix with yq queries: use yq to read current_tag from the images array by selecting the object where .name == "${image}" and getting .newTag (replace the current_tag assignment), and use an in-place yq update to set (.images[] | select(.name == "${image}") | .newTag) = "${tag}" instead of the sed invocation; keep the same conditional logic (checking "${current_tag}" == "${tag}", the --fix branch, echo messages and errors increment) and continue to operate on the same variables (KUSTOMIZATION, image, tag, current_tag, errors).
🤖 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/sync-image-tags.sh`:
- Around line 26-34: The loop that reads submodule commit SHAs uses tr -d '+'
and thus only strips '+' status prefixes; instead, normalize the status char
returned by git submodule status (could be space, '+', '-', or 'U') before using
commit/short/tag: detect and remove a single leading status character when
present, treat a leading '-' or 'U' as an error and exit early, and only then
set short="${commit:0:7}" and compute tag based on TAG_FORMAT; update the logic
around the commit/short/tag variables in the for-loop that iterates over
osac-operator/osac-fulfillment-service/osac-aap to implement these checks and
fail-fast behavior.
---
Nitpick comments:
In @.github/workflows/check-image-tags.yaml:
- Around line 10-18: Add an explicit least-privilege permissions block and a
per-job timeout to the check-image-tags job: update the job "check-image-tags"
(which runs the "Verify image tags match submodule commits" step that runs
scripts/sync-image-tags.sh) to include permissions: contents: read and a
timeout-minutes value (e.g., 10) at the job level so the job only has read
access to the repo/submodules and cannot run indefinitely.
In `@base/kustomization.yaml`:
- Around line 27-35: Replace mutable tag pins under the kustomize images (e.g.,
fulfillment-service, osac-aap, osac-operator) with immutable digest pins (use
the registry-provided sha256:... value via the digest: field instead of newTag:)
and update the image sync tool (scripts/sync-image-tags.sh) to fetch and write
the digest values from the registry or published manifests; also review
overlays/development/kustomization.yaml (and overlays/vmaas-ci,
overlays/hypershift2, overlays/caas-ci) to confirm whether the AAP_EE_IMAGE env
var must be changed from ghcr.io/osac-project/osac-aap:latest to a pinned digest
to avoid runtime drift.
In `@scripts/sync-image-tags.sh`:
- Around line 36-46: Replace the fragile grep/awk/sed YAML handling around
current_tag and the sed fix with yq queries: use yq to read current_tag from the
images array by selecting the object where .name == "${image}" and getting
.newTag (replace the current_tag assignment), and use an in-place yq update to
set (.images[] | select(.name == "${image}") | .newTag) = "${tag}" instead of
the sed invocation; keep the same conditional logic (checking "${current_tag}"
== "${tag}", the --fix branch, echo messages and errors increment) and continue
to operate on the same variables (KUSTOMIZATION, image, tag, current_tag,
errors).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb8fb7a8-97ff-4b27-be42-b846b2526562
📒 Files selected for processing (3)
.github/workflows/check-image-tags.yamlbase/kustomization.yamlscripts/sync-image-tags.sh
| for submodule in osac-operator osac-fulfillment-service osac-aap; do | ||
| commit=$(git -C "${REPO_ROOT}" submodule status "base/${submodule}" | awk '{print $1}' | tr -d '+') | ||
| short="${commit:0:7}" | ||
| image="${IMAGE_NAME[$submodule]}" | ||
|
|
||
| case "${TAG_FORMAT[$submodule]}" in | ||
| sha-SHORT) tag="sha-${short}" ;; | ||
| FULL) tag="${commit}" ;; | ||
| esac |
There was a problem hiding this comment.
Handle all git submodule status prefix characters, not just +.
git submodule status may prefix the SHA with one of: space (in-sync), + (checked-out commit differs from index), - (not initialized), or U (merge conflicts). tr -d '+' only strips +, so if a developer runs this locally without git submodule update --init they'll get commit="-abc1234…", which then yields tag="sha--abc1234" — and worst case --fix writes that garbage into base/kustomization.yaml. Trim the leading status char and fail fast on -/U.
🛡️ Suggested guard
for submodule in osac-operator osac-fulfillment-service osac-aap; do
- commit=$(git -C "${REPO_ROOT}" submodule status "base/${submodule}" | awk '{print $1}' | tr -d '+')
+ status_line=$(git -C "${REPO_ROOT}" submodule status "base/${submodule}")
+ case "${status_line:0:1}" in
+ '-') echo "${submodule}: submodule not initialized; run 'git submodule update --init --recursive'" >&2; exit 2 ;;
+ 'U') echo "${submodule}: submodule has merge conflicts" >&2; exit 2 ;;
+ esac
+ commit=$(awk '{print $1}' <<<"${status_line}" | sed 's/^[[:space:]+-]//')
short="${commit:0:7}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sync-image-tags.sh` around lines 26 - 34, The loop that reads
submodule commit SHAs uses tr -d '+' and thus only strips '+' status prefixes;
instead, normalize the status char returned by git submodule status (could be
space, '+', '-', or 'U') before using commit/short/tag: detect and remove a
single leading status character when present, treat a leading '-' or 'U' as an
error and exit early, and only then set short="${commit:0:7}" and compute tag
based on TAG_FORMAT; update the logic around the commit/short/tag variables in
the for-loop that iterates over osac-operator/osac-fulfillment-service/osac-aap
to implement these checks and fail-fast behavior.
a1a93aa to
3b444ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/sync-image-tags.sh`:
- Around line 36-46: Detect when the image block is missing before attempting to
compare or fix: after computing current_tag (from the grep/awk pipeline using
KUSTOMIZATION and image) check if current_tag is empty (or if the grep for
"name: ${image}$" returns no match) and if so print a clear error like "image
not found in kustomization: ${image}", increment the errors counter and skip the
sed --fix branch; only run the sed replacement (sed -i ... newTag) when the
image block exists to avoid reporting "FIXED -> ..." for an unchanged file and
to make CI failures explicit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a58ffd9-5361-4f7e-9cb7-0de21c43fab0
📒 Files selected for processing (3)
.github/workflows/check-image-tags.yamlbase/kustomization.yamlscripts/sync-image-tags.sh
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/check-image-tags.yaml
| current_tag=$(grep -A2 "name: ${image}$" "${KUSTOMIZATION}" | grep "newTag:" | awk '{print $2}') | ||
|
|
||
| if [[ "${current_tag}" == "${tag}" ]]; then | ||
| echo "${image}: OK (${tag})" | ||
| elif [[ "${1:-}" == "--fix" ]]; then | ||
| sed -i "/name: ${image}$/,/newTag:/{s|newTag:.*|newTag: ${tag}|}" "${KUSTOMIZATION}" | ||
| echo "${image}: FIXED ${current_tag} -> ${tag}" | ||
| else | ||
| echo "${image}: MISMATCH current=${current_tag} expected=${tag}" | ||
| errors=$((errors + 1)) | ||
| fi |
There was a problem hiding this comment.
Surface "image not found in kustomization" explicitly.
If grep "name: ${image}$" matches nothing (image renamed, block removed, indentation drift), current_tag becomes the empty string. In dry-run that produces a vague MISMATCH current= expected=... line, and in --fix the sed range pattern also matches nothing, so the script reports FIXED -> <tag> while the YAML is silently unchanged — a subtle CI green / file unchanged failure mode.
A small defensive check makes the failure obvious:
🛡️ Suggested guard
- current_tag=$(grep -A2 "name: ${image}$" "${KUSTOMIZATION}" | grep "newTag:" | awk '{print $2}')
+ current_tag=$(grep -A2 "name: ${image}$" "${KUSTOMIZATION}" | grep "newTag:" | awk '{print $2}')
+
+ if [[ -z "${current_tag}" ]]; then
+ echo "${image}: ERROR no 'name: ${image}' / newTag entry found in ${KUSTOMIZATION}" >&2
+ errors=$((errors + 1))
+ continue
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| current_tag=$(grep -A2 "name: ${image}$" "${KUSTOMIZATION}" | grep "newTag:" | awk '{print $2}') | |
| if [[ "${current_tag}" == "${tag}" ]]; then | |
| echo "${image}: OK (${tag})" | |
| elif [[ "${1:-}" == "--fix" ]]; then | |
| sed -i "/name: ${image}$/,/newTag:/{s|newTag:.*|newTag: ${tag}|}" "${KUSTOMIZATION}" | |
| echo "${image}: FIXED ${current_tag} -> ${tag}" | |
| else | |
| echo "${image}: MISMATCH current=${current_tag} expected=${tag}" | |
| errors=$((errors + 1)) | |
| fi | |
| current_tag=$(grep -A2 "name: ${image}$" "${KUSTOMIZATION}" | grep "newTag:" | awk '{print $2}') | |
| if [[ -z "${current_tag}" ]]; then | |
| echo "${image}: ERROR no 'name: ${image}' / newTag entry found in ${KUSTOMIZATION}" >&2 | |
| errors=$((errors + 1)) | |
| continue | |
| fi | |
| if [[ "${current_tag}" == "${tag}" ]]; then | |
| echo "${image}: OK (${tag})" | |
| elif [[ "${1:-}" == "--fix" ]]; then | |
| sed -i "/name: ${image}$/,/newTag:/{s|newTag:.*|newTag: ${tag}|}" "${KUSTOMIZATION}" | |
| echo "${image}: FIXED ${current_tag} -> ${tag}" | |
| else | |
| echo "${image}: MISMATCH current=${current_tag} expected=${tag}" | |
| errors=$((errors + 1)) | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sync-image-tags.sh` around lines 36 - 46, Detect when the image block
is missing before attempting to compare or fix: after computing current_tag
(from the grep/awk pipeline using KUSTOMIZATION and image) check if current_tag
is empty (or if the grep for "name: ${image}$" returns no match) and if so print
a clear error like "image not found in kustomization: ${image}", increment the
errors counter and skip the sed --fix branch; only run the sed replacement (sed
-i ... newTag) when the image block exists to avoid reporting "FIXED -> ..."
for an unchanged file and to make CI failures explicit.
|
/retest |
| - name: osac-operator | ||
| newName: ghcr.io/osac-project/osac-operator | ||
| newTag: latest | ||
| newTag: 762dbf9ea1410dcaeac87950dea36b778d127189 |
There was a problem hiding this comment.
Consider fixing the tag format in osac-aap to use short SHA to match operator & fulfillment. This inconsistency is fragile and undocumented
| newName: quay.io/sclorg/postgresql-15-c9s | ||
| newTag: latest | ||
| - name: fulfillment-service | ||
| newName: ghcr.io/osac-project/fulfillment-service |
There was a problem hiding this comment.
The newName removal breaks the kustomization - this only works if the Deployment manifests already reference the full ghcr path. If the base manifests use short names like fulfillment-service, kustomize won't know what to replace.
The base kustomization used :latest tags for osac-operator, fulfillment-service, and osac-aap. This causes the running binary to drift from the CRDs pinned by the submodule, leading to silent runtime failures when fields are renamed or added. Pin each image to the SHA tag matching its submodule commit. Add scripts/sync-image-tags.sh to validate and auto-fix tags. Add a CI check that fails PRs if tags don't match submodules.
3b444ae to
9c4d659
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eranco74, omer-vishlitzky The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
3 participants
https://redhat.atlassian.net/browse/MGMT-24125
Pin osac-operator, fulfillment-service, and osac-aap image tags to the SHA matching their submodule commit instead of floating :latest.
Adds
scripts/sync-image-tags.shwhich validates tags match submodules (run with--fixto auto-update). Adds a CI check that fails PRs on mismatch.Summary by CodeRabbit