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

CC @ezekg

@changeset-bot
Copy link

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

@mmaietta
Copy link
Collaborator Author

@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

@ezekg
Copy link
Contributor

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.

@@ -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

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.

@mmaietta mmaietta marked this pull request as ready for review August 22, 2021 18:29
@mmaietta mmaietta requested a review from develar August 23, 2021 14:25
Copy link
Contributor

@ezekg ezekg left a comment

Choose a reason for hiding this comment

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

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

packages/builder-util-runtime/src/httpExecutor.ts Outdated Show resolved Hide resolved
packages/builder-util-runtime/src/publishOptions.ts Outdated Show resolved Hide resolved
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.

None yet

3 participants