From 404ac94fc0e11aedab598938753d28ac969a699c Mon Sep 17 00:00:00 2001 From: kfcampbell Date: Mon, 20 Sep 2021 16:43:12 -0700 Subject: [PATCH 01/13] Make struct type public --- github/transport.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/github/transport.go b/github/transport.go index f7002d0266..202eb664df 100644 --- a/github/transport.go +++ b/github/transport.go @@ -45,17 +45,17 @@ func NewEtagTransport(rt http.RoundTripper) *etagTransport { return &etagTransport{transport: rt} } -// rateLimitTransport implements GitHub's best practices +// RateLimitTransport implements GitHub's best practices // for avoiding rate limits // https://developer.github.com/v3/guides/best-practices-for-integrators/#dealing-with-abuse-rate-limits -type rateLimitTransport struct { +type RateLimitTransport struct { transport http.RoundTripper delayNextRequest bool m sync.Mutex } -func (rlt *rateLimitTransport) RoundTrip(req *http.Request) (*http.Response, error) { +func (rlt *RateLimitTransport) RoundTrip(req *http.Request) (*http.Response, error) { // Make requests for a single user or client ID serially // This is also necessary for safely saving // and restoring bodies between retries below @@ -113,20 +113,20 @@ func (rlt *rateLimitTransport) RoundTrip(req *http.Request) (*http.Response, err return resp, nil } -func (rlt *rateLimitTransport) lock(req *http.Request) { +func (rlt *RateLimitTransport) lock(req *http.Request) { ctx := req.Context() log.Printf("[TRACE] Acquiring lock for GitHub API request (%q)", ctx.Value(ctxId)) rlt.m.Lock() } -func (rlt *rateLimitTransport) unlock(req *http.Request) { +func (rlt *RateLimitTransport) unlock(req *http.Request) { ctx := req.Context() log.Printf("[TRACE] Releasing lock for GitHub API request (%q)", ctx.Value(ctxId)) rlt.m.Unlock() } -func NewRateLimitTransport(rt http.RoundTripper) *rateLimitTransport { - return &rateLimitTransport{transport: rt} +func NewRateLimitTransport(rt http.RoundTripper) *RateLimitTransport { + return &RateLimitTransport{transport: rt} } // drainBody reads all of b to memory and then returns two equivalent From 5fcd21ebf05b2f4815e2f8a2f435fb6f85b0a402 Mon Sep 17 00:00:00 2001 From: kfcampbell Date: Mon, 20 Sep 2021 16:48:55 -0700 Subject: [PATCH 02/13] Scaffold functional pattern for providing writeDelay --- github/transport.go | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/github/transport.go b/github/transport.go index 202eb664df..14c485d8cd 100644 --- a/github/transport.go +++ b/github/transport.go @@ -13,9 +13,8 @@ import ( ) const ( - ctxEtag = ctxEtagType("etag") - ctxId = ctxIdType("id") - writeDelay = 1 * time.Second + ctxEtag = ctxEtagType("etag") + ctxId = ctxIdType("id") ) // ctxIdType is used to avoid collisions between packages using context @@ -51,6 +50,7 @@ func NewEtagTransport(rt http.RoundTripper) *etagTransport { type RateLimitTransport struct { transport http.RoundTripper delayNextRequest bool + writeDelay time.Duration m sync.Mutex } @@ -64,8 +64,8 @@ func (rlt *RateLimitTransport) RoundTrip(req *http.Request) (*http.Response, err // If you're making a large number of POST, PATCH, PUT, or DELETE requests // for a single user or client ID, wait at least one second between each request. if rlt.delayNextRequest { - log.Printf("[DEBUG] Sleeping %s between write operations", writeDelay) - time.Sleep(writeDelay) + log.Printf("[DEBUG] Sleeping %s between write operations", rlt.writeDelay) + time.Sleep(rlt.writeDelay) } rlt.delayNextRequest = isWriteMethod(req.Method) @@ -125,8 +125,18 @@ func (rlt *RateLimitTransport) unlock(req *http.Request) { rlt.m.Unlock() } -func NewRateLimitTransport(rt http.RoundTripper) *RateLimitTransport { - return &RateLimitTransport{transport: rt} +// NetRateLimitTransport takes in an http.RoundTripper and a variadic list of +// optional functions that modify the RateLimitTransport struct itself. This +// may be used to alter the write delay in between requests, for example. +func NewRateLimitTransport(rt http.RoundTripper, options ...func(*RateLimitTransport)) *RateLimitTransport { + // Default to 1 second of write delay if none is provided + rlt := &RateLimitTransport{transport: rt, writeDelay: 1 * time.Second} + + for _, opt := range options { + opt(rlt) + } + + return rlt } // drainBody reads all of b to memory and then returns two equivalent From 7d51a6a03a68d54cf2a3961e07e52b0e6144a9fa Mon Sep 17 00:00:00 2001 From: kfcampbell Date: Mon, 20 Sep 2021 16:58:26 -0700 Subject: [PATCH 03/13] Add WriteDelay to config and plumb it down --- github/config.go | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/github/config.go b/github/config.go index a994031448..b52aa4193b 100644 --- a/github/config.go +++ b/github/config.go @@ -6,6 +6,7 @@ import ( "net/http" "net/url" "path" + "time" "github.com/google/go-github/v38/github" "github.com/hashicorp/terraform-plugin-sdk/helper/logging" @@ -14,10 +15,11 @@ import ( ) type Config struct { - Token string - Owner string - BaseURL string - Insecure bool + Token string + Owner string + BaseURL string + Insecure bool + WriteDelay time.Duration } type Owner struct { @@ -29,14 +31,13 @@ type Owner struct { IsOrganization bool } -func RateLimitedHTTPClient(client *http.Client) *http.Client { +func RateLimitedHTTPClient(client *http.Client, writeDelay time.Duration) *http.Client { client.Transport = NewEtagTransport(client.Transport) client.Transport = NewRateLimitTransport(client.Transport) client.Transport = logging.NewTransport("Github", client.Transport) return client - } func (c *Config) AuthenticatedHTTPClient() *http.Client { @@ -47,8 +48,7 @@ func (c *Config) AuthenticatedHTTPClient() *http.Client { ) client := oauth2.NewClient(ctx, ts) - return RateLimitedHTTPClient(client) - + return RateLimitedHTTPClient(client, c.WriteDelay) } func (c *Config) Anonymous() bool { @@ -57,7 +57,7 @@ func (c *Config) Anonymous() bool { func (c *Config) AnonymousHTTPClient() *http.Client { client := &http.Client{Transport: &http.Transport{}} - return RateLimitedHTTPClient(client) + return RateLimitedHTTPClient(client, c.WriteDelay) } func (c *Config) NewGraphQLClient(client *http.Client) (*githubv4.Client, error) { @@ -74,7 +74,6 @@ func (c *Config) NewGraphQLClient(client *http.Client) (*githubv4.Client, error) } return githubv4.NewEnterpriseClient(uv4.String(), client), nil - } func (c *Config) NewRESTClient(client *http.Client) (*github.Client, error) { @@ -96,7 +95,6 @@ func (c *Config) NewRESTClient(client *http.Client) (*github.Client, error) { v3client.BaseURL = uv3 return v3client, nil - } func (c *Config) ConfigureOwner(owner *Owner) (*Owner, error) { From 7145af144508044630b0e62af2736eeeb9c3eba5 Mon Sep 17 00:00:00 2001 From: kfcampbell Date: Mon, 20 Sep 2021 18:39:13 -0700 Subject: [PATCH 04/13] Add configuration for write delay --- github/provider.go | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/github/provider.go b/github/provider.go index d9922f45e3..c4bc31db54 100644 --- a/github/provider.go +++ b/github/provider.go @@ -3,6 +3,7 @@ package github import ( "fmt" "log" + "time" "github.com/hashicorp/terraform-plugin-sdk/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/terraform" @@ -42,6 +43,12 @@ func Provider() terraform.ResourceProvider { Default: false, Description: descriptions["insecure"], }, + "write_delay": { + Type: schema.TypeInt, + Optional: true, + Default: 1000, + Description: descriptions["write_delay"], + }, "app_auth": { Type: schema.TypeList, Optional: true, @@ -156,6 +163,7 @@ func init() { "app_auth.id": "The GitHub App ID.", "app_auth.installation_id": "The GitHub App installation instance ID.", "app_auth.pem_file": "The GitHub App PEM file contents.", + "write_delay": "Amount of time in milliseconds to sleep in between writes to GitHub REST API.", } } @@ -219,11 +227,15 @@ func providerConfigure(p *schema.Provider) schema.ConfigureFunc { token = appToken } + writeDelay := d.Get("write_delay").(int) + log.Printf("[DEBUG] Setting write_delay to %d", writeDelay) + config := Config{ - Token: token, - BaseURL: baseURL, - Insecure: insecure, - Owner: owner, + Token: token, + BaseURL: baseURL, + Insecure: insecure, + Owner: owner, + WriteDelay: time.Duration(writeDelay) * time.Millisecond, } meta, err := config.Meta() From 7616ff5827702cefc28387f27bbee43522f8ca76 Mon Sep 17 00:00:00 2001 From: kfcampbell Date: Mon, 20 Sep 2021 20:30:44 -0700 Subject: [PATCH 05/13] Validate writeDelay is positive --- github/provider.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/github/provider.go b/github/provider.go index c4bc31db54..fbe25d37f0 100644 --- a/github/provider.go +++ b/github/provider.go @@ -228,6 +228,9 @@ func providerConfigure(p *schema.Provider) schema.ConfigureFunc { } writeDelay := d.Get("write_delay").(int) + if writeDelay <= 0 { + return nil, fmt.Errorf("write_delay must be greater than 0ms") + } log.Printf("[DEBUG] Setting write_delay to %d", writeDelay) config := Config{ From 679e2e3255f390d99e7269db5b7416811a9ad05f Mon Sep 17 00:00:00 2001 From: kfcampbell Date: Mon, 20 Sep 2021 21:10:51 -0700 Subject: [PATCH 06/13] Add website description of write_delay argument --- website/docs/index.html.markdown | 2 ++ 1 file changed, 2 insertions(+) diff --git a/website/docs/index.html.markdown b/website/docs/index.html.markdown index 97002c65db..e0183d1ed1 100644 --- a/website/docs/index.html.markdown +++ b/website/docs/index.html.markdown @@ -107,6 +107,8 @@ The following arguments are supported in the `provider` block: * `installation_id` - (Required) This is the ID of the GitHub App installation. It can sourced from the `GITHUB_APP_INSTALLATION_ID` environment variable. * `pem_file` - (Required) This is the contents of the GitHub App private key PEM file. It can also be sourced from the `GITHUB_APP_PEM_FILE` environment variable. +* `write_delay` - (Optional) The number of milliseconds to sleep in between write operations in order to satisfy the GitHub REST API rate limits. Defaults to 1000ms or 1 second if not provided. + Note: If you have a PEM file on disk, you can pass it in via `pem_file = file("path/to/file.pem")`. For backwards compatibility, if more than one of `owner`, `organization`, From 6e8e35042f83de2023e9ecf6e2b90d18057f6b61 Mon Sep 17 00:00:00 2001 From: kfcampbell Date: Tue, 21 Sep 2021 17:54:33 -0700 Subject: [PATCH 07/13] Use convenience func to pass writeDelay into rate limiter --- github/config.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/github/config.go b/github/config.go index b52aa4193b..3ac83da1b3 100644 --- a/github/config.go +++ b/github/config.go @@ -33,8 +33,13 @@ type Owner struct { func RateLimitedHTTPClient(client *http.Client, writeDelay time.Duration) *http.Client { + // convenience func to set the write delay on the rate limiter + setWriteDelay := func(rlt *RateLimitTransport) { + rlt.writeDelay = writeDelay + } + client.Transport = NewEtagTransport(client.Transport) - client.Transport = NewRateLimitTransport(client.Transport) + client.Transport = NewRateLimitTransport(client.Transport, setWriteDelay) client.Transport = logging.NewTransport("Github", client.Transport) return client From 213c1df795ebe0b42cabb516ae2fd50fdcffee57 Mon Sep 17 00:00:00 2001 From: kfcampbell Date: Fri, 24 Sep 2021 15:12:40 -0700 Subject: [PATCH 08/13] Move convenience func closer to implementation; add convenience type --- github/config.go | 7 +------ github/transport.go | 11 ++++++++++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/github/config.go b/github/config.go index 3ac83da1b3..1b9f0e92de 100644 --- a/github/config.go +++ b/github/config.go @@ -33,13 +33,8 @@ type Owner struct { func RateLimitedHTTPClient(client *http.Client, writeDelay time.Duration) *http.Client { - // convenience func to set the write delay on the rate limiter - setWriteDelay := func(rlt *RateLimitTransport) { - rlt.writeDelay = writeDelay - } - client.Transport = NewEtagTransport(client.Transport) - client.Transport = NewRateLimitTransport(client.Transport, setWriteDelay) + client.Transport = NewRateLimitTransport(client.Transport, WithWriteDelay(writeDelay)) client.Transport = logging.NewTransport("Github", client.Transport) return client diff --git a/github/transport.go b/github/transport.go index 14c485d8cd..4b91b2f976 100644 --- a/github/transport.go +++ b/github/transport.go @@ -125,10 +125,12 @@ func (rlt *RateLimitTransport) unlock(req *http.Request) { rlt.m.Unlock() } +type RateLimitTransportOption func(*RateLimitTransport) + // NetRateLimitTransport takes in an http.RoundTripper and a variadic list of // optional functions that modify the RateLimitTransport struct itself. This // may be used to alter the write delay in between requests, for example. -func NewRateLimitTransport(rt http.RoundTripper, options ...func(*RateLimitTransport)) *RateLimitTransport { +func NewRateLimitTransport(rt http.RoundTripper, options ...RateLimitTransportOption) *RateLimitTransport { // Default to 1 second of write delay if none is provided rlt := &RateLimitTransport{transport: rt, writeDelay: 1 * time.Second} @@ -139,6 +141,13 @@ func NewRateLimitTransport(rt http.RoundTripper, options ...func(*RateLimitTrans return rlt } +// WithWriteDelay is used to set the write delay between requests +func WithWriteDelay(d time.Duration) RateLimitTransportOption { + return func(rlt *RateLimitTransport) { + rlt.writeDelay = d + } +} + // drainBody reads all of b to memory and then returns two equivalent // ReadClosers yielding the same bytes. func drainBody(b io.ReadCloser) (r1, r2 io.ReadCloser, err error) { From 41f3cef1dd212abeb26c8373007c868aba867e77 Mon Sep 17 00:00:00 2001 From: kfcampbell Date: Fri, 24 Sep 2021 15:15:08 -0700 Subject: [PATCH 09/13] Rename write_delay --> write_delay_ms --- github/provider.go | 12 ++++++------ website/docs/index.html.markdown | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/github/provider.go b/github/provider.go index f04c15637d..5f34ad2188 100644 --- a/github/provider.go +++ b/github/provider.go @@ -43,11 +43,11 @@ func Provider() terraform.ResourceProvider { Default: false, Description: descriptions["insecure"], }, - "write_delay": { + "write_delay_ms": { Type: schema.TypeInt, Optional: true, Default: 1000, - Description: descriptions["write_delay"], + Description: descriptions["write_delay_ms"], }, "app_auth": { Type: schema.TypeList, @@ -164,7 +164,7 @@ func init() { "app_auth.id": "The GitHub App ID.", "app_auth.installation_id": "The GitHub App installation instance ID.", "app_auth.pem_file": "The GitHub App PEM file contents.", - "write_delay": "Amount of time in milliseconds to sleep in between writes to GitHub REST API.", + "write_delay_ms": "Amount of time in milliseconds to sleep in between writes to GitHub REST API.", } } @@ -228,11 +228,11 @@ func providerConfigure(p *schema.Provider) schema.ConfigureFunc { token = appToken } - writeDelay := d.Get("write_delay").(int) + writeDelay := d.Get("write_delay_ms").(int) if writeDelay <= 0 { - return nil, fmt.Errorf("write_delay must be greater than 0ms") + return nil, fmt.Errorf("write_delay_ms must be greater than 0ms") } - log.Printf("[DEBUG] Setting write_delay to %d", writeDelay) + log.Printf("[DEBUG] Setting write_delay_ms to %d", writeDelay) config := Config{ Token: token, diff --git a/website/docs/index.html.markdown b/website/docs/index.html.markdown index e0183d1ed1..0fd0cf479d 100644 --- a/website/docs/index.html.markdown +++ b/website/docs/index.html.markdown @@ -107,7 +107,7 @@ The following arguments are supported in the `provider` block: * `installation_id` - (Required) This is the ID of the GitHub App installation. It can sourced from the `GITHUB_APP_INSTALLATION_ID` environment variable. * `pem_file` - (Required) This is the contents of the GitHub App private key PEM file. It can also be sourced from the `GITHUB_APP_PEM_FILE` environment variable. -* `write_delay` - (Optional) The number of milliseconds to sleep in between write operations in order to satisfy the GitHub REST API rate limits. Defaults to 1000ms or 1 second if not provided. +* `write_delay_ms` - (Optional) The number of milliseconds to sleep in between write operations in order to satisfy the GitHub REST API rate limits. Defaults to 1000ms or 1 second if not provided. Note: If you have a PEM file on disk, you can pass it in via `pem_file = file("path/to/file.pem")`. From 855162510296dd7da533b4c8e6cb5fd469e6a943 Mon Sep 17 00:00:00 2001 From: Keegan Campbell Date: Tue, 28 Sep 2021 22:52:49 -0700 Subject: [PATCH 10/13] Update github/provider.go Co-authored-by: Jeremy Udit --- github/provider.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github/provider.go b/github/provider.go index 5f34ad2188..a9ba5b79b5 100644 --- a/github/provider.go +++ b/github/provider.go @@ -164,7 +164,7 @@ func init() { "app_auth.id": "The GitHub App ID.", "app_auth.installation_id": "The GitHub App installation instance ID.", "app_auth.pem_file": "The GitHub App PEM file contents.", - "write_delay_ms": "Amount of time in milliseconds to sleep in between writes to GitHub REST API.", + "write_delay_ms": "Amount of time in milliseconds to sleep in between writes to GitHub API.", } } From edc347df9b34ed15a014aa564e919dfc6a75efb6 Mon Sep 17 00:00:00 2001 From: kfcampbell Date: Tue, 28 Sep 2021 22:54:23 -0700 Subject: [PATCH 11/13] Describe default for write_delay_ms in help text --- github/provider.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/github/provider.go b/github/provider.go index a9ba5b79b5..abe98c1bd7 100644 --- a/github/provider.go +++ b/github/provider.go @@ -164,7 +164,8 @@ func init() { "app_auth.id": "The GitHub App ID.", "app_auth.installation_id": "The GitHub App installation instance ID.", "app_auth.pem_file": "The GitHub App PEM file contents.", - "write_delay_ms": "Amount of time in milliseconds to sleep in between writes to GitHub API.", + "write_delay_ms": "Amount of time in milliseconds to sleep in between writes to GitHub API. " + + "Defaults to 1000ms or 1s if not set.", } } From 38da0715ad35418c7c76ab370a7137e0030ac298 Mon Sep 17 00:00:00 2001 From: Keegan Campbell Date: Tue, 28 Sep 2021 23:02:38 -0700 Subject: [PATCH 12/13] Update github/transport.go Co-authored-by: Jeremy Udit --- github/transport.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github/transport.go b/github/transport.go index 4b91b2f976..9bebc8161a 100644 --- a/github/transport.go +++ b/github/transport.go @@ -127,7 +127,7 @@ func (rlt *RateLimitTransport) unlock(req *http.Request) { type RateLimitTransportOption func(*RateLimitTransport) -// NetRateLimitTransport takes in an http.RoundTripper and a variadic list of +// NewRateLimitTransport takes in an http.RoundTripper and a variadic list of // optional functions that modify the RateLimitTransport struct itself. This // may be used to alter the write delay in between requests, for example. func NewRateLimitTransport(rt http.RoundTripper, options ...RateLimitTransportOption) *RateLimitTransport { From 63a47332a97d5cc523dd228a0838901e2b375fcd Mon Sep 17 00:00:00 2001 From: Keegan Campbell Date: Tue, 28 Sep 2021 23:02:53 -0700 Subject: [PATCH 13/13] Update website/docs/index.html.markdown Co-authored-by: Jeremy Udit --- website/docs/index.html.markdown | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/docs/index.html.markdown b/website/docs/index.html.markdown index 0fd0cf479d..beef7ed553 100644 --- a/website/docs/index.html.markdown +++ b/website/docs/index.html.markdown @@ -107,7 +107,7 @@ The following arguments are supported in the `provider` block: * `installation_id` - (Required) This is the ID of the GitHub App installation. It can sourced from the `GITHUB_APP_INSTALLATION_ID` environment variable. * `pem_file` - (Required) This is the contents of the GitHub App private key PEM file. It can also be sourced from the `GITHUB_APP_PEM_FILE` environment variable. -* `write_delay_ms` - (Optional) The number of milliseconds to sleep in between write operations in order to satisfy the GitHub REST API rate limits. Defaults to 1000ms or 1 second if not provided. +* `write_delay_ms` - (Optional) The number of milliseconds to sleep in between write operations in order to satisfy the GitHub API rate limits. Defaults to 1000ms or 1 second if not provided. Note: If you have a PEM file on disk, you can pass it in via `pem_file = file("path/to/file.pem")`.