Proposal: Let PR originators merge their own PRs where practical

[sxa]

At the moment whoever does the second approval of a PR in the build and pipelines repositories (or the first approval in the infrastructure repo) frequently merges it.

From my own perspective I generally try to hold off and let the originator merge it unless they do not have access. Reasons as follows:

• The originator of the PR is best place to resolve any issues when it goes wrong, therefore it ensures they are available to do so.
• It allows the originator of any potentially problematic PRs to mitigate the risk of merging by doing so at a time of day where they can run any final pipelines to verify their change
• It may be that there are other changes in other repositories (more likely after Split out jenkins pipeline codebase from shell build scripts #2455) and this makes it easier to co-ordinate changes without marking as draft (which may look like "not ready to review" to people who might otherwise review.

I propose implementing this policy (first proposed in https://adoptopenjdk.slack.com/archives/C09NW3L2J/p1593165920153600?thread_ts=1593160796.150600&cid=C09NW3L2J last year) in both the build, pipelines and infrastructure repositories, and if the originator doesn't merge then the person merging has a responsibility for ensuring any "obvious" side effects from their PR are dealt with in a timely manner. I believe this will give us better stability in our core business of producing openjdk builds. Obviously there will be cases where, for emergency build break reasons for example, something needs to be done quickly so this can be bypassed but that generally applies to a small number of PRs.

All thoughts welcome.

[gdams]

@sxa totally happy with this change, I'd suggest a bot that adds a label ready-to-merge and pings to the author

[M-Davies]

@gdams took the words right out of my mouth for both the approval and bot suggestion. +1 from me for this 👍

[karianna]

Agreed - makes sense with our TZ's as well.

[aahlenst]

While I understand the motivation, I don't want more noise (GitHub already notifies me of approvals). Furthermore, we need tooling to enforce this. Otherwise the whole thing is going to fall apart quickly: How do I know (as approver) whether the person can merge the change? How do we know which repository has that rule and which does not? By reducing collective ownership, we actually discourage new contributiors. If I'm responsible that everything goes smoothly, I have to reserve time and maybe wait hours until the pipelines are through. Of course I don't get a notification for that.

So, if some people want to be in control of their PRs, they should have it. Create a label that indicates that the author wants to merge and find some way to enforce that so that nobody merges by accident.

[sxa]

I don't want more noise (GitHub already notifies me of approvals)

What extra noise are you expecting from this? Is that in relation to having a bot that adds the tag or are you thinking about something else?

If I'm responsible that everything goes smoothly, I have to reserve time and maybe wait hours

I get that, but my counter point is that it's a much better option than the hourse I've spend trying to debug why something's suddenly get wrong, which is a frequent problem that I and @andrew-m-leonard inevitable end up dealing with, which is why I actually don't think the author should have to actively make that decision - I'm sure many authors would rather someone else deal with any fallout from their PRs :-)

Furthermore, we need tooling to enforce this

Superficially that seems slightly at odds with your comment about not wanting more notifications. Honestly I'd personally rather not enforce it unless we can't expect people to just be sensible about it - as I said there may be a need to override such a ruling and I'd rather not have that require more overhead. The title of this says "Let PR originators" i.e. I want to encourage reviewers not to "merge by default" once they've had a quick look over something.

How do we know which repository has that rule and which does not?

We could add that information into the PR template which ought to make it clear - either way I don't think that's a reason not to at least trial it here. I'm curious about why you think it might discourage new contributors though? They shouldn't be affected unless there's a use case I'm not thinking of, or are you thinking about new collaborators who would get the privileges to merge?

[smlambert]

I believe, this issue is at least in part an attempt to address the case where reviewers have not read thoroughly the comments or requests of the originator ("please wait until xxxx has been verified...", "this change needs to be co-ordinated with x and y changes...", "I am asking specifically for Z to review this change..."). When these requests are ignored by reviewers who then proceed to merge, it is not a good thing.

This has also been an issue in the openjdk-tests repo, where hasty reviews and merges have occurred without enough understanding or testing. This is very stressful to the team. It prompted us to add the 2nd reviewer required gate, in order to address it. In the case of the tests repo, I mostly do not want the originator to merge, because we have lots of originators that do not have write access. If I have reviewed something and do want them to merge (because I know they have write permissions, I can request that they merge when they are ready).

I do not want someone who does not understand how to properly test the PR to merge it. Most of the time, we are not in a race at the project. We should be calmly assess each PR and soberly decide to merge them when they are fully tested and on a timing that makes sense for the change (a number of possibilities, including but not limited to 1. merge asap because everything is broken and it can not get worse, or 2. next day, when someone can watch a build, or 3. after an upcoming release, so as not to destabilize the project). How we manage this is really what we need to decide.

If the originator has special requests or comments regarding the PR, those should be read/understood/honoured. We can decide on a default behaviour and then expect originator and reviewers to communicate via comments on the merge plan if it diverges from the default behaviour. All of this does require both reviewers and originator to pay attention and communicate. If everyone already did that, might not need this discussion in the first place.

[aahlenst]

Regarding @sxa's comments:

What extra noise are you expecting from this? Is that in relation to having a bot that adds the tag or are you thinking about something else?

George and Morgan want a bot that pings the author.

I get that, but my counter point is that it's a much better option than the hourse I've spend trying to debug why something's suddenly get wrong

This is not good. But the solution offered here is terrible, too. We already have a contributor problem and we now want to make it worse in the same way OpenJDK did by making it more difficult to contribute (Skara seems to try to undo parts of it). The problem you describe is caused by openjdk-build being in a terrible shape. And we pile PRs on top of it that are not ready. And they aren't ready because we make it impossible for contributors to gauge whether their PRs are in a good shape. I'm not new here and I seldom get it right.

The title of this says "Let PR originators" i.e. I want to encourage reviewers not to "merge by default" once they've had a quick look over something.

Doesn't work. What I propose is a label like "self-merge". If you add that to one of your PRs, some tooling will prevent me from hitting merge. That PR will then wait for you. If someone else wants to merge, they first have to remove that label. That will make them pause.

I'm curious about why you think it might discourage new contributors though?

If I'm new somewhere I want someone to babysit me to through my contributions. The thought of breaking something that impacts a large number of people is terrifying. So by default it should be the reviewer's job to get my PR into the project because then "It wasn't me". If we can get that self-merge thing as an opt-in feature, fine.

Regarding @smlambert's comment:

If the originator has special requests or comments regarding the PR, those should be read/understood/honoured. We can decide on a default behaviour and then expect originator and reviewers to communicate via comments on the merge plan if it diverges from the default behaviour. All of this does require both reviewers and originator to pay attention and communicate. If everyone already did that, might not need this discussion in the first place.

💯 But: It's difficult. We should help people do the right thing without having to manually go through checklists (bad examples: 8u-dev and 11u-dev). The Skara bot seems to be going in the right direction, although it's very noisy.

[smlambert]

I like the self-merge label idea supported by tooling that enforces it. I especially like that we are discussing various ways that we want to improve as a project and a global team. 👍

[M-Davies]

George and Morgan want a bot that pings the author.

That already exists. I just suggest expanding it with more useful information that just (please ping someone to run tests). adoptium/ci-jenkins-pipelines#59 intends to do that by making the noise more useful that just noise. It might help with the babysitting point you raised too

[aahlenst]

I just suggest expanding it with more useful information that just (please ping someone to run tests).

While the bot says what has to be done next, it neither states who can perform those steps nor where to find that information. It also does not require an initial code review which is hopefully the point of this exercise (because of the stateful nature of our machines we don't want to run unverified code on them).

Small side note re: adoptium/ci-jenkins-pipelines#59. A newcomer is unlikely to be able to come up with ideas themselves. They need a clear objective and a step by step guide what to do ("Get info from that API, display it, ..."). As it stands, the ticket is rather for a veteran 😉

[andrew-m-leonard]

Can we make use of "Draft" for this purpose, ie.if the Author would like to merge it then they mark it as Draft, arrange reviews, and when they're happy merge... as the premise here is for some cases we want to be sure it's ready for merge, so until that point it is draft...

[sxa]

Can we make use of "Draft" for this purpose, ie.if the Author would like to merge it then they mark it as Draft, arrange reviews, and when they're happy merge... as the premise here is for some cases we want to be sure it's ready for merge, so until that point it is draft...

My problem with that is as stated in the original description: "[draft] may look like "not ready to review" to people who might otherwise review." I think there's an important difference between draft (I'm still actively changing stuff) and ready to review (Looking for feedback but it doesn't mean I'm totally confident it'll work when merged)

[sxa]

(Writing this quite quickly as I'm supposed to be in a meeting now - apologies if the view isn't clear)

But the solution offered here is terrible, too.
making it more difficult to contribute

I think calling it "terrible" is a bit harsh. This is intended to solve a problem I see on a regular basis at the moment.

So by default it should be the reviewer's job to get my PR into the project because then "It wasn't me".

It should certainly be the reviewers job to encourage the contribution and help them fix any issues, and in the case of new (Read scared, intimidated by the thought of breaking something) contributers this won't change anything (they can't merge themselves after all) so I'm unclear on what the concern is. Adding extra labels (either self-merge or ready-to-merge seem a bit of an extra overhead to me personally, but I'd be ok with them if it's what people want. I just don't want to continue in a situation where PR originators don't have any responsibility post-merge (especially within the current group members - I'm ok if a new contributor encounters a problem with somethig they've put in - in that case the reviewer should hopefully help them debug if it goes in and fails - that's part of the learning experience and will happen regardless)

[sxa]

If I'm new somewhere I want someone to babysit me to through my contributions.

I agree completely which is why I've tried to push for improved documentation but it's often felt like a lone cause - I think we've lost active people in the project rather than gained them over the last year, and the workload has not gone down so it's not that easy to make this happen. I do think that's separate from this proposal though, which should not change that scenario.

[aahlenst]

I think calling it "terrible" is a bit harsh. This is intended to solve a problem I see on a regular basis at the moment.

You haven't noticed the label on my forehead that reads "BEWARE Strong opinions"? 😁

Again, we're trying to fix the wrong problem here. You would see those problems significantly less if the PRs were properly validated. If a bash syntax problem can make it undetected into the main branch and cause build breaks, the problem isn't the person who merges something or that the author isn't there to babysit the problem. I push back because what you're suggesting tries to work around the problem instead of fixing it. The state of openjdk-build has been known for a long time, but we're kicking the can down the road because it hasn't hurt enough, yet. We have money for community managers and what not, but we don't have money or people to fix our core problems.

We need less problems, not more. As soon as we remove the pain with a quick win, we have the tendency to forget it even existed. Example: With 2d94453, PR testing on macOS was disabled by commenting it out. With a82104c, we lost the part that was commented out. Who remembers now that we had macOS PR testing, that it was disabled and that we should bring it back? There's not even an issue for it.

The TSC should be focused on those issues like Sauron's eye in Lord of the Rings.

It should certainly be the reviewers job to encourage the contribution and help them fix any issues, and in the case of new (Read scared, intimidated by the thought of breaking something) contributers this won't change anything (they can't merge themselves after all) so I'm unclear on what the concern is.

We're going to train the reviewers to abandon the PRs once they've hit that "Approve" button because that's the default. As a newcomer/person without privileges, you have then to find and chase somebody to merge on your behalf. That's the OpenJDK experience, and I don't enjoy that one (this isn't a dig towards OpenJDK, things are like they are for a reason).

[sxa]

You haven't noticed the label on my forehead that reads "BEWARE Strong opinions"? grin

Missed that but then I haven't had a video call with you recently ;-) Good example on the macos PR tester though...

We're going to train the reviewers to abandon the PRs once they've hit that "Approve" button because that's the default. As a newcomer/person without privileges, you have then to find and chase somebody to merge on your behalf. That's the OpenJDK experience, and I don't enjoy that one (this isn't a dig towards OpenJDK, things are like they are for a reason).

I hope that won't be the case at this project and if it does then I hope we can handle it. I'm still willing to take the risk on it personally. For now, with such a small team (which is part of the problem ... and isn't a problem for openjdk) I think the approvers (it's pretty much @gdams @karianna @andrew-m-leonard @M-Davies and myself who do them for this repo) all know who is capable to merging things and who isn't so I would hope they wouldn't leave non-committers in the dark and not have too much of an overhead. I'd be delighted if we were able to scale the team to the point where this policy was no longer needed. I really think it's worth trying and if we see such problems then we can revisit. I just don't want to "do nothing"