Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: get the baseRefSha from graphql to reduce rate consumption #1580

Merged
merged 4 commits into from
Oct 31, 2023
Merged

fix: get the baseRefSha from graphql to reduce rate consumption #1580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
18a2a8c
fix: get baseRefSHA from graphql instead of rest
Skisocks Oct 31, 2023
be5f633
test: remove baseSHA test as now returned by graphql
Skisocks Oct 31, 2023
d6221c3
fix: add baseRefOID to scmPR -> graphql conversion
Skisocks Oct 31, 2023
13fc7da
Merge branch 'main' into reduce-rate-consumption
Skisocks Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 12 additions & 14 deletions pkg/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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")
}
Expand All @@ -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,
}
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -1921,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,
Expand Down
11 changes: 3 additions & 8 deletions pkg/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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)
}
Expand Down