k1LoW / awspec

RSpec tests for your AWS resources.
MIT License
1.17k stars 192 forks source link

Configure Error::RequestLimitExceeded backoff in ClientWrap #477

Closed amaltson closed 5 years ago

amaltson commented 5 years ago

We've been running into a few rate limit exceeded issues with some of our tests, and I starting poking around and noticed there was a PR almost 2 years ago that added a backoff and retry mechanism, which is awesome! Looking at the code that's there, the attributes to configure the backoff timing have some defaults and seem to provide a mechanism to configure them:

https://github.com/k1LoW/awspec/blob/d73417b6a61bc258681af37f52cca95e0c55be56/lib/awspec/helper/client_wrap.rb#L7-L11

Looking at the only place where ClientWrap is instantiated, it takes it CLIENT_OPTIONS as an argument:

https://github.com/k1LoW/awspec/blob/e61fbad3c9f877f1b119f1e4c46e75db20548b03/lib/awspec/helper/finder.rb#L159

But CLIENT_OPTIONS is only allowing http_proxy/https_proxy to be configured:

https://github.com/k1LoW/awspec/blob/e61fbad3c9f877f1b119f1e4c46e75db20548b03/lib/awspec/helper/finder.rb#L150-L152

Is there any other mechanism to configure ClientWrap that I'm missing? Or is it hard coded?

Would love to send a PR but want to check before doing so, thanks again for an amazing tool!

k1LoW commented 5 years ago

Hi @amaltson . Thank you for your report!

Is there any other mechanism to configure ClientWrap that I'm missing? Or is it hard coded?

is it hard coded.

Awspec::Helper::ClientWrap allow args to configure. https://github.com/k1LoW/awspec/blob/d73417b6a61bc258681af37f52cca95e0c55be56/lib/awspec/helper/client_wrap.rb#L4

but not used.

Would love to send a PR but want to check before doing so, thanks again for an amazing tool!

Thank you !

amaltson commented 5 years ago

@k1LoW I finally have a bit of time to work on this (happy Hacktoberfest haha), and wanted to run the design by you a bit.

I'm thinking that in order to allow configuration of the ClientWrapper, I don't want to introduce new environment variables in the CLIENT_OPTIONS configuration, that doesn't feel very "RSpec" like https://github.com/k1LoW/awspec/blob/e61fbad3c9f877f1b119f1e4c46e75db20548b03/lib/awspec/helper/finder.rb#L150-L152

Instead, I'm thinking to introduce something like RSpec's configuration block and allow it to be accessible in the same way. In order to make it a top level change, I'll add a new config folder and overload the Awspec module there adding a new configure method that takes in a block that we'll instance_eval.

From a customer's prospective it'll look something like this:

# In the `spec_helper.rb`
require 'awspec'

Awspec.configure do |config|
  config.client_iteration 5
  config.client_backoff_limit 60
end

And within the finder.rb we'd read off that configuration and pass it to the ClientWrapper. What do you think?

k1LoW commented 5 years ago

@amaltson

From a customer's prospective it'll look something like this:

GREAT !!! I like it !!

amaltson commented 5 years ago

:tada: glad I checked with you, thanks! I’ll send over a PR in the next couple days.