Repository: jenkins-x/lighthouse (go)

Issue:
Keeper status description on pull request should say why a PR is in merge pool but not merging
It can be confusing when a PR is all green but not merging, due to another PR being ahead of it in line to be retested and merged after a change to the target branch. It'd be nice if that was made more clear on the Keeper status description.

Pull request:
feat: Set Keeper status desc for green PRs blocked by retests
fixes #1029 

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

Patch:
diff --git a/pkg/keeper/keeper.go b/pkg/keeper/keeper.go
index 2520c7a5..c8b4b704 100644
--- a/pkg/keeper/keeper.go
+++ b/pkg/keeper/keeper.go
@@ -152,6 +152,14 @@ type Pool struct {
 	Error    string
 }
 
+type prWithStatus struct {
+	pr              PullRequest
+	success         bool
+	waitingFor      []int
+	waitingForBatch []int
+	blocks          []blockers.Blocker
+}
+
 // Prometheus Metrics
 var (
 	keeperMetrics = struct {
@@ -264,21 +272,10 @@ func (c *DefaultController) GetHistory() *history.History {
 	return c.History
 }
 
-func prKey(pr *PullRequest) string {
+func (pr *PullRequest) prKey() string {
 	return fmt.Sprintf("%s#%d", string(pr.Repository.NameWithOwner), int(pr.Number))
 }
 
-// org/repo#number -> pr
-func byRepoAndNumber(prs []PullRequest) map[string]PullRequest {
-	m := make(map[string]PullRequest)
-	for _, pr := range prs {
-		p := pr
-		key := prKey(&p)
-		m[key] = pr
-	}
-	return m
-}
-
 // newExpectedContext creates a Context with Expected state.
 func newExpectedContext(c string) Context {
 	return Context{
@@ -322,7 +319,7 @@ func (c *DefaultController) Sync() error {
 
 			for _, pr := range results {
 				p := pr
-				prs[prKey(&p)] = pr
+				prs[p.prKey()] = pr
 			}
 		}
 	} else {
@@ -334,7 +331,7 @@ func (c *DefaultController) Sync() error {
 
 		for _, pr := range results {
 			p := pr
-			prs[prKey(&p)] = pr
+			prs[p.prKey()] = pr
 		}
 	}
 	c.logger.WithField(
@@ -378,16 +375,6 @@ func (c *DefaultController) Sync() error {
 	}
 	filteredPools := c.filterSubpools(c.config().Keeper.MaxGoroutines, rawPools)
 
-	// Notify statusController about the new pool.
-	c.sc.Lock()
-	c.sc.blocks = blocks
-	c.sc.poolPRs = poolPRMap(filteredPools)
-	select {
-	case c.sc.newPoolPending <- true:
-	default:
-	}
-	c.sc.Unlock()
-
 	// Sync subpools in parallel.
 	poolChan := make(chan Pool, len(filteredPools))
 	subpoolsInParallel(
@@ -410,6 +397,15 @@ func (c *DefaultController) Sync() error {
 	sortPools(pools)
 	c.m.Lock()
 	c.pools = pools
+	// Notify statusController about the new pool.
+	c.sc.Lock()
+	c.sc.blocks = blocks
+	c.sc.poolPRs = poolsToStatusPRMap(pools)
+	select {
+	case c.sc.newPoolPending <- true:
+	default:
+	}
+	c.sc.Unlock()
 	// While we're locked, rerun failed-but-rerunnable PipelineRuns.
 	c.logger.WithField("duration", time.Since(start).String()).Debug("Rerunning PipelineRuns failed due to race condition.")
 	err = rerunPipelineRunsWithRaceConditionFailure(c.tektonClient, c.ns, c.logger)
@@ -423,6 +419,82 @@ func (c *DefaultController) Sync() error {
 	return nil
 }
 
+func poolsToStatusPRMap(pools []Pool) map[string]prWithStatus {
+	result := make(map[string]prWithStatus)
+
+	for _, p := range pools {
+		for k, v := range p.toPRsWithStatus() {
+			result[k] = v
+		}
+	}
+
+	return result
+}
+
+func (p *Pool) toPRsWithStatus() map[string]prWithStatus {
+	result := make(map[string]prWithStatus)
+
+	waitingFor := make(map[int]bool)
+	waitingForBatch := make(map[int]bool)
+
+	targets := make(map[int]bool)
+
+	if p.Action == Merge || p.Action == MergeBatch {
+		for _, t := range p.Target {
+			targets[int(t.Number)] = true
+		}
+	}
+
+	for _, w := range p.PendingPRs {
+		waitingFor[int(w.Number)] = true
+		result[w.prKey()] = prWithStatus{
+			pr:      w,
+			success: false,
+		}
+	}
+
+	for _, b := range p.BatchPending {
+		waitingForBatch[int(b.Number)] = true
+		result[b.prKey()] = prWithStatus{
+			pr:      b,
+			success: false,
+		}
+	}
+
+	for _, m := range p.MissingPRs {
+		result[m.prKey()] = prWithStatus{
+			pr:      m,
+			success: false,
+		}
+	}
+
+	for _, s := range p.SuccessPRs {
+		out := prWithStatus{
+			pr:              s,
+			success:         true,
+			waitingFor:      []int{},
+			waitingForBatch: []int{},
+			blocks:          p.Blockers,
+		}
+		// Add waiting for information for succeeded PRs
+		_, inTargets := targets[int(s.Number)]
+		_, inPending := waitingFor[int(s.Number)]
+		_, inBatch := waitingFor[int(s.Number)]
+
+		if !inTargets && !inPending && !inBatch {
+			for k := range waitingFor {
+				out.waitingFor = append(out.waitingFor, k)
+			}
+			for k := range waitingForBatch {
+				out.waitingForBatch = append(out.waitingForBatch, k)
+			}
+		}
+		result[s.prKey()] = out
+	}
+
+	return result
+}
+
 func (c *DefaultController) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	c.m.Lock()
 	defer c.m.Unlock()
@@ -578,18 +650,6 @@ func filterPR(spc scmProviderClient, sp *subpool, pr *PullRequest) bool {
 	return false
 }
 
-// poolPRMap collects all subpool PRs into a map containing all pooled PRs.
-func poolPRMap(subpoolMap map[string]*subpool) map[string]PullRequest {
-	prs := make(map[string]PullRequest)
-	for _, sp := range subpoolMap {
-		for _, pr := range sp.prs {
-			p := pr
-			prs[prKey(&p)] = pr
-		}
-	}
-	return prs
-}
-
 type simpleState string
 
 const (
diff --git a/pkg/keeper/status.go b/pkg/keeper/status.go
index ad6afc7e..fb2ab99a 100644
--- a/pkg/keeper/status.go
+++ b/pkg/keeper/status.go
@@ -39,7 +39,7 @@ import (
 
 const (
 	statusContext string = "keeper"
-	statusInPool         = "In merge pool."
+	statusInPool         = "In merge pool"
 	// statusNotInPool is a format string used when a PR is not in a keeper pool.
 	// The '%s' field is populated with the reason why the PR is not in a
 	// keeper pool or the empty string if the reason is unknown. See requirementDiff.
@@ -82,7 +82,7 @@ type statusController struct {
 	lastSyncStart time.Time
 
 	sync.Mutex
-	poolPRs map[string]PullRequest
+	poolPRs map[string]prWithStatus
 	blocks  blockers.Blockers
 
 	storedState
@@ -234,8 +234,8 @@ func requirementDiff(pr *PullRequest, q *keeper.Query, cc contextChecker) (strin
 // in order to generate a diff for the status description. We choose the query
 // for the repo that the PR is closest to meeting (as determined by the number
 // of unmet/violated requirements).
-func expectedStatus(queryMap *keeper.QueryMap, pr *PullRequest, pool map[string]PullRequest, cc contextChecker, blocks blockers.Blockers, providerType string) (string, string) {
-	if _, ok := pool[prKey(pr)]; !ok {
+func expectedStatus(queryMap *keeper.QueryMap, pr *PullRequest, pool map[string]prWithStatus, cc contextChecker, blocks blockers.Blockers, providerType string) (string, string) {
+	if _, ok := pool[pr.prKey()]; !ok {
 		// if the branch is blocked forget checking for a diff
 		blockingIssues := blocks.GetApplicable(string(pr.Repository.Owner.Login), string(pr.Repository.Name), string(pr.BaseRef.Name))
 		var numbers []string
@@ -265,7 +265,28 @@ func expectedStatus(queryMap *keeper.QueryMap, pr *PullRequest, pool map[string]
 		}
 		return scmprovider.StatusPending, fmt.Sprintf(statusNotInPool, minDiff)
 	}
-	return scmprovider.StatusSuccess, statusInPool
+	return scmprovider.StatusSuccess, statusForPRInPool(pool[pr.prKey()])
+}
+
+func statusForPRInPool(pr prWithStatus) string {
+	if pr.success {
+		if len(pr.waitingForBatch) > 0 {
+			return fmt.Sprintf("%s, waiting for batch run and merge of PRs %s.", statusInPool, prList(pr.waitingForBatch))
+		}
+		if len(pr.waitingFor) > 0 {
+			return fmt.Sprintf("%s, waiting for merge of PR(s) %s.", statusInPool, prList(pr.waitingFor))
+		}
+	}
+	return statusInPool + "."
+}
+
+func prList(nums []int) string {
+	var withHash []string
+	for _, n := range nums {
+		withHash = append(withHash, fmt.Sprintf("#%d", n))
+	}
+
+	return strings.Join(withHash, ", ")
 }
 
 // targetURL determines the URL used for more details in the status
@@ -290,14 +311,14 @@ func targetURL(c config.Getter, pr *PullRequest, log *logrus.Entry) string {
 	return link
 }
 
-func (sc *statusController) setStatuses(all []PullRequest, pool map[string]PullRequest, blocks blockers.Blockers) {
+func (sc *statusController) setStatuses(all []PullRequest, pool map[string]prWithStatus, blocks blockers.Blockers) {
 	// queryMap caches which queries match a repo.
 	// Make a new one each sync loop as queries will change.
 	queryMap := sc.config().Keeper.Queries.QueryMap()
 	processed := sets.NewString()
 
 	process := func(pr *PullRequest) {
-		processed.Insert(prKey(pr))
+		processed.Insert(pr.prKey())
 		log := sc.logger.WithFields(pr.logFields())
 		contexts, err := headContexts(log, sc.spc, pr)
 		if err != nil {
@@ -364,7 +385,7 @@ func (sc *statusController) setStatuses(all []PullRequest, pool map[string]PullR
 	for key, poolPR := range pool {
 		if !processed.Has(key) {
 			p := poolPR
-			process(&p)
+			process(&p.pr)
 		}
 	}
 }
@@ -421,7 +442,7 @@ func (sc *statusController) waitSync() {
 	}
 }
 
-func (sc *statusController) sync(pool map[string]PullRequest, blocks blockers.Blockers) {
+func (sc *statusController) sync(pool map[string]prWithStatus, blocks blockers.Blockers) {
 	sc.lastSyncStart = time.Now()
 	defer func() {
 		duration := time.Since(sc.lastSyncStart)


Test patch:
diff --git a/pkg/keeper/keeper_test.go b/pkg/keeper/keeper_test.go
index 772d08a1..9fb72b7e 100644
--- a/pkg/keeper/keeper_test.go
+++ b/pkg/keeper/keeper_test.go
@@ -734,7 +734,7 @@ func TestDividePool(t *testing.T) {
 		npr.Repository.Name = githubql.String(p.repo)
 		npr.Repository.Owner.Login = githubql.String(p.org)
 		npr.Repository.URL = githubql.String(fmt.Sprintf("https://github.com/%s/%s.git", p.org, p.repo))
-		pulls[prKey(&npr)] = npr
+		pulls[npr.prKey()] = npr
 	}
 	var pjs []v1alpha1.LighthouseJob
 	for _, pj := range testPJs {
diff --git a/pkg/keeper/status_test.go b/pkg/keeper/status_test.go
index ae21b6e1..14dfe6b9 100644
--- a/pkg/keeper/status_test.go
+++ b/pkg/keeper/status_test.go
@@ -46,6 +46,8 @@ func TestExpectedStatus(t *testing.T) {
 		contexts          []Context
 		inPool            bool
 		blocks            []int
+		pending           []int
+		batchPending      []int
 
 		state string
 		desc  string
@@ -206,82 +208,115 @@ func TestExpectedStatus(t *testing.T) {
 			state: scmprovider.StatusError,
 			desc:  fmt.Sprintf(statusNotInPool, " Merging is blocked by issues 1, 2."),
 		},
+		{
+			name:    "in pool behind pending",
+			inPool:  true,
+			pending: []int{1},
+
+			state: scmprovider.StatusSuccess,
+			desc:  fmt.Sprintf("%s, waiting for merge of PR(s) #1.", statusInPool),
+		},
+		{
+			name:         "in pool behind batch",
+			inPool:       true,
+			batchPending: []int{1, 2, 3},
+
+			state: scmprovider.StatusSuccess,
+			desc:  fmt.Sprintf("%s, waiting for batch run and merge of PRs #1, #2, #3.", statusInPool),
+		},
 	}
 
 	for _, tc := range testcases {
-		t.Logf("Test Case: %q\n", tc.name)
-		secondQuery := keeper.Query{
-			Orgs:      []string{""},
-			Labels:    []string{"1", "2", "3", "4", "5", "6", "7"}, // lots of requirements
-			Milestone: "v1.0",
-		}
-		if tc.sameBranchReqs {
-			secondQuery.ExcludedBranches = tc.branchExcludeList
-			secondQuery.IncludedBranches = tc.branchIncludeList
-		}
-		queriesByRepo := keeper.Queries{
-			keeper.Query{
-				Orgs:             []string{""},
-				ExcludedBranches: tc.branchExcludeList,
-				IncludedBranches: tc.branchIncludeList,
-				Labels:           neededLabels,
-				MissingLabels:    forbiddenLabels,
-				Milestone:        "v1.0",
-			},
-			secondQuery,
-		}.QueryMap()
-		var pr PullRequest
-		pr.BaseRef = struct {
-			Name   githubql.String
-			Prefix githubql.String
-		}{
-			Name: githubql.String(tc.baseref),
-		}
-		for _, label := range tc.labels {
-			pr.Labels.Nodes = append(
-				pr.Labels.Nodes,
-				struct{ Name githubql.String }{Name: githubql.String(label)},
-			)
-		}
-		if len(tc.contexts) > 0 {
-			pr.HeadRefOID = githubql.String("head")
-			pr.Commits.Nodes = append(
-				pr.Commits.Nodes,
-				struct{ Commit Commit }{
-					Commit: Commit{
-						Status: struct{ Contexts []Context }{
-							Contexts: tc.contexts,
+		t.Run(tc.name, func(t *testing.T) {
+			secondQuery := keeper.Query{
+				Orgs:      []string{""},
+				Labels:    []string{"1", "2", "3", "4", "5", "6", "7"}, // lots of requirements
+				Milestone: "v1.0",
+			}
+			if tc.sameBranchReqs {
+				secondQuery.ExcludedBranches = tc.branchExcludeList
+				secondQuery.IncludedBranches = tc.branchIncludeList
+			}
+			queriesByRepo := keeper.Queries{
+				keeper.Query{
+					Orgs:             []string{""},
+					ExcludedBranches: tc.branchExcludeList,
+					IncludedBranches: tc.branchIncludeList,
+					Labels:           neededLabels,
+					MissingLabels:    forbiddenLabels,
+					Milestone:        "v1.0",
+				},
+				secondQuery,
+			}.QueryMap()
+			var pr PullRequest
+			pr.BaseRef = struct {
+				Name   githubql.String
+				Prefix githubql.String
+			}{
+				Name: githubql.String(tc.baseref),
+			}
+			for _, label := range tc.labels {
+				pr.Labels.Nodes = append(
+					pr.Labels.Nodes,
+					struct{ Name githubql.String }{Name: githubql.String(label)},
+				)
+			}
+			if len(tc.contexts) > 0 {
+				pr.HeadRefOID = githubql.String("head")
+				pr.Commits.Nodes = append(
+					pr.Commits.Nodes,
+					struct{ Commit Commit }{
+						Commit: Commit{
+							Status: struct{ Contexts []Context }{
+								Contexts: tc.contexts,
+							},
+							OID: githubql.String("head"),
 						},
-						OID: githubql.String("head"),
 					},
-				},
-			)
-		}
-		if tc.milestone != "" {
-			pr.Milestone = &struct {
-				Title githubql.String
-			}{githubql.String(tc.milestone)}
-		}
-		var pool map[string]PullRequest
-		if tc.inPool {
-			pool = map[string]PullRequest{"#0": {}}
-		}
-		blocks := blockers.Blockers{
-			Repo: map[blockers.OrgRepo][]blockers.Blocker{},
-		}
-		var items []blockers.Blocker
-		for _, block := range tc.blocks {
-			items = append(items, blockers.Blocker{Number: block})
-		}
-		blocks.Repo[blockers.OrgRepo{Org: "", Repo: ""}] = items
+				)
+			}
+			if tc.milestone != "" {
+				pr.Milestone = &struct {
+					Title githubql.String
+				}{githubql.String(tc.milestone)}
+			}
+			var pool map[string]prWithStatus
+			if tc.inPool {
+				withStatus := prWithStatus{
+					pr:      pr,
+					success: false,
+				}
+				if len(tc.pending) > 0 {
+					withStatus.waitingFor = append(withStatus.waitingFor, tc.pending...)
+					withStatus.success = true
+				}
+				if len(tc.batchPending) > 0 {
+					withStatus.waitingForBatch = append(withStatus.waitingForBatch, tc.batchPending...)
+					withStatus.success = true
+				}
+				pool = map[string]prWithStatus{"#0": withStatus}
+			}
+			blocks := blockers.Blockers{
+				Repo: map[blockers.OrgRepo][]blockers.Blocker{},
+			}
+			var items []blockers.Blocker
+			for _, block := range tc.blocks {
+				items = append(items, blockers.Blocker{Number: block})
+			}
+			blocks.Repo[blockers.OrgRepo{Org: "", Repo: ""}] = items
 
-		state, desc := expectedStatus(queriesByRepo, &pr, pool, &keeper.ContextPolicy{}, blocks, "fake")
-		if state != tc.state {
-			t.Errorf("Expected status state %q, but got %q.", string(tc.state), string(state))
-		}
-		if desc != tc.desc {
-			t.Errorf("Expected status description %q, but got %q.", tc.desc, desc)
-		}
+			state, desc := expectedStatus(queriesByRepo, &pr, pool, &keeper.ContextPolicy{}, blocks, "fake")
+			if state != tc.state {
+				t.Errorf("Expected status state %q, but got %q.", string(tc.state), string(state))
+			}
+			expectedDesc := tc.desc
+			if expectedDesc == statusInPool {
+				expectedDesc = statusInPool + "."
+			}
+			if desc != expectedDesc {
+				t.Errorf("Expected status description %q, but got %q.", expectedDesc, desc)
+			}
+		})
 	}
 }
 
@@ -303,7 +338,7 @@ func TestSetStatuses(t *testing.T) {
 			inPool:     true,
 			hasContext: true,
 			state:      githubql.StatusStateSuccess,
-			desc:       statusInPool,
+			desc:       statusInPool + ".",
 
 			shouldSet: false,
 		},
@@ -365,44 +400,49 @@ func TestSetStatuses(t *testing.T) {
 		},
 	}
 	for _, tc := range testcases {
-		var pr PullRequest
-		pr.Commits.Nodes = []struct{ Commit Commit }{{}}
-		if tc.hasContext {
-			pr.Commits.Nodes[0].Commit.Status.Contexts = []Context{
-				{
-					Context:     githubql.String(GetStatusContextLabel()),
-					State:       tc.state,
-					Description: githubql.String(tc.desc),
-				},
+		t.Run(tc.name, func(t *testing.T) {
+			var pr PullRequest
+			pr.Commits.Nodes = []struct{ Commit Commit }{{}}
+			if tc.hasContext {
+				pr.Commits.Nodes[0].Commit.Status.Contexts = []Context{
+					{
+						Context:     githubql.String(GetStatusContextLabel()),
+						State:       tc.state,
+						Description: githubql.String(tc.desc),
+					},
+				}
+			}
+			pool := make(map[string]prWithStatus)
+			if tc.inPool {
+				pool[pr.prKey()] = prWithStatus{
+					pr:      pr,
+					success: false,
+				}
+			}
+			fc := &fgc{}
+			ca := &config.Agent{}
+			ca.Set(&config.Config{})
+			// setStatuses logs instead of returning errors.
+			// Construct a logger to watch for errors to be printed.
+			log := logrus.WithField("component", "keeper")
+			initialLog, err := log.String()
+			if err != nil {
+				t.Fatalf("Failed to get log output before testing: %v", err)
 			}
-		}
-		pool := make(map[string]PullRequest)
-		if tc.inPool {
-			pool[prKey(&pr)] = pr
-		}
-		fc := &fgc{}
-		ca := &config.Agent{}
-		ca.Set(&config.Config{})
-		// setStatuses logs instead of returning errors.
-		// Construct a logger to watch for errors to be printed.
-		log := logrus.WithField("component", "keeper")
-		initialLog, err := log.String()
-		if err != nil {
-			t.Fatalf("Failed to get log output before testing: %v", err)
-		}
 
-		sc := &statusController{spc: fc, config: ca.Config, logger: log}
-		sc.setStatuses([]PullRequest{pr}, pool, blockers.Blockers{})
-		if str, err := log.String(); err != nil {
-			t.Fatalf("For case %s: failed to get log output: %v", tc.name, err)
-		} else if str != initialLog {
-			t.Errorf("For case %s: error setting status: %s", tc.name, str)
-		}
-		if tc.shouldSet && !fc.setStatus {
-			t.Errorf("For case %s: should set but didn't", tc.name)
-		} else if !tc.shouldSet && fc.setStatus {
-			t.Errorf("For case %s: should not set but did", tc.name)
-		}
+			sc := &statusController{spc: fc, config: ca.Config, logger: log}
+			sc.setStatuses([]PullRequest{pr}, pool, blockers.Blockers{})
+			if str, err := log.String(); err != nil {
+				t.Fatalf("For case %s: failed to get log output: %v", tc.name, err)
+			} else if str != initialLog {
+				t.Errorf("For case %s: error setting status: %s", tc.name, str)
+			}
+			if tc.shouldSet && !fc.setStatus {
+				t.Errorf("For case %s: should set but didn't", tc.name)
+			} else if !tc.shouldSet && fc.setStatus {
+				t.Errorf("For case %s: should not set but did", tc.name)
+			}
+		})
 	}
 }
 
@@ -449,12 +489,14 @@ func TestTargetUrl(t *testing.T) {
 	}
 
 	for _, tc := range testcases {
-		ca := &config.Agent{}
-		ca.Set(&config.Config{ProwConfig: config.ProwConfig{Keeper: tc.config}})
-		log := logrus.WithField("controller", "status-update")
-		if actual, expected := targetURL(ca.Config, tc.pr, log), tc.expectedURL; actual != expected {
-			t.Errorf("%s: expected target URL %s but got %s", tc.name, expected, actual)
-		}
+		t.Run(tc.name, func(t *testing.T) {
+			ca := &config.Agent{}
+			ca.Set(&config.Config{ProwConfig: config.ProwConfig{Keeper: tc.config}})
+			log := logrus.WithField("controller", "status-update")
+			if actual, expected := targetURL(ca.Config, tc.pr, log), tc.expectedURL; actual != expected {
+				t.Errorf("%s: expected target URL %s but got %s", tc.name, expected, actual)
+			}
+		})
 	}
 }