Skip to content

Commit a6e0400

Browse files
committed
cmd/gerritbot: don't query GitHub as much
Use maintner as a fast-path to determine whether comments are duplicates or not. Update golang/go#18517 Change-Id: I3ddc380e6e202371903ce62ae9e4c07e9805c1ed Reviewed-on: https://go-review.googlesource.com/93435 Reviewed-by: Brad Fitzpatrick <[email protected]>
1 parent e008d53 commit a6e0400

File tree

1 file changed

+86
-33
lines changed

1 file changed

+86
-33
lines changed

cmd/gerritbot/gerritbot.go

Lines changed: 86 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -379,12 +379,30 @@ func (b *bot) processPullRequest(ctx context.Context, pr *github.PullRequest) er
379379
return nil
380380
}
381381

382+
if err := b.syncGerritCommentsToGitHub(ctx, pr, cl); err != nil {
383+
return fmt.Errorf("syncGerritCommentsToGitHub: %v", err)
384+
}
385+
382386
if lastRev == "" {
383387
log.Printf("Imported CL https://go-review.googlesource.com/q/%s does not have %s footer; skipping",
384388
cl.ChangeID(), prefixGitFooterLastRev)
385389
return nil
386390
}
387391

392+
if pr.Head.GetSHA() == lastRev {
393+
log.Printf("Change https://go-review.googlesource.com/q/%s is up to date; nothing to do.",
394+
cl.ChangeID())
395+
return nil
396+
}
397+
// Import PR to existing Gerrit Change.
398+
if err := b.importGerritChangeFromPR(ctx, pr, cl); err != nil {
399+
return fmt.Errorf("importGerritChangeFromPR(%v, %v): %v", shortLink, cl, err)
400+
}
401+
b.pendingCLs[shortLink] = prHeadSHA
402+
return nil
403+
}
404+
405+
func (b *bot) syncGerritCommentsToGitHub(ctx context.Context, pr *github.PullRequest, cl *maintner.GerritCL) error {
388406
repo := pr.GetBase().GetRepo()
389407
for _, m := range cl.Messages {
390408
if m.Author.Email() == cl.Owner().Email() {
@@ -396,22 +414,12 @@ func (b *bot) processPullRequest(ctx context.Context, pr *github.PullRequest) er
396414
397415
---
398416
Please don’t reply on this GitHub thread. Visit [golang.org/cl/%d](https://go-review.googlesource.com/c/%s/+/%d#message-%s).
399-
After addressing review feedback, remember to
400-
[publish your drafts](https://github.com/golang/go/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it)!`,
417+
After addressing review feedback, remember to [publish your drafts](https://github.com/golang/go/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it)!`,
401418
m.Author.Name(), m.Message, cl.Number, cl.Project.Project(), cl.Number, m.Meta.Hash.String())
402-
b.postGitHubMessageNoDup(ctx, repo.GetOwner().GetLogin(), repo.GetName(), pr.GetNumber(), msg)
403-
}
404-
405-
if pr.Head.GetSHA() == lastRev {
406-
log.Printf("Change https://go-review.googlesource.com/q/%s is up to date; nothing to do.",
407-
cl.ChangeID())
408-
return nil
409-
}
410-
// Import PR to existing Gerrit Change.
411-
if err := b.importGerritChangeFromPR(ctx, pr, cl); err != nil {
412-
return fmt.Errorf("importGerritChangeFromPR(%v, %v): %v", shortLink, cl, err)
419+
if err := b.postGitHubMessageNoDup(ctx, repo.GetOwner().GetLogin(), repo.GetName(), pr.GetNumber(), msg); err != nil {
420+
return fmt.Errorf("postGitHubMessageNoDup: %v", err)
421+
}
413422
}
414-
b.pendingCLs[shortLink] = prHeadSHA
415423
return nil
416424
}
417425

@@ -621,36 +629,81 @@ func (b *bot) postGitHubMessage(ctx context.Context, owner, repo string, issueNu
621629
// postGitHubMessageNoDup ensures that the message being posted on an issue does not already have the
622630
// same exact content. These comments can be toggled by the user via a slash command /comments {on|off}
623631
// at the beginning of a message.
624-
func (b *bot) postGitHubMessageNoDup(ctx context.Context, owner, repo string, issueNum int, msg string) error {
625-
issue, _, err := b.githubClient.Issues.Get(ctx, owner, repo, issueNum)
626-
if err != nil {
627-
return fmt.Errorf("b.githubClient.Issues.Get(%q, %q, %d): %v", owner, repo, issueNum, err)
632+
// TODO(andybons): This logic is shared by gopherbot. Consolidate it somewhere.
633+
func (b *bot) postGitHubMessageNoDup(ctx context.Context, org, repo string, issueNum int, msg string) error {
634+
gr := b.corpus.GitHub().Repo(org, repo)
635+
if gr == nil {
636+
return fmt.Errorf("unknown github repo %s/%s", org, repo)
628637
}
629-
ownerID := issue.GetUser().GetID()
630-
opts := &github.IssueListCommentsOptions{
631-
ListOptions: github.ListOptions{
632-
PerPage: 1000,
633-
},
638+
var since time.Time
639+
var noComment bool
640+
var ownerID int64
641+
if gi := gr.Issue(int32(issueNum)); gi != nil {
642+
ownerID = gi.User.ID
643+
var dup bool
644+
gi.ForeachComment(func(c *maintner.GitHubComment) error {
645+
since = c.Updated
646+
// TODO: check for exact match?
647+
if strings.Contains(c.Body, msg) {
648+
dup = true
649+
return nil
650+
}
651+
if c.User.ID == ownerID && strings.HasPrefix(c.Body, "/comments ") {
652+
if strings.HasPrefix(c.Body, "/comments off") {
653+
noComment = true
654+
} else if strings.HasPrefix(c.Body, "/comments on") {
655+
noComment = false
656+
}
657+
}
658+
return nil
659+
})
660+
if dup {
661+
// Comment's already been posted. Nothing to do.
662+
return nil
663+
}
634664
}
635-
comments, _, err := b.githubClient.Issues.ListComments(ctx, owner, repo, issueNum, opts)
665+
// See if there is a dup comment from when GerritBot last got
666+
// its data from maintner.
667+
ics, _, err := b.githubClient.Issues.ListComments(ctx, org, repo, int(issueNum), &github.IssueListCommentsOptions{
668+
Since: since,
669+
ListOptions: github.ListOptions{PerPage: 1000},
670+
})
636671
if err != nil {
637-
return fmt.Errorf("b.githubClient.Issues.ListComments(%q, %q, %d, %+v): %v", owner, repo, issueNum, opts, err)
672+
return err
638673
}
639-
var noComment bool
640-
for _, ic := range comments {
641-
b := ic.GetBody()
642-
if b == msg {
674+
for _, ic := range ics {
675+
if strings.Contains(ic.GetBody(), msg) {
676+
// Dup.
643677
return nil
644678
}
645-
if ic.GetUser().GetID() == ownerID && strings.HasPrefix(b, "/comments ") {
646-
noComment = strings.HasPrefix(b, "/comments off")
647-
if strings.HasPrefix(b, "/comments on") {
679+
}
680+
681+
if ownerID == 0 {
682+
issue, _, err := b.githubClient.Issues.Get(ctx, org, repo, issueNum)
683+
if err != nil {
684+
return err
685+
}
686+
ownerID = int64(issue.GetUser().GetID())
687+
}
688+
for _, ic := range ics {
689+
if strings.Contains(ic.GetBody(), msg) {
690+
// Dup.
691+
return nil
692+
}
693+
body := ic.GetBody()
694+
if int64(ic.GetUser().GetID()) == ownerID && strings.HasPrefix(body, "/comments ") {
695+
if strings.HasPrefix(body, "/comments off") {
696+
noComment = true
697+
} else if strings.HasPrefix(body, "/comments on") {
648698
noComment = false
649699
}
650700
}
651701
}
652702
if noComment {
653703
return nil
654704
}
655-
return b.postGitHubMessage(ctx, owner, repo, issueNum, msg)
705+
_, _, err = b.githubClient.Issues.CreateComment(ctx, org, repo, int(issueNum), &github.IssueComment{
706+
Body: github.String(msg),
707+
})
708+
return err
656709
}

0 commit comments

Comments
 (0)