From f72df8f025e16354f61023f2df431218ac60f4ee Mon Sep 17 00:00:00 2001 From: Kevin Burke Date: Tue, 18 Jul 2017 21:12:37 -0700 Subject: [PATCH] cmd/gopherbot: remove sequential order assumption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously we assumed that Gerrit issued CL numbers in sequential order: If it issued CL N, any new CL would be in the range [n+1, ∞). However, observation shows this not to be true; I submitted a number of CL's just now and got this ordering: 49910, 49911, 49891, 49852. This explains why there are ten people who have yet to be congratulated for submitting their first CL: Gerrit issued a newer CL number and we ignored any CL's older than that. I didn't catch this in testing because I wasn't running in daemon mode locally, so mostRecentCL was always 0. Change-Id: I822ca12a093bd6c186701a4d49b47b2671bcee8a Reviewed-on: https://go-review.googlesource.com/49853 Reviewed-by: Jessie Frazelle Reviewed-by: Brad Fitzpatrick --- cmd/gopherbot/gopherbot.go | 14 ++++---------- maintner/gerrit.go | 6 ++++-- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/cmd/gopherbot/gopherbot.go b/cmd/gopherbot/gopherbot.go index a805d79a..7bd15878 100644 --- a/cmd/gopherbot/gopherbot.go +++ b/cmd/gopherbot/gopherbot.go @@ -189,8 +189,6 @@ type gopherbot struct { // github issues. It's updated at the end of the run of tasks. maxIssueMod time.Time knownContributors map[string]bool - // Most recent CL processed by the contributor congratulation script - mostRecentCL int32 } var tasks = []struct { @@ -763,24 +761,21 @@ var congratsEpoch = time.Date(2017, 6, 17, 0, 0, 0, 0, time.UTC) func (b *gopherbot) congratulateNewContributors(ctx context.Context) error { cls := make(map[string]*maintner.GerritCL) - newHighestCL := b.mostRecentCL b.corpus.Gerrit().ForeachProjectUnsorted(func(gp *maintner.GerritProject) error { if gp.Server() != "go.googlesource.com" { return nil } return gp.ForeachCLUnsorted(func(cl *maintner.GerritCL) error { + // CLs can be returned by maintner in any order. Note also that + // Gerrit CL numbers are sparse (CL N does not guarantee that CL N-1 + // exists) and Gerrit issues CL's out of order - it may issue CL N, + // then CL (N - 18), then CL (N - 40). if b.knownContributors == nil { b.knownContributors = make(map[string]bool) } if cl.Commit == nil { return nil } - if cl.Number <= b.mostRecentCL { - return nil - } - if cl.Number > newHighestCL { - newHighestCL = cl.Number - } email := cl.Commit.Author.Str if b.knownContributors[email] { return nil @@ -839,7 +834,6 @@ func (b *gopherbot) congratulateNewContributors(ctx context.Context) error { } b.knownContributors[email] = true } - b.mostRecentCL = newHighestCL return nil } diff --git a/maintner/gerrit.go b/maintner/gerrit.go index d6a144f5..11fcf4dd 100644 --- a/maintner/gerrit.go +++ b/maintner/gerrit.go @@ -210,8 +210,10 @@ type GerritCL struct { // Project is the project this CL is part of. Project *GerritProject - // Number is the CL number on the Gerrit - // server. (e.g. 1, 2, 3) + // Number is the CL number on the Gerrit server (e.g. 1, 2, 3). Gerrit CL + // numbers are sparse (CL N does not guarantee that CL N-1 exists) and + // Gerrit issues CL's out of order - it may issue CL N, then CL (N - 18), + // then CL (N - 40). Number int32 Created time.Time