Skip to content

Commit

Permalink
avoid multiple "GetAssignableUsers" calls when pushing a new stack
Browse files Browse the repository at this point in the history
As described in #199, we were calling GetAssignableUsers (a graphql
API query) for each new PR created. This is a property of the
repository, though, and we don't expect it to be changing while we're
pushing a single stack.

Stash the slice of RepoAssignee the first time it's needed and
continue to use it for the rest of the calls. Added a test case to
verify the behaviour.

commit-id:dd9d9f40
  • Loading branch information
wwade authored and Eitan Joffe committed Mar 15, 2022
1 parent 16d206b commit 15b7c94
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 8 deletions.
10 changes: 7 additions & 3 deletions spr/spr.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ func (sd *stackediff) AmendCommit(ctx context.Context) {
sd.mustgit("rebase -i --autosquash --autostash", nil)
}

func (sd *stackediff) addReviewers(ctx context.Context, pr *github.PullRequest, reviewers []string) {
assignable := sd.github.GetAssignableUsers(ctx)
func (sd *stackediff) addReviewers(ctx context.Context,
pr *github.PullRequest, reviewers []string, assignable []github.RepoAssignee) {
userIDs := make([]string, 0, len(reviewers))
for _, r := range reviewers {
found := false
Expand Down Expand Up @@ -151,6 +151,7 @@ func (sd *stackediff) UpdatePullRequests(ctx context.Context, reviewers []string
}

updateQueue := make([]prUpdate, 0)
var assignable []github.RepoAssignee

// iterate through local_commits and update pull_requests
for commitIndex, c := range localCommits {
Expand Down Expand Up @@ -184,7 +185,10 @@ func (sd *stackediff) UpdatePullRequests(ctx context.Context, reviewers []string
githubInfo.PullRequests = append(githubInfo.PullRequests, pr)
updateQueue = append(updateQueue, prUpdate{pr, c, prevCommit})
if len(reviewers) != 0 {
sd.addReviewers(ctx, pr, reviewers)
if assignable == nil {
assignable = sd.github.GetAssignableUsers(ctx)
}
sd.addReviewers(ctx, pr, reviewers, assignable)
}
}
}
Expand Down
22 changes: 17 additions & 5 deletions spr/spr_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,20 +112,32 @@ func TestSPRBasicFlowFourCommits(t *testing.T) {
gitmock.ExpectFetch()
gitmock.ExpectLogAndRespond([]*git.Commit{&c4, &c3, &c2, &c1})
gitmock.ExpectPushCommits([]*git.Commit{&c3, &c4})

// For the first "create" call we should call GetAssignableUsers
githubmock.ExpectCreatePullRequest(c3, &c2)
githubmock.ExpectGetAssignableUsers()
githubmock.ExpectAddReviewers([]string{mockclient.NobodyUserID})

// For the first "create" call we should *not* call GetAssignableUsers
githubmock.ExpectCreatePullRequest(c4, &c3)
githubmock.ExpectAddReviewers([]string{mockclient.NobodyUserID})

githubmock.ExpectUpdatePullRequest(c1, nil)
githubmock.ExpectUpdatePullRequest(c2, &c1)
githubmock.ExpectUpdatePullRequest(c3, &c2)
githubmock.ExpectUpdatePullRequest(c4, &c3)
githubmock.ExpectGetInfo()
s.UpdatePullRequests(ctx, nil)
s.UpdatePullRequests(ctx, []string{mockclient.NobodyLogin})
lines = strings.Split(output.String(), "\n")
fmt.Printf("OUT: %s\n", output.String())
assert.Equal("[✔✔✔✔] 1 : test commit 4", lines[0])
assert.Equal("[✔✔✔✔] 1 : test commit 3", lines[1])
assert.Equal("[✔✔✔✔] 1 : test commit 2", lines[2])
assert.Equal("[✔✔✔✔] 1 : test commit 1", lines[3])
assert.Equal([]string{
"warning: not updating reviewers for PR #1",
"warning: not updating reviewers for PR #1",
"[✔✔✔✔] 1 : test commit 4",
"[✔✔✔✔] 1 : test commit 3",
"[✔✔✔✔] 1 : test commit 2",
"[✔✔✔✔] 1 : test commit 1",
}, lines[:6])
output.Reset()

// 'git spr -m' :: MergePullRequest :: commits=[a1, a2, a3, a4]
Expand Down

0 comments on commit 15b7c94

Please sign in to comment.