Skip to content

Commit

Permalink
fix: add a rate limit to write operations
Browse files Browse the repository at this point in the history 
We have noticed while performing large amounts of merges during a very
short amount of time, that the Github API very quickly rate limits us,
leading to unmerged Pull Requests that amass over time.

We haven't been able to discover exactly which operations cause the rate
limiter to hit, but this Pull Request intends to improve on the
functionality.

Furthermore, we are proposing that we prevent the first quit while there
are operations in progress. The second quit will always go through. A
warning message is written if a quit is attempted while there are
ongoing operations.
  • Loading branch information
@quoral
quoral committed Mar 11, 2024
1 parent 1b74e65 commit 27754f5
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 16 deletions.
1 change: 1 addition & 0 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ require (
github.com/charmbracelet/lipgloss v0.10.0
github.com/shurcooL/githubv4 v0.0.0-20240120211514-18a1ae0e79dc
github.com/spf13/cobra v1.8.0
golang.org/x/time v0.0.0-20191024005414-555d28b269f0
)

require (
Expand Down
1 change: 1 addition & 0 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ golang.org/x/text v0.13.0 h1:ablQoSUd0tRdKxZewP80B+BaqeKJuVhuRxj/dkrun3k=
golang.org/x/text v0.13.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE=
golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
golang.org/x/time v0.0.0-20191024005414-555d28b269f0 h1:/5xXl8Y5W96D+TtHSlonuFqGHIWVuyCkGJLwGh9JJFs=
golang.org/x/time v0.0.0-20191024005414-555d28b269f0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20190114222345-bf090417da8b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
Expand Down
9 changes: 8 additions & 1 deletion tui.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"github.com/shurcooL/githubv4"
)

const operationInProgressText = "Preventing quit due to in progress operations. To quit, repeat quit command."

var _ tea.Model = App{}

func newApp(_ *githubv4.Client, query pullRequestQuery, pullRequests []pullRequest) App {
Expand All @@ -21,7 +23,8 @@ type App struct {
listView ListView
detailsView DetailsView

isShowingDetails bool
isShowingDetails bool
preventedQuitOnce bool
}

func (a App) Init() tea.Cmd {
Expand All @@ -41,6 +44,10 @@ func (a App) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
switch msg := msg.(type) {
case tea.KeyMsg:
if msg.String() == "ctrl+c" || msg.String() == "q" {
if a.listView.hasWorkInProgress() && !a.preventedQuitOnce {
a.preventedQuitOnce = true
return a, a.listView.listModel.NewStatusMessage(operationInProgressText)
}
return a, tea.Quit
}

Expand Down
30 changes: 26 additions & 4 deletions tui_cmds.go
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package main

import (
"context"
"fmt"
"time"

"github.com/atotto/clipboard"
tea "github.com/charmbracelet/bubbletea"
"github.com/einride/gh-dependabot/internal/gh"
"golang.org/x/time/rate"
)

type errorMessage struct {
Expand All @@ -25,21 +28,36 @@ const (
MethodDependabot mergeMethod = "@dependabot merge"
)

func mergePullRequest(pr pullRequest, method mergeMethod) tea.Cmd {
type commander struct {
limiter *rate.Limiter
}

func newCommander() commander {
limiter := rate.NewLimiter(rate.Every(time.Second*2), 1)
return commander{
limiter: limiter,
}
}

func (c commander) mergePullRequest(pr pullRequest, method mergeMethod) tea.Cmd {
return func() tea.Msg {
switch method {
case MethodRebase:
fallthrough
case MethodMerge:
fallthrough
case MethodSquash:
//nolint:errcheck // hard coded limiter burst, can't fail
c.limiter.Wait(context.Background())
if _, err := gh.Run("pr", "review", "--approve", pr.url); err != nil {
return errorMessage{err: err}
}
if _, err := gh.Run("pr", "merge", "--auto", string(method), pr.url); err != nil {
return errorMessage{err: err}
}
case MethodDependabot:
//nolint:errcheck // hard coded limiter burst, can't fail
c.limiter.Wait(context.Background())
if _, err := gh.Run("pr", "review", "--approve", "--body", string(method), pr.url); err != nil {
return errorMessage{err: err}
}
Expand All @@ -54,8 +72,10 @@ type pullRequestRebased struct {
pr pullRequest
}

func rebasePullRequest(pr pullRequest) tea.Cmd {
func (c commander) rebasePullRequest(pr pullRequest) tea.Cmd {
return func() tea.Msg {
//nolint:errcheck // hard coded limiter burst, can't fail
c.limiter.Wait(context.Background())
if _, err := gh.Run("pr", "comment", "--body", "@dependabot rebase", pr.url); err != nil {
return errorMessage{err: err}
}
Expand All @@ -67,8 +87,10 @@ type pullRequestRecreated struct {
pr pullRequest
}

func recreatePullRequest(pr pullRequest) tea.Cmd {
func (c commander) recreatePullRequest(pr pullRequest) tea.Cmd {
return func() tea.Msg {
//nolint:errcheck // hard coded limiter burst, can't fail
c.limiter.Wait(context.Background())
if _, err := gh.Run("pr", "comment", "--body", "@dependabot recreate", pr.url); err != nil {
return errorMessage{err: err}
}
Expand All @@ -80,7 +102,7 @@ type pullRequestOpenedInBrowser struct {
pr pullRequest
}

func openInBrowser(pr pullRequest) tea.Cmd {
func (c commander) openInBrowser(pr pullRequest) tea.Cmd {
return func() tea.Msg {
if _, err := gh.Run("pr", "view", "--web", pr.url); err != nil {
return errorMessage{err: err}
Expand Down
67 changes: 56 additions & 11 deletions tui_list.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package main

import (
"fmt"
"sync"

"github.com/charmbracelet/bubbles/key"
"github.com/charmbracelet/bubbles/list"
Expand Down Expand Up @@ -81,8 +82,11 @@ func (d keyMap) Bindings() []key.Binding {
}

type ListView struct {
listModel list.Model
keyMap *keyMap
listModel list.Model
keyMap *keyMap
commander commander
inProgressOperations map[string]int
mapMutex *sync.Mutex
}

func newListView(query pullRequestQuery, pullRequests []pullRequest) ListView {
Expand All @@ -93,15 +97,47 @@ func newListView(query pullRequestQuery, pullRequests []pullRequest) ListView {
listModel.AdditionalFullHelpKeys = keyMap.Bindings
listModel.AdditionalShortHelpKeys = listModel.AdditionalFullHelpKeys
return ListView{
listModel: listModel,
keyMap: keyMap,
listModel: listModel,
keyMap: keyMap,
commander: newCommander(),
inProgressOperations: make(map[string]int),
mapMutex: &sync.Mutex{},
}
}

func (m ListView) Init() tea.Cmd {
return nil
}

func (m ListView) hasWorkInProgress() bool {
m.mapMutex.Lock()
defer m.mapMutex.Unlock()
return len(m.inProgressOperations) > 0
}

func (m ListView) markInProgress(repository, number string) {
m.mapMutex.Lock()
defer m.mapMutex.Unlock()
repoString := fmt.Sprintf("%s/%s", repository, number)
if _, ok := m.inProgressOperations[repoString]; !ok {
m.inProgressOperations[repoString] = 0
}
m.inProgressOperations[repoString]++
}

func (m ListView) markDone(repository, number string) {
repoString := fmt.Sprintf("%s/%s", repository, number)
m.mapMutex.Lock()
defer m.mapMutex.Unlock()
if _, ok := m.inProgressOperations[repoString]; !ok {
return
}
m.inProgressOperations[repoString]--
if m.inProgressOperations[repoString] <= 0 {
delete(m.inProgressOperations, repoString)
}
}

func (m ListView) Update(msg tea.Msg) (ListView, tea.Cmd) {
var cmds []tea.Cmd
switch msg := msg.(type) {
Expand All @@ -110,12 +146,15 @@ func (m ListView) Update(msg tea.Msg) (ListView, tea.Cmd) {
cmds = append(cmds, m.listModel.NewStatusMessage(msg.err.Error()))
case pullRequestMerged:
m.listModel.StopSpinner()
m.markDone(msg.pr.repository, msg.pr.number)
cmds = append(cmds, m.listModel.NewStatusMessage("Approved "+msg.pr.url))
case pullRequestRebased:
m.listModel.StopSpinner()
m.markDone(msg.pr.repository, msg.pr.number)
cmds = append(cmds, m.listModel.NewStatusMessage("Rebased "+msg.pr.url))
case pullRequestRecreated:
m.listModel.StopSpinner()
m.markDone(msg.pr.repository, msg.pr.number)
cmds = append(cmds, m.listModel.NewStatusMessage("Recreated "+msg.pr.url))
case pullRequestOpenedInBrowser:
m.listModel.StopSpinner()
Expand All @@ -127,59 +166,65 @@ func (m ListView) Update(msg tea.Msg) (ListView, tea.Cmd) {
switch {
case key.Matches(msg, m.keyMap.mergeRebase):
if selectedItem, ok := m.listModel.SelectedItem().(pullRequest); ok {
m.markInProgress(selectedItem.repository, selectedItem.number)
m.listModel.RemoveItem(m.listModel.Index())
cmds = append(
cmds,
m.listModel.StartSpinner(),
mergePullRequest(selectedItem, MethodRebase),
m.commander.mergePullRequest(selectedItem, MethodRebase),
)
}
case key.Matches(msg, m.keyMap.mergeDefault):
if selectedItem, ok := m.listModel.SelectedItem().(pullRequest); ok {
m.markInProgress(selectedItem.repository, selectedItem.number)
m.listModel.RemoveItem(m.listModel.Index())
cmds = append(
cmds,
m.listModel.StartSpinner(),
mergePullRequest(selectedItem, MethodMerge),
m.commander.mergePullRequest(selectedItem, MethodMerge),
)
}
case key.Matches(msg, m.keyMap.mergeSquash):
if selectedItem, ok := m.listModel.SelectedItem().(pullRequest); ok {
m.markInProgress(selectedItem.repository, selectedItem.number)
m.listModel.RemoveItem(m.listModel.Index())
cmds = append(
cmds,
m.listModel.StartSpinner(),
mergePullRequest(selectedItem, MethodSquash),
m.commander.mergePullRequest(selectedItem, MethodSquash),
)
}
case key.Matches(msg, m.keyMap.mergeDependabot):
if selectedItem, ok := m.listModel.SelectedItem().(pullRequest); ok {
m.markInProgress(selectedItem.repository, selectedItem.number)
m.listModel.RemoveItem(m.listModel.Index())
cmds = append(
cmds,
m.listModel.StartSpinner(),
mergePullRequest(selectedItem, MethodDependabot),
m.commander.mergePullRequest(selectedItem, MethodDependabot),
)
}
case key.Matches(msg, m.keyMap.rebase):
if selectedItem, ok := m.listModel.SelectedItem().(pullRequest); ok {
m.markInProgress(selectedItem.repository, selectedItem.number)
cmds = append(
cmds,
m.listModel.StartSpinner(),
rebasePullRequest(selectedItem),
m.commander.rebasePullRequest(selectedItem),
)
}
case key.Matches(msg, m.keyMap.recreate):
if selectedItem, ok := m.listModel.SelectedItem().(pullRequest); ok {
m.markInProgress(selectedItem.repository, selectedItem.number)
cmds = append(
cmds,
m.listModel.StartSpinner(),
recreatePullRequest(selectedItem),
m.commander.recreatePullRequest(selectedItem),
)
}
case key.Matches(msg, m.keyMap.browse):
if selectedItem, ok := m.listModel.SelectedItem().(pullRequest); ok {
cmds = append(cmds, openInBrowser(selectedItem))
cmds = append(cmds, m.commander.openInBrowser(selectedItem))
}
case key.Matches(msg, m.keyMap.view):
if selectedItem, ok := m.listModel.SelectedItem().(pullRequest); ok {
Expand Down

0 comments on commit 27754f5

Please sign in to comment.