Skip to content

Commit

Permalink
Explicitly meter calls to Github API. Remove retrier.
Browse files Browse the repository at this point in the history 
We do this because the Github API triggers abuse warnings if a single
user makes lots of concurrent requests. By having a shared ticker, we
ensure that Github API requests don't happen in parallel.

Moreover, we also ensure that there's a global rate limit of 1 request
to the GithubAPI per 720 milliseconds. We're following the approach
shown here: google/go-github#431

This means that we'll stay within the rate limit requirement
of 5000 requests-per-hour for authenticated users.

```
5000 QPH = 83.3 QPM = 1.38 QPS = 720ms/query
```
  • Loading branch information
@nathanleiby
nathanleiby committed Dec 8, 2017
1 parent f022324 commit bacb88c
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 75 deletions.
2 changes: 1 addition & 1 deletion cmd/merge.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func mergeOneRepo(r initialize.Repo, ctx context.Context) error {
PRNumber: prNumber,
CommitSHA: pushOutput.CommitSHA,
}
output, err := merge.Merge(ctx, input)
output, err := merge.Merge(ctx, input, githubLimiter)
if err != nil {
o := struct {
merge.Output
Expand Down
2 changes: 1 addition & 1 deletion cmd/push.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func pushOneRepo(r initialize.Repo, ctx context.Context) error {
BranchName: planOutput.BranchName,
RepoOwner: r.Owner,
}
output, err := push.Push(ctx, input)
output, err := push.Push(ctx, input, githubLimiter)
if err != nil {
o := struct {
push.Output
Expand Down
5 changes: 5 additions & 0 deletions cmd/root.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"os"
"path"
"path/filepath"
"time"

"github.com/Clever/microplane/initialize"
"github.com/spf13/cobra"
Expand All @@ -14,6 +15,10 @@ import (
var workDir string
var cliVersion string

// Github's rate limit for authenticated requests is 5000 QPH = 83.3 QPM = 1.38 QPS = 720ms/query
// We also use a global limiter to prevent concurrent requests, which trigger Github's abuse detection
var githubLimiter = time.NewTicker(720 * time.Millisecond)

var rootCmd = &cobra.Command{
Use: "mp",
Short: "Microplane makes git changes across many repos",
Expand Down
50 changes: 11 additions & 39 deletions merge/merge.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,10 @@ import (
"os"
"time"

"github.com/eapache/go-resiliency/retrier"
"github.com/google/go-github/github"
"golang.org/x/oauth2"
)

// Command represents a command to run.
type Command struct {
Path string
Args []string
}

// Input to Push()
type Input struct {
// Org on Github, e.g. "Clever"
Expand All @@ -42,30 +35,19 @@ type Error struct {
}

// Merge an open PR in Github
func Merge(ctx context.Context, input Input) (Output, error) {
func Merge(ctx context.Context, input Input, githubLimiter *time.Ticker) (Output, error) {
// Create Github Client
ts := oauth2.StaticTokenSource(
&oauth2.Token{AccessToken: os.Getenv("GITHUB_API_TOKEN")},
)
tc := oauth2.NewClient(ctx, ts)

client := github.NewClient(tc)

// Configure a retrier - exponential backoff upon hitting Github API rate limit errors
r := retrier.New(retrier.ExponentialBackoff(10, time.Second), retrier.WhitelistClassifier{
&github.RateLimitError{},
&github.AbuseRateLimitError{},
})
r.SetJitter(0.5)

// OK to merge?

// (1) Check if the PR is mergeable
var pr *github.PullRequest
err := r.Run(func() error {
var prErr error
pr, _, prErr = client.PullRequests.Get(ctx, input.Org, input.Repo, input.PRNumber)
return prErr
})
<-githubLimiter.C
pr, _, err := client.PullRequests.Get(ctx, input.Org, input.Repo, input.PRNumber)
if err != nil {
return Output{Success: false}, err
}
Expand All @@ -75,13 +57,9 @@ func Merge(ctx context.Context, input Input) (Output, error) {
}

// (2) Check commit status
var status *github.CombinedStatus
err = r.Run(func() error {
var csErr error
opt := &github.ListOptions{}
status, _, csErr = client.Repositories.GetCombinedStatus(ctx, input.Org, input.Repo, input.CommitSHA, opt)
return csErr
})
opt := &github.ListOptions{}
<-githubLimiter.C
status, _, err := client.Repositories.GetCombinedStatus(ctx, input.Org, input.Repo, input.CommitSHA, opt)
if err != nil {
return Output{Success: false}, err
}
Expand All @@ -97,12 +75,8 @@ func Merge(ctx context.Context, input Input) (Output, error) {
// Merge the PR
options := &github.PullRequestOptions{}
commitMsg := ""
var result *github.PullRequestMergeResult
err = r.Run(func() error {
var mergeErr error
result, _, mergeErr = client.PullRequests.Merge(ctx, input.Org, input.Repo, input.PRNumber, commitMsg, options)
return mergeErr
})
<-githubLimiter.C
result, _, err := client.PullRequests.Merge(ctx, input.Org, input.Repo, input.PRNumber, commitMsg, options)
if err != nil {
return Output{Success: false}, err
}
Expand All @@ -112,10 +86,8 @@ func Merge(ctx context.Context, input Input) (Output, error) {
}

// Delete the branch
err = r.Run(func() error {
_, deleteErr := client.Git.DeleteRef(ctx, input.Org, input.Repo, "heads/"+*pr.Head.Ref)
return deleteErr
})
<-githubLimiter.C
_, err = client.Git.DeleteRef(ctx, input.Org, input.Repo, "heads/"+*pr.Head.Ref)
if err != nil {
return Output{Success: false}, err
}
Expand Down
52 changes: 18 additions & 34 deletions push/push.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import (
"strings"
"time"

"github.com/eapache/go-resiliency/retrier"
"github.com/google/go-github/github"
"golang.org/x/oauth2"

"github.com/google/go-github/github"
)

// Command represents a command to run.
Expand Down Expand Up @@ -71,7 +71,7 @@ func (o Output) String() string {
}

// Push pushes the commit to Github and opens a pull request
func Push(ctx context.Context, input Input) (Output, error) {
func Push(ctx context.Context, input Input, githubLimiter *time.Ticker) (Output, error) {
// Get the commit SHA from the last commit
cmd := Command{Path: "git", Args: []string{"log", "-1", "--pretty=format:%H"}}
gitLog := exec.CommandContext(ctx, cmd.Path, cmd.Args...)
Expand All @@ -90,54 +90,36 @@ func Push(ctx context.Context, input Input) (Output, error) {
return Output{Success: false}, errors.New(string(output))
}

// Open a pull request, if one doesn't exist already
// Create Github Client
ts := oauth2.StaticTokenSource(
&oauth2.Token{AccessToken: os.Getenv("GITHUB_API_TOKEN")},
)
tc := oauth2.NewClient(ctx, ts)
client := github.NewClient(tc)

// Open a pull request, if one doesn't exist already
head := fmt.Sprintf("%s:%s", input.RepoOwner, input.BranchName)
base := "master"

// Configure a retrier - exponential backoff upon hitting Github API rate limit errors
r := retrier.New(retrier.ExponentialBackoff(10, time.Second), retrier.WhitelistClassifier{
&github.RateLimitError{},
&github.AbuseRateLimitError{},
})
r.SetJitter(0.5)

var pr *github.PullRequest
err = r.Run(func() error {
var prErr error
pr, prErr = findOrCreatePR(ctx, client, input.RepoOwner, input.RepoName, &github.NewPullRequest{
Title: &input.PRMessage,
Head: &head,
Base: &base,
})
return prErr
})
pr, err := findOrCreatePR(ctx, client, input.RepoOwner, input.RepoName, &github.NewPullRequest{
Title: &input.PRMessage,
Head: &head,
Base: &base,
}, githubLimiter)
if err != nil {
return Output{Success: false}, err
}

if pr.Assignee == nil || pr.Assignee.Login == nil || *pr.Assignee.Login != input.PRAssignee {
err = r.Run(func() error {
var addErr error
_, _, addErr = client.Issues.AddAssignees(ctx, input.RepoOwner, input.RepoName,
*pr.Number, []string{input.PRAssignee})
return addErr
})
<-githubLimiter.C
_, _, err := client.Issues.AddAssignees(ctx, input.RepoOwner, input.RepoName, *pr.Number, []string{input.PRAssignee})
if err != nil {
return Output{Success: false}, err
}
}

var cs *github.CombinedStatus
err = r.Run(func() error {
var getErr error
cs, _, getErr = client.Repositories.GetCombinedStatus(ctx, input.RepoOwner, input.RepoName, *pr.Head.SHA, nil)
return getErr
})
<-githubLimiter.C
cs, _, err := client.Repositories.GetCombinedStatus(ctx, input.RepoOwner, input.RepoName, *pr.Head.SHA, nil)
if err != nil {
return Output{Success: false}, err
}
Expand Down Expand Up @@ -171,10 +153,12 @@ func Push(ctx context.Context, input Input) (Output, error) {
}, nil
}

func findOrCreatePR(ctx context.Context, client *github.Client, owner string, name string, pull *github.NewPullRequest) (*github.PullRequest, error) {
func findOrCreatePR(ctx context.Context, client *github.Client, owner string, name string, pull *github.NewPullRequest, githubLimiter *time.Ticker) (*github.PullRequest, error) {
var pr *github.PullRequest
<-githubLimiter.C
newPR, _, err := client.PullRequests.Create(ctx, owner, name, pull)
if err != nil && strings.Contains(err.Error(), "pull request already exists") {
<-githubLimiter.C
existingPRs, _, err := client.PullRequests.List(ctx, owner, name, &github.PullRequestListOptions{
Head: *pull.Head,
Base: *pull.Base,
Expand Down

0 comments on commit bacb88c

Please # to comment.