cmd/coordinator: resolve stale TryBot threads
Also, respond to the most recent thread even if there are multiple ones
for the same commit.
This was way harder than it should be.
Change-Id: Id6868b267c019cd70e504310ecf9c79dac29a17b
Reviewed-on: https://go-review.googlesource.com/c/build/+/317971
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Filippo Valsorda <filippo@golang.org>
Trust: Katie Hockman <katie@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
diff --git a/cmd/coordinator/coordinator.go b/cmd/coordinator/coordinator.go
index 9c44088..715557c 100644
--- a/cmd/coordinator/coordinator.go
+++ b/cmd/coordinator/coordinator.go
@@ -1396,6 +1396,73 @@
return "autogenerated:trybots~" + s
}
+func isTryBotsTag(s string) bool {
+ return strings.HasPrefix(s, "autogenerated:trybots~")
+}
+
+// A commentThread is a thread of Gerrit comments.
+type commentThread struct {
+ // root is the first comment in the thread.
+ root gerrit.CommentInfo
+ // thread is a list of all the comments in the thread, including the root,
+ // sorted chronologically.
+ thread []gerrit.CommentInfo
+ // unresolved is the thread unresolved state, based on the last comment.
+ unresolved bool
+}
+
+// listPatchSetThreads returns a list of PATCHSET_LEVEL comment threads, sorted
+// by the time at which they were started.
+func listPatchSetThreads(gerritClient *gerrit.Client, changeID string) ([]*commentThread, error) {
+ comments, err := gerritClient.ListChangeComments(context.Background(), changeID)
+ if err != nil {
+ return nil, err
+ }
+ patchSetComments := comments["/PATCHSET_LEVEL"]
+ if len(patchSetComments) == 0 {
+ return nil, nil
+ }
+
+ // The API doesn't sort comments chronologically, but "the state of
+ // resolution of a comment thread is stored in the last comment in that
+ // thread chronologically", so first of all sort them by time.
+ sort.Slice(patchSetComments, func(i, j int) bool {
+ return patchSetComments[i].Updated.Time().Before(patchSetComments[j].Updated.Time())
+ })
+
+ // roots is a map of message IDs to their thread root.
+ roots := make(map[string]string)
+ threads := make(map[string]*commentThread)
+ var result []*commentThread
+ for _, c := range patchSetComments {
+ if c.InReplyTo == "" {
+ roots[c.ID] = c.ID
+ threads[c.ID] = &commentThread{
+ root: c,
+ thread: []gerrit.CommentInfo{c},
+ unresolved: *c.Unresolved,
+ }
+ if c.Unresolved != nil {
+ threads[c.ID].unresolved = *c.Unresolved
+ }
+ result = append(result, threads[c.ID])
+ continue
+ }
+
+ root, ok := roots[c.InReplyTo]
+ if !ok {
+ return nil, fmt.Errorf("%s has no parent", c.ID)
+ }
+ roots[c.ID] = root
+ threads[root].thread = append(threads[root].thread, c)
+ if c.Unresolved != nil {
+ threads[root].unresolved = *c.Unresolved
+ }
+ }
+
+ return result, nil
+}
+
func (ts *trySet) statusPage() string {
return "https://farmer.golang.org/try?commit=" + ts.Commit[:8]
}
@@ -1417,15 +1484,42 @@
}
}
- gerritClient := pool.NewGCEConfiguration().GerritClient()
- ctx := context.Background()
unresolved := true
- if err := gerritClient.SetReview(ctx, ts.ChangeTriple(), ts.Commit, gerrit.ReviewInput{
+ ri := gerrit.ReviewInput{
Tag: tryBotsTag("beginning"),
Comments: map[string][]gerrit.CommentInput{
"/PATCHSET_LEVEL": {{Message: msg, Unresolved: &unresolved}},
},
- }); err != nil {
+ }
+
+ // Mark as resolved old TryBot threads that don't have human comments on them.
+ gerritClient := pool.NewGCEConfiguration().GerritClient()
+ if patchSetThreads, err := listPatchSetThreads(gerritClient, ts.ChangeTriple()); err == nil {
+ for _, t := range patchSetThreads {
+ if !t.unresolved {
+ continue
+ }
+ hasHumanComments := false
+ for _, c := range t.thread {
+ if !isTryBotsTag(c.Tag) {
+ hasHumanComments = true
+ break
+ }
+ }
+ if hasHumanComments {
+ continue
+ }
+ unresolved := false
+ ri.Comments["/PATCHSET_LEVEL"] = append(ri.Comments["/PATCHSET_LEVEL"], gerrit.CommentInput{
+ Message: "Superseded.",
+ Unresolved: &unresolved,
+ })
+ }
+ } else {
+ log.Printf("Error getting Gerrit threads on %s: %v", ts.ChangeTriple(), err)
+ }
+
+ if err := gerritClient.SetReview(context.Background(), ts.ChangeTriple(), ts.Commit, ri); err != nil {
log.Printf("Error leaving Gerrit comment on %s: %v", ts.Commit[:8], err)
}
}
@@ -1624,15 +1718,14 @@
var inReplyTo string
gerritClient := pool.NewGCEConfiguration().GerritClient()
- ctx := context.Background()
- if comments, err := gerritClient.ListChangeComments(ctx, ts.ChangeTriple()); err == nil {
- for _, ci := range comments["/PATCHSET_LEVEL"] {
- if strings.Contains(ci.Message, ts.statusPage()) {
- inReplyTo = ci.ID
+ if patchSetThreads, err := listPatchSetThreads(gerritClient, ts.ChangeTriple()); err == nil {
+ for _, t := range patchSetThreads {
+ if t.root.Tag == tryBotsTag("beginning") && strings.Contains(t.root.Message, ts.statusPage()) {
+ inReplyTo = t.root.ID
}
}
} else {
- log.Printf("Error getting Gerrit comments on %s: %v", ts.ChangeTriple(), err)
+ log.Printf("Error getting Gerrit threads on %s: %v", ts.ChangeTriple(), err)
}
// Mark resolved if TryBots are happy.
@@ -1653,7 +1746,7 @@
"TryBot-Result": gerritScore,
}
}
- if err := gerritClient.SetReview(ctx, ts.ChangeTriple(), ts.Commit, ri); err != nil {
+ if err := gerritClient.SetReview(context.Background(), ts.ChangeTriple(), ts.Commit, ri); err != nil {
log.Printf("Error leaving Gerrit comment on %s: %v", ts.Commit[:8], err)
}
}
diff --git a/cmd/coordinator/coordinator_test.go b/cmd/coordinator/coordinator_test.go
index 52442fc..3d83d71 100644
--- a/cmd/coordinator/coordinator_test.go
+++ b/cmd/coordinator/coordinator_test.go
@@ -21,6 +21,7 @@
"golang.org/x/build/buildenv"
"golang.org/x/build/dashboard"
+ "golang.org/x/build/gerrit"
"golang.org/x/build/internal/buildgo"
"golang.org/x/build/internal/coordinator/pool"
"golang.org/x/build/maintner/maintnerd/apipb"
@@ -422,3 +423,38 @@
}
}
}
+
+// listPatchSetThreadsResponse is the response to
+// https://go-review.googlesource.com/changes/go~master~I92400996cb051ab30e99bfffafd91ff32a1e7087/comments
+var listPatchSetThreadsResponse = []byte(`)]}'
+{"/PATCHSET_LEVEL":[{"author":{"_account_id":5976,"name":"Go Bot","email":"gobot@golang.org","tags":["SERVICE_USER"]},"tag":"autogenerated:trybots~beginning","change_message_id":"af128c803aa192eb5c191c1567713f5b123d3adb","unresolved":true,"patch_set":1,"id":"aaf7aa39_658707c2","updated":"2021-04-27 18:20:09.000000000","message":"SlowBots beginning. Status page: https://farmer.golang.org/try?commit\u003d39ad506d","commit_id":"39ad506d874d4711015184f52585b4215c9b84cc"},{"author":{"_account_id":5976,"name":"Go Bot","email":"gobot@golang.org","tags":["SERVICE_USER"]},"tag":"autogenerated:trybots~beginning","change_message_id":"a00ee30c652a61afeb5ba7657e5823b2d56159ac","unresolved":true,"patch_set":1,"id":"cb0c9011_d26d6550","updated":"2021-04-27 07:10:41.000000000","message":"SlowBots beginning. Status page: https://farmer.golang.org/try?commit\u003d39ad506d","commit_id":"39ad506d874d4711015184f52585b4215c9b84cc"},{"author":{"_account_id":6365,"name":"Bryan C. Mills","email":"bcmills@google.com"},"change_message_id":"c3dc9da7c814efe691876649d8b1acd086d34921","unresolved":false,"patch_set":1,"id":"da1249e7_bc007148","updated":"2021-04-27 07:10:22.000000000","message":"TRY\u003dlongtest","commit_id":"39ad506d874d4711015184f52585b4215c9b84cc"},{"author":{"_account_id":6365,"name":"Bryan C. Mills","email":"bcmills@google.com"},"change_message_id":"1908c5ae7cd3fa1adc7579bfb4fa0798a33dafd2","unresolved":false,"patch_set":1,"id":"043375d0_558208b0","in_reply_to":"50a54b3c_f95f3567","updated":"2021-04-27 18:32:56.000000000","message":"#41863","commit_id":"39ad506d874d4711015184f52585b4215c9b84cc"},{"author":{"_account_id":6365,"name":"Bryan C. Mills","email":"bcmills@google.com"},"change_message_id":"061f7a6231e09027e96b7c81093b32db9cd6a6f3","unresolved":false,"patch_set":1,"id":"49f73b27_b0261bc8","in_reply_to":"aaf7aa39_658707c2","updated":"2021-04-27 19:17:53.000000000","message":"Ack","commit_id":"39ad506d874d4711015184f52585b4215c9b84cc"},{"author":{"_account_id":5976,"name":"Go Bot","email":"gobot@golang.org","tags":["SERVICE_USER"]},"tag":"autogenerated:trybots~failed","change_message_id":"d7e2ff4f58b281bc3120ad79a9c5bf7c87c0ec1b","unresolved":true,"patch_set":1,"id":"50a54b3c_f95f3567","in_reply_to":"c3e462db_b5e1efca","updated":"2021-04-27 07:22:38.000000000","message":"1 of 26 SlowBots failed.\nFailed on linux-arm64-aws: https://storage.googleapis.com/go-build-log/39ad506d/linux-arm64-aws_5dc1efb9.log\n\nConsult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test *exactly* your git commit, without rebasing. If your commit\u0027s git parent is old, the failure might\u0027ve already been fixed.\n\nSlowBot builds that ran:\n* linux-amd64-longtest\n","commit_id":"39ad506d874d4711015184f52585b4215c9b84cc"},{"author":{"_account_id":5976,"name":"Go Bot","email":"gobot@golang.org","tags":["SERVICE_USER"]},"tag":"autogenerated:trybots~happy","change_message_id":"9ed90c9e9b43e3c2c4d2b6ab8e8a121686d5da88","unresolved":false,"patch_set":1,"id":"13677404_911c1149","in_reply_to":"c3e462db_b5e1efca","updated":"2021-04-27 18:46:54.000000000","message":"SlowBots are happy.\n\nSlowBot builds that ran:\n* linux-amd64-longtest\n","commit_id":"39ad506d874d4711015184f52585b4215c9b84cc"},{"author":{"_account_id":5976,"name":"Go Bot","email":"gobot@golang.org","tags":["SERVICE_USER"]},"tag":"autogenerated:trybots~progress","change_message_id":"ab963b29aa95907e30e9f6a51ef1f080807bc8ab","unresolved":true,"patch_set":1,"id":"c3e462db_b5e1efca","in_reply_to":"cb0c9011_d26d6550","updated":"2021-04-27 07:18:11.000000000","message":"Build is still in progress... Status page: https://farmer.golang.org/try?commit\u003d39ad506d\nFailed on linux-arm64-aws: https://storage.googleapis.com/go-build-log/39ad506d/linux-arm64-aws_5dc1efb9.log\nOther builds still in progress; subsequent failure notices suppressed until final report.\n\nConsult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test *exactly* your git commit, without rebasing. If your commit\u0027s git parent is old, the failure might\u0027ve already been fixed.\n","commit_id":"39ad506d874d4711015184f52585b4215c9b84cc"}]}
+`)
+
+func TestListPatchSetThreads(t *testing.T) {
+ s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ w.Header().Set("Content-Type", "application/json; charset=UTF-8")
+ w.WriteHeader(200)
+ w.Write(listPatchSetThreadsResponse)
+ }))
+ defer s.Close()
+ gerritClient := gerrit.NewClient(s.URL, gerrit.NoAuth)
+ threads, err := listPatchSetThreads(gerritClient, "go~master~I92400996cb051ab30e99bfffafd91ff32a1e7087")
+ if err != nil {
+ t.Fatal(err)
+ }
+ var mostRecentTryBotThread string
+ for _, tr := range threads {
+ if tr.unresolved {
+ t.Errorf("thread %s is unresolved", tr.root.ID)
+ }
+ if tr.root.Tag == tryBotsTag("beginning") {
+ mostRecentTryBotThread = tr.root.ID
+ }
+ if tr.root != tr.thread[0] {
+ t.Errorf("the root is not the first comment in thread")
+ }
+ }
+ if mostRecentTryBotThread != "aaf7aa39_658707c2" {
+ t.Errorf("wrong most recent TryBot thread: got %s, want %s", mostRecentTryBotThread, "aaf7aa39_658707c2")
+ }
+}
diff --git a/gerrit/gerrit.go b/gerrit/gerrit.go
index d4d1fe6..4f41796 100644
--- a/gerrit/gerrit.go
+++ b/gerrit/gerrit.go
@@ -503,12 +503,15 @@
// 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"`
+ 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"`
+ InReplyTo string `json:"in_reply_to,omitempty"`
+ Unresolved *bool `json:"unresolved,omitempty"`
+ Tag string `json:"tag,omitempty"`
}
// ListFiles retrieves a map of filenames to FileInfo's for the given change ID and revision.
