OSAC-3490: Use explicit per-component IMAGE_NAME to fix ghcr.io collision - #39
Conversation
WalkthroughThe workflows now use fixed image repositories for the operator and fulfillment service instead of deriving repositories from the GitHub repository. ChangesContainer image identity updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/publish-image.yaml:
- Around line 28-33: Align the image repository used by the workflow and
deployment chart: update the repository reference in the chart’s service values
configuration to match IMAGE_NAME, ghcr.io/osac-project/fulfillment-service,
while preserving the existing image tag behavior.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6fcaf6d6-b8a8-47e5-8920-d1a627bff3fa
📒 Files selected for processing (2)
.github/workflows/build-image.yaml.github/workflows/publish-image.yaml
|
Good catch from the CodeRabbit review -- confirmed and fixed in the latest commit. publish-charts.yaml's publish-service-chart job had the same root-cause leak: it independently derived REPO from github.repository to build the image reference written into charts/service/values.yaml, which would have pointed fulfillment-service's chart at a now-nonexistent ghcr.io/osac-project/osac image once the IMAGE_NAME fix lands. Re-read the full file to confirm scope before touching anything: REPO is used exactly once in that job (the image-reference construction), so hardcoding it to osac-project/fulfillment-service is safe. Left every other REPO usage in the file untouched -- osac-operator's three jobs all use REPO correctly for real gh api/gh release create calls against the actual repo, and confirmed osac-operator's own chart hardcodes its repository field statically already (this file's operator job only ever sed-replaces the tag), so osac-operator was never affected by this particular leak. |
|
Reviewed from scratch — pulled the branch fresh, read the full current content of all three files, and independently verified every claim rather than trusting the description. (1) Content confirmed as described. (2) osac-operator's chart genuinely unaffected — verified via both the values file AND the full job steps, not just one: And That (3) Fulfillment-service (4) Grepped and reasoned through every
Also confirmed: Verdict: both commits are correct, complete, and properly scoped. No third leak point found. Good to merge (mind the likely rebase against #29 once that lands). |
2d724e0 to
0504c5c
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz 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 |
|
@eliorerz: This pull request references OSAC-3490 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 bug 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. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-image.yaml (1)
75-75: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin all actions to full commit SHAs.
Lines 75, 81, 114, 136, and 162 use mutable major tags. Replace each ref with a 40-character commit SHA and retain the action version in a comment.
As per path instructions,
.github/workflows/**/*files must pin actions by full SHA, not tag.Also applies to: 81-81, 114-114, 136-136, 162-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-image.yaml at line 75, Update every action reference in the workflow, including the steps at lines 75, 81, 114, 136, and 162, to use its immutable 40-character commit SHA instead of a mutable major tag, and retain the corresponding action version in an inline comment.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/build-image.yaml:
- Line 75: Update every action reference in the workflow, including the steps at
lines 75, 81, 114, 136, and 162, to use its immutable 40-character commit SHA
instead of a mutable major tag, and retain the corresponding action version in
an inline comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ceb7624a-1c15-4a44-bf9b-c15d1739c27a
📒 Files selected for processing (3)
.github/workflows/build-image.yaml.github/workflows/publish-charts.yaml.github/workflows/publish-image.yaml
Removed several approvers and reviewers, added new ones.
…project#1316) OSAC project context was available through bootstrap-managed design files and Claude rules, so a fresh checkout or a coding task without a planning skill could miss it. Track the full context in `docs/agent-context/` and route every agent there through root/component `AGENTS.md`. - Preserve networking decisions, Enclave Wizard integration, feature dimensions, and review expectations alongside the code. Refresh the networking summary against accepted designs: IPv4, one tenant attachment, immutable create/read/delete contracts, and readiness/deletion guards. Distinguish target contracts from current implementation. - Correct former workspace/docs/installer/UI/E2E paths, document ownership and maintenance, and retain essential Git/Jira conventions in root `AGENTS.md`. - Add thin root `CLAUDE.md` and `GEMINI.md` imports of `AGENTS.md`; Codex and Cursor use the canonical instructions directly. - Coordinate with [enhancement-proposals osac-project#338](osac-project/enhancement-proposals#338) and [osac-ai-skills osac-project#39](osac-project/osac-ai-skills#39). The EP workflow stages full canonical context from one recorded OSAC commit into a single `reference/` tree shared by both review workspaces. Review skills use `docs/` in OSAC checkouts and `reference/` in CI. **Merge order: this OSAC PR → enhancement-proposals osac-project#338 → osac-ai-skills osac-project#39.** This keeps automated EP reviews supplied with complete context while shared skills switch to forwarding documents. Jira: https://redhat.atlassian.net/browse/OSAC-3245 ## Validation - Relative documentation links and all four workflow forwarding targets resolve; all owned AGENTS instruction chains fit 32 KiB. - Read-only Codex discovery from a fresh worktree without bootstrap: networking metadata, CLI/API request headers, installer values, and PRD drafting all found the required context. - OSAC consumer fan-out smoke against the updated shared repository; shared PROJECT_ROOT and Codex fan-out smoke tests pass. - Installer `make helm-validate` with CI's Helm v3.22.0 passes. - Applicable pre-commit hooks, including staged gitleaks, pass. - Shared skillsaw lint and skill-version checks pass; PRD/design evaluation harness workspace smoke passes. Full model-scored review evaluations were not run. - Performance, security, and simplification preflight reviews: no findings. No runtime code or API schemas change. --- _This PR description was drafted with AI assistance ([create-pr](https://github.com/osac-project/osac-ai-skills/tree/main/skills/create-pr) v0.2.0). Review for accuracy._ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary This change adds canonical project context in `docs/agent-context/` and routes coding agents to it through root and installer `AGENTS.md` files. Root `CLAUDE.md` and `GEMINI.md` now import `AGENTS.md`. The Codex guide explains how to load component instructions and follow context references. The new documents cover networking decisions, Enclave Wizard integration, feature dimensions, and review patterns. The networking guidance distinguishes target contracts from current implementation. It covers IPv4, tenant attachment limits, resource lifecycle operations, readiness, and deletion guards. The change also updates AI policy and documentation indexes. It corrects references to canonical instructions and context sources. ## Impact - **API surface, controllers, database, and authentication:** No changes reported. - **Deployment:** No runtime or deployment behavior changes reported. The new guidance describes installer and Wizard workflows. - **CI and tests:** The supplied summary reports validation and smoke checks, but does not provide independently verifiable results here. No test files changed in the supplied change summary. - **Documentation:** Adds the canonical context documents and updates agent instructions and documentation guides. - **Backward compatibility:** No runtime compatibility impact is reported. Existing `.design/context/*.md` references are intended to forward to the new documents; their forwarding changes are outside this PR. ## Risk classification Risk label and classification criteria are unavailable in the supplied information. The labeling instructions and applied label were not provided, so this summary cannot determine the classification or whether the PR was close to another label. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Eran Cohen <eranco@redhat.com>
Summary
Fixes a currently-live bug:
osac-operator'sbuild-image.yamlandfulfillment-service'spublish-image.yamlboth setIMAGE_NAME: ${{ github.repository }}. Before the mono-repo merge that resolved to two different values; now both resolve toosac-project/osacfor both workflows, so both components push toghcr.io/osac-project/osacand stomp each other's mutable:latest/:maintags -- last-write-wins between two entirely different binaries.sha-<commit>tags don't collide since they're unique per commit; only the mutable tags do.Fix
Gave each workflow an explicit, component-specific
IMAGE_NAMEliteral instead of deriving it fromgithub.repository, restoring the original per-component image names:build-image.yaml(osac-operator):IMAGE_NAME: osac-project/osac-operatorpublish-image.yaml(fulfillment-service):IMAGE_NAME: osac-project/fulfillment-serviceVerification
Read both files' full current content fresh rather than trusting the ticket's line numbers (they'd shifted by one from the ticket's snapshot). Traced every
IMAGE_NAME/REGISTRY/github.repositoryusage in both files, not just the env line:build-image.yamlhas twoimages: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}references (the main manager image and the manifest-container image) -- both correctly inherit from the same corrected env var, so the manifest image also lands under the right per-component path.publish-image.yamlhas exactly one such reference.IMAGE_NAME/REGISTRYderivation exists in either file, and no othergithub.repositoryusage in either file relates to image naming.Also grepped every workflow in the repo for
github.repositoryto make sure no other file has the same unfixed anti-pattern: everything else (e2e-*.yml, publish-charts.yaml, scan-workflow-logs.yml) uses it for benign repo-identification purposes (checkout source,gh api/release calls), never as part of an image name. Found one directly relevant precedent while doing this:osac-aap's ownexecution-environment.ymlalready hardcodesimages: ghcr.io/osac-project/osac-aapwith a comment explicitly calling out this exact collision class -- confirming someone already applied the same fix pattern proactively for that component. This PR brings the other two components in line with that established pattern.Also searched the mono-repo and org-wide for any file (Helm values, kustomize, docs) hardcoding the currently-broken shared
ghcr.io/osac-project/osacpath expecting it to mean a specific component -- found none. (The only otherghcr.io/osac-project/osac-prefixed hits found wereosac-csi-driver's own image name in a different repo, unrelated.)Validated with a YAML parse check and
yamllint -c .yamllint.yaml --stricton both files -- clean.Follow-up fix: same collision leaking into the service chart (via CodeRabbit review)
publish-charts.yaml'spublish-service-chartjob (fulfillment-service) independently derivedREPO: ${{ github.repository }}and used it to buildapp_image="${registry}/${REPO}:${app_version}", written intocharts/service/values.yaml'simages.servicefield at release time. Same root cause, a second leak point: once theIMAGE_NAMEfix above lands, fulfillment-service's real image only exists atghcr.io/osac-project/fulfillment-service, so this job would have injected a reference to an image that no longer exists atghcr.io/osac-project/osac-- breaking every real fulfillment-service chart release.Fixed by hardcoding that job's
REPOenv var toosac-project/fulfillment-service, matching theIMAGE_NAMEset inpublish-image.yaml.Re-read the full file to confirm scope:
REPOinpublish-service-chartis used exactly once, only for the image-reference construction above -- nothing else in that job needs it. The other fourREPO: ${{ github.repository }}occurrences in this file (osac-operator'spublish-operator-crds-chart/publish-operator-chart/create-operator-releasejobs) all use it correctly for realgh api/gh release createcalls against the actual triggering repo and are untouched -- confirmed osac-operator's own chart (charts/operator/values.yaml) already hardcodes itsrepository:field statically and this file's operator job only ever sed-replaces the tag, never the repository, so osac-operator was never affected by this particular leak.Validated with a YAML parse check and
yamllint -c .yamllint.yaml --strict-- clean.Unblocks
OSAC-3368 (updating the enclave repo's hardcoded image references) was blocked on this -- there was no correct, stable image name to point at for either component. This restores
ghcr.io/osac-project/osac-operatorandghcr.io/osac-project/fulfillment-serviceas the correct stable names, matching what OSAC-3368 already expects.Summary by CodeRabbit