diff --git a/README.md b/README.md index c79054b..92cef14 100644 --- a/README.md +++ b/README.md @@ -116,8 +116,23 @@ release. Some options such as `--check-pr` will modify this behavior. yet. 6. **Upload release assets**. Any files written to `$ASSETS_DIR` will be uploaded as release assets. -7. **Publish the release**. -8. **Emit output** including release version, tag, change level, etc. +7. **Push the release target** (e.g. a bake commit produced by the pre-tag + hook) to its branch on the remote. Pushing the target before publishing + ensures that on a repository with + [Immutable Releases](https://docs.github.com/en/code-security/concepts/supply-chain-security/immutable-releases) + enabled, a rejected push to a protected branch does not permanently reserve + the tag name. +8. **Publish the release**. +9. **Emit output** including release version, tag, change level, etc. + +If the push in step 7 fails, the draft release and the pushed tag are cleaned +up so the run can be retried. If step 8 (publish) fails, the draft release, +the tag, and the pushed target are left in place — once publish has been +attempted there is no way to tell whether the server processed it, and +deleting the release or tag at that point would permanently reserve the tag +name on Immutable Releases repositories. Recover by publishing the draft +release manually, or by deleting the draft release and tag and reverting the +target branch by hand. ## Recipes diff --git a/release.go b/release.go index e5b2c66..e0ff54d 100644 --- a/release.go +++ b/release.go @@ -65,8 +65,23 @@ func (o *Runner) cleanupAfterErr() error { return err } -func (o *Runner) addErrCleanup(fn func() error) { - o.errCleanups = append(o.errCleanups, fn) +// addErrCleanup registers fn to run during cleanupAfterErr. The returned +// cancel function deactivates the cleanup so that it becomes a no-op. Callers +// must invoke cancel once the action that fn would undo has reached a point +// where rolling it back is no longer safe or correct — for example, after +// PublishRelease has been attempted, because the server may have processed +// the publish even if the client saw an error, and on a repository with +// Immutable Releases enabled, deleting either the release or its tag in that +// state permanently reserves the tag name. +func (o *Runner) addErrCleanup(fn func() error) (cancel func()) { + active := true + o.errCleanups = append(o.errCleanups, func() error { + if !active { + return nil + } + return fn() + }) + return func() { active = false } } type Result struct { @@ -327,7 +342,7 @@ func (o *Runner) run(ctx context.Context) (_ *Result, errOut error) { if err != nil { return nil, err } - o.addErrCleanup(func() error { + cancelTagDelete := o.addErrCleanup(func() error { _, e := o.runCmd(ctx, nil, "git", "push", o.PushRemote, "--delete", result.ReleaseTag) return e }) @@ -335,7 +350,7 @@ func (o *Runner) run(ctx context.Context) (_ *Result, errOut error) { result.CreatedTag = true if o.CreateRelease { - err = o.createRelease(ctx, result) + err = o.createRelease(ctx, result, cancelTagDelete) if err != nil { return nil, err } @@ -356,7 +371,17 @@ func (o *Runner) rejectShallowCheckout(ctx context.Context) error { } // createRelease handles the creation of a GitHub release, including notes, assets, and publishing. -func (o *Runner) createRelease(ctx context.Context, result *Result) error { +// +// To remain compatible with repositories that have Immutable Releases enabled +// (https://docs.github.com/en/code-security/concepts/supply-chain-security/immutable-releases), +// the release is created as a draft, all assets are uploaded, the release +// target is pushed, and only then is the release published. This matches the +// workflow GitHub recommends for immutable releases: create draft → upload +// assets → publish. Once PublishRelease has been attempted both destructive +// cleanups (DeleteRelease and the tag delete) are cancelled, because the +// server may have processed the publish even if the client saw an error, and +// deleting either side in that state permanently reserves the tag name. +func (o *Runner) createRelease(ctx context.Context, result *Result, cancelTagDelete func()) error { releaseNotes, err := o.getReleaseNotes(ctx, result) if err != nil { return err @@ -368,7 +393,7 @@ func (o *Runner) createRelease(ctx context.Context, result *Result) error { return err } - o.addErrCleanup(func() error { + cancelDeleteRelease := o.addErrCleanup(func() error { return o.GithubClient.DeleteRelease(ctx, o.repoOwner(), o.repoName(), rel.ID) }) @@ -382,13 +407,21 @@ func (o *Runner) createRelease(ctx context.Context, result *Result) error { return nil } - err = o.GithubClient.PublishRelease(ctx, o.repoOwner(), o.repoName(), o.MakeLatest, rel.ID) + // Push the target before publishing so the release stays a draft (and no + // immutable-tag row exists) until the most failure-prone step — the push + // to a possibly-protected branch — has succeeded. + err = o.pushTarget(ctx) if err != nil { return err } - // push target last because it cannot be easily rolled back - return o.pushTarget(ctx) + // Publish boundary: from here on, neither the release nor the tag may be + // deleted by cleanup. A PublishRelease error does not guarantee the + // server did not process the publish. + cancelDeleteRelease() + cancelTagDelete() + + return o.GithubClient.PublishRelease(ctx, o.repoOwner(), o.repoName(), o.MakeLatest, rel.ID) } func (o *Runner) uploadAssets(ctx context.Context, uploadURL string) error { diff --git a/release_test.go b/release_test.go index 6061ad5..7ede9ae 100644 --- a/release_test.go +++ b/release_test.go @@ -764,4 +764,127 @@ echo bar > "$ASSETS_DIR/bar.txt" ChangeLevel: changeLevelMinor, }, got) }) + + t.Run("pushTarget failure cleans up draft release and tag", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + repos := setupGit(t) + githubClient := mocks.NewMockGithubClient(gomock.NewController(t)) + githubClient.EXPECT().CompareCommits(gomock.Any(), "orgName", "repoName", "v2.0.0", repos.taggedCommits["head"], -1).Return( + &github.CommitComparison{ + AheadBy: 1, + Commits: []string{repos.taggedCommits["head"]}, + }, nil, + ) + githubClient.EXPECT().CompareCommits(gomock.Any(), "orgName", "repoName", mergeSha, repos.taggedCommits["head"], 0).Return( + &github.CommitComparison{AheadBy: 0}, nil, + ) + githubClient.EXPECT().ListMergedPullsForCommit(gomock.Any(), "orgName", "repoName", repos.taggedCommits["head"]).Return( + []github.BasePull{{Number: 2, MergeCommitSha: mergeSha, Labels: []string{labelBreaking}}}, nil, + ) + githubClient.EXPECT().GenerateReleaseNotes(gomock.Any(), "orgName", "repoName", "v3.0.0", "v2.0.0").Return( + "release notes", nil, + ) + githubClient.EXPECT().CreateRelease(gomock.Any(), "orgName", "repoName", "v3.0.0", "release notes", false).Return( + &github.RepoRelease{ + ID: 1, + UploadURL: "localhost", + }, nil, + ) + // The release is still a draft when pushTarget fails, so DeleteRelease + // cleanup runs safely. No PublishRelease expectation: the publish must + // not be attempted when pushTarget fails. + githubClient.EXPECT().DeleteRelease(gomock.Any(), "orgName", "repoName", int64(1)).Return(nil) + preHook := ` +#!/bin/sh +set -e +git config user.name 'tester' +git config user.email 'tester' +current_branch=$(git rev-parse --abbrev-ref HEAD) +echo "bake" > bake.txt +git add bake.txt > /dev/null +git commit -m "bake commit" > /dev/null +echo "$current_branch" > "$RELEASE_TARGET" +` + runner := &Runner{ + CheckoutDir: repos.clone, + Ref: repos.taggedCommits["head"], + TagPrefix: "v", + Repo: "orgName/repoName", + PushRemote: "origin", + GithubClient: githubClient, + CreateRelease: true, + PreTagHook: preHook, + TempDir: t.TempDir(), + } + _, err := runner.run(ctx) + // Push to a checked-out branch of a non-bare origin is rejected by + // the default receive.denyCurrentBranch=refuse setting. + require.ErrorContains(t, err, "remote rejected") + ok, err := localTagExists(ctx, repos.origin, "v3.0.0") + require.NoError(t, err) + require.False(t, ok, "tag must be cleaned up after pushTarget failure") + }) + + t.Run("PublishRelease failure does not delete release or tag", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + repos := setupGit(t) + // Allow pushes to origin's currently checked-out branch so pushTarget + // succeeds and we reach PublishRelease. + mustRunCmd(t, repos.origin, "git", "config", "receive.denyCurrentBranch", "ignore") + githubClient := mocks.NewMockGithubClient(gomock.NewController(t)) + githubClient.EXPECT().CompareCommits(gomock.Any(), "orgName", "repoName", "v2.0.0", repos.taggedCommits["head"], -1).Return( + &github.CommitComparison{ + AheadBy: 1, + Commits: []string{repos.taggedCommits["head"]}, + }, nil, + ) + githubClient.EXPECT().CompareCommits(gomock.Any(), "orgName", "repoName", mergeSha, repos.taggedCommits["head"], 0).Return( + &github.CommitComparison{AheadBy: 0}, nil, + ) + githubClient.EXPECT().ListMergedPullsForCommit(gomock.Any(), "orgName", "repoName", repos.taggedCommits["head"]).Return( + []github.BasePull{{Number: 2, MergeCommitSha: mergeSha, Labels: []string{labelBreaking}}}, nil, + ) + githubClient.EXPECT().GenerateReleaseNotes(gomock.Any(), "orgName", "repoName", "v3.0.0", "v2.0.0").Return( + "release notes", nil, + ) + githubClient.EXPECT().CreateRelease(gomock.Any(), "orgName", "repoName", "v3.0.0", "release notes", false).Return( + &github.RepoRelease{ + ID: 1, + UploadURL: "localhost", + }, nil, + ) + githubClient.EXPECT().PublishRelease(gomock.Any(), "orgName", "repoName", "", int64(1)).Return(errors.New("publish failed")) + // No DeleteRelease expectation: once PublishRelease has been attempted + // the cleanup must not delete the release. The mock controller fails + // the test if DeleteRelease is invoked. + preHook := ` +#!/bin/sh +set -e +git config user.name 'tester' +git config user.email 'tester' +current_branch=$(git rev-parse --abbrev-ref HEAD) +echo "bake" > bake.txt +git add bake.txt > /dev/null +git commit -m "bake commit" > /dev/null +echo "$current_branch" > "$RELEASE_TARGET" +` + runner := &Runner{ + CheckoutDir: repos.clone, + Ref: repos.taggedCommits["head"], + TagPrefix: "v", + Repo: "orgName/repoName", + PushRemote: "origin", + GithubClient: githubClient, + CreateRelease: true, + PreTagHook: preHook, + TempDir: t.TempDir(), + } + _, err := runner.run(ctx) + require.ErrorContains(t, err, "publish failed") + ok, err := localTagExists(ctx, repos.origin, "v3.0.0") + require.NoError(t, err) + require.True(t, ok, "tag must not be deleted once PublishRelease has been attempted") + }) }