internal/task: update codereview.cfg and create a CL for review

1. Add optional parameter reviewers to gopls release flow.
2. Add a Task to update code review configuration in release branch.
3. Add a Action to wait for CL to be submitted.

A local relui screenshot is at https://go.dev/issue/57643#issuecomment-2259049777

For golang/go#57643

Change-Id: Iae3ebd5dd7bc9040979185aa353e2a4afed8cb54
Reviewed-on: https://go-review.googlesource.com/c/build/+/601241
Reviewed-by: Dmitri Shuralyov <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Hongxiang Jiang <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>

Author: h9jiang

cmd/relui/main.go

@@ -309,7 +309,8 @@ func main() {
 	dh.RegisterDefinition("Tag a new version of x/telemetry/config (if necessary)", tagTelemetryTasks.NewDefinition())
 
 	releaseGoplsTasks := task.ReleaseGoplsTasks{
-		Gerrit: gerritClient,
+		Gerrit:     gerritClient,
+		CloudBuild: cloudBuildClient,
 	}
 	dh.RegisterDefinition("Release a new version of gopls", releaseGoplsTasks.NewDefinition())
 

internal/task/releasegopls.go

@@ -5,9 +5,11 @@
 package task
 
 import (
+	"errors"
 	"fmt"
 	"slices"
 	"strings"
+	"time"
 
 	"golang.org/x/build/gerrit"
 	wf "golang.org/x/build/internal/workflow"
@@ -17,7 +19,8 @@ import (
 // ReleaseGoplsTasks implements a new workflow definition include all the tasks
 // to release a gopls.
 type ReleaseGoplsTasks struct {
-	Gerrit GerritClient
+	Gerrit     GerritClient
+	CloudBuild CloudBuildClient
 }
 
 // NewDefinition create a new workflow definition for releasing gopls.
@@ -27,16 +30,24 @@ func (r *ReleaseGoplsTasks) NewDefinition() *wf.Definition {
 	// TODO(hxjiang): provide potential release versions in the relui where the
 	// coordinator can choose which version to release instead of manual input.
 	version := wf.Param(wd, wf.ParamDef[string]{Name: "version"})
+	reviewers := wf.Param(wd, reviewersParam)
 	semversion := wf.Task1(wd, "validating input version", r.isValidVersion, version)
-	_ = wf.Action1(wd, "creating new branch if minor release", r.createBranchIfMinor, semversion)
+	branchCreated := wf.Action1(wd, "creating new branch if minor release", r.createBranchIfMinor, semversion)
+	changeID := wf.Task2(wd, "updating branch's codereview.cfg", r.updateCodeReviewConfig, semversion, reviewers, wf.After(branchCreated))
+	_ = wf.Action1(wd, "await config CL submission", r.AwaitSubmission, changeID)
 	return wd
 }
 
+// goplsReleaseBranchName returns the branch name for given input release version.
+func goplsReleaseBranchName(semv semversion) string {
+	return fmt.Sprintf("gopls-release-branch.%v.%v", semv.Major, semv.Minor)
+}
+
 // createBranchIfMinor create the release branch if the input version is a minor
 // release.
 // All patch releases under the same minor version share the same release branch.
 func (r *ReleaseGoplsTasks) createBranchIfMinor(ctx *wf.TaskContext, semv semversion) error {
-	branch := fmt.Sprintf("gopls-release-branch.%v.%v", semv.Major, semv.Minor)
+	branch := goplsReleaseBranchName(semv)
 
 	// Require gopls release branch existence if this is a non-minor release.
 	if semv.Patch != 0 {
@@ -62,6 +73,77 @@ func (r *ReleaseGoplsTasks) createBranchIfMinor(ctx *wf.TaskContext, semv semver
 	return err
 }
 
+// updateCodeReviewConfig ensures codereview.cfg contains the expected
+// configuration.
+//
+// It returns the change ID, or "" if the CL was not created.
+func (r *ReleaseGoplsTasks) updateCodeReviewConfig(ctx *wf.TaskContext, semv semversion, reviewers []string) (string, error) {
+	const configFile = "codereview.cfg"
+	const configFmt = `issuerepo: golang/go
+branch: %s
+parent-branch: master
+`
+
+	branch := goplsReleaseBranchName(semv)
+	clTitle := fmt.Sprintf("all: update %s for %s", configFile, branch)
+
+	// Query for an existing pending config CL, to avoid duplication.
+	query := fmt.Sprintf(`message:%q status:open owner:[email protected] repo:tools branch:%q -age:7d`, clTitle, branch)
+	changes, err := r.Gerrit.QueryChanges(ctx, query)
+	if err != nil {
+		return "", err
+	}
+	if len(changes) > 0 {
+		ctx.Printf("not creating CL: found existing CL %d", changes[0].ChangeNumber)
+		return changes[0].ChangeID, nil
+	}
+
+	head, err := r.Gerrit.ReadBranchHead(ctx, "tools", branch)
+	if err != nil {
+		return "", err
+	}
+
+	before, err := r.Gerrit.ReadFile(ctx, "tools", head, configFile)
+	if err != nil && !errors.Is(err, gerrit.ErrResourceNotExist) {
+		return "", err
+	}
+
+	after := fmt.Sprintf(configFmt, branch)
+	// Skip CL creation as config has not changed.
+	if string(before) == after {
+		return "", nil
+	}
+
+	changeInput := gerrit.ChangeInput{
+		Project: "tools",
+		Subject: fmt.Sprintf("%s\n\nThis is an automated CL which updates the %s.", clTitle, configFile),
+		Branch:  branch,
+	}
+
+	files := map[string]string{
+		configFile: string(after),
+	}
+
+	ctx.Printf("creating auto-submit change to %s under branch %q in x/tools repo.", configFile, branch)
+	return r.Gerrit.CreateAutoSubmitChange(ctx, changeInput, reviewers, files)
+}
+
+// AwaitSubmission waits for the CL with the given change ID to be submitted.
+//
+// The return value is the submitted commit hash, or "" if changeID is "".
+func (r *ReleaseGoplsTasks) AwaitSubmission(ctx *wf.TaskContext, changeID string) error {
+	if changeID == "" {
+		ctx.Printf("not awaiting: no CL was created")
+		return nil
+	}
+
+	ctx.Printf("awaiting review/submit of %v", ChangeLink(changeID))
+	_, err := AwaitCondition(ctx, 10*time.Second, func() (string, bool, error) {
+		return r.Gerrit.Submitted(ctx, changeID, "")
+	})
+	return err
+}
+
 func (r *ReleaseGoplsTasks) isValidVersion(ctx *wf.TaskContext, ver string) (semversion, error) {
 	if !semver.IsValid(ver) {
 		return semversion{}, fmt.Errorf("the input %q version does not follow semantic version schema", ver)

internal/task/releasegopls_test.go

@@ -158,7 +158,7 @@ func TestCreateBranchIfMinor(t *testing.T) {
 				t.Errorf("createBranchIfMinor() should return nil but return err: %v", err)
 			}
 
-			// Created branch should have same revision as master branch's HEAD.
+			// Created branch should have same revision as master branch's head.
 			if tc.wantBranch != "" {
 				gotRevision, err := gerritClient.ReadBranchHead(ctx, "tools", tc.wantBranch)
 				if err != nil {
@@ -171,3 +171,144 @@ func TestCreateBranchIfMinor(t *testing.T) {
 		})
 	}
 }
+
+func TestUpdateCodeReviewConfig(t *testing.T) {
+	ctx := context.Background()
+	testcases := []struct {
+		name       string
+		version    string
+		config     string
+		wantCommit bool
+		wantConfig string
+	}{
+		{
+			name:       "should update the codereview.cfg with version 1.2 for input minor release 1.2.0",
+			version:    "v1.2.0",
+			config:     "foo",
+			wantCommit: true,
+			wantConfig: `issuerepo: golang/go
+branch: gopls-release-branch.1.2
+parent-branch: master
+`,
+		},
+		{
+			name:       "should update the codereview.cfg with version 1.2 for input patch release 1.2.3",
+			version:    "v1.2.3",
+			config:     "foo",
+			wantCommit: true,
+			wantConfig: `issuerepo: golang/go
+branch: gopls-release-branch.1.2
+parent-branch: master
+`,
+		},
+		{
+			name:    "no need to update the config for a minor release 1.3.0",
+			version: "v1.3.0",
+			config: `issuerepo: golang/go
+branch: gopls-release-branch.1.3
+parent-branch: master
+`,
+			wantCommit: false,
+			wantConfig: `issuerepo: golang/go
+branch: gopls-release-branch.1.3
+parent-branch: master
+`,
+		},
+		{
+			name:    "no need to update the config for a patch release 1.3.3",
+			version: "v1.3.3",
+			config: `issuerepo: golang/go
+branch: gopls-release-branch.1.3
+parent-branch: master
+`,
+			wantCommit: false,
+			wantConfig: `issuerepo: golang/go
+branch: gopls-release-branch.1.3
+parent-branch: master
+`,
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			tools := NewFakeRepo(t, "tools")
+			_ = tools.Commit(map[string]string{
+				"go.mod": "module golang.org/x/tools\n",
+				"go.sum": "\n",
+			})
+			_ = tools.Commit(map[string]string{
+				"codereview.cfg": tc.config,
+			})
+
+			gerritClient := NewFakeGerrit(t, tools)
+
+			headMaster, err := gerritClient.ReadBranchHead(ctx, "tools", "master")
+			if err != nil {
+				t.Fatalf("ReadBranchHead should be able to get revision of master branch's head: %v", err)
+			}
+
+			configMaster, err := gerritClient.ReadFile(ctx, "tools", headMaster, "codereview.cfg")
+			if err != nil {
+				t.Fatalf("ReadFile should be able to read the codereview.cfg file from master branch head: %v", err)
+			}
+
+			semv, _ := parseSemver(tc.version)
+			releaseBranch := goplsReleaseBranchName(semv)
+			if _, err := gerritClient.CreateBranch(ctx, "tools", releaseBranch, gerrit.BranchInput{Revision: headMaster}); err != nil {
+				t.Fatalf("failed to create the branch %q: %v", releaseBranch, err)
+			}
+
+			headRelease, err := gerritClient.ReadBranchHead(ctx, "tools", releaseBranch)
+			if err != nil {
+				t.Fatalf("ReadBranchHead should be able to get revision of release branch's head: %v", err)
+			}
+
+			tasks := &ReleaseGoplsTasks{
+				Gerrit:     gerritClient,
+				CloudBuild: NewFakeCloudBuild(t, gerritClient, "", nil, fakeGo),
+			}
+
+			_, err = tasks.updateCodeReviewConfig(&workflow.TaskContext{Context: ctx, Logger: &testLogger{t, ""}}, semv, nil)
+			if err != nil {
+				t.Fatalf("updateCodeReviewConfig() returns error: %v", err)
+			}
+
+			// master branch's head commit should not change.
+			headMasterAfter, err := gerritClient.ReadBranchHead(ctx, "tools", "master")
+			if err != nil {
+				t.Fatalf("ReadBranchHead() should be able to get revision of master branch's head: %v", err)
+			}
+			if headMasterAfter != headMaster {
+				t.Errorf("updateCodeReviewConfig() should not change master branch's head, got = %s want = %s", headMasterAfter, headMaster)
+			}
+
+			// master branch's head codereview.cfg content should not change.
+			configMasterAfter, err := gerritClient.ReadFile(ctx, "tools", headMasterAfter, "codereview.cfg")
+			if err != nil {
+				t.Fatalf("ReadFile() should be able to read the codereview.cfg file from master branch head: %v", err)
+			}
+			if diff := cmp.Diff(configMaster, configMasterAfter); diff != "" {
+				t.Errorf("updateCodeReviewConfig() should not change codereview.cfg content in master branch (-want +got) \n %s", diff)
+			}
+
+			// verify the release branch commit have the expected behavior.
+			headReleaseAfter, err := gerritClient.ReadBranchHead(ctx, "tools", releaseBranch)
+			if err != nil {
+				t.Fatalf("ReadBranchHead() should be able to get revision of master branch's head: %v", err)
+			}
+			if tc.wantCommit && headReleaseAfter == headRelease {
+				t.Errorf("updateCodeReviewConfig() should have one commit to release branch, head of branch got = %s want = %s", headRelease, headReleaseAfter)
+			} else if !tc.wantCommit && headReleaseAfter != headRelease {
+				t.Errorf("updateCodeReviewConfig() should have not change release branch's head, got = %s want = %s", headRelease, headReleaseAfter)
+			}
+
+			// verify the release branch configreview.cfg have the expected content.
+			configReleaseAfter, err := gerritClient.ReadFile(ctx, "tools", headReleaseAfter, "codereview.cfg")
+			if err != nil {
+				t.Fatalf("ReadFile() should be able to read the codereview.cfg file from release branch head: %v", err)
+			}
+			if diff := cmp.Diff(tc.wantConfig, string(configReleaseAfter)); diff != "" {
+				t.Errorf("codereview.cfg mismatch (-want +got) \n %s", diff)
+			}
+		})
+	}
+}