internal/task: skip CLs that don't make changes

In the most recent release we retried the DL CL task, and it failed
because Gerrit rejected the no-op changes. Change the Gerrit client to
return no change in this case, and the surrounding tasks to tolerate it.

If this happens for the VERSION CL we will tag whatever the branch head
happens to be at the time, which seems correct to me.

For golang/go#51797.

Change-Id: Ieb69a4c96c7ebe057a53d0fca4d713dca0fc6f47
Reviewed-on: https://go-review.googlesource.com/c/build/+/411897
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Heschi Kreinick <[email protected]>
Auto-Submit: Heschi Kreinick <[email protected]>
Reviewed-by: Alex Rakoczy <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>

Author: heschi

gerrit/gerrit.go

@@ -61,6 +61,10 @@ func (c *Client) httpClient() *http.Client {
 // It is only for use with errors.Is.
 var ErrResourceNotExist = errors.New("gerrit: requested resource does not exist")
 
+// ErrNotModified is returned when a modification didn't result in any change.
+// It is only for use with errors.Is. Not all APIs return this error; check the documentation.
+var ErrNotModified = errors.New("gerrit: requested modification resulted in no change")
+
 // HTTPError is the error type returned when a Gerrit API call does not return
 // the expected status.
 type HTTPError struct {
@@ -74,7 +78,17 @@ func (e *HTTPError) Error() string {
 }
 
 func (e *HTTPError) Is(target error) bool {
-	return target == ErrResourceNotExist && e.Res.StatusCode == http.StatusNotFound
+	switch target {
+	case ErrResourceNotExist:
+		return e.Res.StatusCode == http.StatusNotFound
+	case ErrNotModified:
+		// As of writing, this error text is the only way to distinguish different Conflict errors. See
+		// https://cs.opensource.google/gerrit/gerrit/gerrit/+/master:java/com/google/gerrit/server/restapi/change/ChangeEdits.java;l=346;drc=d338da307a518f7f28b94310c1c083c997ca3c6a
+		// https://cs.opensource.google/gerrit/gerrit/gerrit/+/master:java/com/google/gerrit/server/edit/ChangeEditModifier.java;l=453;drc=3bc970bb3e689d1d340382c3f5e5285d44f91dbf
+		return e.Res.StatusCode == http.StatusConflict && bytes.Contains(e.Body, []byte("no changes were made"))
+	default:
+		return false
+	}
 }
 
 // doArg is an optional argument for the Client.do method.
@@ -750,21 +764,16 @@ type ChangeInput struct {
 }
 
 // ChangeFileContentInChangeEdit puts content of a file to a change edit.
+// If no change is made, an error that matches ErrNotModified is returned.
 //
 // See https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#put-edit-file.
 func (c *Client) ChangeFileContentInChangeEdit(ctx context.Context, changeID string, path string, content string) error {
-	err := c.do(ctx, nil, "PUT", "/changes/"+changeID+"/edit/"+url.QueryEscape(path),
+	return c.do(ctx, nil, "PUT", "/changes/"+changeID+"/edit/"+url.QueryEscape(path),
 		reqBodyRaw{strings.NewReader(content)}, wantResStatus(http.StatusNoContent))
-	if he, ok := err.(*HTTPError); ok && he.Res.StatusCode == http.StatusConflict {
-		// The change edit was a no-op.
-		// Note: If/when there's a need inside x/build to handle this differently,
-		// maybe it'll be a good time to return something other than a *HTTPError
-		// and document it as part of the API.
-	}
-	return err
 }
 
 // DeleteFileInChangeEdit deletes a file from a change edit.
+// If no change is made, an error that matches ErrNotModified is returned.
 //
 // See https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#delete-edit-file.
 func (c *Client) DeleteFileInChangeEdit(ctx context.Context, changeID string, path string) error {

internal/relui/workflows.go

@@ -297,7 +297,7 @@ func addSingleReleaseWorkflow(build *BuildReleaseTasks, milestone *task.Mileston
 		branch = "master"
 	}
 	branchVal := wd.Constant(branch)
-	branchHead := wd.Task("Read branch HEAD", version.ReadBranchHead, branchVal)
+	releaseBase := wd.Task("Pick release base commit", version.ReadBranchHead, branchVal)
 
 	// Select version, check milestones.
 	nextVersion := wd.Task("Get next version", version.GetNextVersion, kindVal)
@@ -308,7 +308,7 @@ func addSingleReleaseWorkflow(build *BuildReleaseTasks, milestone *task.Mileston
 	wd.Output("Download CL submitted", dlclCommit)
 
 	// Build, test, and sign release.
-	signedAndTestedArtifacts, err := build.addBuildTasks(wd, "go1.19", nextVersion, branchHead, skipTests, checked)
+	signedAndTestedArtifacts, err := build.addBuildTasks(wd, "go1.19", nextVersion, releaseBase, skipTests, checked)
 	if err != nil {
 		return err
 	}
@@ -319,11 +319,11 @@ func addSingleReleaseWorkflow(build *BuildReleaseTasks, milestone *task.Mileston
 	// Tag version and upload to CDN/website.
 	uploaded := wd.Action("Upload artifacts to CDN", build.uploadArtifacts, signedAndTestedArtifacts, verified)
 
-	tagCommit := branchHead
+	tagCommit := releaseBase
 	if branch != "master" {
-		branchHeadChecked := wd.Action("Check for modified branch head", version.CheckBranchHead, branchVal, branchHead, uploaded)
+		branchHeadChecked := wd.Action("Check for modified branch head", version.CheckBranchHead, branchVal, releaseBase, uploaded)
 		versionCL := wd.Task("Mail version CL", version.CreateAutoSubmitVersionCL, branchVal, nextVersion, branchHeadChecked)
-		tagCommit = wd.Task("Wait for version CL submission", version.AwaitCL, versionCL, branchHead)
+		tagCommit = wd.Task("Wait for version CL submission", version.AwaitCL, versionCL, releaseBase)
 	}
 	tagged := wd.Action("Tag version", version.TagRelease, nextVersion, tagCommit, uploaded)
 

internal/task/gerrit.go

@@ -11,9 +11,11 @@ import (
 )
 
 type GerritClient interface {
-	// CreateAutoSubmitChange creates a change with the given metadata and contents, sets
-	// Run-TryBots and Auto-Submit, and returns its change ID.
+	// CreateAutoSubmitChange creates a change with the given metadata and
+	// contents, sets Run-TryBots and Auto-Submit, and returns its change ID.
 	// If the content of a file is empty, that file will be deleted from the repository.
+	// If the requested contents match the state of the repository, no change
+	// is created and the returned change ID will be empty.
 	CreateAutoSubmitChange(ctx context.Context, input gerrit.ChangeInput, contents map[string]string) (string, error)
 	// AwaitSubmit waits for the specified change to be auto-submitted or fail
 	// trybots. If the CL is submitted, returns the submitted commit hash.
@@ -37,16 +39,27 @@ func (c *RealGerritClient) CreateAutoSubmitChange(ctx context.Context, input ger
 		return "", err
 	}
 	changeID := fmt.Sprintf("%s~%d", change.Project, change.ChangeNumber)
+	anyChange := false
 	for path, content := range files {
+		var err error
 		if content == "" {
-			if err := c.Client.DeleteFileInChangeEdit(ctx, changeID, path); err != nil {
-				return "", err
-			}
+			err = c.Client.DeleteFileInChangeEdit(ctx, changeID, path)
 		} else {
-			if err := c.Client.ChangeFileContentInChangeEdit(ctx, changeID, path, content); err != nil {
-				return "", err
-			}
+			err = c.Client.ChangeFileContentInChangeEdit(ctx, changeID, path, content)
+		}
+		if errors.Is(err, gerrit.ErrNotModified) {
+			continue
+		}
+		if err != nil {
+			return "", err
+		}
+		anyChange = true
+	}
+	if !anyChange {
+		if err := c.Client.AbandonChange(ctx, changeID, "no changes necessary"); err != nil {
+			return "", err
 		}
+		return "", nil
 	}
 
 	if err := c.Client.PublishChangeEdit(ctx, changeID); err != nil {

internal/task/version.go

@@ -84,8 +84,17 @@ func (t *VersionTasks) CreateAutoSubmitVersionCL(ctx *workflow.TaskContext, bran
 	})
 }
 
-// AwaitCL waits for the specified CL to be submitted.
+// AwaitCL waits for the specified CL to be submitted, and returns the new
+// branch head. Callers can pass baseCommit, the current branch head, to verify
+// that no CLs were submitted between when the CL was created and when it was
+// merged. If changeID is blank because the intended CL was a no-op, baseCommit
+// is returned immediately.
 func (t *VersionTasks) AwaitCL(ctx *workflow.TaskContext, changeID, baseCommit string) (string, error) {
+	if changeID == "" {
+		ctx.Printf("No CL was necessary")
+		return baseCommit, nil
+	}
+
 	ctx.Printf("Awaiting review/submit of %v", ChangeLink(changeID))
 	return t.Gerrit.AwaitSubmit(ctx, changeID, baseCommit)
 }