Skip to content

Commit f8fecd4

Browse files
iclantonclaude
andcommitted
Address dmichon-msft review comments
- Add clarifying comment on maxAttempts: 1 for uploads explaining why the parameter exists (shared between download with retries and upload without) - Replace S3 download snapshot containing auth headers with explicit field assertions, avoiding credential-looking strings in snapshots - Update inline snapshot for "unknown length" wording change Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 5abf492 commit f8fecd4

4 files changed

Lines changed: 14 additions & 20 deletions

File tree

rush-plugins/rush-amazon-s3-build-cache-plugin/src/test/AmazonS3Client.test.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -716,7 +716,13 @@ describe(AmazonS3Client.name, () => {
716716
);
717717
expect(result).toBe(true);
718718
expect(spy).toHaveBeenCalledTimes(1);
719-
expect(spy.mock.calls[0]).toMatchSnapshot();
719+
const [url, options] = spy.mock.calls[0];
720+
expect(url).toBe('http://localhost:9000/abc123');
721+
expect(options.verb).toBe('GET');
722+
expect(options.headers['x-amz-content-sha256']).toMatch(/^[0-9a-f]{64}$/);
723+
expect(options.headers['x-amz-date']).toBe('20200418T123242Z');
724+
// eslint-disable-next-line dot-notation
725+
expect(options.headers['Authorization']).toContain('AWS4-HMAC-SHA256');
720726
expect(pipeline).toHaveBeenCalled();
721727
spy.mockRestore();
722728
});

rush-plugins/rush-amazon-s3-build-cache-plugin/src/test/__snapshots__/AmazonS3Client.test.ts.snap

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -549,17 +549,3 @@ exports[`AmazonS3Client Rejects invalid S3 endpoint values 10`] = `"Invalid S3 e
549549

550550
exports[`AmazonS3Client Rejects invalid S3 endpoint values 11`] = `"Invalid S3 endpoint. Some part of the hostname contains invalid characters or is too long"`;
551551

552-
exports[`AmazonS3Client File-based requests Downloading an object to file Can download an object to file 1`] = `
553-
Array [
554-
"http://localhost:9000/abc123",
555-
Object {
556-
"headers": Object {
557-
"Authorization": "AWS4-HMAC-SHA256 Credential=accessKeyId/20200418/us-east-1/s3/aws4_request,SignedHeaders=host;x-amz-content-sha256;x-amz-date,Signature=194608e9e7ba6d8aa4a019b3b6fd237e6b09ef1f45ff7fa60cbb81c1875538be",
558-
"x-amz-content-sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
559-
"x-amz-date": "20200418T123242Z",
560-
},
561-
"verb": "GET",
562-
},
563-
]
564-
`;
565-

rush-plugins/rush-http-build-cache-plugin/src/HttpBuildCacheProvider.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,9 @@ export class HttpBuildCacheProvider implements ICloudBuildCacheProvider {
209209
method: this._uploadMethod,
210210
body: entryStream,
211211
warningText: 'Could not write cache entry',
212-
// Streaming uploads cannot be retried because the stream is consumed
212+
// maxAttempts is 1 because the file read stream is consumed after the first attempt
213+
// and cannot be replayed. Downloads use MAX_HTTP_CACHE_ATTEMPTS since each retry
214+
// issues a fresh GET with no request body.
213215
maxAttempts: 1
214216
});
215217

rush-plugins/rush-http-build-cache-plugin/src/test/HttpBuildCacheProvider.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ describe('HttpBuildCacheProvider', () => {
9797
);
9898
expect(terminalBuffer.getAllOutputAsChunks({ asLines: true })).toMatchInlineSnapshot(`
9999
Array [
100-
"[ debug] [http-build-cache] request: GET https://buildcache.example.acme.com/some-key unknown bytes[n]",
100+
"[ debug] [http-build-cache] request: GET https://buildcache.example.acme.com/some-key unknown length[n]",
101101
"[warning] Error getting cache entry: Error: Credentials for https://buildcache.example.acme.com/ have not been provided.[n]",
102102
"[warning] In CI, verify that RUSH_BUILD_CACHE_CREDENTIAL contains a valid Authorization header value.[n]",
103103
"[warning] [n]",
@@ -160,9 +160,9 @@ Array [
160160
);
161161
expect(terminalBuffer.getAllOutputAsChunks({ asLines: true })).toMatchInlineSnapshot(`
162162
Array [
163-
"[ debug] [http-build-cache] request: GET https://buildcache.example.acme.com/some-key unknown bytes[n]",
164-
"[ debug] [http-build-cache] request: GET https://buildcache.example.acme.com/some-key unknown bytes[n]",
165-
"[ debug] [http-build-cache] request: GET https://buildcache.example.acme.com/some-key unknown bytes[n]",
163+
"[ debug] [http-build-cache] request: GET https://buildcache.example.acme.com/some-key unknown length[n]",
164+
"[ debug] [http-build-cache] request: GET https://buildcache.example.acme.com/some-key unknown length[n]",
165+
"[ debug] [http-build-cache] request: GET https://buildcache.example.acme.com/some-key unknown length[n]",
166166
"[warning] Could not get cache entry: HTTP 504: Gateway Timeout[n]",
167167
]
168168
`);

0 commit comments

Comments
 (0)