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 <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Hongxiang Jiang <hxjiang@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
diff --git a/cmd/relui/main.go b/cmd/relui/main.go
index 4d2cac8..def4682 100644
--- a/cmd/relui/main.go
+++ b/cmd/relui/main.go
@@ -309,7 +309,8 @@
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())
diff --git a/internal/task/releasegopls.go b/internal/task/releasegopls.go
index 8f72251..243d527 100644
--- a/internal/task/releasegopls.go
+++ b/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 @@
// 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 @@
// 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 @@
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:gobot@golang.org 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)
diff --git a/internal/task/releasegopls_test.go b/internal/task/releasegopls_test.go
index 19b5248..ebe33a6 100644
--- a/internal/task/releasegopls_test.go
+++ b/internal/task/releasegopls_test.go
@@ -158,7 +158,7 @@
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 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)
+ }
+ })
+ }
+}
