Clarify dont-land-on labels

[gibfahn]

The COLLABORATOR_GUIDE has a section on backporting tags, but it doesn't include anything about the dont-land-on labels.

If something has dont-land-on-v7.x does that mean it doesn't apply cleanly and needs a backport PR, or that it shouldn't be backported? I'd like to help the release team out by adding these labels, but I'm not entirely clear which ones to add.

AFAIK, PRs have the following states for each release branch (using v6.x as an example):

1. Should be considered for landing on v6.x: lts-watch-v6.x
2. Should be considered for landing on v6.x, but doesn't apply cleanly (needs backport PR): ???
3. Shouldn't be considered for landing on v6.x: EDIT: dont-land-on-v6.x
4. Will be landed on v6.x: lts-watch-v6.x and land-on-v6.x
5. Has been landed on v6.x-staging: land-on-v6.x
6. Has been landed on v6.x (released): ``

So 1., 2., and 3. can be applied by a collaborator, but 4., 5., and 6. shouldn't (they're used by the release team for triaging backports). Is this correct? What labels should a backport PR have?

Related: #10058 (comment) and #10294 (comment)

cc/ @nodejs/release

COLLABORATOR_GUIDE

When you send your pull request, consider including information about
whether your change is breaking. If you think your patch can be backported,
please feel free to include that information in the PR thread.

Several LTS related issue and PR labels have been provided:

• lts-watch-v4.x - tells the LTS WG that the issue/PR needs to be considered
  for landing in the v4.x-staging branch.
• lts-watch-v0.10 - tells the LTS WG that the issue/PR needs to be considered
  for landing in the v0.10-staging branch.
• lts-watch-v0.12 - tells the LTS WG that the issue/PR needs to be considered
  for landing in the v0.12-staging branch.
• land-on-v4.x - tells the release team that the commit should be landed
  in a future v4.x release
• land-on-v0.10 - tells the release team that the commit should be landed
  in a future v0.10 release
• land-on-v0.12 - tells the release team that the commit should be landed
  in a future v0.12 release

Any collaborator can attach these labels to any PR/issue. As commits are
landed into the staging branches, the lts-watch- label will be removed.
Likewise, as commits are landed in a LTS release, the land-on- label will
be removed.

Collaborators are encouraged to help the LTS WG by attaching the appropriate
lts-watch- label to any PR that may impact an LTS release.

[addaleax]

If something has dont-land-on-v7.x does that mean it doesn't apply cleanly and needs a backport PR, or that it shouldn't be backported?

Okay, so far my understanding has been: dont-land-on-v?.x means that something should not be backported (independent of whether it lands cleanly or not) to the corresponding branch. If something requires a manual backport, the original PR receives the label too, because it won’t require another backport.

I think I already mentioned that I’m kind of unsure about whether letting the bot add dont-land labels makes sense, but ultimately I guess we have to trust the releasers about this?

[gibfahn]

@Fishrock123 as someone who's both in the release team and also working on the Github bot.

Maybe we need another label to add to the confusion? Maybe doesnt-land-on-v6.x?

[MylesBorins]

dont-land-on-v?.x means exactly that... this PR shouldn't be landed. This tag is applied when a PR either should not land, or does not land cleanly but has been backported

lts-watch can be added at any time, it means that it should be considered for backporting. This can happen before or after review / landing on master. It should also be included on PRs that don't land cleanly but have not been manually backported

When a commit is backported to a staging branch it receives land-on

[sam-github]

dont-land-on-v?.x means exactly that... this PR shouldn't be landed

That's not my understanding. If a PR conflicts, and the bot adds the label, and I look at my PR, and then change it so that it no longer conflicts, then I can remove the label, right? And then the PR becomes something that should be landed?

Or, if my PR conflicts, but only because it depends on commits that are on master that haven't yet gotten merged to vx.x-staging, it means "make sure your dependent commits make it to staging and then remove the label" (this latter happened to me recently).

So, I think it means don't land "as-is at the time of labelling". Or more precisely, "remove from branch-diff's list of auto-generated candidate commits".

There is another variant of don't land: do not land this because we don't want it backported or cleaned up or present on older versions, for any reason.

I worry this sounds like nit-picking, but its hard to understand how to work with labels ATM, and to know when or if PRs will make it back onto the LTS branches.

+:many: to @gibfahn for trying to lay out the different states of PRs above.

The release process isn't a deterministic FSM, its OK if there are clearly marked stages that aren't absolutely predictable from the code or labels, and where its clear that a contributor needs to interact with people, or watch LTS backport/release proposals (or something). But its helpful if its described in docs so they know they have to do that.

[MylesBorins]

The bot labeling dont-land-v7.x has not been successful imho. I have discussed with @Fishrock123 removing that

edit: to expand. As the person doing most work with these labels the above comment of mine is an outline of exactly how they have been used.

I believe the v7.x labels being added when a commit is not landing cleanly was added to make backporting to v7.x easier. I do not think that the ambiguity of the label. We should not be treating various branches any different in the backporting process. Having the dont-land label simplifies using branch-diff.. but I think we should likely have a separate label for not-landing-cleanly for things we still want to land

thoughts @Fishrock123

[sam-github]

Is the problem with the auto-labelling that it isn't robust because it doesn't consider dependent commits in master? Or that its checked against the wrong branch? Or that it is done only at original PR time and doesn't get reevaulated?

[MylesBorins]

the problem with the auto labeling for do-not-land is that it isn't an accurate use of that label and can result in commits accidentally being ignored. Something not landing cleanly does not mean it shouldn't land

[sam-github]

Right, so the auto-analysis is worthwhile, but should be surfaced some other way, with do-not-land being left as a human decision.

[Trott]

Anybody want to try to PR this info into the guide?

[gibfahn]

I'm 90% sure @addaleax already wrote this down, unfortunately I can never find it for some reason.

[jasnell]

Closing due to lack of further discussion in more than a year