Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert ENV['AWS_MAX_ATTEMPTS'] string value to int #2319

Merged
merged 4 commits into from May 29, 2020

Conversation

jesseadams
Copy link
Contributor

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

With ruby 2.7.1 I am unable to set AWS_MAX_ATTEMPTS to a custom value with an environment variable.

# Wont work because it is a String and not an Integer
irb(main):001:0> ENV['AWS_MAX_ATTEMPTS'] = '300'

# Also fails with TypeError
irb(main):002:0> ENV['AWS_MAX_ATTEMPTS'] = 300
Traceback (most recent call last):
        5: from /usr/bin/irb:23:in `<main>'
        4: from /usr/bin/irb:23:in `load'
        3: from /usr/lib/ruby/gems/2.7.0/gems/irb-1.2.1/exe/irb:11:in `<top (required)>'
        2: from (irb):2
        1: from (irb):2:in `[]='
TypeError (no implicit conversion of Integer into String)

This was likely missed with tests because of the stubbing of ENV here: https://github.com/aws/aws-sdk-ruby/blob/master/gems/aws-sdk-core/spec/shared_spec_helper.rb#L19

This simple change keeps tests passing and will enable you to set AWS_MAX_ATTEMPTS to a String in ruby, which then gets converted into an integer when resolved.

Thanks for your time!

@mullermp
Copy link
Contributor

mullermp commented May 28, 2020

Thanks for opening this pull request. I think you can call .to_i directly instead of using && with another expression.

@mullermp
Copy link
Contributor

I would also want to fix the test to catch this error before merging, too.

@jesseadams
Copy link
Contributor Author

@mullermp - We only want to convert it to an integer if it is set. If it is unset, nil will convert to 0 and then fallback methods of resolving the value won't be tried. Unless I am incorrect?

@mullermp
Copy link
Contributor

@mullermp - We only want to convert it to an integer if it is set. If it is unset, nil will convert to 0 and then fallback methods of resolving the value won't be tried. Unless I am incorrect?

Ah yeah! Sorry about that.

@jesseadams
Copy link
Contributor Author

@mullermp - I updated the tests to not stub ENV for the three related tests in question. Before my change:

Finished in 31.91 seconds (files took 1.67 seconds to load)
1224 examples, 1 failure

Failed examples:

rspec ./spec/aws/plugins/retry_errors_spec.rb:88 # Aws::Plugins::RetryErrors ENV is not stubbed can configure max_attempts using ENV with precedence over config

After my change:

Finished in 32.16 seconds (files took 1.69 seconds to load)
1224 examples, 0 failures

@mullermp
Copy link
Contributor

Thanks for taking the initiative on this. Please see my feedback. :) Happy to also contribute here.

@jesseadams
Copy link
Contributor Author

jesseadams commented May 28, 2020

@mullermp - I am totally open to you collaborating on this. I am out of time today but I can take another look tomorrow.

Regarding the test cases: '1' and '-1' make sense. 1 does not as you will immediately get a TypeError in ruby when you try to set the value. I don't think this is a useful test for the scope of this utility. We also have test case for 'string' which also makes sense.

I was just trying to make the least amount of impact/changes while still being able to prove out the failure and my fix. If you have a preferred refactor or approach to accomplish this I am more than willing to give it a shot or let you hijack this PR and take it over the finish line. My ruby is honestly fairly rusty these days.

Thanks!

@mullermp
Copy link
Contributor

No worries! I went ahead and updated your branch. I think these changes are less intrusive. It was unfortunate on our part that ENV was stubbed as a hash, allowing integers to be set when in reality they never could be. It'd be better instead of using ENV.clear to stub ENV somehow but I wasn't sure how to construct a new ENV object. I thought about using double() but I would have to clear ENV anyway.

@mullermp mullermp requested review from mullermp, alextwoods and a team and removed request for mullermp and alextwoods May 29, 2020 17:10
@jesseadams
Copy link
Contributor Author

@mullermp - Awesome, thanks for your prompt work on this. I appreciate it.

@mullermp
Copy link
Contributor

Thanks for the contribution! (and for opening this issue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants