OSAC-2185: Gate chart publish on image build success - #81
Conversation
Trigger publish-charts.yaml via workflow_run on the sibling image workflow completing instead of independently on push:tags:v*. A new guard job checks the image build concluded successfully for the same tag before any chart publishing proceeds, and fails loudly if it did not. Assisted-by: Cursor <noreply@cursor.com>
|
@minmzzhang: This pull request references OSAC-2185 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe chart publishing workflow now runs after successful container image publication, validates the upstream version tag and commit, checks out that commit, uses its tag for chart versioning, verifies tag stability, and creates or edits the matching GitHub Release. ChangesChart publishing workflow
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Sequence Diagram(s)sequenceDiagram
participant ImageWorkflow
participant Guard
participant ChartPublisher
participant GitHubAPI
participant GitHubRelease
ImageWorkflow->>Guard: workflow_run completion, head_branch, head_sha
Guard->>ChartPublisher: validated tag and commit SHA
ChartPublisher->>GitHubAPI: verify tag resolves to guarded SHA
ChartPublisher->>GitHubRelease: create or edit release for TAG
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish-charts.yaml (1)
57-64: 🔒 Security & Privacy | 🔵 TrivialChart artifact is pushed without a signature.
The OCI-packaged chart is pushed to the registry with no Sigstore/cosign signing step, so consumers can't verify provenance. Consider signing the packaged chart (or the resulting OCI artifact) with cosign before/after
helm push.As per path instructions, "Sign artifacts with Sigstore/cosign."
🤖 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/publish-charts.yaml around lines 57 - 64, Add a Sigstore/cosign signing step for the packaged chart artifact in the workflow around helm package and helm push. Sign the chart or resulting OCI artifact using the existing chart_file and registry references, ensuring signing occurs before consumers can retrieve the pushed artifact and preserving the current packaging and push flow.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.
Inline comments:
In @.github/workflows/publish-charts.yaml:
- Around line 20-26: Update the affected workflow steps, including “Check image
build result” and the steps using github.actor, secrets.GITHUB_TOKEN, and
github.repository, to pass these GitHub expressions through step-level env
variables. Reference only the corresponding shell variables inside each run
script, preserving the existing validation, logging, and publish behavior while
eliminating direct template interpolation in shell commands.
- Around line 13-18: Harden the release guard that sets outputs in the workflow
by validating that workflow_run.head_branch is a semver-style tag rather than
merely starting with “v”, or document the required coupling to
publish-image.yaml if retaining the current check. Ensure branch names beginning
with “v” cannot trigger chart publication unless they match the intended tag
format.
- Around line 36-40: Update the actions/checkout step in the publish workflow to
pin uses to its complete immutable commit SHA instead of the mutable v4 tag, and
retain the corresponding version in an inline comment.
- Around line 10-19: Add an explicit empty permissions declaration to the guard
job in the publish workflow, alongside its existing name, runs-on, and if
configuration. Keep the guard job’s outputs and steps unchanged, ensuring it
runs without any unnecessary GITHUB_TOKEN permissions.
---
Outside diff comments:
In @.github/workflows/publish-charts.yaml:
- Around line 57-64: Add a Sigstore/cosign signing step for the packaged chart
artifact in the workflow around helm package and helm push. Sign the chart or
resulting OCI artifact using the existing chart_file and registry references,
ensuring signing occurs before consumers can retrieve the pushed artifact and
preserving the current packaging and push flow.
🪄 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: Enterprise
Run ID: 3f339e31-029e-46e4-9b75-9cf1a095802d
📒 Files selected for processing (1)
.github/workflows/publish-charts.yaml
- Scope guard job to permissions: {} (was inheriting default token scope)
- Pass workflow_run.head_branch/conclusion through env instead of direct
${{ }} interpolation in shell (script-injection hardening)
- Validate the tag against a proper semver grammar in guard, not just
startsWith(..., 'v')
- Pin actions/checkout to a full SHA (v4.2.2) instead of the mutable v4 tag
- Verify the tag still resolves to the guarded commit immediately before
creating the GitHub Release, guarding against a force-push/retag race
Note: not adding Sigstore/cosign chart signing here - that's a separate,
larger effort beyond this ticket's scope, flagged for a follow-up if desired.
Assisted-by: Cursor <noreply@cursor.com>
|
Pushed a follow-up commit addressing the CodeRabbit review:
Intentionally not doing in this PR: Sigstore/cosign signing of the published chart artifacts. That's a separate, larger effort (registry/verification infra, signing key strategy) that's outside OSAC-2185's scope (gating chart publish on image build success). Happy to track it as its own follow-up ticket if wanted. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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-charts.yaml:
- Around line 27-28: Update the semver_re validation used by the publish
workflow so HEAD_BRANCH follows strict SemVer numeric rules, rejecting leading
zeros in major, minor, patch, and numeric prerelease identifiers while
preserving valid tags with the v prefix and supported prerelease/build metadata.
- Around line 27-28: Update the semver_re validation in the publish workflow to
reject release tags containing +build metadata, while preserving valid
v-prefixed semantic versions and prerelease suffixes. Ensure chart publishing
accepts only tags whose resulting image references are compatible with the image
workflow.
- Around line 76-80: Update the publish workflow’s chart publication steps to
perform keyless Sigstore/cosign signing of the OCI chart after it is pushed to
GHCR, and add the minimal required OIDC and package permissions without granting
broader access. Ensure the signing step references the exact published chart
digest or tag and preserves the existing guarded-commit verification flow.
- Around line 76-86: Move the tag verification step using CURRENT_SHA and
GUARDED_SHA before the helm push and release creation steps, and invoke gh
release create with --verify-tag so it refuses to recreate a missing tag.
Preserve the existing mismatch failure behavior and ensure both chart publishing
and release creation use the verified immutable tag.
- Around line 82-84: Update the tag verification logic around CURRENT_SHA to
dereference annotated tags and resolve refs/tags/${TAG} to the underlying commit
SHA before comparing with GUARDED_SHA. Preserve the existing mismatch error and
release refusal behavior once both values represent commit SHAs.
🪄 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: Enterprise
Run ID: fbca7bfc-1334-4a36-ab5d-3cc13f12b7c0
📒 Files selected for processing (1)
.github/workflows/publish-charts.yaml
- Extract the tag-vs-guarded-commit verification into a shared .github/scripts/verify-tag-matches-sha.sh instead of duplicating the bash logic at each checkpoint - Dereference annotated tags before comparing SHAs (the ref lookup returns the tag-object SHA, not the commit SHA, for annotated tags) - Add --verify-tag to gh release create as an extra safety net - Reject SemVer tags with leading zeros (e.g. v01.2.3, v1.2.3-01) The early (post-checkout) and late (pre-release) verification checkpoints were already both present from the previous round; this just removes the duplicated bash between them. Assisted-by: Cursor <noreply@cursor.com>
|
Pushed a follow-up commit addressing round 2 of the CodeRabbit review:
One finding I did not apply: the claim that |
992c05c to
c0f34b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-charts.yaml:
- Line 27: The semver validation regex in the publish workflow still permits
+build metadata that Docker tags cannot use. Update the semver_re validation
used for app_version to reject build-metadata suffixes while preserving valid
v-prefixed semantic versions and existing prerelease validation.
- Around line 60-67: Update the publish chart workflow’s run blocks, including
the step identified by id “run,” to remove direct GitHub expression
interpolation from shell commands. Pass github.actor, secrets.GITHUB_TOKEN, and
github.repository through step environment variables, then reference those
variables inside the shell; reuse the existing REPO environment variable for
repository-slug usages around the chart publishing commands.
🪄 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: Enterprise
Run ID: d134ee1c-a252-41d0-8348-500a5f385048
📒 Files selected for processing (2)
.github/scripts/verify-tag-matches-sha.sh.github/workflows/publish-charts.yaml
- Reject SemVer tags with build metadata (+...) - Docker/OCI tags cannot contain '+', so a tag like v1.2.3+build.1 would pass the old regex but produce an unusable image reference downstream - Route github.actor/secrets.GITHUB_TOKEN/github.repository through env in the chart-packaging step instead of splicing them directly into the run: shell (script-injection hardening, missed in this step during the earlier rounds even though it was applied elsewhere) - Same for the repo slug in the Create GitHub Release step Assisted-by: Cursor <noreply@cursor.com>
|
Pushed a follow-up commit addressing round 3 of the CodeRabbit review:
|
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-charts.yaml:
- Around line 100-107: Update the release flow around gh release create and gh
release edit so the tag is protected from force-moves or its SHA is revalidated
immediately before publishing. Ensure the release can only be created or edited
for the commit matching GUARDED_SHA, preserving the existing guarded-tag
workflow.
🪄 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: Enterprise
Run ID: e549a005-d8d0-4c7d-a3aa-935197825767
📒 Files selected for processing (1)
.github/workflows/publish-charts.yaml
- In the shared verify-tag-matches-sha.sh: capture the gh api response in
a variable before parsing with read (a failing API call under set -e
was previously masked by read's own exit status, falling through to a
generic tag-mismatch error instead of surfacing the real failure)
- Switch to the officially documented singular git/ref/{ref} endpoint for
retrieval instead of the plural git/refs/{ref} form (only documented
for PATCH/DELETE) - both return identical data today, but only the
documented one is guaranteed to keep doing so
Assisted-by: Cursor <noreply@cursor.com>
|
Pushed a follow-up commit fixing two issues in the shared
|
|
/lgtm as discussed offline with @minmzzhang we should extract common parts to a common workflow later |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: minmzzhang, rawagner, rccrdpccl 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 |
4 participants
Summary
publish-charts.yamlandpublish-image.yamlcurrently trigger independently off the samev*tag push, with no dependency between them — a chart can be marked "published" even if the matching container image never got built. This exact class of gap caused the OSAC-2174 production incident in a sibling repo (chart referenced an image that was never built, resulting inImagePullBackOff).This PR changes
publish-charts.yamlto trigger viaworkflow_runonPublish container imagecompleting, gated by a newguardjob:main).The
publish-ui-chartjob nowneeds: guard, pins its checkout toneeds.guard.outputs.sha, and usesneeds.guard.outputs.taginstead of$(git describe --tags)/$GITHUB_REF_NAME(which don't reliably resolve to the tag under aworkflow_runevent) in both the chart-packaging script and the "Create GitHub Release" step.Testing
Live end-to-end verified on a personal fork, not just static analysis:
v*test tag → image build ran → succeeded →publish-chartstriggered afterward (not in parallel) → chart published → GitHub Release created. Full run: green.pushtomain(non-tag) completing the image workflow correctly results inpublish-chartsbeing skipped, not a false green.actionlint(0 errors) on the final workflow file.Acceptance criteria (OSAC-2185)
publish-chartsto fail loudly rather than complete green.Jira: https://redhat.atlassian.net/browse/OSAC-2185
Summary by CodeRabbit