Add way to opt in to uploading assets with signed URLs #328
Conversation
lencioni
force-pushed
the
signed-urls
branch
2 times, most recently
from
c73ae85 to
c1d1f5b
Compare
This will allow folks to opt in to using signed URLs by setting the HAPPO_SIGNED_URL env var. This requires a separate change to land on the server side before it can be used. To support this change, I ended up doing some bigger refactoring to the react integration test. This change should make the test a lot more realistic, because it now avoids mocking the makeRequest method in favor of spinning up a server that responds with mock responses.
lencioni
force-pushed
the
signed-urls
branch
from
c1d1f5b to
598d0d5
Compare
| @@ -252,7 +351,8 @@ describe('with multiple targets', () => { | |||
|
|
|||
| it('resolves assets correctly', async () => { | |||
| await subject(); | |||
| const zip = new AdmZip(config.targets.chrome.assetsPackage); | |||
There was a problem hiding this comment.
This was pretty confusing for me, but I believe that the previous version of the tests were cramming in the location of the local zip file in the API response, which the domRunner would then eventually store in the chrome target under the assetsPackage property.
In this commit I ended up changing it so the mock server responds in the same way that the actual server would, which means that the path in the response is now where it is stored on S3. And then to find the zip file, I'm looking in the formData that was posted in the request more directly.
This makes these tests more realistic and removes a number of layers of confusing indirection.
| // Upload the assets to the signed URL using node's built-in fetch. | ||
| const putRes = await fetch(signedUrlRes.signedUrl, { | ||
| method: 'PUT', | ||
| body: streamOrBuffer, | ||
| headers: { | ||
| 'Content-Type': 'application/zip', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Do we want to retry this request if it fails?
| if (process.env.HAPPO_SIGNED_URL) { | ||
| return uploadAssetsWithSignedUrl(streamOrBuffer, options); | ||
| } |
There was a problem hiding this comment.
I am doing this as an opt-in for now. Once we are confident with this approach, we can roll it out as the default option or perhaps the only option.
Add way to opt in to uploading assets with signed URLs
This will allow folks to opt in to using signed URLs by setting the
HAPPO_SIGNED_URL env var.
This requires a separate change to land on the server side before it can
be used.
To support this change, I ended up doing some bigger refactoring to the
react integration test. This change should make the test a lot more
realistic, because it now avoids mocking the makeRequest method in favor
of spinning up a server that responds with mock responses.