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

Replace unused maxRetries with maxAttempts #2709

Open
trivikr opened this issue Aug 25, 2021 · 2 comments
Open

Replace unused maxRetries with maxAttempts #2709

trivikr opened this issue Aug 25, 2021 · 2 comments
Assignees
Labels
bug This issue is a bug. p3 This is a minor priority issue queued This issues is on the AWS team's backlog

Comments

@trivikr
Copy link
Member

trivikr commented Aug 25, 2021

Describe the bug

The maxRetries configuration was added in remote credentials provider back in Jun 15, 2017 in
d2b251e

The configuration maxRetries was changed to maxAttempts in middleware-retry to be compliant with other SDKs and retry strategy #1244

This change wasn't done in credential providers though.

Proposed fix

  • Add new configuration maxAttempts in RemoteProviderConfig, so that it reuses the value from retry while fetching credentials.
  • Instead of deleting existing maxRetries, call it deprecated in favor of maxAttempts.

Additional context

The bug was noticed while debugging retry strategy options for RemoteProviderConfig requested in #2706

@trivikr trivikr added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 25, 2021
@vudh1 vudh1 self-assigned this Nov 8, 2021
@vudh1 vudh1 removed their assignment Nov 15, 2021
@ajredniwja ajredniwja added needs-review This issue/pr needs review from an internal developer. and removed needs-triage This issue or PR still needs to be triaged. labels Jun 24, 2022
@ajredniwja ajredniwja added the p2 This is a standard priority issue label Aug 15, 2022
@yenfryherrerafeliz yenfryherrerafeliz added queued This issues is on the AWS team's backlog and removed needs-review This issue/pr needs review from an internal developer. labels May 3, 2023
@ajredniwja ajredniwja added p3 This is a minor priority issue and removed p2 This is a standard priority issue labels Sep 18, 2023
@andre-araujo
Copy link

@trivikr
Copy link
Member Author

trivikr commented Jul 23, 2024

The internal utility providerConfigFromInit is called from:

These utilities are further called from other providers or re-exported, like:

The ask for this issue is to remove maxRetries configuration from credential providers, as part of standardization, since we use maxAttempts in clients. It requires three things to be done:

  • Deprecate maxRetries in init used by fromContainerMetadata and getInstanceMetadataProvider in favor of maxAttempts
  • Add JSDoc that maxAttempts is recommended, and it's value if one more than maxRetries
  • In resolving the values, like in providerConfigFromInit favor value in maxAttempts over that in maxRetries, and use it for any attempts made in internal code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p3 This is a minor priority issue queued This issues is on the AWS team's backlog
Projects
None yet
Development

No branches or pull requests

5 participants