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

feat: Adding Keygen as an official publisher/updater for electron-builder #6167

Merged
merged 6 commits into from Aug 25, 2021

Conversation

@mmaietta
Copy link
Collaborator

@mmaietta mmaietta commented Aug 19, 2021

CC @ezekg

@changeset-bot
Copy link

@changeset-bot changeset-bot bot commented Aug 19, 2021

🦋 Changeset detected

Latest commit: 0880d1b

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

This PR includes changesets to release 12 packages
Name Type
app-builder-lib Minor
builder-util Minor
builder-util-runtime Minor
electron-publish Minor
electron-updater Minor
dmg-builder Minor
electron-builder-squirrel-windows Minor
electron-builder Minor
electron-forge-maker-appimage Minor
electron-forge-maker-nsis-web Minor
electron-forge-maker-nsis Minor
electron-forge-maker-snap Minor

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

Loading

@mmaietta
Copy link
Collaborator Author

@mmaietta mmaietta commented Aug 19, 2021

@ezekg few quick notes 🙂

If you want to ignore 5xx errors, I'd suggest also checking for other common 500s: 500, 503 and 504.

I actually pulled this logic from Bintray, but I'm more than happy to scan for the other errors as well. Can just check for any status code that starts with 50?

if (attemptNumber < 3 && ((e instanceof HttpError && e.statusCode === 502) || e.code === "EPIPE")) {
continue
}

The data and errors properties will never be returned at the same time. So on API error, this will be accessing undefined.id, which will throw.

I actually pulled this from the documentation that both would be returned. Would you be willing to update it? I found it confusing.
https://keygen.sh/docs/api/#releases-upsert
const { data, errors } = await response.json()

I'll update the logic accordingly in my PR

Loading

@ezekg
Copy link
Contributor

@ezekg ezekg commented Aug 19, 2021

I actually pulled this logic from Bintray, but I'm more than happy to scan for the other errors as well. Can just check for any status code that starts with 50?

Yep, generally, you can use statusCode >= 500 && statusCode <= 599 to test for a range of HTTP status codes.

I will update the API docs to make return values clearer when an error occurs, thanks for the feedback.

Loading

@mmaietta mmaietta force-pushed the feat/KeygenPublisher branch from 3df3246 to daf7b7d Aug 22, 2021
@@ -46,6 +46,17 @@ test("file url generic", async () => {
await validateDownload(updater)
})

test.ifEnv(process.env.KEYGEN_TOKEN)("file url keygen", async () => {
const updater = await createNsisUpdater()
updater.addAuthHeader(`Bearer ${process.env.KEYGEN_TOKEN}`)
Copy link
Collaborator Author

@mmaietta mmaietta Aug 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ezekg! Quick note for documentation: The end-user will need to know this detail of adding the "Bearer " prefix on their own. I think the function's name makes it self-explanatory that it's the Authorization header, but might still be worth noting in the docs.

Loading

@mmaietta mmaietta marked this pull request as ready for review Aug 22, 2021
@mmaietta mmaietta requested a review from develar Aug 23, 2021
ezekg
ezekg approved these changes Aug 23, 2021
Copy link
Contributor

@ezekg ezekg left a comment

Overall, looks great. A couple notes/nits which can be safely ignored. Excited for this! 🙂

Loading

Loading
packages/builder-util-runtime/src/httpExecutor.ts Outdated Show resolved Hide resolved
Loading
packages/builder-util-runtime/src/publishOptions.ts Outdated Show resolved Hide resolved
Loading
@mmaietta mmaietta merged commit f45110c into electron-userland:master Aug 25, 2021
7 checks passed
Loading
@github-actions github-actions bot mentioned this pull request Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants