10000 Retry cloud cache requests by ricsam · Pull Request #1 · ricsam/rushstack · GitHub
[go: up one dir, main page]
Skip to content

Retry cloud cache requests #1

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.

Sign up for GitHub

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

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Retry cloud cache requests #1

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ee86d08
[rush-amazon-s3-build-cache-plugin] add retry logic to AmazonS3Client
ricsam Feb 23, 2022
b0c5ebf
[rush-amazon-s3-build-cache-plugin] verify that retry logic works
ricsam Feb 23, 2022
773aacc
[rush-amazon-s3-build-cache-plugin] add tests to verify 400 and 401 r…
ricsam Feb 24, 2022
9df26d3
don't retry if 400 or 401 response code
ricsam Feb 24, 2022
28c9f02
handle 400, 401, 403 the same, where such response throws an error im…
ricsam Mar 3, 2022
aa436fc
Some code cleanup.
iclanton Mar 6, 2022
721a434
warn user if 403 error when no credentials are set
ricsam Mar 10, 2022
1803eaf
rush change
ricsam Mar 10, 2022
038bf87
Rush change.
iclanton Apr 1, 2022
9f5457a
disable eslint no-explicit-any used for mocked interface in test
ricsam Apr 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
handle 400, 401, 403 the same, where such response throws an error im… 
…mediately if credentials are provided. Without credentials the error is silenced, e.g. if someone builds rush locally without credentials
  • Loading branch information
@ricsam
ricsam committed Mar 3, 2022
commit 28c9f0283a62f3caf8bce78ca1f5022e65973f71
7 changes: 6 additions & 1 deletion rush-plugins/rush-amazon-s3-build-cache-plugin/src/AmazonS3Client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,12 @@ export class AmazonS3Client {
hasNetworkError: false,
response: undefined
};
} else if (response.status === 403 && !this._credentials) {
} else if (
(response.status === 400 || response.status === 401 || response.status === 403) &&
!this._credentials
) {
// unauthorized due to not providing credentials,
// silence error for better DX when e.g. running locally without credentials
return {
hasNetworkError: false,
response: undefined
Expand Down
129 changes: 67 additions & 62 deletions rush-plugins/rush-amazon-s3-build-cache-plugin/src/test/AmazonS3Client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ describe(AmazonS3Client.name, () => {
expect(spy).toHaveBeenCalledTimes(1);
}
expect(spy.mock.calls[0]).toMatchSnapshot();
spy.mockRestore();

if (error) {
throw error;
Expand All @@ -278,7 +279,7 @@ describe(AmazonS3Client.name, () => {
async function runAndExpectErrorAsync(fnAsync: () => Promise<unknown>): Promise<void> {
try {
await fnAsync();
fail('Expected an error to be thrown');
throw new Error('Expected an error to be thrown');
} catch (e) {
// The way an error is formatted changed in Node 16. Normalize, so snapshots match.
(e as Error).message = (e as Error).message.replace(
Expand Down Expand Up @@ -395,75 +396,79 @@ describe(AmazonS3Client.name, () => {
spy.mockRestore();
});

it('should not retry on 400 error', async () => {
const spy = jest.spyOn(global, 'setTimeout');
await runAndExpectErrorAsync(
async () =>
await makeGetRequestAsync(
credentials,
DUMMY_OPTIONS,
'abc123',
{
responseInit: {
status: 400,
statusText: 'Bad Request'
if (credentials) {
it('should not retry on 400 error', async () => {
const spy = jest.spyOn(global, 'setTimeout');
await runAndExpectErrorAsync(
async () =>
await makeGetRequestAsync(
credentials,
DUMMY_OPTIONS,
'abc123',
{
responseInit: {
status: 400,
statusText: 'Bad Request'
}
},
{
shouldRetry: false
}
},
{
shouldRetry: false
}
)
);
expect(setTimeout).toHaveBeenCalledTimes(0);
spy.mockReset();
spy.mockRestore();
});

it('should not retry on 401 error', async () => {
const spy = jest.spyOn(global, 'setTimeout');
await runAndExpectErrorAsync(
async () =>
await makeGetRequestAsync(
credentials,
DUMMY_OPTIONS,
'abc123',
{
responseInit: {
status: 401,
statusText: 'Unauthorized'
)
);
expect(setTimeout).toHaveBeenCalledTimes(0);
spy.mockReset();
spy.mockRestore();
});

it('should not retry on 401 error', async () => {
const spy = jest.spyOn(global, 'setTimeout');
await runAndExpectErrorAsync(
async () =>
await makeGetRequestAsync(
credentials,
DUMMY_OPTIONS,
'abc123',
{
responseInit: {
status: 401,
statusText: 'Unauthorized'
}
},
{
shouldRetry: false
}
},
{
shouldRetry: false
}
)
);
expect(setTimeout).toHaveBeenCalledTimes(0);
spy.mockReset();
spy.mockRestore();
});
)
);
expect(setTimeout).toHaveBeenCalledTimes(0);
spy.mockReset();
spy.mockRestore();
});
}
}

describe('Without credentials', () => {
registerGetTests(undefined);

it('Handles missing credentials object', async () => {
const result: Buffer | undefined = await makeGetRequestAsync(
undefined,
DUMMY_OPTIONS,
'abc123',
{
responseInit: {
status: 403,
statusText: 'Unauthorized'
for (const code of [400, 401, 403]) {
it(`Handles missing credentials object when ${code}`, async () => {
const result: Buffer | undefined = await makeGetRequestAsync(
undefined,
DUMMY_OPTIONS,
'abc123',
{
responseInit: {
status: code,
statusText: 'Unauthorized'
}
},
{
shouldRetry: false
}
},
{
shouldRetry: false
}
);
expect(result).toBeUndefined();
});
);
expect(result).toBeUndefined();
});
}
});

function registerGetWithCredentialsTests(credentials: IAmazonS3Credentials): void {
Expand Down
10 changes: 3 additions & 7 deletions ...gins/rush-amazon-s3-build-cache-plugin/src/test/__snapshots__/AmazonS3Client.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,7 @@ Array [

exports[`AmazonS3Client Making requests Getting an object Without credentials Handles an unexpected error 2`] = `[Error: Amazon S3 responded with status code 500 (Server Error)]`;

exports[`AmazonS3Client Making requests Getting an object Without credentials Handles missing credentials object 1`] = `
exports[`AmazonS3Client Making requests Getting an object Without credentials Handles missing credentials object when 400 1`] = `
Array [
"http://localhost:9000/abc123",
Object {
Expand All @@ -444,7 +444,7 @@ Array [
]
`;

exports[`AmazonS3Client Making requests Getting an object Without credentials should not retry on 400 error 1`] = `
exports[`AmazonS3Client Making requests Getting an object Without credentials Handles missing credentials object when 401 1`] = `
Array [
"http://localhost:9000/abc123",
Object {
Expand All @@ -463,9 +463,7 @@ Array [
]
`;

exports[`AmazonS3Client Making requests Getting an object Without credentials should not retry on 400 error 2`] = `[Error: Amazon S3 responded with status code 400 (Bad Request)]`;

exports[`AmazonS3Client Making requests Getting an object Without credentials should not retry on 401 error 1`] = `
exports[`AmazonS3Client Making requests Getting an object Without credentials Handles missing credentials object when 403 1`] = `
Array [
"http://localhost:9000/abc123",
Object {
Expand All @@ -484,8 +482,6 @@ Array [
]
`;

exports[`AmazonS3Client Making requests Getting an object Without credentials should not retry on 401 error 2`] = `[Error: Amazon S3 responded with status code 401 (Unauthorized)]`;

exports[`AmazonS3Client Making requests Uploading an object Throws an error if credentials are not provided 1`] = `[TypeError: Cannot read properties of undefined (reading 'body')]`;

exports[`AmazonS3Client Making requests Uploading an object With credentials Handles an unexpected error code 1`] = `
Expand Down
0