fix(build): use FullCommit in goreleaser to match CI image tags#658
fix(build): use FullCommit in goreleaser to match CI image tags#658
Conversation
Replace the commit-based release flow with tag-only operations so that pre-release (RC/beta) tags can be promoted to stable on the exact same SHA. Changelog output (tools/changelog) feeds GitHub Release notes instead of being committed to the repository.
ShortCommit (7 chars) produced :sha-<7chars> tags but on-push.yaml tags images with the full 40-char github.sha. The mismatch causes ImagePullBackOff for dev-build validator images. Switch to FullCommit and add a contract test that documents the SHA-length dependency.
This comment was marked as resolved.
This comment was marked as resolved.
Coverage Report ✅
Coverage BadgeNo Go source files changed in this PR. |
njhensley
left a comment
There was a problem hiding this comment.
This PR nominally does one thing (flip ShortCommit → FullCommit) but its diff contains two logically separate changes:
- The stated fix —
.goreleaser.yaml+ a new contract test. Exactly what we wanted coming out of #655. ~10 lines. - An unannounced release-workflow refactor — removes
bump-prepare/bump-finalize/bump-abort, addsbump-promoteand a newtools/changelogscript, stops committingCHANGELOG.md, rewritesRELEASING.md+site/docs/project/releasing.md. ~170 lines across 6 files.
The PR title, description, and checked "Type of Change" cover only (1). A reviewer focused on the #655 fix will approve without noticing (2).
Recommendation: split into two PRs. The FullCommit fix is tiny and should land immediately. The release refactor deserves its own title, description, and review — the RC-to-stable same-SHA promotion flow is genuinely useful, but it has edge cases (see inline) that shouldn't ride in under an unrelated bugfix.
If you'd rather keep it bundled, at minimum update the PR description and title to cover both changes, and consider whether it should land as a merge commit (preserve the two logical commits) rather than a squash (collapses to an understated title).
Other repo-state concern not easily anchored inline: CHANGELOG.md is now orphaned. After this PR, nothing updates it, but it still sits in the repo. Either delete it or prepend a header noting it's historical and that current release notes live in GitHub Releases.
Inline comments cover the specifics.
- Contract test now verifies .goreleaser.yaml contains FullCommit - tools/changelog: fix || "" to || true for pipefail safety - tools/changelog: emit unknown cliff.toml groups instead of dropping - tools/bump promote: verify tag via refs/tags/ (not generic rev-parse) - tools/bump promote: check SHA is reachable from origin/main
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tools/changelog (1)
21-24:⚠️ Potential issue | 🟠 MajorGrep pipeline can abort the script when output is empty.
Under
set -euo pipefail, ifgit-cliffproduces no output or all lines are filtered by thegrepchain, the pipeline exits with code 1 and the script aborts. This was flagged in a previous review and remains unaddressed.Consider appending
|| trueto the pipeline or replacing thegrepchain with a singleawkthat doesn't fail on empty input:🛠️ Proposed fix using `|| true`
# Generate raw changelog, strip noise lines -RAW=$(git-cliff --unreleased --strip header \ - | grep -v '<!-- .* -->' \ - | grep -vi 'Signed-off-by:' \ - | grep -vi 'Co-authored-by:') +RAW=$(git-cliff --unreleased --strip header \ + | grep -v '<!-- .* -->' \ + | grep -vi 'Signed-off-by:' \ + | grep -vi 'Co-authored-by:' || true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/changelog` around lines 21 - 24, The pipeline assigning RAW can fail under set -euo pipefail if git-cliff yields no lines and the grep chain exits nonzero; update the assignment that uses `git-cliff --unreleased --strip header | grep -v '<!-- .* -->' | grep -vi 'Signed-off-by:' | grep -vi 'Co-authored-by:'` so it never causes the script to abort—either append `|| true` to the pipeline or replace the grep chain with a single awk filter that safely handles empty input; ensure the variable RAW remains set (possibly to an empty string) when no lines match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tools/changelog`:
- Around line 21-24: The pipeline assigning RAW can fail under set -euo pipefail
if git-cliff yields no lines and the grep chain exits nonzero; update the
assignment that uses `git-cliff --unreleased --strip header | grep -v '<!-- .*
-->' | grep -vi 'Signed-off-by:' | grep -vi 'Co-authored-by:'` so it never
causes the script to abort—either append `|| true` to the pipeline or replace
the grep chain with a single awk filter that safely handles empty input; ensure
the variable RAW remains set (possibly to an empty string) when no lines match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 300ac19d-4d34-40ab-be0c-714d2067e22a
📒 Files selected for processing (3)
pkg/validator/catalog/catalog_test.gotools/bumptools/changelog
njhensley
left a comment
There was a problem hiding this comment.
Follow-up fixes address all four inline comments cleanly: contract test now actually enforces FullCommit in .goreleaser.yaml, || true replaces || "", unknown cliff.toml groups are emitted instead of dropped, and cmd_promote verifies the RC SHA is an ancestor of origin/main (plus tightens tag lookup to refs/tags/). LGTM.
Summary
Switch goreleaser ldflags from
{{.ShortCommit}}(7 chars) to{{.FullCommit}}(40 chars) so dev-build image tags match whaton-push.yamlactually pushes.Motivation / Context
PR #655 added commit-based image resolution for dev builds, but the CLI receives a 7-char
ShortCommitvia goreleaser while CI tags images with the full 40-chargithub.sha. The resulting:sha-<7chars>tag doesn't exist in the registry, causingImagePullBackOff.Fixes: #655
Related: #654
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/api,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
.goreleaser.yamllines 34 and 69:{{.ShortCommit}}→{{.FullCommit}}for bothaicrandaicrdbuildsTestResolveImageCIContractthat documents the full-vs-short SHA dependency and will catch future driftaicr --versionnow prints a 40-char SHA instead of 7Testing
go test -race ./pkg/validator/catalog/... golangci-lint run -c .golangci.yaml ./pkg/validator/catalog/...All tests pass, zero lint issues. New
TestResolveImageCIContractverifies full SHA produces a different (correct) tag than short SHA.Risk Assessment
Rollout notes: No migration needed. Only affects the commit string baked into binaries at build time. Release builds are unaffected (version tag takes precedence over commit).
Checklist
make testwith-race)make lint)git commit -S)