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(cli): Remove broken error handling #47

Merged
merged 1 commit into from
Oct 30, 2017
Merged

fix(cli): Remove broken error handling #47

merged 1 commit into from
Oct 30, 2017

Conversation

danez
Copy link
Contributor

@danez danez commented Sep 1, 2017

data.state is not available when using Q.all() and only works with Q.allSettled().
As Q.all() is now used it does not make sense to do this check as the Promise
will be rejected immediately with an error if one of the calls to github fails.

This is broken since 7c1e4d9

Although I'm not super sure about this fix as I think the referenced commit might have broken more stuff, as Q.all() is not waiting for all promises to be fulfilled and instead cancels everything as soon as one fails, leaving the user in an undefined state which requests actually were send and which ones were canceled.
In contrast Q.allSettled() waits till everything is finished and you have to handle failures yourself with the state property.
Also the commit message of the change says that the user did not noticed that something failed which is wrong, as the state check was in place I guess. And it says it fixes #3 which seems unrelated.
So my ultimate solution would be to revert the commit in question but you need to decide :)

Fixes #46

https://github.com/kriskowal/q#combination

data.state is not available when using Q.all() and only works with Q.allSettled().
As Q.all() is now used it does not make sense to do this check as the Promise
will be rejected immediately with an error if one of the calls to github fails.
@hutson
Copy link
Contributor

hutson commented Oct 30, 2017

Thank you @danez for submitting this pull request.

Although I'm not super sure about this fix

Your change matches what we did with conventional-gitlab-releaser to fix the same bug over there. That change was just never ported to conventional-github-releaser.

So thank you for helping us, and the entire community, by submitting a fix to this project. 👍

I think the referenced commit might have broken more stuff, as Q.all() is not waiting for all promises to be fulfilled and instead cancels everything as soon as one fails, leaving the user in an undefined state

The previous behavior caused the CLI to return success if a single request to GitHub succeeded.

process.exit(1); was only called if every request to GitHub failed.

If one or more requests to GitHub failed your project/repository would be in an undefined state and you would not know that it was.

By failing on just a single request, we are raising the issue to the awareness of the project owner so that they can take the appropriate steps to correct the failure.

Though, in hindsight, we could have kept allSettled and just failed if a single request failed to return fullfilled as its state.

Also the commit message of the change says that the user did not noticed that something failed which is wrong, as the state check was in place I guess.

Yep, my understanding of what was happening was incorrect. As you noted, allSettled does distinguish between success and failure, just not in the way I've been familiar with (such as registering a success or failure handler).

And it says it fixes #3 which seems unrelated.

It's unrelated. Closing #3 was caused by cherry picking the commit from conventional-gitlab-releaser, where this change fixed issue number 3 over on that project.

@hutson hutson merged commit fc5f97d into conventional-changelog:master Oct 30, 2017
@hutson hutson self-assigned this Oct 30, 2017
@hutson hutson added the defect label Oct 30, 2017
@danez danez deleted the patch-1 branch October 30, 2017 16:46
@hutson
Copy link
Contributor

hutson commented Oct 30, 2017

Version v1.1.13 has been released.

hutson added a commit that referenced this pull request Mar 19, 2018
fix(cli): Remove broken error handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Error when publishing a release to github Scopes for token
2 participants