-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
tools: avoid crashing CQ when git push fails #36861
Conversation
Ping @mmarchini |
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.
LGTM. I assume this was not tested, correct? If that's the case I'd suggest whoever lands this to keep an eye on the CQ right after landing to ensure it doesn't break anything, and if it does be ready to open a revert and fast-track it.
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.
Minor typo (not even added in this PR)
|
||
rm output output.json | ||
if ! grep -q '. Post "Landed in .*/pull/'"${pr}" output; then | ||
commit_queue_failed "$pr" | ||
# If `git node land --abort` fails, we're in unknown state. Better to stop | ||
# the script here, current PR was removed from the queue so it shouldn't | ||
# interfer again in the future |
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.
# interfer again in the future | |
# interfere again in the future |
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.
This can be fixed in another PR, this PR may need an immediate revert if it breaks the CQ.
gitHubCurl "$(commentsUrl "${1}")" POST --data @output.json | ||
|
||
rm output output.json | ||
} | ||
|
||
# TODO(mmarchini): should this be set with whoever added the label for each PR? | ||
git config --local user.email "[email protected]" |
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.
Still iojs.org?
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
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.
Out of curiosity, why? Just haven't gotten around to changing it?
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.
Probably. I don't have context on it tbh.
Landed in 864195b...240c8bc |
PR-URL: #36861 Reviewed-By: Mary Marchini <[email protected]>
CQ just landed #38870 (comment), seems like everything is working as expected 🚀 |
PR-URL: #36861 Reviewed-By: Mary Marchini <[email protected]>
PR-URL: #36861 Reviewed-By: Mary Marchini <[email protected]>
PR-URL: #36861 Reviewed-By: Mary Marchini <[email protected]>
PR-URL: #36861 Reviewed-By: Mary Marchini <[email protected]>
PR-URL: nodejs#36861 Reviewed-By: Mary Marchini <[email protected]>
This is useful in case someone pushed to the master branch while the CQ was trying to land a PR, or if the CQ bot doesn't have the correct credentials. Currently it silently crashes, without adding a
commit-queue-failed
label on the PRs; with this PR, it should always add a label and comment on the PR./cc @mmarchini