-
Notifications
You must be signed in to change notification settings - Fork 379
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
feat: Enable Retries For Auth Requests #1791
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only thing I see is a linter warning (not sure if this was pre-existing).
*/ | ||
protected async refreshToken( | ||
refreshToken?: string | null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, yes, however since its introduction a few years ago I think this would cause more confusion for customers rather than cleaning it up:
I included this change in the current PR as it was causing a lint issue.
@@ -401,12 +401,12 @@ describe('AwsClient', () => { | |||
// Simulate error during region retrieval. | |||
const scope = nock(metadataBaseUrl) | |||
.get('/latest/meta-data/placement/availability-zone') | |||
.reply(500); | |||
.reply(400); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were the status codes here all changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically 500s are retryable and tests would fail with an unexpected error. As the tests want a error on the first call, I changed it to 400.
Fixes #1366
Fixes #1779
Fixes #1780
Fixes #1785
Provides relief for:
Related:
Follow-up:
🦕