diff --git a/cmd/backport-batch/README.md b/cmd/backport-batch/README.md index 60eac648d..665ced89a 100644 --- a/cmd/backport-batch/README.md +++ b/cmd/backport-batch/README.md @@ -36,7 +36,7 @@ The pull request body goes to stdout, built from the line's own `.github/PULL_RE `--defer N[,N] --reason TEXT` takes originals out of the open batch pull request, usually the ones a failing check was pinned on. It builds the batch branch again off `origin/` without their units: while the batch still sits on the line's head, the units before the first one it drops stay as they are, and every later one, a pushed resolution included, is picked again in its own shape, a merge's picks one by one and then the merge with its message. When dropping one leaves a later commit that no longer applies, the run stops and names that commit's original, to defer as well. It adds no new original. It also takes out an original that got `backport/skip` after the batch picked it, which a run never removes on its own and the body lists as still on the branch. The body records each deferred original with the reason in the `backport-batch` block; a run that continues this batch keeps them out, and the next batch tries them again. A batch that carries a merge commit other than an original's, as GitHub's update-branch button makes, is refused, since picking such a merge again would lose what it resolved. -`--push` pushes the branch, refusing anything but a fast-forward of what the remote has, then opens the draft pull request against the line with `gh pr create --draft`, or updates the open batch pull request with `gh pr edit`. When the branch it refuses to push over has no pull request open on it, as a run that stopped between its push and opening the pull request leaves it, the refusal names the `run --onto --push` that continues it. While a batch pull request is open on another branch it refuses, so a line never has two. It never pushes to or edits a batch pull request whose body has no `backport-batch` block either: someone wrote that body by hand, and the batch is theirs to merge or close. After `--defer` alone it replaces the branch, with `--force-with-lease=refs/heads/:`, where head is what `git ls-remote` reads for the branch and has to be the head the rebuild started from: a commit pushed since is never dropped, by the tool's check or, if one lands between that check and the push, by the remote. +`--push` pushes the branch, refusing anything but a fast-forward of what the remote has, then opens the draft pull request against the line with `gh pr create --draft`, or updates the open batch pull request with `gh pr edit`, and prints its URL on stderr. When the branch it refuses to push over has no pull request open on it, as a run that stopped between its push and opening the pull request leaves it, the refusal names the `run --onto --push` that continues it. While a batch pull request is open on another branch it refuses, so a line never has two. It never pushes to or edits a batch pull request whose body has no `backport-batch` block either: someone wrote that body by hand, and the batch is theirs to merge or close. After `--defer` alone it replaces the branch, with `--force-with-lease=refs/heads/:`, where head is what `git ls-remote` reads for the branch and has to be the head the rebuild started from: a commit pushed since is never dropped, by the tool's check or, if one lands between that check and the push, by the remote. ### check diff --git a/cmd/backport-batch/assemble.go b/cmd/backport-batch/assemble.go index 48c95428b..d3fa66ac0 100644 --- a/cmd/backport-batch/assemble.go +++ b/cmd/backport-batch/assemble.go @@ -517,9 +517,26 @@ func (a *app) publish(b *batch, branch, head, title, body string, rebuilt bool) return err } if b.pr == nil { - return a.gh.createPull(b.line.branch(), branch, title, f.Name()) + url, err := a.gh.createPull(b.line.branch(), branch, title, f.Name()) + if err != nil { + return err + } + fmt.Fprintf(a.stderr, "opened draft %s\n", url) + return nil } - return a.gh.editPull(b.pr.Number, title, f.Name()) + if err := a.gh.editPull(b.pr.Number, title, f.Name()); err != nil { + return err + } + fmt.Fprintf(a.stderr, "updated %s\n", pullURL(b.pr)) + return nil +} + +// pullURL is where a pull request is on GitHub. +func pullURL(pr *pullRequest) string { + if pullURLRE.MatchString(pr.HTMLURL) { + return pr.HTMLURL + } + return fmt.Sprintf("%spull/%d", ourRepoURL, pr.Number) } // pushArgs pushes head to branch: as a fast-forward, which the remote checks, diff --git a/cmd/backport-batch/assemble_test.go b/cmd/backport-batch/assemble_test.go index 513ae6ff2..31dfd7dda 100644 --- a/cmd/backport-batch/assemble_test.go +++ b/cmd/backport-batch/assemble_test.go @@ -258,9 +258,17 @@ func TestRunPush(t *testing.T) { f.lines() f.pr(5, "fix(app): five", "", map[string]string{"app/five.txt": "5\n"}) gh := &fakeGH{} - if code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--push"); code != 0 { + code, stdout, stderr := f.exec(gh, "run", "--line", "release-1.1", "--push") + if code != 0 { t.Fatalf("run --push = %d\n%s", code, stderr) } + if !strings.Contains(stderr, "opened draft https://github.com/cozystack/cozystack/pull/90\n") { + t.Errorf("the run does not print the pull request it opened:\n%s", stderr) + } + l, _ := parseLine("release-1.1") + if _, err := parseDeferred(stdout, l); err != nil || !strings.HasPrefix(stdout, "## What this PR does") || strings.Contains(stdout, "https://github.com/cozystack/cozystack/pull/90") { + t.Errorf("stdout is no longer the body alone (%v):\n%s", err, stdout) + } head := f.git("rev-parse", "HEAD") if got := f.remoteHead(batchBranch1); got != head { t.Errorf("remote %s = %q, want %s", batchBranch1, got, head) @@ -272,9 +280,13 @@ func TestRunPush(t *testing.T) { f.git("switch", "-q", "main") f.pr(6, "fix(app): six", "", map[string]string{"app/six.txt": "6\n"}) gh.prs = []pullRequest{f.batchPR(90, "release-1.1", batchBranch1, gh.bodies[0])} - if code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--onto", head, "--push"); code != 0 { + code, _, stderr = f.exec(gh, "run", "--line", "release-1.1", "--onto", head, "--push") + if code != 0 { t.Fatalf("run --onto --push = %d\n%s", code, stderr) } + if !strings.Contains(stderr, "updated https://github.com/cozystack/cozystack/pull/90\n") { + t.Errorf("the run does not print the pull request it updated:\n%s", stderr) + } if got := f.subjects("origin/release-1.1", f.remoteHead(batchBranch1)); !slices.Equal(got, []string{"fix(app): five (#5)", "fix(app): six (#6)"}) { t.Errorf("the pushed batch carries %q, want #5 then #6", got) } diff --git a/cmd/backport-batch/fixture_test.go b/cmd/backport-batch/fixture_test.go index cb14f8e7e..2ac7b4639 100644 --- a/cmd/backport-batch/fixture_test.go +++ b/cmd/backport-batch/fixture_test.go @@ -309,9 +309,9 @@ func (g *fakeGH) openPulls(base string) ([]pullRequest, error) { return out, g.err } -func (g *fakeGH) createPull(base, head, title, bodyFile string) error { +func (g *fakeGH) createPull(base, head, title, bodyFile string) (string, error) { g.created = append(g.created, base+" "+head+" "+title) - return g.readBody(bodyFile) + return fmt.Sprintf("https://github.com/%s/pull/90", upstreamRepo), g.readBody(bodyFile) } func (g *fakeGH) editPull(number int, title, bodyFile string) error { diff --git a/cmd/backport-batch/github.go b/cmd/backport-batch/github.go index b77ac0945..4b6a92507 100644 --- a/cmd/backport-batch/github.go +++ b/cmd/backport-batch/github.go @@ -21,6 +21,7 @@ import ( "fmt" "regexp" "strconv" + "strings" ) // openPullsCap is the page size of the open pull request listing. The REST @@ -43,6 +44,7 @@ var ( batchBranchRE = regexp.MustCompile(`^backport-batch/(release-\d+\.\d+)-(\d{8})$`) loginRE = regexp.MustCompile(`^[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?$`) botLoginRE = regexp.MustCompile(`^[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?\[bot\]$`) + pullURLRE = regexp.MustCompile(`^https://[^/\s]+/[^/\s]+/[^/\s]+/pull/[1-9][0-9]*$`) ) // batchBotEnv names the GitHub App bot the scheduled batch opens its pull @@ -107,7 +109,8 @@ type github interface { permission(login string) (string, error) // labelled lists the pull requests merged into main that carry label. labelled(label string) ([]int, error) - createPull(base, head, title, bodyFile string) error + // createPull opens the draft and returns its URL. + createPull(base, head, title, bodyFile string) (string, error) editPull(number int, title, bodyFile string) error comments(number int) ([]issueComment, error) createComment(number int, bodyFile string) error @@ -175,9 +178,16 @@ func (ghCLI) labelled(label string) ([]int, error) { return numbers, nil } -func (ghCLI) createPull(base, head, title, bodyFile string) error { - _, err := capture("gh", "pr", "create", "--draft", "--base", base, "--head", head, "--title", title, "--body-file", bodyFile) - return err +func (ghCLI) createPull(base, head, title, bodyFile string) (string, error) { + out, err := capture("gh", "pr", "create", "--draft", "--base", base, "--head", head, "--title", title, "--body-file", bodyFile) + if err != nil { + return "", err + } + url := strings.TrimSpace(out) + if !pullURLRE.MatchString(url) { + return "", fmt.Errorf("gh pr create printed %q, not the URL of a pull request", url) + } + return url, nil } func (ghCLI) editPull(number int, title, bodyFile string) error { diff --git a/cmd/backport-batch/overrides_test.go b/cmd/backport-batch/overrides_test.go index f1b1a5bcb..38eca5e0b 100644 --- a/cmd/backport-batch/overrides_test.go +++ b/cmd/backport-batch/overrides_test.go @@ -230,13 +230,17 @@ func TestGHCLI(t *testing.T) { // A batch pull request carries no label: the release workflows act on a // pull request cozystack-ci[bot] opened only when it has the release label. - write("") - if err := (ghCLI{}).createPull("release-1.7", batchBranch1, "fix(backport): batch", "/tmp/body.md"); err != nil { - t.Fatal(err) + write("https://github.com/cozystack/cozystack/pull/5300\n") + if url, err := (ghCLI{}).createPull("release-1.7", batchBranch1, "fix(backport): batch", "/tmp/body.md"); err != nil || url != "https://github.com/cozystack/cozystack/pull/5300" { + t.Errorf("createPull = %q, %v; want the URL gh printed", url, err) } if want := "pr create --draft --base release-1.7 --head " + batchBranch1 + " --title fix(backport): batch --body-file /tmp/body.md"; called() != want { t.Errorf("gh %q, want gh %q", called(), want) } + write("Warning: 1 uncommitted change\n") + if _, err := (ghCLI{}).createPull("release-1.7", batchBranch1, "fix(backport): batch", "/tmp/body.md"); err == nil { + t.Error("createPull took output that is no pull request URL") + } write("") if err := (ghCLI{}).editPull(90, "fix(backport): batch", "/tmp/body.md"); err != nil { t.Fatal(err)