Skip to content
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

Proposal: Asynchronous semver-minor backporting review #212

Closed
gibfahn opened this issue May 10, 2017 · 9 comments
Closed

Proposal: Asynchronous semver-minor backporting review #212

gibfahn opened this issue May 10, 2017 · 9 comments

Comments

@gibfahn
Copy link
Member

gibfahn commented May 10, 2017

It was suggested in the last LTS meeting that it might be worth trying to decide about semver-minor backports asynchronously (i.e. in Github), the same way we decide on PRs or ctc-review items.

Here's the first thing that occurred to me, feel free to suggest alternatives.

We might also just decide that this is too slow/inefficient, and that we should just keep doing what we're currently doing. I think it's worth having the discussion even if the answer is "let's not do this".

Strawman proposal:

If a PR seems obviously unsuited for backporting then someone can just add the dont-land-on tags and cc/ @nodejs/lts in a comment. If anyone disagrees then it becomes an lts-review PR.

Otherwise same rules as for landing PRs, if someone adds the lts-review tag to the landed PR, then LTS members can vote in the issue.

Response Result
All +1s Good to backport
All -1s dont-land-on
Both +1s and -1s Review at next meeting
+1 but needs to bake baking-for-lts

We can have a 48/72 hour timeline, but try to leave it a lot longer by default.

@jasnell
Copy link
Member

jasnell commented May 10, 2017

Perhaps this: semver-patch commits can be cherry-picked in to LTS staging branches at any time so long as they do not have the dont-land-* labels. semver-minor commits require a backport pull request that must be signed off by at least two LTS WG members before it can land in an LTS staging branch following a 48/72 hour rule. For such PRs, a new commit metadata line LTS-Reviewed-By: ... can be added distinguishing who signed off.

@sam-github
Copy link
Contributor

I like @jasnell 's idea of using the PR process and PR approvals to approve semver-minor backports, where only with lack of consensus does it need to be raised to the LTS WG for discussion (probably rare).

Would we need a new metadata type? There was some discussion recently, and I think we'd decided on backported PRs using a system where the original PR metadata would be indented, and the backport PR would get the normal PR metadata.

I'm having trouble finding that discussion, though.

@gibfahn
Copy link
Member Author

gibfahn commented May 10, 2017

semver-minor commits require a backport pull request

So I guess this would be the key change, raising PRs even if the change cherry-picks cleanly.

+1 to that idea.

@gibfahn
Copy link
Member Author

gibfahn commented May 10, 2017

Would we need a new metadata type? There was some discussion recently, and I think we'd decided on backported PRs using a system where the original PR metadata would be indented, and the backport PR would get the normal PR metadata.

Don't think it was finalised, but we discussed in the last LTS meeting.

@MylesBorins
Copy link
Contributor

MylesBorins commented May 15, 2017

We should discuss this today. I'm not sure that this is worthwhile tbqh. We are only doing a single semver minor release per quarter, there has never really been a timecrunch or blocking on getting semver minors in. We need a single meeting to discuss and agree on which ones should land, and this hasn't proven to be a blocking issue in the past (imho).

@sam-github
Copy link
Contributor

I think async might get wider participation than just the few people we get into the LTS meeting, and if anyone (even one person) objects (or even just wants it discussed), or it fails some kind of quorum, it can always fallback to the meeting.

@MylesBorins
Copy link
Contributor

that's a really good point @sam-github

Should we try it for the next round?

@gibfahn gibfahn removed the lts-agenda label Jun 5, 2017
@gibfahn
Copy link
Member Author

gibfahn commented Jun 5, 2017

Taking this off the agenda, we discussed and agreed to try it.

@MylesBorins
Copy link
Contributor

We did this last time, but having a group to review at the end was v effective

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants