From 18a2a8c1c2bbc9fbe3650b10df24ffd7d166d249 Mon Sep 17 00:00:00 2001 From: Skisocks Date: Tue, 31 Oct 2023 14:21:35 +0000 Subject: [PATCH 1/3] fix: get baseRefSHA from graphql instead of rest --- pkg/keeper/keeper.go | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/pkg/keeper/keeper.go b/pkg/keeper/keeper.go index e1526da7e..944b6897a 100644 --- a/pkg/keeper/keeper.go +++ b/pkg/keeper/keeper.go @@ -620,13 +620,13 @@ func filterSubpool(spc scmProviderClient, sp *subpool) *subpool { // filterPR indicates if a PR should be filtered out of the subpool. // Specifically we filter out PRs that: -// - Have known merge conflicts. -// - Have failing or missing status contexts. -// - Have pending required status contexts that are not associated with a -// PipelineActivity. (This ensures that the 'keeper' context indicates that the pending -// status is preventing merge. Required PipelineActivity statuses are allowed to be -// 'pending' because this prevents kicking PRs from the pool when Keeper is -// retesting them.) +// - Have known merge conflicts. +// - Have failing or missing status contexts. +// - Have pending required status contexts that are not associated with a +// PipelineActivity. (This ensures that the 'keeper' context indicates that the pending +// status is preventing merge. Required PipelineActivity statuses are allowed to be +// 'pending' because this prevents kicking PRs from the pool when Keeper is +// retesting them.) func filterPR(spc scmProviderClient, sp *subpool, pr *PullRequest) bool { log := sp.log.WithFields(pr.logFields()) // Skip PRs that are known to be unmergeable. @@ -1539,8 +1539,8 @@ func (c *DefaultController) dividePool(pool map[string]PullRequest, pjs []v1alph org := string(pr.Repository.Owner.Login) repo := string(pr.Repository.Name) branch := string(pr.BaseRef.Name) - branchRef := string(pr.BaseRef.Prefix) + string(pr.BaseRef.Name) cloneURL := string(pr.Repository.URL) + baseSHA := string(pr.BaseRefOID) if cloneURL == "" { return nil, errors.New("no clone URL specified for repository") } @@ -1549,22 +1549,18 @@ func (c *DefaultController) dividePool(pool map[string]PullRequest, pjs []v1alph } fn := poolKey(org, repo, branch) if sps[fn] == nil { - sha, err := c.spc.GetRef(org, repo, strings.TrimPrefix(branchRef, "refs/")) - if err != nil { - return nil, err - } sps[fn] = &subpool{ log: c.logger.WithFields(logrus.Fields{ "org": org, "repo": repo, "branch": branch, - "base-sha": sha, + "base-sha": baseSHA, "clone-url": cloneURL, }), org: org, repo: repo, branch: branch, - sha: sha, + sha: baseSHA, cloneURL: cloneURL, } } @@ -1601,6 +1597,7 @@ type PullRequest struct { BaseRef GraphQLBaseRef HeadRefName githubql.String `graphql:"headRefName"` HeadRefOID githubql.String `graphql:"headRefOid"` + BaseRefOID githubql.String Mergeable githubql.MergeableState Repository Repository Commits struct { From be5f6331341b2260c22064ff45cbbaf681804a5a Mon Sep 17 00:00:00 2001 From: Skisocks Date: Tue, 31 Oct 2023 14:26:31 +0000 Subject: [PATCH 2/3] test: remove baseSHA test as now returned by graphql --- pkg/keeper/keeper_test.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/pkg/keeper/keeper_test.go b/pkg/keeper/keeper_test.go index d08e46d6b..ea9087f17 100644 --- a/pkg/keeper/keeper_test.go +++ b/pkg/keeper/keeper_test.go @@ -754,18 +754,17 @@ func TestDividePool(t *testing.T) { baseSHA: "123", }, } - fc := &fgc{ - refs: map[string]string{"k/t-i heads/master": "123"}, - } + fc := &fgc{} c := &DefaultController{ spc: fc, logger: logrus.WithField("component", "keeper"), } pulls := make(map[string]PullRequest) - for _, p := range testPulls { + for idx, p := range testPulls { npr := PullRequest{Number: githubql.Int(p.number)} npr.BaseRef.Name = githubql.String(p.branch) npr.BaseRef.Prefix = "refs/heads/" + npr.BaseRefOID = githubql.String(testPJs[idx].baseSHA) 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)) @@ -794,10 +793,6 @@ func TestDividePool(t *testing.T) { } for _, sp := range sps { name := fmt.Sprintf("%s/%s %s", sp.org, sp.repo, sp.branch) - sha := fc.refs[sp.org+"/"+sp.repo+" heads/"+sp.branch] - if sp.sha != sha { - t.Errorf("For subpool %s, got sha %s, expected %s.", name, sp.sha, sha) - } if len(sp.prs) == 0 { t.Errorf("Subpool %s has no PRs.", name) } From d6221c3848d313b4851bd94772510bf6ef0fd966 Mon Sep 17 00:00:00 2001 From: Skisocks Date: Tue, 31 Oct 2023 14:49:49 +0000 Subject: [PATCH 3/3] fix: add baseRefOID to scmPR -> graphql conversion --- pkg/keeper/keeper.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/keeper/keeper.go b/pkg/keeper/keeper.go index 944b6897a..afd026da1 100644 --- a/pkg/keeper/keeper.go +++ b/pkg/keeper/keeper.go @@ -1918,6 +1918,7 @@ func scmPRToGraphQLPR(scmPR *scm.PullRequest, scmRepo *scm.Repository) *PullRequ BaseRef: baseRef, HeadRefName: githubql.String(scmPR.Source), HeadRefOID: githubql.String(scmPR.Head.Sha), + BaseRefOID: githubql.String(scmPR.Base.Sha), Mergeable: mergeable, Repository: scmRepoToGraphQLRepo(scmRepo), Labels: labels,