scripts: new attempt-backport script for PRs

[Fishrock123]

This is a script to automatically check if PRs can cleanly land on staging branches in the node repo.

I've been using TestOrgPleaseIgnore/node#29 and TestOrgPleaseIgnore/node#30 to test it and it seems to be working fine now.

This is however a first iteration and I'm sure it can be improved lots.

cc @nodejs/github-bot & @thealphanerd

Edit: some unresolved questions/issues:

• Should it unlabel if the PR later passes?

[rvagg]

/cc @nodejs/build

[phillipj]

A little more use of the bunyan logger, really valuable for doing post-mortem on PRs in production. See ./lib/node-repo.js for inspiration. Other than that it looks good enough to test live IMO 👍

Couple of notes for future improvements:

• Some tests would be nice to ensure we don't break this later
• Put queue outside of process memory too handle redeploys

The latter is already an issue, and might be subject to a bigger discussion. E.g. for the Travis build poller (./lib/pollTravis.js) where a redeploy could easily make inline status on a PR stall for eternity.

[Fishrock123]

where a redeploy could easily make inline status on a PR stall for eternity.

Definitely a bug, but pushing anything to the PR branch will unblock that.

[phillipj]

Definitely a bug, but pushing anything to the PR branch will unblock that.

Definitely, tho I assume not many contributors would know that.

[Fishrock123]

Updated and I tried to get it working at TestOrgPleaseIgnore/node#31 but I wasn't able to get the new label removal working?

I have no idea how to test this, there are so many (necessary) moving parts. 😓

[phillipj]

Yeah I've noticed that as well, it's generally quite painful to test much
of what the bot is doing. The test code quickly gets more complex than the
actual source code 😕

On Friday, 4 November 2016, Jeremiah Senkpiel [email protected]
wrote:

Updated and I tried to get it working at TestOrgPleaseIgnore/node#31
TestOrgPleaseIgnore/node#31 but I wasn't able
to get the new label removal working?

I have no idea how to test this, there are so many (necessary) moving
parts. 😓

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#90 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABLLE2E9_l2YGwcqWgEZj-9Z-r5uYLvHks5q62f1gaJpZM4Kn2MJ
.

[phillipj]

Should $NODE_REPO_DIR be an empty directory, or should the node repo already be cloned into that directory? I might get that setup in the ansible playbook before merging/deploying this.

[Fishrock123]

@phillipj it will need to be a github clone of the node repo.

I thought of adding cloning logic into this but decided against it.

[Fishrock123]

Looping @mscdex's comment (#77 (comment)) from the original thread which I somehow missed:

Why use the lts-watch-* labels? Shouldn't those be added by users whether there are merge conflicts or not?

Also, how would you know which labels should be added (which branches to test), unless you mean to try all branches that are not specifically excluded via do-not-land-on-* labels?

Another issue is how would you keep the labels up-to-date? If someone hasn't updated their PR/branch in awhile, there could be merge conflicts at some point.

If we stick with labels, maybe a new set of labels would be best? Like lands-cleanly-on-* or something?

Is it possible to have a webhook like Travis and other services that shows the information at the bottom of the PR? If so, would that be better than labels?

Thoughts everyone?

[MylesBorins]

I think we should stick with lts-watch. As the primary user of them at this time what I mostly care about is getting rid of unlabelled commits. People are pretty good at knowing that a commit is not properly labelled, and every commit is going to be audited one more time before it is backported. Adding an extra label is just another step that we will have to add to the auditing process.

edit: just to clarify... what I think we should be optimizing for is ensuring that commits are properly labelled for backporting, the fact that it "lands cleanly or doesn't" is simply a heuristic we can use for this

[phillipj]

I thought of adding cloning logic into this but decided against it.

@Fishrock123 sounds good. I'll be able to fix what's needed in the bot's ansible playbook, so we're sure the server running the bot, has the repo checked out and gets $NODE_REPO_DIR set.

[Fishrock123]

@phillipj what's your thoughts on moving this forward? Should I just try to test it more?

[phillipj]

@Fishrock123 mainly getting some feedback on nodejs/build#531, otherwise it LGTM.

After that, if you feel confident enough to test it on the actual node repo, I'm totally fine with that. Just got auto deploy upon merge to master working here, so you'll be able to deploy this yourself when you feel like it.

[phillipj]

@Fishrock123 it's ready -- the bot server now has the nodejs/node repo cloned and $NODE_REPO_DIR set to the repo directory

[phillipj]

Awesome, really looking forward to seeing this in action! 😃

[Fishrock123]

nodejs/node#9648 appeared to be a case where the script failed prematurely because I was pointing to the wrong git remote (upstream) as it were on my machine.

187cbd8 should fix that by running git commands against origin.

[mscdex]

I think having the bot applying the dont-land-on-* labels is a bit of a misnomer. My understanding of those labels is that they indicate intention, not whether the PR can currently be backported cleanly or not. I think the recent PR that @Fishrock123 pointed out is a good example of this problem.

I think a different set of labels should be used by the bot, but the problem with that is that we're already hitting the 100 label ceiling (this is a separate issue that needs discussion too, because not everyone is aware of it).

[Fishrock123]

I think having the bot applying the dont-land-on-* labels is a bit of a misnomer. My understanding of those labels is that they indicate intention, not whether the PR can currently be backported cleanly or not.

We directly check dont-land-on-* labels as part of the release process, along with semver-*. They already do not just indicate intention, they are used as a hard stop measure for PRs that should not land to the specified branch.

[Fishrock123]

something's still not right: nodejs/node#9472

[Fishrock123]

Another: nodejs/node#9637

015096f - disabling for now, I clearly don't have something right still, although to dig further I'll need more debug output from here on than the default logger provides. 😞

[phillipj]

Ping me if you need some help debugging on the bot server, e.g. do a clean checkout of the node repo again.