-
Notifications
You must be signed in to change notification settings - Fork 116
get-metadata: fail with a non-zero exit status #199
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #199 +/- ##
=======================================
Coverage 93.73% 93.73%
=======================================
Files 17 17
Lines 654 654
=======================================
Hits 613 613
Misses 41 41 Continue to review full report at Codecov.
|
components/git/metadata.js
Outdated
return getMetadata(Object.assign({}, config, argv, parsed), cli) | ||
.then(({status}) => { | ||
if (status === false) { | ||
process.exit(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just let this throw here so it goes to the catch below, and replace process.exit(-1)
below to process.exit(-1)
? (I don't even know what I was thinking when I put -1
there :/)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean process.exit(1)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, like so
.then(({status}) => {
if (status === false) {
throw new Error(`PR checks failed`);
}
})
.catch((err) => {
if (cli.spinner.enabled) {
cli.spinner.fail();
}
cli.error(err);
process.exit(1);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. That was exactly what I had been thinking about.
@joyeecheung take a look now. |
@ryzokuken thanks a lot! |
Fixes: #130
@joyeecheung @cPhost Please take a look. I hope this is what was needed to be done.