From 358f08889d6f22a21a8de4fdcbfae7f071aa8fc6 Mon Sep 17 00:00:00 2001 From: Prasanth Krishnan Date: Tue, 9 Jul 2024 16:18:08 -0700 Subject: [PATCH] Classify errors; userError vs servicefault - Branch protection not enabled = userError - No required status checks not enabled = userError - Cannot acquire lock to add review = TooManyRequestError Refactored errors into a separate package to avoid import cycle --- errors.go => errors/errors.go | 10 ++- errors_test.go => errors/errors_test.go | 62 ++++++++++++++++--- github/api.go | 4 +- github/github.go | 28 +++++++-- github/mock_api.go | 18 +++--- pullrequest/dispatcher.go | 4 +- pullrequest/dispatcher_test.go | 8 +-- pullrequest/event_filter.go | 4 +- pullrequest/review/dedup_reviewer.go | 6 +- pullrequest/review/mutex_reviewer.go | 8 ++- .../review/pre_cond_validation_reviewer.go | 14 ++--- pullrequest/review/rate_limited_reviewer.go | 4 +- .../review/rate_limited_reviewer_test.go | 4 +- pullrequest/review/reviewer.go | 14 ++--- rate/sliding_window.go | 4 +- rate/sliding_window_test.go | 8 +-- webhook/controller.go | 13 ++-- 17 files changed, 144 insertions(+), 69 deletions(-) rename errors.go => errors/errors.go (92%) rename errors_test.go => errors/errors_test.go (53%) diff --git a/errors.go b/errors/errors.go similarity index 92% rename from errors.go rename to errors/errors.go index ab17ff6..027a0a6 100644 --- a/errors.go +++ b/errors/errors.go @@ -1,4 +1,4 @@ -package prbot +package errors import ( "context" @@ -28,6 +28,14 @@ func (e APIError) Render(_ http.ResponseWriter, r *http.Request) error { return nil } +func (e APIError) IsUserError() bool { + return e.StatusCode >= 400 && e.StatusCode < 500 +} + +func (e APIError) IsServiceError() bool { + return !e.IsUserError() +} + func RenderError(w http.ResponseWriter, r *http.Request, err error) { var ae APIError if errors.As(err, &ae) { diff --git a/errors_test.go b/errors/errors_test.go similarity index 53% rename from errors_test.go rename to errors/errors_test.go index 925c269..d1ef135 100644 --- a/errors_test.go +++ b/errors/errors_test.go @@ -1,4 +1,4 @@ -package prbot_test +package errors_test import ( "bytes" @@ -11,15 +11,17 @@ import ( "github.com/go-chi/chi/v5/middleware" "github.com/google/go-github/v50/github" - prbot "github.com/marqeta/pr-bot" + pe "github.com/marqeta/pr-bot/errors" "github.com/stretchr/testify/assert" ) func TestRenderError(t *testing.T) { + ctx := context.TODO() + ctx = context.WithValue(ctx, middleware.RequestIDKey, "requestID") //nolint:goerr113 errRandom := errors.New("random") - apiError := prbot.APIError{ + apiError := pe.APIError{ RequestID: "requestID", ErrorCode: "code", Message: "msg", @@ -29,12 +31,12 @@ func TestRenderError(t *testing.T) { tests := []struct { name string err error - wantErr prbot.APIError + wantErr pe.APIError }{ { name: "Should render APIError", err: apiError, - wantErr: prbot.APIError{ + wantErr: pe.APIError{ RequestID: "requestID", ErrorCode: "code", Message: "msg", @@ -42,10 +44,54 @@ func TestRenderError(t *testing.T) { ErrorText: errRandom.Error(), }, }, + { + name: "Should render TooManyRequestError", + err: pe.TooManyRequestError(ctx, "random", errRandom), + wantErr: pe.APIError{ + RequestID: "requestID", + ErrorCode: "TooManyRequestsError", + Message: "random", + StatusCode: http.StatusTooManyRequests, + ErrorText: errRandom.Error(), + }, + }, + { + name: "Should render InValidRequestError", + err: pe.InValidRequestError(ctx, "random", errRandom), + wantErr: pe.APIError{ + RequestID: "requestID", + ErrorCode: "InValidRequestError", + Message: "random", + StatusCode: http.StatusBadRequest, + ErrorText: errRandom.Error(), + }, + }, + { + name: "Should render ServiceFault", + err: pe.ServiceFault(ctx, "random", errRandom), + wantErr: pe.APIError{ + RequestID: "requestID", + ErrorCode: "ServiceFault", + Message: "random", + StatusCode: http.StatusInternalServerError, + ErrorText: errRandom.Error(), + }, + }, + { + name: "Should render UserError", + err: pe.UserError(ctx, "random", errRandom), + wantErr: pe.APIError{ + RequestID: "requestID", + ErrorCode: "UserError", + Message: "random", + StatusCode: http.StatusBadRequest, + ErrorText: errRandom.Error(), + }, + }, { name: "Should render non APIError", err: errRandom, - wantErr: prbot.APIError{ + wantErr: pe.APIError{ RequestID: "requestID", ErrorCode: "Unknown", StatusCode: http.StatusInternalServerError, @@ -57,9 +103,9 @@ func TestRenderError(t *testing.T) { t.Run(tt.name, func(t *testing.T) { req, _ := NewRequest(t, tt.wantErr.RequestID) rr := httptest.NewRecorder() - prbot.RenderError(rr, req, tt.err) + pe.RenderError(rr, req, tt.err) assert.Equal(t, tt.wantErr.StatusCode, rr.Code) - var res prbot.APIError + var res pe.APIError err := json.Unmarshal(rr.Body.Bytes(), &res) assert.Nil(t, err, "error when unmarshalling response") assert.Equal(t, tt.wantErr, res) diff --git a/github/api.go b/github/api.go index 2aed0c4..af9c5a0 100644 --- a/github/api.go +++ b/github/api.go @@ -4,7 +4,7 @@ import ( "context" "github.com/google/go-github/v50/github" - prbot "github.com/marqeta/pr-bot" + pe "github.com/marqeta/pr-bot/errors" "github.com/marqeta/pr-bot/id" "github.com/shurcooL/githubv4" ) @@ -47,7 +47,7 @@ type API interface { AddReview(ctx context.Context, id id.PR, summary, event string) error EnableAutoMerge(ctx context.Context, id id.PR, method githubv4.PullRequestMergeMethod) error IssueComment(ctx context.Context, id id.PR, comment string) error - IssueCommentForError(ctx context.Context, id id.PR, err prbot.APIError) error + IssueCommentForError(ctx context.Context, id id.PR, err pe.APIError) error ListAllTopics(ctx context.Context, id id.PR) ([]string, error) ListRequiredStatusChecks(ctx context.Context, id id.PR, branch string) ([]string, error) ListFilesInRootDir(ctx context.Context, id id.PR, branch string) ([]string, error) diff --git a/github/github.go b/github/github.go index a4da26f..b1f7271 100644 --- a/github/github.go +++ b/github/github.go @@ -8,7 +8,7 @@ import ( "github.com/go-chi/chi/v5/middleware" "github.com/google/go-github/v50/github" - prbot "github.com/marqeta/pr-bot" + pe "github.com/marqeta/pr-bot/errors" "github.com/marqeta/pr-bot/id" "github.com/marqeta/pr-bot/metrics" "github.com/shurcooL/githubv4" @@ -59,7 +59,7 @@ func (gh *githubDao) ListReviews(ctx context.Context, id id.PR) ([]*github.PullR func (gh *githubDao) GetBranchProtection(ctx context.Context, id id.PR, branch string) (*github.Protection, error) { b, resp, err := gh.v3.Repositories.GetBranchProtection(ctx, id.Owner, id.Repo, branch) if err != nil { - return nil, err + return nil, classifyError(ctx, resp, fmt.Sprintf("error getting branch protection for PR %v", id.URL), err) } gh.emitTokenExpiration(ctx, resp) return b, nil @@ -84,7 +84,7 @@ func (gh *githubDao) ListFilesInRootDir(ctx context.Context, id id.PR, branch st Ref: branch, }) if err != nil { - return nil, err + return nil, classifyError(ctx, resp, fmt.Sprintf("error listing files on repo for PR %v", id.URL), err) } gh.emitTokenExpiration(ctx, resp) filenames := make([]string, len(files)) @@ -101,7 +101,8 @@ func (gh *githubDao) ListFilesInRootDir(ctx context.Context, id id.PR, branch st func (gh *githubDao) ListRequiredStatusChecks(ctx context.Context, id id.PR, branch string) ([]string, error) { checks, resp, err := gh.v3.Repositories.ListRequiredStatusChecksContexts(ctx, id.Owner, id.Repo, branch) if err != nil { - return nil, err + return nil, classifyError(ctx, resp, + fmt.Sprintf("error listing required status checks on repo for PR %v", id.URL), err) } gh.emitTokenExpiration(ctx, resp) return checks, nil @@ -173,7 +174,7 @@ func (gh *githubDao) AddReview(ctx context.Context, id id.PR, summary, event str } // IssueCommentForError implements Dao -func (gh *githubDao) IssueCommentForError(ctx context.Context, id id.PR, apiError prbot.APIError) error { +func (gh *githubDao) IssueCommentForError(ctx context.Context, id id.PR, apiError pe.APIError) error { b, err := json.MarshalIndent(apiError, "", " ") if err != nil { return err @@ -200,3 +201,20 @@ func (gh *githubDao) UI(id id.PR) string { } return fmt.Sprintf("https://%s/ui/eval/%s/pull/%d", gh.serverHost, id.RepoFullName, id.Number) } + +func classifyError(ctx context.Context, resp *github.Response, msg string, err error) error { + if resp == nil { + return err + } + if isClientError(resp) { + return pe.UserError(ctx, msg, err) + } + return pe.ServiceFault(ctx, msg, err) +} + +func isClientError(resp *github.Response) bool { + if resp == nil { + return false + } + return resp.StatusCode >= 400 && resp.StatusCode < 500 +} diff --git a/github/mock_api.go b/github/mock_api.go index d209ecf..62951da 100644 --- a/github/mock_api.go +++ b/github/mock_api.go @@ -5,12 +5,12 @@ package github import ( context "context" - id "github.com/marqeta/pr-bot/id" + errors "github.com/marqeta/pr-bot/errors" githubv4 "github.com/shurcooL/githubv4" - mock "github.com/stretchr/testify/mock" + id "github.com/marqeta/pr-bot/id" - prbot "github.com/marqeta/pr-bot" + mock "github.com/stretchr/testify/mock" v50github "github.com/google/go-github/v50/github" ) @@ -218,11 +218,11 @@ func (_c *MockAPI_IssueComment_Call) RunAndReturn(run func(context.Context, id.P } // IssueCommentForError provides a mock function with given fields: ctx, _a1, err -func (_m *MockAPI) IssueCommentForError(ctx context.Context, _a1 id.PR, err prbot.APIError) error { +func (_m *MockAPI) IssueCommentForError(ctx context.Context, _a1 id.PR, err errors.APIError) error { ret := _m.Called(ctx, _a1, err) var r0 error - if rf, ok := ret.Get(0).(func(context.Context, id.PR, prbot.APIError) error); ok { + if rf, ok := ret.Get(0).(func(context.Context, id.PR, errors.APIError) error); ok { r0 = rf(ctx, _a1, err) } else { r0 = ret.Error(0) @@ -239,14 +239,14 @@ type MockAPI_IssueCommentForError_Call struct { // IssueCommentForError is a helper method to define mock.On call // - ctx context.Context // - _a1 id.PR -// - err prbot.APIError +// - err errors.APIError func (_e *MockAPI_Expecter) IssueCommentForError(ctx interface{}, _a1 interface{}, err interface{}) *MockAPI_IssueCommentForError_Call { return &MockAPI_IssueCommentForError_Call{Call: _e.mock.On("IssueCommentForError", ctx, _a1, err)} } -func (_c *MockAPI_IssueCommentForError_Call) Run(run func(ctx context.Context, _a1 id.PR, err prbot.APIError)) *MockAPI_IssueCommentForError_Call { +func (_c *MockAPI_IssueCommentForError_Call) Run(run func(ctx context.Context, _a1 id.PR, err errors.APIError)) *MockAPI_IssueCommentForError_Call { _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(id.PR), args[2].(prbot.APIError)) + run(args[0].(context.Context), args[1].(id.PR), args[2].(errors.APIError)) }) return _c } @@ -256,7 +256,7 @@ func (_c *MockAPI_IssueCommentForError_Call) Return(_a0 error) *MockAPI_IssueCom return _c } -func (_c *MockAPI_IssueCommentForError_Call) RunAndReturn(run func(context.Context, id.PR, prbot.APIError) error) *MockAPI_IssueCommentForError_Call { +func (_c *MockAPI_IssueCommentForError_Call) RunAndReturn(run func(context.Context, id.PR, errors.APIError) error) *MockAPI_IssueCommentForError_Call { _c.Call.Return(run) return _c } diff --git a/pullrequest/dispatcher.go b/pullrequest/dispatcher.go index e1a0ec5..65048c9 100644 --- a/pullrequest/dispatcher.go +++ b/pullrequest/dispatcher.go @@ -9,7 +9,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" "github.com/go-chi/httplog" "github.com/google/go-github/v50/github" - prbot "github.com/marqeta/pr-bot" + pe "github.com/marqeta/pr-bot/errors" "github.com/marqeta/pr-bot/id" "github.com/marqeta/pr-bot/metrics" ) @@ -126,5 +126,5 @@ func (d *dispatcher) Dispatch(ctx context.Context, _ string, eventName string, e } func parseError(ctx context.Context, err error) error { - return prbot.InValidRequestError(ctx, "error parsing webhook event", err) + return pe.InValidRequestError(ctx, "error parsing webhook event", err) } diff --git a/pullrequest/dispatcher_test.go b/pullrequest/dispatcher_test.go index 8337112..83a4201 100644 --- a/pullrequest/dispatcher_test.go +++ b/pullrequest/dispatcher_test.go @@ -8,7 +8,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" "github.com/google/go-github/v50/github" - prbot "github.com/marqeta/pr-bot" + pe "github.com/marqeta/pr-bot/errors" "github.com/marqeta/pr-bot/id" "github.com/marqeta/pr-bot/metrics" "github.com/marqeta/pr-bot/pullrequest" @@ -220,7 +220,7 @@ func Test_dispatcher_Dispatch(t *testing.T) { setExpectations: func(_ id.PR, _ *github.PullRequestEvent, _ *pullrequest.MockEventFilter, _ *pullrequest.MockEventHandler) { }, - wantErr: prbot.InValidRequestError(ctx, "error parsing webhook event", pullrequest.ErrMismatchedEvent), + wantErr: pe.InValidRequestError(ctx, "error parsing webhook event", pullrequest.ErrMismatchedEvent), }, { name: "Error when Event action is nil", @@ -232,7 +232,7 @@ func Test_dispatcher_Dispatch(t *testing.T) { setExpectations: func(_ id.PR, _ *github.PullRequestEvent, _ *pullrequest.MockEventFilter, _ *pullrequest.MockEventHandler) { }, - wantErr: prbot.InValidRequestError(ctx, "error parsing webhook event", pullrequest.ErrEventActionNotFound), + wantErr: pe.InValidRequestError(ctx, "error parsing webhook event", pullrequest.ErrEventActionNotFound), }, { name: "Error when Event PR is nil", @@ -247,7 +247,7 @@ func Test_dispatcher_Dispatch(t *testing.T) { setExpectations: func(_ id.PR, _ *github.PullRequestEvent, _ *pullrequest.MockEventFilter, _ *pullrequest.MockEventHandler) { }, - wantErr: prbot.InValidRequestError(ctx, "error parsing webhook event", pullrequest.ErrPRNotFound), + wantErr: pe.InValidRequestError(ctx, "error parsing webhook event", pullrequest.ErrPRNotFound), }, { name: "Should return error from event handler", diff --git a/pullrequest/event_filter.go b/pullrequest/event_filter.go index f88ed48..4ccc45d 100644 --- a/pullrequest/event_filter.go +++ b/pullrequest/event_filter.go @@ -5,8 +5,8 @@ import ( "regexp" "github.com/go-chi/httplog" - prbot "github.com/marqeta/pr-bot" "github.com/marqeta/pr-bot/configstore" + pe "github.com/marqeta/pr-bot/errors" gh "github.com/marqeta/pr-bot/github" "github.com/marqeta/pr-bot/id" ) @@ -106,7 +106,7 @@ func (f *repoFilter) ShouldHandle(ctx context.Context, id id.PR) (bool, error) { func (f *repoFilter) hasIgnoreTopic(ctx context.Context, id id.PR, cfg *RepoFilterCfg) (bool, error) { topics, err := f.dao.ListAllTopics(ctx, id) if err != nil { - return true, prbot.ServiceFault(ctx, "error listing topics on repo", err) + return true, pe.ServiceFault(ctx, "error listing topics on repo", err) } for _, topic := range topics { if cfg.IgnoreTopicsMap[topic] { diff --git a/pullrequest/review/dedup_reviewer.go b/pullrequest/review/dedup_reviewer.go index 4a0865b..ec1adf1 100644 --- a/pullrequest/review/dedup_reviewer.go +++ b/pullrequest/review/dedup_reviewer.go @@ -29,7 +29,7 @@ func (d *DedupReviewer) Approve(ctx context.Context, id id.PR, body string, opts return err } if d.checkForReview(reviews, types.Approve) { - oplog.Info().Msgf("PR already has a review of type %v or higher", types.Approve) + oplog.Info().Msgf("PR already has a review of type %v or higher %v", types.Approve, id.URL) return nil } return d.delegate.Approve(ctx, id, body, opts) @@ -44,7 +44,7 @@ func (d *DedupReviewer) Comment(ctx context.Context, id id.PR, body string) erro return err } if d.checkForReview(reviews, types.Comment) { - oplog.Info().Msgf("PR already has a review of type %v or higher", types.Comment) + oplog.Info().Msgf("PR already has a review of type %v or higher %v", types.Comment, id.URL) return nil } return d.delegate.Comment(ctx, id, body) @@ -59,7 +59,7 @@ func (d *DedupReviewer) RequestChanges(ctx context.Context, id id.PR, body strin return err } if d.checkForReview(reviews, types.RequestChanges) { - oplog.Info().Msgf("PR already has a review of type %v or higher", types.RequestChanges) + oplog.Info().Msgf("PR already has a review of type %v or higher %v", types.RequestChanges, id.URL) return nil } return d.delegate.RequestChanges(ctx, id, body) diff --git a/pullrequest/review/mutex_reviewer.go b/pullrequest/review/mutex_reviewer.go index 64768e2..29a1e56 100644 --- a/pullrequest/review/mutex_reviewer.go +++ b/pullrequest/review/mutex_reviewer.go @@ -9,6 +9,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/dynamodb" "github.com/go-chi/httplog" + pe "github.com/marqeta/pr-bot/errors" "github.com/marqeta/pr-bot/id" ) @@ -33,7 +34,7 @@ func (r *mutexReviewer) releaseLock(ctx context.Context, lock *dynamolock.Lock, oplog := httplog.LogEntry(ctx) success, err := r.locker.ReleaseLockWithContext(ctx, lock) if !success { - oplog.Err(err).Msgf("lock for PR %v was already released", id.URL) + oplog.Error().Msgf("lock for PR %v was already released", id.URL) } if err != nil { oplog.Err(err).Msgf("error releasing lock for PR %v", id.URL) @@ -49,7 +50,7 @@ func (r *mutexReviewer) acquireLock(ctx context.Context, id id.PR) (*dynamolock. ) if err != nil { oplog.Err(err).Msgf("error acquiring lock for PR %v", id.URL) - return nil, err + return nil, pe.TooManyRequestError(ctx, "Error acquiring lock", err) } return lock, nil } @@ -58,6 +59,7 @@ func (r *mutexReviewer) acquireLock(ctx context.Context, id id.PR) (*dynamolock. func (r *mutexReviewer) Approve(ctx context.Context, id id.PR, body string, opts ApproveOptions) error { lock, err := r.acquireLock(ctx, id) if err != nil { + // TODO: handle burst of PR events better. return err } defer r.releaseLock(ctx, lock, id) @@ -68,6 +70,7 @@ func (r *mutexReviewer) Approve(ctx context.Context, id id.PR, body string, opts func (r *mutexReviewer) Comment(ctx context.Context, id id.PR, body string) error { lock, err := r.acquireLock(ctx, id) if err != nil { + // TODO: handle burst of PR events better. return err } defer r.releaseLock(ctx, lock, id) @@ -78,6 +81,7 @@ func (r *mutexReviewer) Comment(ctx context.Context, id id.PR, body string) erro func (r *mutexReviewer) RequestChanges(ctx context.Context, id id.PR, body string) error { lock, err := r.acquireLock(ctx, id) if err != nil { + // TODO: handle burst of PR events better. return err } defer r.releaseLock(ctx, lock, id) diff --git a/pullrequest/review/pre_cond_validation_reviewer.go b/pullrequest/review/pre_cond_validation_reviewer.go index 21b69e7..b6594e4 100644 --- a/pullrequest/review/pre_cond_validation_reviewer.go +++ b/pullrequest/review/pre_cond_validation_reviewer.go @@ -5,7 +5,7 @@ import ( "errors" "github.com/go-chi/httplog" - prbot "github.com/marqeta/pr-bot" + pe "github.com/marqeta/pr-bot/errors" gh "github.com/marqeta/pr-bot/github" "github.com/marqeta/pr-bot/id" "github.com/marqeta/pr-bot/metrics" @@ -24,16 +24,15 @@ func (p *preCondValidationReviewer) Approve(ctx context.Context, id id.PR, body oplog := httplog.LogEntry(ctx) if !opts.AutoMergeEnabled { - ae := prbot.UserError(ctx, AutoMergeError, ErrAutoMergeDisabled) + ae := pe.UserError(ctx, AutoMergeError, ErrAutoMergeDisabled) oplog.Error().Msgf("Auto merge is disabled in repo for pr %v", id.URL) return ae } checks, err := p.api.ListRequiredStatusChecks(ctx, id, opts.DefaultBranch) if err != nil { - oplog.Err(err).Msgf("error listing required status checks on repo for PR %v", id.URL) - ae := prbot.ServiceFault(ctx, "Error listing status checks on Repo", err) - return ae + oplog.Err(err).Send() + return err } for _, check := range checks { @@ -46,9 +45,8 @@ func (p *preCondValidationReviewer) Approve(ctx context.Context, id id.PR, body // No required blackbird-ci check, check if it is a BB build repo files, err := p.api.ListFilesInRootDir(ctx, id, opts.DefaultBranch) if err != nil { - oplog.Err(err).Msgf("error listing files on repo for PR %v", id.URL) - ae := prbot.ServiceFault(ctx, "Error listing files on Repo", err) - return ae + oplog.Err(err).Send() + return err } for _, file := range files { diff --git a/pullrequest/review/rate_limited_reviewer.go b/pullrequest/review/rate_limited_reviewer.go index 6144094..d718e1b 100644 --- a/pullrequest/review/rate_limited_reviewer.go +++ b/pullrequest/review/rate_limited_reviewer.go @@ -6,7 +6,7 @@ import ( "net/http" "github.com/go-chi/httplog" - prbot "github.com/marqeta/pr-bot" + pe "github.com/marqeta/pr-bot/errors" gh "github.com/marqeta/pr-bot/github" "github.com/marqeta/pr-bot/id" "github.com/marqeta/pr-bot/rate" @@ -23,7 +23,7 @@ func (r *rateLimitedReviewer) Approve(ctx context.Context, id id.PR, body string oplog := httplog.LogEntry(ctx) err := r.throttler.ShouldThrottle(ctx, id) if err != nil { - var ae prbot.APIError + var ae pe.APIError if errors.As(err, &ae) && ae.StatusCode == http.StatusTooManyRequests { oplog.Err(ae).Msgf("request throttled for PR %v", id.URL) // TODO publish error in UI and/or as comments on PR diff --git a/pullrequest/review/rate_limited_reviewer_test.go b/pullrequest/review/rate_limited_reviewer_test.go index f73aca7..c6da3bd 100644 --- a/pullrequest/review/rate_limited_reviewer_test.go +++ b/pullrequest/review/rate_limited_reviewer_test.go @@ -5,7 +5,7 @@ import ( "errors" "testing" - prbot "github.com/marqeta/pr-bot" + pe "github.com/marqeta/pr-bot/errors" gh "github.com/marqeta/pr-bot/github" "github.com/marqeta/pr-bot/id" "github.com/marqeta/pr-bot/pullrequest/review" @@ -18,7 +18,7 @@ func Test_rateLimitedReviewer_Approve(t *testing.T) { ctx := context.Background() //nolint:goerr113 errRandom := errors.New("random error") - errThrottled := prbot.TooManyRequestError(ctx, "throttled", lim.ErrLimitExhausted) + errThrottled := pe.TooManyRequestError(ctx, "throttled", lim.ErrLimitExhausted) type args struct { id id.PR body string diff --git a/pullrequest/review/reviewer.go b/pullrequest/review/reviewer.go index 7f6d97c..8054fab 100644 --- a/pullrequest/review/reviewer.go +++ b/pullrequest/review/reviewer.go @@ -6,7 +6,7 @@ import ( "strings" "github.com/go-chi/httplog" - prbot "github.com/marqeta/pr-bot" + pe "github.com/marqeta/pr-bot/errors" gh "github.com/marqeta/pr-bot/github" "github.com/marqeta/pr-bot/id" "github.com/marqeta/pr-bot/metrics" @@ -47,7 +47,7 @@ func (r *reviewer) Approve(ctx context.Context, id id.PR, body string, opts Appr err = r.api.AddReview(ctx, id, body, gh.Approve) if err != nil { oplog.Err(err).Msgf("error approving PR") - ae := prbot.ServiceFault(ctx, "Error approving PR", err) + ae := pe.ServiceFault(ctx, "Error approving PR", err) // TODO publish error in UI and/or as comments on PR return ae } @@ -63,7 +63,7 @@ func (r *reviewer) Comment(ctx context.Context, id id.PR, body string) error { err := r.api.AddReview(ctx, id, body, gh.Comment) if err != nil { oplog.Err(err).Msgf("error reviewing PR with reviewType:comment %v", id.URL) - ae := prbot.ServiceFault(ctx, "error reviewing PR with reviewType:comment", err) + ae := pe.ServiceFault(ctx, "error reviewing PR with reviewType:comment", err) // TODO publish error in UI and/or as comments on PR return ae } @@ -78,7 +78,7 @@ func (r *reviewer) RequestChanges(ctx context.Context, id id.PR, body string) er err := r.api.AddReview(ctx, id, body, gh.RequestChanges) if err != nil { oplog.Err(err).Msgf("error reviewing PR with reviewType:changes_requested %v", id.URL) - ae := prbot.ServiceFault(ctx, "error reviewing PR with reviewType:changes_requested", err) + ae := pe.ServiceFault(ctx, "error reviewing PR with reviewType:changes_requested", err) // TODO publish error in UI and/or as comments on PR return ae } @@ -95,7 +95,7 @@ func NewReviewer(dao gh.API, metrics metrics.Emitter) Reviewer { func (r *reviewer) handleAutoMergeError(ctx context.Context, id id.PR, err error) error { msg := strings.ToLower(err.Error()) if strings.Contains(msg, "pull request auto merge is not allowed") { - ae := prbot.UserError(ctx, AutoMergeError, err) + ae := pe.UserError(ctx, AutoMergeError, err) r.metrics.EmitDist(ctx, "autoMergeDisabled", 1.0, id.ToTags()) // TODO publish error in UI and/or as comments on PR return ae @@ -103,12 +103,12 @@ func (r *reviewer) handleAutoMergeError(ctx context.Context, id id.PR, err error if strings.Contains(msg, "pull request is in has_hooks status") || strings.Contains(msg, "pull request is in clean status") { friendlyErr := fmt.Errorf("enable atleast one branch protection rule on the default branch : %w", err) - ae := prbot.UserError(ctx, AutoMergeError, friendlyErr) + ae := pe.UserError(ctx, AutoMergeError, friendlyErr) r.metrics.EmitDist(ctx, "noBranchProtectionRules", 1.0, id.ToTags()) // TODO publish error in UI and/or as comments on PR return ae } - ae := prbot.ServiceFault(ctx, AutoMergeError, err) + ae := pe.ServiceFault(ctx, AutoMergeError, err) // TODO publish error in UI and/or as comments on PR return ae } diff --git a/rate/sliding_window.go b/rate/sliding_window.go index 6e177ff..ef037d1 100644 --- a/rate/sliding_window.go +++ b/rate/sliding_window.go @@ -5,8 +5,8 @@ import ( "errors" "fmt" - prbot "github.com/marqeta/pr-bot" "github.com/marqeta/pr-bot/configstore" + pe "github.com/marqeta/pr-bot/errors" "github.com/marqeta/pr-bot/id" lim "github.com/mennanov/limiters" ) @@ -46,7 +46,7 @@ func (sw *swLimiter) ShouldThrottle(ctx context.Context, id id.PR) error { if err != nil && errors.Is(err, lim.ErrLimitExhausted) { // throttled msg := fmt.Sprintf("%v throttled request for key %v, try again in %v", sw.Name(), sw.Key(id), waitTime) - return prbot.TooManyRequestError(ctx, msg, err) + return pe.TooManyRequestError(ctx, msg, err) } return err } diff --git a/rate/sliding_window_test.go b/rate/sliding_window_test.go index b96c65c..c183a8e 100644 --- a/rate/sliding_window_test.go +++ b/rate/sliding_window_test.go @@ -7,8 +7,8 @@ import ( "testing" "time" - prbot "github.com/marqeta/pr-bot" "github.com/marqeta/pr-bot/configstore" + pe "github.com/marqeta/pr-bot/errors" "github.com/marqeta/pr-bot/id" "github.com/marqeta/pr-bot/rate" "github.com/stretchr/testify/assert" @@ -51,7 +51,7 @@ func Test_swLimiter_ShouldThrottle(t *testing.T) { args: args{ id: ID("owner1", "repo1", "author1"), }, - wantErr: prbot.TooManyRequestError(ctx, + wantErr: pe.TooManyRequestError(ctx, fmt.Sprintf("%v throttled request for key %v, try again in %v", "Mock", "Repo/owner1/repo1", 5*time.Second), lim.ErrLimitExhausted), @@ -73,7 +73,7 @@ func Test_swLimiter_ShouldThrottle(t *testing.T) { args: args{ id: ID("owner1", "repo1", "author1"), }, - wantErr: prbot.TooManyRequestError(ctx, + wantErr: pe.TooManyRequestError(ctx, fmt.Sprintf("%v throttled request for key %v, try again in %v", "Mock", "Org/owner1", 5*time.Second), lim.ErrLimitExhausted), @@ -95,7 +95,7 @@ func Test_swLimiter_ShouldThrottle(t *testing.T) { args: args{ id: ID("owner1", "repo1", "author1"), }, - wantErr: prbot.TooManyRequestError(ctx, + wantErr: pe.TooManyRequestError(ctx, fmt.Sprintf("%v throttled request for key %v, try again in %v", "Mock", "Author/author1", 5*time.Second), lim.ErrLimitExhausted), diff --git a/webhook/controller.go b/webhook/controller.go index 53b45fa..daea672 100644 --- a/webhook/controller.go +++ b/webhook/controller.go @@ -8,7 +8,8 @@ import ( "github.com/go-chi/httplog" "github.com/go-chi/render" "github.com/google/go-github/v50/github" - prbot "github.com/marqeta/pr-bot" + + pe "github.com/marqeta/pr-bot/errors" "github.com/marqeta/pr-bot/opa/evaluation" ) @@ -32,8 +33,8 @@ func (c *controller) HandleEvent(w http.ResponseWriter, r *http.Request) { payload, err := c.parser.ValidatePayload(r, []byte(c.webhookSecrect)) if err != nil { oplog.Err(err).Msg("could not validate webhook payload") - prbot.RenderError(w, r, - prbot.InValidRequestError(ctx, "could not validate webhook payload", err)) + pe.RenderError(w, r, + pe.InValidRequestError(ctx, "could not validate webhook payload", err)) return } @@ -41,8 +42,8 @@ func (c *controller) HandleEvent(w http.ResponseWriter, r *http.Request) { event, err := c.parser.ParseWebHook(github.WebHookType(r), payload) if err != nil { oplog.Err(err).Msg("could not parse webhook") - prbot.RenderError(w, r, - prbot.InValidRequestError(ctx, "could not parse webhook", err)) + pe.RenderError(w, r, + pe.InValidRequestError(ctx, "could not parse webhook", err)) return } @@ -65,7 +66,7 @@ func (c *controller) HandleEvent(w http.ResponseWriter, r *http.Request) { if err != nil { oplog.Err(err).Msg("Error Handling Event") - prbot.RenderError(w, r, err) + pe.RenderError(w, r, err) } else { _ = render.Render(w, r, &EventResponse{ StatusCode: http.StatusAccepted,