-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Replacing archiver
package with direct use of zip-stream
for fixing macOS upload issues
#1690
base: main
Are you sure you want to change the base?
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.
package-lock.json
).catch(err => { | ||
throw new InvalidResponseError( | ||
`createZipUploadStream: response from backend was not ok: ${err}` | ||
) | ||
}) |
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.
these awaited async functions should bubble up thrown errors, we shouldn't need this if we have an outer try/catch
async.eachSeries(uploadSpecification, addFileToZip, (error: unknown) => { | ||
if (error) { | ||
core.error('Failed to add a file to the zip:') | ||
core.info(error.toString()) // Convert error to string | ||
return | ||
} | ||
zip.finalize() // Finalize the archive once all files have been added | ||
}) |
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.
instead of introducing a new dependency on async
, can we just await on a group of these promises?
e.g.
await Promise.all(uploadSpecification.map(addFileToZip))
or something like that? eachSeries
does one at a time, does it have to do that for a specific reason? and do we know if this will impact speed?
describe('upload-artifact', () => { | ||
beforeEach(() => { | ||
noopLogs() | ||
// noopLogs() |
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.
Should this be uncommented?
) | ||
).catch(err => { | ||
throw new InvalidResponseError( | ||
`createZipUploadStream: response from backend was not ok: ${err}` |
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.
This error message might be a bit misleading since errors can come from that function without reaching out to a backend.
This PR replaces the previous
archiver
package in upload-artifacts for the direct use of thezip-stream
so that we can better hook into the finalize portion of the lifecycle. This should resolve issues on macOS where upload streams are incorrectly closed before the upload is finished due to a pre-existing issue witharchiver
.