Repository navigation
Should issues creating churn on 100s of files require backports to land? #16249
Description
I was just reviewing #14162 which created a new lint rule and touched 294 with a very subtle style churn. I have no problems with this type of change landing, but I think that we may want to consider pre-opening backport PRs as part of the sign off requirements.
The potential for running into conflicts mid audit becomes quite large when touching a large percentage of the tests at once like this.
Activity
joyeecheung commentedon Oct 17, 2017
SGTM, although we could lower the number 100 if necessary. Maybe a new label like
needs-pre-backportso the person landing PRs will be reminded to double-check?MylesBorins commentedon Oct 17, 2017
another commit touching > 100 files
#13757
This seems to be a pattern in the last couple months that we had not done in the past
@nodejs/tsc how would you suggest going from here? Should we add this to the agenda?
@nodejs/collaborators would enjoy some more insight on this
Trott commentedon Oct 18, 2017
I love this idea.
jasnell commentedon Oct 18, 2017
I'm -0 on requiring a backport PR in order for the original to land. I'd be more inclined to not backport these types of changes (and yes, I fully realize that not doing so creates it's own backporting burden)... but I would argue that these kinds of broad brush changes do not necessarily need to be backported at all.
joyeecheung commentedon Oct 18, 2017
@jasnell One problem is, if they have touched enough files then not backporting them will result in more conflicts later, because new PRs will come to depend on those changes, especially those touching a lot of tests
jasnell commentedon Oct 18, 2017
Yeah, I've been through that pain first hand, which is why I'm only -0. I don't think this should be a hard rule, but a useful recommendation. There are just going to be cases where backporting them is not necessary
mcollina commentedon Oct 18, 2017
I think this is a great idea. It should simplify the work of the release team.
cjihrig commentedon Oct 18, 2017
I think this is a good idea.
BridgeAR commentedon Oct 18, 2017
I like the idea. There might be some difficulties with that as well but I think we should give it a try.
gibfahn commentedon Oct 18, 2017
I like the idea, and I agree that if it touches enough files then backporting is necessary for the sake of the backporting team.
My worry would be that this will lead to us backporting things straight away. How would this work if we wanted to leave the PR to bake in current for a release or two?
vsemozhetbyt commentedon Oct 18, 2017
I recall that, for some of my big-churn PRs, LTS maintainers ask me to promise to do a backport when the time comes. Maybe we can just formalize such promises? Or this does not guarantee us anything?
vsemozhetbyt commentedon Oct 18, 2017
We can also ask that all the reviewers with LGTM would promise to help with a backport if the author cannot do it due to some circumstances (+ re-review the backport).
vsemozhetbyt commentedon Oct 18, 2017
Also, maybe it is worth to mention in the guides that a contributor may need to ask in an issue if some big-churn PR would be accepted (and would have reviewers and backport guarantees) to prevent disappointing and wasting time.
gibfahn commentedon Oct 18, 2017
I think we should formalize both of these things for all PRs, to help set expectations. If your PR gets landed and needs a backport, you should be ready to do said backport. Obviously help is available if you're unsure.
MylesBorins commentedon Oct 19, 2017
I like all of these ideas! Does anyone feel like taking the action item of formalizing some of this in the Contributor guide?
joyeecheung commentedon Oct 20, 2017
@MylesBorins I think this actually belongs to the collaborator guide?
So far the ideas are:
gibfahn commentedon Oct 20, 2017
This is for all contributors though, not just collaborators, so maybe Contributing makes more sense.
joyeecheung commentedon Oct 20, 2017
@gibfahn Yeah. but my instincts was this should be placed where the "semver-major needs two TSC sign-off" is, and turns out that's in the collaborator guide. I guess that makes sense because it's the people landing it should be the most careful.
MylesBorins commentedon Oct 23, 2017
I'll put something together and submit to the collaborator's guide
srl295 commentedon Oct 23, 2017