Post 2.3.0 PR merging
|
Carles Cufi
Hi all,
We currently have 170+ Pull Requests that pass CI but require one or more approvals in order to be merged, in part due to the fact that we have increased the minimum number of approvals required from 1 to 2. Help reviewing those PRs is greatly appreciated: https://github.com/zephyrproject-rtos/zephyr/pulls?q=is%3Apr+is%3Aopen+base%3Amaster+review%3Arequired+status%3Asuccess+-label%3ADNM+draft%3Afalse Thanks, Carles |
|
|
|
Simon Glass
Hi Carles, On Fri, 12 Jun 2020 at 03:19, Carles Cufi <carles.cufi@...> wrote: Hi all, Perhaps given the situation this should be reversed? Regards, Simon
|
|
|
|
Carles Cufi
Hi Simon,
A decision like that would need to go through the TSC, and in order to be able to vote we’d need to have clear stats on how many PRs are actually blocked by this policy. I was thinking about this recently, and given that those in Zephyr with merge rights can in fact add their +1 and they typically rely on a maintainer’s review to decide whether to merge a PR or not, this might not have as big as an impact as I originally thought.
That said, I believe we need a new GitHub filter to help us. Right now, we have one that lists PRs that are approved, passing CI and ready to merge, but what we need is one that shows all PRs that are partially approved (i.e. a single +1) and passing CI. Then those of us with merge rights could go over those regularly (like we do for the former) and, provided there’s a +1 from a maintainer, we could take one additional look at the PR, approve it and merge it.
I’d very much like to hear the opinion of others with merge rights in this thread.
Regards,
Carles
From: Simon Glass <sjg@...>
Sent: 06 July 2020 19:46 To: Cufi, Carles <Carles.Cufi@...> Cc: devel@...; users@... Subject: Re: [Zephyr-devel] Post 2.3.0 PR merging
Hi Carles,
On Fri, 12 Jun 2020 at 03:19, Carles Cufi <carles.cufi@...> wrote:
Perhaps given the situation this should be reversed?
Regards, Simon
|
|
|
|
Hi Charles:
This just my personal opinion on the subject. Two approvals is the right thing to do and what the TSC approved earlier this year.
BUT, if the second +1 is just a rubber stamp approval of the maintainer’s +1 then there is no point having two approvals.
The second +1 should come from someone who is: knowledgeable in the area, has studied the changes, and approves.
Lawrence King Principal Developer +1(416)627-7302
From: devel@... <devel@...>
On Behalf Of Carles Cufi
Sent: Monday, July 6, 2020 2:23 PM To: Simon Glass <sjg@...> Cc: devel@...; users@... Subject: Re: [Zephyr-devel] Post 2.3.0 PR merging
Hi Simon,
A decision like that would need to go through the TSC, and in order to be able to vote we’d need to have clear stats on how many PRs are actually blocked by this policy. I was thinking about this recently, and given that those in Zephyr with merge rights can in fact add their +1 and they typically rely on a maintainer’s review to decide whether to merge a PR or not, this might not have as big as an impact as I originally thought.
That said, I believe we need a new GitHub filter to help us. Right now, we have one that lists PRs that are approved, passing CI and ready to merge, but what we need is one that shows all PRs that are partially approved (i.e. a single +1) and passing CI. Then those of us with merge rights could go over those regularly (like we do for the former) and, provided there’s a +1 from a maintainer, we could take one additional look at the PR, approve it and merge it.
I’d very much like to hear the opinion of others with merge rights in this thread.
Regards,
Carles
From: Simon Glass <sjg@...>
Hi Carles,
On Fri, 12 Jun 2020 at 03:19, Carles Cufi <carles.cufi@...> wrote:
Perhaps given the situation this should be reversed?
Regards, Simon
|
|
|
|
Simon Glass
Hi Carles, OK I see. Since you called out the 2-approver change as a cause of the bottleneck, I'm assuming there is data available on this. I was looking around for discussion about the decision. The closest I found was here. Is there something else? Regards, SImon On Mon, 6 Jul 2020 at 12:23, Cufi, Carles <Carles.Cufi@...> wrote:
|
|
|
|
Carles Cufi
Hi Simon,
Not to my knowledge, no. But perhaps Ioannis or Anas know if it’s also documented elsewhere.
Regarding the “stamp of approval” that was mentioned earlier in the thread, I think we could adopt a middle ground. If the person with merge rights is reasonably knowledgeable about the area of the change, and understands the implications, their
+1 could count even if they are not a maintainer. I feel that in many cases this could unblock PRs without compromising the quality of the code being merged.
Carles
Am 06.07.2020 um 22:20 schrieb Simon Glass <sjg@...>:
|
|
|
|
Nashif, Anas
Hi,
Here some data…
What I have just posted on slack…
So, go do some reviews please 😊
Thanks Anas
From:
<devel@...> on behalf of Carles Cufi <carles.cufi@...>
Hi Simon,
Not to my knowledge, no. But perhaps Ioannis or Anas know if it’s also documented elsewhere.
Regarding the “stamp of approval” that was mentioned earlier in the thread, I think we could adopt a middle ground. If the person with merge rights is reasonably knowledgeable about the area of the change, and understands the implications, their +1 could count even if they are not a maintainer. I feel that in many cases this could unblock PRs without compromising the quality of the code being merged.
Carles
|
|
|
|
Simon Glass
Hi Anas, Less scientifically I just did some reviews and perhaps half of them already had one approval. So presumably they were blocked. I guess you are saying it's only a third. OK. But that's a lot. Also with two reviewers, people can be somewhat less careful... How do you deal with the case of people being happy with it but wanting review from someone else before approving? I saw a few where it seemed no one would take the plunge. Regards, SImon On Tue, 7 Jul 2020 at 12:57, Nashif, Anas <anas.nashif@...> wrote:
|
|
|
