Skip to content
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

fix: parallel release creation with Keygen publisher #6989

Conversation

ezekg
Copy link
Contributor

@ezekg ezekg commented Jul 6, 2022

Looks like the previous changes for the Keygen publisher (moving from v1.0 to v1.1 of Keygen's API) contained a bug that slipped through the cracks, as far as automated tests go. I didn't do any manual tests, so this wasn't caught.

Under a normal publish, electron-builder will upload multiple artifacts. The Keygen publisher has a find-or-create method for creating a new release bucket, before uploading artifacts. What I didn't account for in #6941 was that electron-builder uploads these artifacts in parallel, so the find-or-create method isn't as robust as it needed to be.

This results in electron-builder trying to create the same release n times in parallel (n = number of artifacts to upload), causing all but 1 of the release creation requests to fail due to conflict errors.

This results in a failed publish for all users using the Keygen provider and electron-builder v23.3.0.

This wasn't caught in tests because the test for the Keygen publisher only uploaded a single artifact, so a conflict will never occur. That test has been modified to upload multiple artifacts, rather than a single artifact.

This PR resolves the issue by making an additional fetch if a conflict error is encountered.

@changeset-bot
Copy link

changeset-bot bot commented Jul 6, 2022

🦋 Changeset detected

Latest commit: f03a4fc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
app-builder-lib Patch
dmg-builder Patch
electron-builder-squirrel-windows Patch
electron-builder Patch
electron-forge-maker-appimage Patch
electron-forge-maker-nsis-web Patch
electron-forge-maker-nsis Patch
electron-forge-maker-snap Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Jul 6, 2022

Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
🔨 Latest commit f03a4fc
🔍 Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/62c5f718509d10000997619e
😎 Deploy Preview https://deploy-preview-6989--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ezekg ezekg marked this pull request as ready for review July 6, 2022 20:35
@ezekg
Copy link
Contributor Author

ezekg commented Jul 6, 2022

As an aside — as @mmaietta and I briefly discussed here, this may be a good indication that before/after hooks are needed for publishers. It'd be much nicer to be able to tell electron-builder to create the release before uploading any artifacts, then publish the release after uploading all artifacts. This would alleviate any need for this find-or-create method.

IMO, somehow allowing publishers to define before/after hooks seems like a good solution here.

@ezekg ezekg force-pushed the fix/keygen-publisher-parallel-uploads branch from c715499 to f03a4fc Compare July 6, 2022 20:56
@mmaietta mmaietta merged commit 7ad5101 into electron-userland:master Jul 6, 2022
@mmaietta
Copy link
Collaborator

mmaietta commented Jul 6, 2022

this may be a good indication that before/after hooks are needed for publishers. It'd be much nicer to be able to tell electron-builder to create the release before uploading any artifacts, then publish the release after uploading all artifacts. This would alleviate any need for this find-or-create method.

I'm in full agreement of this approach as well.

@github-actions github-actions bot mentioned this pull request Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants