diff --git a/cli/release-automation/internal/automation/release_pr_checks.go b/cli/release-automation/internal/automation/release_pr_checks.go index 6dfb32af1..8ee927aae 100644 --- a/cli/release-automation/internal/automation/release_pr_checks.go +++ b/cli/release-automation/internal/automation/release_pr_checks.go @@ -95,6 +95,7 @@ func runReleasePleasePRChecksWithDeps(ctx context.Context, stdout io.Writer, std writeReleasePRCheckLine(stdout, "No pending release-please PR found for "+config.targetBranch+".") return 0 } + releasePRs = orderReleasePRsForChecks(releasePRs, config.targetBranch) // One component's failing checks must not strand the other components' // release PRs in draft, so every PR is processed and the failures are diff --git a/cli/release-automation/internal/automation/release_pr_checks_test.go b/cli/release-automation/internal/automation/release_pr_checks_test.go index 3f62de9af..df901dcb4 100644 --- a/cli/release-automation/internal/automation/release_pr_checks_test.go +++ b/cli/release-automation/internal/automation/release_pr_checks_test.go @@ -890,6 +890,49 @@ func TestReleasePRChecksLeaveUnityPackagePullRequestDraft(t *testing.T) { assertReleasePRCheckLogDoesNotContainLine(t, commandLogText, "gh pr ready 2002 --repo owner/repository") } +// Verifies that the Unity package release PR's checks are dispatched before another component's even when the listing puts the package PR last, so post-publish's bounded wait for that PR never sits behind another component's checks. +func TestReleasePRChecksDispatchUnityPackagePullRequestFirst(t *testing.T) { + prListJSON := `[` + + `{"number":2001,"headRefName":"release-please--branches--main--components--dispatcher","headRefOid":"dispatcher123","title":"chore(main): release dispatcher 3.6.0","url":"https://example.test/pr/2001"},` + + `{"number":2002,"headRefName":"release-please--branches--main--components--unity-package","headRefOid":"package123","title":"chore(main): release 3.6.0","url":"https://example.test/pr/2002"}` + + `]` + + exitCode, _, stderr, stub := runMultiReleasePRCheck(t, prListJSON, nil) + + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) + } + dispatchedBranches := []string{} + for _, commandLine := range stub.commandLog { + if strings.HasPrefix(commandLine, "gh workflow run ") { + dispatchedBranches = append(dispatchedBranches, commandLine) + } + } + if len(dispatchedBranches) == 0 || !strings.Contains(dispatchedBranches[0], "--components--unity-package") { + t.Fatalf("expected the first dispatch to target the Unity package release branch, got %v", dispatchedBranches) + } + if !strings.Contains(dispatchedBranches[len(dispatchedBranches)-1], "--components--dispatcher") { + t.Fatalf("expected the dispatcher release branch to be dispatched after the package one, got %v", dispatchedBranches) + } +} + +// Verifies that failing Unity package checks, which now run first, still let the next component's release PR be marked ready, and the command reports only the package PR as failed. +func TestReleasePRChecksContinueAfterUnityPackageChecksFail(t *testing.T) { + prListJSON := `[` + + `{"number":2001,"headRefName":"release-please--branches--main--components--dispatcher","headRefOid":"dispatcher123","title":"chore(main): release dispatcher 3.6.0","url":"https://example.test/pr/2001"},` + + `{"number":2002,"headRefName":"release-please--branches--main--components--unity-package","headRefOid":"package123","title":"chore(main): release 3.6.0","url":"https://example.test/pr/2002"}` + + `]` + + exitCode, _, stderr, stub := runMultiReleasePRCheck(t, prListJSON, map[string]bool{"21": true}) + + if exitCode != 1 { + t.Fatalf("expected exit code 1, got %d\nstderr: %s", exitCode, stderr) + } + commandLogText := strings.Join(stub.commandLog, "\n") + assertReleasePRCheckLogContainsLine(t, commandLogText, "gh pr ready 2001 --repo owner/repository") + assertReleasePRCheckLogContains(t, stderr, "Release PR checks failed for #2002; the errors above say why.") +} + // Verifies that one component's failing checks still let the other component's release PR be finalized, and the command reports the failure. func TestReleasePRChecksContinueAfterOneComponentFails(t *testing.T) { prListJSON := `[` + @@ -922,7 +965,7 @@ func TestReleasePRChecksNameEveryFailedPullRequestLast(t *testing.T) { } stderrLines := strings.Split(strings.TrimRight(stderr, "\n"), "\n") lastLine := stderrLines[len(stderrLines)-1] - expected := "Release PR checks failed for #2001, #2002; the errors above say why." + expected := "Release PR checks failed for #2002, #2001; the errors above say why." if lastLine != expected { t.Fatalf("expected last stderr line %q, got %q\nstderr: %s", expected, lastLine, stderr) } diff --git a/cli/release-automation/internal/automation/release_pr_lookup.go b/cli/release-automation/internal/automation/release_pr_lookup.go index b95d239c1..1bf4f7270 100644 --- a/cli/release-automation/internal/automation/release_pr_lookup.go +++ b/cli/release-automation/internal/automation/release_pr_lookup.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "sort" "strings" "time" ) @@ -85,6 +86,22 @@ func releasePRCheckComponentFromHeadRef(headRefName string, targetBranch string) return strings.TrimPrefix(headRefName, componentPrefix) } +// orderReleasePRsForChecks puts the Unity package release pull request first and +// keeps every other pull request in listed order. Checks run one pull request at a +// time, and dispatcher-publish's post-publish waits a bounded time for the package +// pull request's checks before merging it; a package pull request listed after +// another component's waited behind that component's checks and timed out. +func orderReleasePRsForChecks(releasePRs []releasePullRequest, targetBranch string) []releasePullRequest { + ordered := append([]releasePullRequest(nil), releasePRs...) + isPackage := func(releasePR releasePullRequest) bool { + return releasePRCheckComponentFromHeadRef(releasePR.HeadRefName, targetBranch) == mergePackageReleasePRComponent + } + sort.SliceStable(ordered, func(i, j int) bool { + return isPackage(ordered[i]) && !isPackage(ordered[j]) + }) + return ordered +} + // releasePleaseReleaseTitleMatches says whether a title is a release-please release title. A // release pull request carries one, and so does the commit that squash-merges it, with the pull // request number appended. diff --git a/cli/release-automation/internal/automation/release_pr_lookup_test.go b/cli/release-automation/internal/automation/release_pr_lookup_test.go index 248e5e855..066faa433 100644 --- a/cli/release-automation/internal/automation/release_pr_lookup_test.go +++ b/cli/release-automation/internal/automation/release_pr_lookup_test.go @@ -3,6 +3,7 @@ package automation import ( "context" "errors" + "slices" "strings" "testing" "time" @@ -58,3 +59,33 @@ func TestFindReleasePRCheckPullRequestsWithRetryStopsOnInterruptedWait(t *testin t.Fatalf("expected the wait error after one lookup, got %v after %d lookups", err, calls) } } + +// Verifies the Unity package release pull request is moved to the front while every other pull request keeps its listed order, including the combined branch and a listing without a package pull request. +func TestOrderReleasePRsForChecks(t *testing.T) { + const prefix = "release-please--branches--main--components--" + runner := releasePullRequest{Number: 1, HeadRefName: prefix + "uloop-project-runner"} + dispatcher := releasePullRequest{Number: 2, HeadRefName: prefix + "dispatcher"} + unityPackage := releasePullRequest{Number: 3, HeadRefName: prefix + "unity-package"} + combined := releasePullRequest{Number: 4, HeadRefName: "release-please--branches--main"} + cases := []struct { + name string + listed []releasePullRequest + want []int + }{ + {name: "package listed last", listed: []releasePullRequest{runner, dispatcher, unityPackage}, want: []int{3, 1, 2}}, + {name: "package already first", listed: []releasePullRequest{unityPackage, dispatcher}, want: []int{3, 2}}, + {name: "package only", listed: []releasePullRequest{unityPackage}, want: []int{3}}, + {name: "no package", listed: []releasePullRequest{runner, dispatcher}, want: []int{1, 2}}, + {name: "combined branch is not the package", listed: []releasePullRequest{combined, unityPackage}, want: []int{3, 4}}, + } + for _, testCase := range cases { + ordered := orderReleasePRsForChecks(testCase.listed, "main") + got := []int{} + for _, releasePR := range ordered { + got = append(got, releasePR.Number) + } + if !slices.Equal(got, testCase.want) { + t.Fatalf("%s: order = %v, want %v", testCase.name, got, testCase.want) + } + } +} diff --git a/docs/dispatcher-pin-release-order.md b/docs/dispatcher-pin-release-order.md index 0eba7fc2d..2567763e7 100644 --- a/docs/dispatcher-pin-release-order.md +++ b/docs/dispatcher-pin-release-order.md @@ -39,7 +39,9 @@ and is merged only by automation; the other two are the ones people merge. 2. `post-publish` stamps the pin on `main`, then marks the **unity-package** release pull request ready and merges it, once that pull request's head records the dispatcher tag just published and has a successful run of every - required workflow for that exact head commit. + required workflow for that exact head commit. The release PR checks run one + pull request at a time, so the unity-package one always goes first; behind + another component's checks it would outlast the time `post-publish` waits. 3. A package release that carries no dispatcher bump never reaches step 2, so `release-please.yml` merges it instead: after the release PR checks, it marks the package pull request ready and merges it, but only while no