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(publisher-electron-release-server): throw an exception for 4xx/5xx HTTP status codes #1538

Merged
merged 1 commit into from
Mar 3, 2020

Conversation

pierrickouw
Copy link
Contributor

@pierrickouw pierrickouw commented Mar 2, 2020

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

node-fetch doesn't fail when a 4xx is raised (https://www.npmjs.com/package/node-fetch#handling-client-and-server-errors).

Our release process rely on this to make sure the files are properly updated during a electron-forge publish, but sometimes the Electron Release Server might send 4xx or 5xx errors and we are fooled into thinking "everything is ok" 🙂

Do not hesitate if you need more information !

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! This seems like a good change, conceptually. I've made a couple of comments about the implementation.

It would be nice to have a regression test for this. Could you add one using the fetch-mock module?

@pierrickouw pierrickouw force-pushed the fix-fetch-status-ers branch 2 times, most recently from d13cd82 to f702f35 Compare March 3, 2020 00:17
@pierrickouw
Copy link
Contributor Author

@malept thanks for your feedbacks! I added one test and implemented your suggestion.

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Thanks for writing the test! I'm concerned that the test doesn't test the change you've made. I've made more detailed comments below.

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for working with me on the changes.

@malept malept changed the title fix(publisher-electron-release-server): make 4xx status code fail fix(publisher-electron-release-server): throw an exception for 4xx/5xx HTTP status codes Mar 3, 2020
@malept malept merged commit f223fc8 into electron:master Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants