maintner/maintnerd/maintapi: look for TRY= in inline comments
The Gerrit UI has changed its default behavior when leaving top-level
comments on a CL. Previously, that text was a part of the CL-scoped
messages, but now it is located in a patchset-level inline comment.
This change updates the logic in maintapi to look for TRY= comments
in the new place. This requires making an additional Gerrit API call
to the new ListChangeComments endpoint, because the inline comment
data is not tracked by the maintner corpus, and it can't be fetched
via the existing QueryChanges API call.
Start using the patch set number as returned by the Gerrit API,
instead of parsing it out of the message prefix with a regexp,
as this is simpler and should be more robust.
Document and simplify tryWorkItem slightly by removing support for
nil *gerrit.ChangeInfo. This was only needed for tests, but it's easy
to adjust tests to not depend on this. That makes the code in the
helper less indented and less complicated.
During these changes, the unintentional pitfall where the TRY= comment
had to be preceded by a blank link has also been removed. It only needs
to be on its own line now, as it was likely originally intended.
Fixes golang/go#40106.
Fixes golang/go#38747.
Change-Id: I64e85c18fefc23e1796de61dd8ba7025ea411706
Reviewed-on: https://go-review.googlesource.com/c/build/+/241323
Reviewed-by: Filippo Valsorda <filippo@golang.org>
diff --git a/gerrit/gerrit.go b/gerrit/gerrit.go
index 4d732b4..e7f7fa8 100644
--- a/gerrit/gerrit.go
+++ b/gerrit/gerrit.go
@@ -489,6 +489,28 @@
return &change, nil
}
+// ListChangeComments retrieves a map of published comments for the given change ID.
+// The map key is the file path (such as "maintner/git_test.go" or "/PATCHSET_LEVEL").
+// For the API call, see https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#list-change-comments.
+func (c *Client) ListChangeComments(ctx context.Context, changeID string) (map[string][]CommentInfo, error) {
+ var m map[string][]CommentInfo
+ if err := c.do(ctx, &m, "GET", "/changes/"+changeID+"/comments"); err != nil {
+ return nil, err
+ }
+ return m, nil
+}
+
+// CommentInfo contains information about an inline comment.
+// See https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#comment-info.
+type CommentInfo struct {
+ PatchSet int `json:"patch_set,omitempty"`
+ ID string `json:"id"`
+ Path string `json:"path,omitempty"`
+ Message string `json:"message,omitempty"`
+ Updated TimeStamp `json:"updated"`
+ Author *AccountInfo `json:"author,omitempty"`
+}
+
// ListFiles retrieves a map of filenames to FileInfo's for the given change ID and revision.
// For the API call, see https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#list-files
func (c *Client) ListFiles(ctx context.Context, changeID, revision string) (map[string]*FileInfo, error) {
diff --git a/maintner/maintnerd/apipb/api.pb.go b/maintner/maintnerd/apipb/api.pb.go
index 82c2ea5..a71cc18 100644
--- a/maintner/maintnerd/apipb/api.pb.go
+++ b/maintner/maintnerd/apipb/api.pb.go
@@ -200,6 +200,7 @@
// For subrepos, it contains elements that correspond to go_commit.
GoVersion []*MajorMinor `protobuf:"bytes,7,rep,name=go_version,json=goVersion" json:"go_version,omitempty"`
// try_message is the list of TRY=xxxx messages associated with Run-TryBot votes.
+ // It's sorted from oldest to newest.
TryMessage []*TryVoteMessage `protobuf:"bytes,8,rep,name=try_message,json=tryMessage" json:"try_message,omitempty"`
}
diff --git a/maintner/maintnerd/apipb/api.proto b/maintner/maintnerd/apipb/api.proto
index 577b80c..d806164 100644
--- a/maintner/maintnerd/apipb/api.proto
+++ b/maintner/maintnerd/apipb/api.proto
@@ -69,6 +69,7 @@
repeated MajorMinor go_version = 7;
// try_message is the list of TRY=xxxx messages associated with Run-TryBot votes.
+ // It's sorted from oldest to newest.
repeated TryVoteMessage try_message = 8;
}
diff --git a/maintner/maintnerd/maintapi/api.go b/maintner/maintnerd/maintapi/api.go
index a5c2185..74e7b28 100644
--- a/maintner/maintnerd/maintapi/api.go
+++ b/maintner/maintnerd/maintapi/api.go
@@ -12,7 +12,6 @@
"log"
"regexp"
"sort"
- "strconv"
"strings"
"sync"
"time"
@@ -100,55 +99,60 @@
return
}
-var (
- tryCommentRx = regexp.MustCompile(`(?m)^TRY=(.*)$`)
- patchSetRx = regexp.MustCompile(`^Patch Set (\d{1,4}):`)
-)
+var tryCommentRx = regexp.MustCompile(`(?m)^TRY=(.*)$`)
-func tryWorkItem(cl *maintner.GerritCL, ci *gerrit.ChangeInfo) *apipb.GerritTryWorkItem {
- work := &apipb.GerritTryWorkItem{
+// tryWorkItem creates a GerritTryWorkItem for
+// the Gerrit CL specified by cl, ci, comments.
+func tryWorkItem(cl *maintner.GerritCL, ci *gerrit.ChangeInfo, comments map[string][]gerrit.CommentInfo) *apipb.GerritTryWorkItem {
+ w := &apipb.GerritTryWorkItem{
Project: cl.Project.Project(),
Branch: strings.TrimPrefix(cl.Branch(), "refs/heads/"),
ChangeId: cl.ChangeID(),
Commit: cl.Commit.Hash.String(),
}
- if ci != nil {
- if ci.CurrentRevision != "" {
- // In case maintner is behind.
- work.Commit = ci.CurrentRevision
- work.Version = int32(ci.Revisions[ci.CurrentRevision].PatchSetNumber)
+ if ci.CurrentRevision != "" {
+ // In case maintner is behind.
+ w.Commit = ci.CurrentRevision
+ w.Version = int32(ci.Revisions[ci.CurrentRevision].PatchSetNumber)
+ }
+ // Look for "TRY=" comments. Only consider messages that are accompanied
+ // by a Run-TryBot+1 vote, as a way of confirming the comment author has
+ // Trybot Access (see https://golang.org/wiki/GerritAccess#trybot-access-may-start-trybots).
+ for _, m := range ci.Messages {
+ // msg is like:
+ // "Patch Set 2: Run-TryBot+1\n\n(1 comment)"
+ // "Patch Set 2: Run-TryBot+1 Code-Review-2"
+ // "Uploaded patch set 2."
+ // "Removed Run-TryBot+1 by Brad Fitzpatrick <bradfitz@golang.org>\n"
+ // "Patch Set 1: Run-TryBot+1\n\n(2 comments)"
+ if msg := m.Message; !strings.HasPrefix(msg, "Patch Set ") ||
+ !strings.Contains(firstLine(msg), "Run-TryBot+1") {
+ continue
}
- // Also include any "TRY=foo" comments (just the "foo"
- // aprt) from messages that accompany Run-TryBot+1
- // votes.
- for _, m := range ci.Messages {
- // msg is like:
- // "Patch Set 2: Run-TryBot+1\n\nTRY=foo2"
- // "Patch Set 2: Run-TryBot+1 Code-Review-2"
- // "Uploaded patch set 2."
- // "Removed Run-TryBot+1 by Brad Fitzpatrick <bradfitz@golang.org>\n"
- // "Patch Set 1: Run-TryBot+1\n\nTRY=baz"
- msg := m.Message
- if !strings.Contains(msg, "\n\nTRY=") ||
- !strings.HasPrefix(msg, "Patch Set ") ||
- !strings.Contains(firstLine(msg), "Run-TryBot+1") {
+ // Get "TRY=foo" comments (just the "foo" part)
+ // from matching patchset-level comments. They
+ // are posted on the magic "/PATCHSET_LEVEL" path, see https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#file-id.
+ for _, c := range comments["/PATCHSET_LEVEL"] {
+ // It should be sufficient to match by equal time only.
+ // But check that author and patch set match too in order to be more strict.
+ if !c.Updated.Equal(m.Time) || c.Author.NumericID != m.Author.NumericID || c.PatchSet != m.RevisionNumber {
continue
}
- pm := patchSetRx.FindStringSubmatch(msg)
- var patchSet int
- if pm != nil {
- patchSet, _ = strconv.Atoi(pm[1])
+ if len(w.TryMessage) > 0 && c.PatchSet <= int(w.TryMessage[len(w.TryMessage)-1].Version) {
+ continue
}
- if tm := tryCommentRx.FindStringSubmatch(msg); tm != nil && patchSet > 0 {
- work.TryMessage = append(work.TryMessage, &apipb.TryVoteMessage{
- Message: tm[1],
- AuthorId: m.Author.NumericID,
- Version: int32(patchSet),
- })
+ tm := tryCommentRx.FindStringSubmatch(c.Message)
+ if tm == nil {
+ continue
}
+ w.TryMessage = append(w.TryMessage, &apipb.TryVoteMessage{
+ Message: tm[1],
+ AuthorId: c.Author.NumericID,
+ Version: int32(c.PatchSet),
+ })
}
}
- return work
+ return w
}
func firstLine(s string) string {
@@ -253,7 +257,11 @@
log.Printf("nil Gerrit CL %v", ci.ChangeNumber)
continue
}
- work := tryWorkItem(cl, ci)
+ comments, err := gerritc.ListChangeComments(ctx, ci.ID)
+ if err != nil {
+ return nil, err
+ }
+ work := tryWorkItem(cl, ci, comments)
if work.Project == "go" {
// Trybot on Go repo. Set the GoVersion field based on branch name.
if work.Branch == "master" {
diff --git a/maintner/maintnerd/maintapi/api_test.go b/maintner/maintnerd/maintapi/api_test.go
index 88b53b9..2c7b429 100644
--- a/maintner/maintnerd/maintapi/api_test.go
+++ b/maintner/maintnerd/maintapi/api_test.go
@@ -124,16 +124,17 @@
func TestTryWorkItem(t *testing.T) {
c := getGoData(t)
tests := []struct {
- proj string
- clnum int32
- ci *gerrit.ChangeInfo
- want string
+ proj string
+ clnum int32
+ ci *gerrit.ChangeInfo
+ comments map[string][]gerrit.CommentInfo
+ want string
}{
// Same Change-Id, different branch:
- {"go", 51430, nil, `project:"go" branch:"master" change_id:"I0bcae339624e7d61037d9ea0885b7bd07491bbb6" commit:"45a4609c0ae214e448612e0bc0846e2f2682f1b2" `},
- {"go", 51450, nil, `project:"go" branch:"release-branch.go1.9" change_id:"I0bcae339624e7d61037d9ea0885b7bd07491bbb6" commit:"7320506bc58d3a55eff2c67b2ec65cfa94f7b0a7" `},
+ {"go", 51430, &gerrit.ChangeInfo{}, nil, `project:"go" branch:"master" change_id:"I0bcae339624e7d61037d9ea0885b7bd07491bbb6" commit:"45a4609c0ae214e448612e0bc0846e2f2682f1b2" `},
+ {"go", 51450, &gerrit.ChangeInfo{}, nil, `project:"go" branch:"release-branch.go1.9" change_id:"I0bcae339624e7d61037d9ea0885b7bd07491bbb6" commit:"7320506bc58d3a55eff2c67b2ec65cfa94f7b0a7" `},
// Different project:
- {"build", 51432, nil, `project:"build" branch:"master" change_id:"I1f71836da7008e58d3e76e2cc3170e96cd57ddf6" commit:"9251bc9950baff61d95da0761e2e4bfab61ed210" `},
+ {"build", 51432, &gerrit.ChangeInfo{}, nil, `project:"build" branch:"master" change_id:"I1f71836da7008e58d3e76e2cc3170e96cd57ddf6" commit:"9251bc9950baff61d95da0761e2e4bfab61ed210" `},
// With comments:
{
@@ -146,16 +147,36 @@
},
Messages: []gerrit.ChangeMessageInfo{
{
- Author: &gerrit.AccountInfo{NumericID: 1234},
- Message: "Patch Set 1: Run-TryBot+1\n\nTRY=foo",
+ Author: &gerrit.AccountInfo{NumericID: 1234},
+ Message: "Patch Set 1: Run-TryBot+1\n\n(1 comment)",
+ Time: gerrit.TimeStamp(time.Date(2020, 7, 7, 23, 27, 23, 0, time.UTC)),
+ RevisionNumber: 1,
},
{
- Author: &gerrit.AccountInfo{NumericID: 5678},
- Message: "Patch Set 2: Foo-2 Run-TryBot+1\n\nTRY=bar",
+ Author: &gerrit.AccountInfo{NumericID: 5678},
+ Message: "Patch Set 2: Foo-2 Run-TryBot+1\n\n(1 comment)",
+ Time: gerrit.TimeStamp(time.Date(2020, 7, 7, 23, 28, 47, 0, time.UTC)),
+ RevisionNumber: 2,
},
},
},
- want: `project:"go" branch:"master" change_id:"I358eb7b11768df8c80fb7e805abd4cd01d52bb9b" commit:"f99d33e72efdea68fce39765bc94479b5ebed0a9" version:88 try_message:<message:"foo" author_id:1234 version:1 > try_message:<message:"bar" author_id:5678 version:2 > `,
+ comments: map[string][]gerrit.CommentInfo{
+ "/PATCHSET_LEVEL": {
+ {
+ PatchSet: 1,
+ Message: "TRY=foo",
+ Updated: gerrit.TimeStamp(time.Date(2020, 7, 7, 23, 27, 23, 0, time.UTC)),
+ Author: &gerrit.AccountInfo{NumericID: 1234},
+ },
+ {
+ PatchSet: 2,
+ Message: "A preceding sentence.\nTRY=bar, baz\nA following sentence.",
+ Updated: gerrit.TimeStamp(time.Date(2020, 7, 7, 23, 28, 47, 0, time.UTC)),
+ Author: &gerrit.AccountInfo{NumericID: 5678},
+ },
+ },
+ },
+ want: `project:"go" branch:"master" change_id:"I358eb7b11768df8c80fb7e805abd4cd01d52bb9b" commit:"f99d33e72efdea68fce39765bc94479b5ebed0a9" version:88 try_message:<message:"foo" author_id:1234 version:1 > try_message:<message:"bar, baz" author_id:5678 version:2 > `,
},
}
for _, tt := range tests {
@@ -164,7 +185,7 @@
t.Errorf("CL %d in %s not found", tt.clnum, tt.proj)
continue
}
- got := fmt.Sprint(tryWorkItem(cl, tt.ci))
+ got := fmt.Sprint(tryWorkItem(cl, tt.ci, tt.comments))
if got != tt.want {
t.Errorf("tryWorkItem(%q, %v) mismatch:\n got: %#q\nwant: %#q", tt.proj, tt.clnum, got, tt.want)
}
