Repository navigation
github labelling to more clearly indicate state of PRs wrt. backporting #183
Description
This is relevant to both LTS (backporting to what is 6.x and 4.x at this moment) as well as the maintenance of Current (which is 7.x at this moment).
I think it would be useful to have a backport-to-v#.# label with the meaning that this PRs content is wanted on v#.#, but needs backport because it does not land clean.
In general, I would like for the intentions wrt. backporting and landing of any PR to be possible to know from the github labels. This will help people looking at a PR in github to understand what is going to happen to it. It should also help with tool automation, because the tools can rely on the labels more and less on discretion of the person doing the release. And it would make communication more clear. Perhaps we could even write a tool that given a PR number, would predict the releases it will be in, so people know when to expect their features to appear in LTS, or if they will never appear because they were rejected as not stable enough.
The state of any PR wrt. to the next branch back (for master, next back is 7.x at the moment, for 7.x, the next back is 6.x, etc.) could be described by a set of labels like:
dont-land: this PR will not be landed (if it won't be landed because it can't, but its wanted, then it should also have abackport-tolabel, but if its just not wanted, this label is enough to remove from it from the branch diff)backport-to: this PR does not land, but should be backportedlts-watch: indicates that someone is requesting the LTS team to consider this PR for landing on a LTS veresion. Note: LTS team will consider, but there isn't a label indicating the result of their consideration, and the label isn't required for a PR to be landed, LTS will proactively pick PRs that are appropriate. I think the current state is a bit vague.land-on: for master, anything that is notsemver-majorordont-landshould land on current, so this is not needed. For LTS, I don't think this label is (currently) required for a PR to land on a LTS version, but I think it should be applied to all PRs that did land or will land, so that its clear what their destiny is.
Backport PRs themselves can use exactly the labels above, same meaning.
If the github bot starts doing labelling again, I think what would be useful is something like auto-land-failed-v#.# (or something of the like). That way, when a PR lands on master, people can see the labels, and if it doesn't land on Current, or on any LTS, and the author thinks it should, they can start to prepare backports right away.
I think this would make preparing the 7.x much easier. Also, if the github bot isn't correctly realizing that PRs land (perhaps because they don't cherrypick standalone, but do land when the preceeding commits all land), then maybe branch-diff needs to learn to set labels like dont-land on PRs, so their authors get a notification and can choose to backport.
Activity
sam-github commentedon Feb 11, 2017
nodejs/node#9469 (comment)
Probably need a label for semver-major security fixes that should be backported, because that is allowed for security fixes.
@nodejs/lts @MylesBorins
italoacasas commentedon Feb 12, 2017
I think the
backport-tolabel is going to help a lot, I'm +1 on that one.jasnell commentedon Feb 13, 2017
@MylesBorins should weigh in on this but we've been talking about doing away with the
lts-watchlabel. It's just not all that useful. I'd argue theland-onlabels fall into the same boat.sam-github commentedon Feb 13, 2017
The problem with
lts-watchis its just a request, and there is no way to know the resolution of the request.As is, there is no way to know if a PR will or did land from master onto any current or LTS branches, or to know if it was, or will be, backported.
@jasnell you think that state can't be indicated with labels, or that is not useful to know whether a PR will backport?
I need to know the state of my PRs all the time. Our customers are running LTS, when I PR something to master, its because I want it to hit a LTS release where our customers can use it, and I'm doing a fair bit of manual trolling of release-in-progress PRs ATM to make sure it makes it in.
jasnell commentedon Feb 13, 2017
I'm all for the
backport-toanddont-landlabelssam-github commentedon Feb 13, 2017
but not
land-on? If its missingbackport-to, anddont-land, its either landable-as-is, or is not going to land. Distinguishing between those two states is pretty important!I do branch-diffs myself regularly now, see nodejs/node#11185 (comment), I haven't found another way to know what is not making it in.
jasnell commentedon Feb 13, 2017
That's something @MylesBorins should weigh in on more but in practice it hasn't proven to be very useful when actually tracking which PRs need to land. The
dont-landlabels have proven to be more effective.gibfahn commentedon Feb 13, 2017
We currently have/need three different types of labels:
lts-watch-v7.x) or don't (dont-land-on-v7.x) want something backported.???) or doesn't backport cleanly (???)land-on-v7.x), doesn't need to land on (dont-land-on-v7.x), or has already landed on (landed-on-v7.x) a release line.1. Labels for people raising PRs
If we have a well-defined backport process then do we need this? For example we can say that any
semver-minororsemver-patchPR should be landed oncurrent(and then after a few weeksLTS), and onlysemver-patchreleases (or only the critical subset of those releases) should be landed onmaintenance.EDIT: Actually, looking at the COLLABORATOR_GUIDE, our policy is still to only land
semver-patchcommits into LTS unless there's been anlts-review.Then the question is how to ask why a PR is/isn't being backported. I'd have thought @mentioning nodejs/LTS should be okay.
2. Labels for the GitHub bot
This is basically a helper for
branch-diff, which is a helper for the releaser. I think it'd be good to prefix any labels so we know they're automatic, e.g. withbot-.If something requires a manual backport, adding a label doesn't generate a notification, so it might be helpful for the bot to comment as well (cc/ @nodejs/github-bot).
3. Labels for the release/LTS team
Who decides whether something should be backported? According to the docs it should be @nodejs/LTS (not sure if that applies to
currentas well asLTSrelease lines). I assume in practice it's whoever cuts the release, i.e. @nodejs/release.Whoever does it probably needs labels that take priority over the ones the bot sets (for when the bot makes a mistake).
mscdex commentedon Feb 13, 2017
Personally I'd be happy with just two sets of labels: one to indicate intention ("want-backport-v4.x", etc.) and the other the bot adds to show whether the PR can currently land on particular branches without manual intervention (e.g. "no-conflicts-v4.x", etc.) (this one is a bit of an issue with the bot currently, since it does seem to automatically resolve conflicts where possible).
italoacasas commentedon Feb 13, 2017
I feel like if we add too many labels we are going to fix one problem and create another one.
About backporting. Normally the person doing the release is going to start a conversation when a commit is not landing in the
stagingbranch. Thebackport-to-*make totally sense because that way we can remove the commit from thebranch-difftool adding thedon't-landlabel and track the need for backporting, that way we don't loose the PR, on my case because we don't have the label, I have a gist to track the commits that are not landing right now - https://github.com/proxy/gist.github.com/italoacasas/d3212151d158a8a7792e76b5e1efd38cgibfahn commentedon Feb 13, 2017
@mscdex
Do you mean the intention of the person who raises the PR? I'm not sure how helpful that actually is. What do you see as the use for that?
mscdex commentedon Feb 13, 2017
Not necessarily just the PR author, but it could be anyone I suppose. It would be helpful because it may not get backported immediately for any number of reasons, so when it comes time to make a new release for example, a releaser could see that there are x PRs that have yet to go into staging (that may/may not land "cleanly"), so they might ping the author in the PR about it.
Having just a bot-managed "no-conflicts-*" label does not help with this because you may not want the PR to be backported to those branches. For example, if I know node v4.x and v6.x have a slow
fn.bind(), I would not want a particular PR that usesfn.bind()(possibly extensively) to land on those branches, even if they land "cleanly." So only adding a "want-backport-v7.x" label would show that I only want it to be backported to that branch.Also, I don't think we need a third set of labels to show whether something landed or not. IMHO just removing the "want-backport-*" label after backporting it should be sufficient.
gibfahn commentedon Feb 13, 2017
@mscdex okay sure, but that seems like a decision that should be made by the LTS team, not just by any collaborator, hence my suggestion of having labels for the LTS/release team to use to override the
botlabels.In any case, I think we'd only need the negative
dont-land-onversion, as you'd assume everything that can be backported would be picked up bybranch-diffif applicable.mscdex commentedon Feb 13, 2017
Why? I don't see a problem with the current process that seems to involve everyone (including LTS team members), including the PR author, when deciding whether something should be backported. I think there are many occasions when the PR author knows if they should be backported based on the changes being made (e.g. my
fn.bind()example).mscdex commentedon Feb 13, 2017
I think the automatic bot labeling can be beneficial to PR authors so they know (immediately) whether they will need to open up a separate PR for manual backporting.
As far as the negation goes, I don't care which way it is ("want-backport-v7.x" or the combination of "dont-land-on-v4.x" and "dont-land-on-v6.x"), as long as the general meaning stays the same -- the indication of intention.
MylesBorins commentedon Feb 13, 2017
After the last backport I was pretty convinced anything aside from the dont-land label was unnecessary, if something shouldn't be backported you should know and be explicit.
A lot of people don't label things and we have to batch review a lot of stuff no matter what, so it doesn't end up saving that much time in the review process.
We could potentially pair dont-land and land-on to be more explicit that the commit was backported for PRs that don't land cleanly.
phillipj commentedon Feb 13, 2017
Linking with nodejs/github-bot#120
italoacasas commentedon Mar 1, 2017
Well, after reviewing the destruction I caused with the release of 7.7.0, one of the biggest issue that I found was using the
dont-land-on-v7.xlabel to avoid getting the commit when usingbranch-diff, 7 commits related with tracing were not included in the release because of the tag.I think at least the
backport-to-v#label should be created.As a clarification, I want to point that this was not the main problem, but the label can help to avoid having something similar in the future.
targos commentedon Mar 2, 2017
There is also something else that caused the 7.7.0 issue.
It was not obvious at first that nodejs/node#9304 would be backported to 7.x and the backport (nodejs/node#11106) happened more than 1 month after the original PR.
In the mean time, other PRs landed on master to fix issues introduced by nodejs/node#9304 (including the header issue). Those got the
dont-land-on-v7.xlabel and I think it was not a mistake to put this label.The real issue is that when nodejs/node#11106 was opened, we should have realized that other PRs needed to be included with it. GitHub doesn't provide a good way to mark issues as dependent on other issues but we probably need to come up with a solution for this.
gibfahn commentedon Jun 14, 2017
This labelling exists now.
What I've been doing (copied from @MylesBorins) is adding a comment on a PR that depends on another one to say "needs to land with #xxxx". It's not ideal but it should work (as long as people do it). Not sure there's a better way of doing it, but happy to continue the discussion.