From 48e7f54320753d3654119f9ed43703c37ac6d501 Mon Sep 17 00:00:00 2001 From: Frank Bagherzadeh Date: Tue, 30 May 2023 12:42:21 -0400 Subject: [PATCH 1/2] Make merge gatekeeper more resilient (#1) * ground works * cleanup * comments --- README.md | 3 +- action.yml | 5 +++ go.sum | 4 +-- internal/cli/validate.go | 11 +++++- internal/github/github.go | 34 ++++++++++++++++--- internal/github/retry_roundtripper.go | 48 +++++++++++++++++++++++++++ 6 files changed, 95 insertions(+), 10 deletions(-) create mode 100644 internal/github/retry_roundtripper.go diff --git a/README.md b/README.md index eb57441..c51c4c7 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ At UPSIDER, we have a few internal repositories set up with a monorepo structure We are looking to add a few more features, such as extra signoff from non-coder, label based check, etc. -NOTE: +NOTE: (\*1) There are some other hacks, such as using an empty job with the same name to override the status, but those solutions do not provide the flexible control we are after. @@ -85,6 +85,7 @@ There are some customisation available for Merge Gatekeeper. | `token` | `GITHUB_TOKEN` or Personal Access Token with `repo` scope | Yes | | `self` | The name of Merge Gatekeeper job, and defaults to `merge-gatekeeper`. This is used to check other job status, and do not check Merge Gatekeeper itself. If you updated the GitHub Action job name from `merge-gatekeeper` to something else, you would need to specify the new name with this value. | | | `interval` | Check interval to recheck the job status. Default is set to 5 (sec). | | +| `github-client-retry` | Retry the request if the GitHub client returns 5xx error. Default is set to 0. | | | `timeout` | Timeout setup to give up further check. Default is set to 600 (sec). | | | `ignored` | Jobs to ignore regardless of their statuses. Defined as a comma-separated list. | | | `ref` | Git ref to check out. This falls back to the HEAD for given PR, but can be set to any ref. | | diff --git a/action.yml b/action.yml index 402a94a..1cb638e 100644 --- a/action.yml +++ b/action.yml @@ -15,6 +15,10 @@ inputs: description: "set validate interval second (default 5)" required: false default: "5" + github-client-retry: + description: "set retry count for GitHub client (default 0)" + required: false + default: "0" timeout: description: "set validate timeout second (default 600)" required: false @@ -35,6 +39,7 @@ runs: - "--token=${{ inputs.token }}" - "--self=${{ inputs.self }}" - "--interval=${{ inputs.interval }}" + - "--github-client-retry=${{ inputs.github-client-retry }}" - "--ref=${{ inputs.ref }}" - "--timeout=${{ inputs.timeout }}" - "--ignored=${{ inputs.ignored }}" diff --git a/go.sum b/go.sum index 4782260..e481bc1 100644 --- a/go.sum +++ b/go.sum @@ -115,11 +115,9 @@ github.com/google/go-cmp v0.5.1/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.3/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= -github.com/google/go-cmp v0.5.5 h1:Khx7svrCpmxxtHBq5j2mp/xVjsi8hQMfNLvJFAlrGgU= github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= +github.com/google/go-cmp v0.5.6 h1:BKbKCqvP6I+rmFHt06ZmyQtvB8xAkWdhFyr0ZUNZcxQ= github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= -github.com/google/go-github v17.0.0+incompatible h1:N0LgJ1j65A7kfXrZnUDaYCs/Sf4rEjNlfyDHW9dolSY= -github.com/google/go-github v17.0.0+incompatible/go.mod h1:zLgOLi98H3fifZn+44m+umXrS52loVEgC2AApnigrVQ= github.com/google/go-github/v38 v38.1.0 h1:C6h1FkaITcBFK7gAmq4eFzt6gbhEhk7L5z6R3Uva+po= github.com/google/go-github/v38 v38.1.0/go.mod h1:cStvrz/7nFr0FoENgG6GLbp53WaelXucT+BBz/3VKx4= github.com/google/go-querystring v1.0.0/go.mod h1:odCYkC5MyYFN7vkCjXpyrEuKhc/BUO6wN/zVPAxq5ck= diff --git a/internal/cli/validate.go b/internal/cli/validate.go index 0d2b3ba..f298186 100644 --- a/internal/cli/validate.go +++ b/internal/cli/validate.go @@ -3,6 +3,7 @@ package cli import ( "context" "fmt" + "net/http" "os" "strings" "time" @@ -25,6 +26,7 @@ var ( validateInvalSecond uint selfJobName string ignoredJobs string + githubClientRetry int ) func validateCmd() *cobra.Command { @@ -45,7 +47,12 @@ func validateCmd() *cobra.Command { return fmt.Errorf("github owner or repository is empty. owner: %s, repository: %s", owner, repo) } - statusValidator, err := status.CreateValidator(github.NewClient(ctx, ghToken), + t := http.DefaultTransport + if githubClientRetry > 0 { + t = github.NewRetryTransport(githubClientRetry) + } + statusValidator, err := status.CreateValidator( + github.NewClient(ctx, ghToken, github.WithTransport(t)), status.WithSelfJob(selfJobName), status.WithGitHubOwnerAndRepo(owner, repo), status.WithGitHubRef(ghRef), @@ -72,6 +79,8 @@ func validateCmd() *cobra.Command { cmd.PersistentFlags().StringVarP(&ignoredJobs, "ignored", "i", "", "set ignored jobs (comma-separated list)") + cmd.PersistentFlags().IntVar(&githubClientRetry, "github-client-retry", 0, "set retry count for GitHub client") + return cmd } diff --git a/internal/github/github.go b/internal/github/github.go index 53de468..712e86b 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -2,6 +2,7 @@ package github import ( "context" + "net/http" "github.com/google/go-github/v38/github" "golang.org/x/oauth2" @@ -25,17 +26,40 @@ type Client interface { ListCheckRunsForRef(ctx context.Context, owner, repo, ref string, opts *ListCheckRunsOptions) (*ListCheckRunsResults, *Response, error) } +type clientConfigOption func(*clientConfig) + type client struct { ghc *github.Client } -func NewClient(ctx context.Context, token string) Client { +type clientConfig struct { + transport http.RoundTripper +} + +func WithTransport(transport http.RoundTripper) clientConfigOption { + return func(c *clientConfig) { + c.transport = transport + } +} + +func NewClient(ctx context.Context, token string, opts ...clientConfigOption) Client { + clientConfig := &clientConfig{} + + for _, opt := range opts { + opt(clientConfig) + } + return &client{ - ghc: github.NewClient(oauth2.NewClient(ctx, oauth2.StaticTokenSource( - &oauth2.Token{ - AccessToken: token, + ghc: github.NewClient(&http.Client{ + Transport: &oauth2.Transport{ + Base: clientConfig.transport, + Source: oauth2.ReuseTokenSource(nil, oauth2.StaticTokenSource( + &oauth2.Token{ + AccessToken: token, + }, + )), }, - ))), + }), } } diff --git a/internal/github/retry_roundtripper.go b/internal/github/retry_roundtripper.go new file mode 100644 index 0000000..e6687f9 --- /dev/null +++ b/internal/github/retry_roundtripper.go @@ -0,0 +1,48 @@ +package github + +import ( + "log" + "math" + "net/http" + "time" +) + +type RetryTransport struct { + next http.RoundTripper + Retries int + log log.Logger +} + +func NewRetryTransport(retry int) *RetryTransport { + return &RetryTransport{ + next: http.DefaultTransport, + Retries: retry, + } +} + +func (t *RetryTransport) RoundTrip(req *http.Request) (*http.Response, error) { + var ( + resp *http.Response + err error + ) + + for i := 0; i <= t.Retries; i++ { + resp, err = t.next.RoundTrip(req) + if err != nil { + return resp, err + } + + if resp.StatusCode >= 500 && resp.StatusCode < 600 { + log.Printf("Retrying request due to status code: %d\n", resp.StatusCode) + delay := time.Duration(math.Pow(2, float64(i))) * time.Second + time.Sleep(delay) + + resp.Body.Close() + continue + } + + break + } + + return resp, err +} From 2b96649c0daea1f1446b4f61f17b41c879482497 Mon Sep 17 00:00:00 2001 From: Frank Bagherzadeh Date: Wed, 16 Aug 2023 14:42:11 -0400 Subject: [PATCH 2/2] make merge gatekeeper more resilient (#2) * ground works * cleanup * comments * use importer to update docs * tests * rerender --- README.md | 20 +++--- docs/action-usage.md | 17 ++--- internal/github/retry_roundtripper_test.go | 76 ++++++++++++++++++++++ 3 files changed, 95 insertions(+), 18 deletions(-) create mode 100644 internal/github/retry_roundtripper_test.go diff --git a/README.md b/README.md index c51c4c7..4d13b7d 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ At UPSIDER, we have a few internal repositories set up with a monorepo structure We are looking to add a few more features, such as extra signoff from non-coder, label based check, etc. -NOTE: +NOTE: (\*1) There are some other hacks, such as using an empty job with the same name to override the status, but those solutions do not provide the flexible control we are after. @@ -80,15 +80,15 @@ There are some customisation available for Merge Gatekeeper. -| Name | Description | Required | -| ---------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :------: | -| `token` | `GITHUB_TOKEN` or Personal Access Token with `repo` scope | Yes | -| `self` | The name of Merge Gatekeeper job, and defaults to `merge-gatekeeper`. This is used to check other job status, and do not check Merge Gatekeeper itself. If you updated the GitHub Action job name from `merge-gatekeeper` to something else, you would need to specify the new name with this value. | | -| `interval` | Check interval to recheck the job status. Default is set to 5 (sec). | | -| `github-client-retry` | Retry the request if the GitHub client returns 5xx error. Default is set to 0. | | -| `timeout` | Timeout setup to give up further check. Default is set to 600 (sec). | | -| `ignored` | Jobs to ignore regardless of their statuses. Defined as a comma-separated list. | | -| `ref` | Git ref to check out. This falls back to the HEAD for given PR, but can be set to any ref. | | +| Name | Description | Required | +| --------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :------: | +| `token` | `GITHUB_TOKEN` or Personal Access Token with `repo` scope | Yes | +| `self` | The name of Merge Gatekeeper job, and defaults to `merge-gatekeeper`. This is used to check other job status, and do not check Merge Gatekeeper itself. If you updated the GitHub Action job name from `merge-gatekeeper` to something else, you would need to specify the new name with this value. | | +| `interval` | Check interval to recheck the job status. Default is set to 5 (sec). | | +| `timeout` | Timeout setup to give up further check. Default is set to 600 (sec). | | +| `ignored` | Jobs to ignore regardless of their statuses. Defined as a comma-separated list. | | +| `ref` | Git ref to check out. This falls back to the HEAD for given PR, but can be set to any ref. | | +| `github-client-retry` | Retry the request if the GitHub client returns 5xx error. Default is set to 0. | | diff --git a/docs/action-usage.md b/docs/action-usage.md index d4cfcb1..b35c7b7 100644 --- a/docs/action-usage.md +++ b/docs/action-usage.md @@ -4,14 +4,15 @@ -| Name | Description | Required | -| ---------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :------: | -| `token` | `GITHUB_TOKEN` or Personal Access Token with `repo` scope | Yes | -| `self` | The name of Merge Gatekeeper job, and defaults to `merge-gatekeeper`. This is used to check other job status, and do not check Merge Gatekeeper itself. If you updated the GitHub Action job name from `merge-gatekeeper` to something else, you would need to specify the new name with this value. | | -| `interval` | Check interval to recheck the job status. Default is set to 5 (sec). | | -| `timeout` | Timeout setup to give up further check. Default is set to 600 (sec). | | -| `ignored` | Jobs to ignore regardless of their statuses. Defined as a comma-separated list. | | -| `ref` | Git ref to check out. This falls back to the HEAD for given PR, but can be set to any ref. | | +| Name | Description | Required | +| --------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :------: | +| `token` | `GITHUB_TOKEN` or Personal Access Token with `repo` scope | Yes | +| `self` | The name of Merge Gatekeeper job, and defaults to `merge-gatekeeper`. This is used to check other job status, and do not check Merge Gatekeeper itself. If you updated the GitHub Action job name from `merge-gatekeeper` to something else, you would need to specify the new name with this value. | | +| `interval` | Check interval to recheck the job status. Default is set to 5 (sec). | | +| `timeout` | Timeout setup to give up further check. Default is set to 600 (sec). | | +| `ignored` | Jobs to ignore regardless of their statuses. Defined as a comma-separated list. | | +| `ref` | Git ref to check out. This falls back to the HEAD for given PR, but can be set to any ref. | | +| `github-client-retry` | Retry the request if the GitHub client returns 5xx error. Default is set to 0. | | diff --git a/internal/github/retry_roundtripper_test.go b/internal/github/retry_roundtripper_test.go new file mode 100644 index 0000000..c95ec6f --- /dev/null +++ b/internal/github/retry_roundtripper_test.go @@ -0,0 +1,76 @@ +package github + +import ( + "math" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +func TestRoundTripSuccess(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + rt := NewRetryTransport(1) + rt.next = http.DefaultTransport + + req, err := http.NewRequest(http.MethodGet, srv.URL, nil) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + + startTime := time.Now() + resp, err := rt.RoundTrip(req) + elapsedTime := time.Since(startTime) + if err != nil { + t.Fatalf("error during request: %v", err) + } + + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Errorf("expected status code %d, got %d", http.StatusOK, resp.StatusCode) + } + + minElapsedTime := time.Second + if elapsedTime > minElapsedTime { + t.Errorf("expected elapsed time <= %v, got %v", minElapsedTime, elapsedTime) + } +} + +func TestRoundTrip(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "Service Unavailable", http.StatusServiceUnavailable) + })) + defer srv.Close() + + retryCount := 2 + rt := NewRetryTransport(retryCount) + rt.next = http.DefaultTransport + + req, err := http.NewRequest(http.MethodGet, srv.URL, nil) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + + startTime := time.Now() + resp, err := rt.RoundTrip(req) + elapsedTime := time.Since(startTime) + if err != nil { + t.Fatalf("error during request: %v", err) + } + + defer resp.Body.Close() + + if resp.StatusCode != http.StatusServiceUnavailable { + t.Errorf("expected status code %d, got %d", http.StatusServiceUnavailable, resp.StatusCode) + } + + minElapsedTime := time.Duration(math.Pow(2, float64(retryCount))) * time.Second + if elapsedTime < minElapsedTime { + t.Errorf("expected elapsed time >= %v, got %v", minElapsedTime, elapsedTime) + } +}