Use a stream when uploading database contents - #1465
robertbrignull merged 3 commits into
Conversation
Sorry, something went wrong.
|
I tested this in another repo and the database upload was successful. |
Sorry, something went wrong.
|
Hm, looks like there's a genuine unit test failure related to the Windows file system 👁️ |
Sorry, something went wrong.
|
Looks reasonable to me. @angelapwen is right. I wonder if you need to explicitly close the read stream before you can delete it. |
Sorry, something went wrong.
| { | ||
| owner: repositoryNwo.owner, | ||
| repo: repositoryNwo.repo, | ||
| language, | ||
| name: `${language}-database`, | ||
| data: bundledDbReadStream, | ||
| headers: { | ||
| authorization: `token ${apiDetails.auth}`, | ||
| "Content-Type": "application/zip", | ||
| "Content-Length": bundledDbSize, | ||
| }, | ||
| } |
Check warning
Code scanning / CodeQL
File data in outbound network request
There was a problem hiding this comment.
This is intended and not changing behaviour in this PR
Sorry, something went wrong.
|
Thanks @angelapwen and @aeisenberg. I've added a finally block to close the stream. I'm glad we have those windows unit tests to catch this! |
Sorry, something went wrong.
Closes #1383
Instead of loading the entire database contents into memory, pass a ReadStream to request. Internally, octokit uses node-fetch and that is capable of accepting a stream. I then had to add the content length manually because the database upload API requires this header, but it wasn't populated when passing a stream to request.
Finally, I moved slightly more logic inside the try-catch. This should mean that if the database upload fails again for a user it won't block the main analysis and generally won't be an issue.
Merge / deployment checklist