-
Notifications
You must be signed in to change notification settings - Fork 30k
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
build: use purple merge instead of close on CQ #35001
Conversation
If the bot can make modifications to the head branch, force push to the head branch so it lands with the purple icon on GitHub instead of the red icon when the PR is closed. Let's gooooooooo! Ref: nodejs/node-core-utils#264
I most definitely have not tested it yet 🙃 |
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.
Should work 😄 , great idea 👍
@@ -76,10 +80,18 @@ for pr in "$@"; do | |||
git node land --abort --yes | |||
else | |||
rm output | |||
head=$(gitHubCurl "$(prUrl "$pr")" GET | jq '{ ref: .head.ref, remote: .head.repo.ssh_url, can_modify: .maintainer_can_modify }') |
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.
Could we split the value right here to avoid the "$(echo "$head" ..
below so that it will be a bit cleaner. Perhaps like:
head=$(gitHubCurl "$(prUrl "$pr")" GET)
head_ref="$(echo "$head" | jq -r '.head.ref')"
remote_url="$(echo "$head" | jq -r '.head.repo.ssh_url')"
can_modify="$(echo "$head" | jq -r '.maintainer_can_modify')"
if [ "$(echo "$head" | jq -r .can_modify)" == "true" ]; then | ||
git push --force "$(echo "$head" | jq -r .remote)" master:"$(echo "$head" | jq -r .ref)" | ||
# Give GitHub time to sync before pushing to master | ||
sleep 5s |
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 might increase this to a whole minute or something, I just did this manually with a 15 seconds gap between push to PR branch and push to nodejs/node and I still got a race condition.
Note how even though I pushed to the PR branch first, GitHub only registered the event after I pushed to nodejs/node, which lead to the PR getting automatically closed and losing the commits and file changes in the UI
If that's a propagation issue on GitHub (which is what it seems like), an API call to confirm if the push propagated probably wouldn't work :/, so long sleep might be the way to go.
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.
You have to wait until the origin/pull/123 ref has updated, which a git fetch can verify.
I think this has been superseded by #40666? |
If the bot can make modifications to the head branch, force push to the
head branch so it lands with the purple icon on GitHub instead of the
red icon when the PR is closed. Let's gooooooooo!
Ref: nodejs/node-core-utils#264
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes