fix(2457): add retry logic to EcsCredentialProvider #2477
Conversation
annuh
changed the title
|
I couldn't run the tests locally in PHP8 (#2403) With these steps it worked:
+ "config": {
+ "platform": {
+ "php": "7.4"
+ }
+ }
<php>
+ <ini name="memory_limit" value="1024M" />
...
</php>
|
| $this->timeout = (float) getenv(self::ENV_TIMEOUT) | ||
| ?: (isset($config['timeout']) | ||
| ? $config['timeout'] | ||
| : 1.0); | ||
| $this->retries = (int) getenv(self::ENV_RETRIES) | ||
| ?: (isset($config['retries']) | ||
| ? $config['retries'] | ||
| : 3); |
There was a problem hiding this comment.
Personally, I think it's more logical to to use a different fallback here:
- Use
$config['timeout'], otherwise - Use
getenv(self::ENV_TIMEOUT), otherwise - Fallback to
1.0
Some environment variables may already be provided, outside of the application. So if you need multiple ECS credential providers, there is no way to set these configs per provider?
| $this->timeout = (float) getenv(self::ENV_TIMEOUT) | |
| ?: (isset($config['timeout']) | |
| ? $config['timeout'] | |
| : 1.0); | |
| $this->retries = (int) getenv(self::ENV_RETRIES) | |
| ?: (isset($config['retries']) | |
| ? $config['retries'] | |
| : 3); | |
| $this->timeout = (int) isset($config['timeout']) | |
| ? $config['timeout'] | |
| : (getenv(self::ENV_TIMEOUT) ?: 1.0); | |
| $this->retries = (int) isset($config['retries']) | |
| ? $config['retries'] | |
| : (getenv(self::ENV_RETRIES) ?: 3); |
What do you think?
There was a problem hiding this comment.
I agree that looking at the most explicit (config) first should be the way to go here. Let's go with that and try to incorporate the null coalescing operator now that we've deprecated < 7.2
|
@stobrien89 can you please take another look at this PR? 🙂 |
|
Hi @annuh, Haven't forgotten about this. We've been sidetracked with some major work on core functionality— should have the capacity to give this a thorough review by the middle of next week. |
|
@stobrien89 Do you have time to look at this? |
|
Hi all, Apologies for the delay. We're about to merge #2776 , which adds additional logic to make this more of a generalized "container provider", so I can take another pass at this while we're on the topic. |
@stobrien89 I fixed the merge conflicts and applied your suggestions. This PR is ready for review now 🙂 |
|
Had to close and re-open to trigger the CI. Will update you once reviewed! |
|
@stobrien89 any updates? |
|
Hello @stobrien89, I hope you are doing well. Do you have a timeline for us when we can expect this PR to be merged? We would greatly appreciate if a timeline can be shared with us. |
…voke` method is called.
…-ecs-credentials-provider Fixes for issue 2457 support retries ecs credentials provider
|
|
||
| public function testReadsRetriesFromEnvironment() | ||
| { | ||
| putenv('AWS_METADATA_SERVICE_NUM_ATTEMPTS=1'); |
There was a problem hiding this comment.
This needs to be cleaned up— it's being read by the InstanceProfileProvider tests 🙃
| $this->retries = (int) isset($config['retries']) | ||
| ? $config['retries'] | ||
| : ((int) getenv(self::ENV_RETRIES) ?: self::DEFAULT_ENV_RETRIES); | ||
| $this->attempts = 0; |
There was a problem hiding this comment.
Not a big deal, but this could be removed from the constructor
| * @param array $creds | ||
| * @return \Closure | ||
| */ | ||
| private function getTestClient( |
There was a problem hiding this comment.
Can you move this to the bottom of the file?
|
Thanks for the feedback @stobrien89! Thank you for the clear communication! Have a nice day 😄 |
…ntials-provider Updated code to line up with latest comments and expectations.
|
Goodmorning @stobrien89, Hope you had a good weekend. I am guessing you are busy with other thing which is fine, but I am wondering if you happen to have a timeline for when we can expect this PR to be merged. Have a nice day. |
|
Thanks for merging @stobrien89! Have an amazing day😄 |
Fixes #2457
This adds retry-logic for EcsCredentialsProvider. I re-used most of the logic from the InstanceProfileCredentialsProvider.