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 <gobot@golang.org>
Run-TryBot: Heschi Kreinick <heschi@google.com>
Auto-Submit: Heschi Kreinick <heschi@google.com>
Reviewed-by: Alex Rakoczy <alex@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
diff --git a/gerrit/gerrit.go b/gerrit/gerrit.go
index e62dadc..d2914c4 100644
--- a/gerrit/gerrit.go
+++ b/gerrit/gerrit.go
@@ -61,6 +61,10 @@
// 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) 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 @@
}
// 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 {
diff --git a/internal/relui/workflows.go b/internal/relui/workflows.go
index c1eb400..aa57775 100644
--- a/internal/relui/workflows.go
+++ b/internal/relui/workflows.go
@@ -297,7 +297,7 @@
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 @@
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 @@
// 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)
diff --git a/internal/task/gerrit.go b/internal/task/gerrit.go
index 21d7ec5..2f9f825 100644
--- a/internal/task/gerrit.go
+++ b/internal/task/gerrit.go
@@ -11,9 +11,11 @@
)
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 @@
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 {
diff --git a/internal/task/version.go b/internal/task/version.go
index 1bc01b8..d02ccae 100644
--- a/internal/task/version.go
+++ b/internal/task/version.go
@@ -84,8 +84,17 @@
})
}
-// 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)
}
