[governance]Do we need to define who should approve for the PRs?
Hi all, One colleague pinged me to discuss the process about review and approve. What he said was “PR的approve我们的流程是不是需要固定一下，好像感觉没有明确谁最终approve，大家都在看”. In English, it seems that many committers may have reviewed but no one approves usually, should we define who should approve? For your information, currently, one PR will not be merged until both /lgtm and /approve are labeled. Only committers/maintainers can /lgtm and /approve. In my opinion, it is up to each SIG to decide whether they need identify who should do /approve. In my team, we committers follow the non-written rule: 1. the PR should be reviewed, and all the comments should be handled and replied. If not, /lgtm will not be given. 2. if one committer gives the first /lgtm, the committer doesn’t give /approve usually, unless the PR is obviously an easy one. In other word, 2 committers to agree on the PR are a must. What is your opinion or suggestion? Regards Fred 李永乐
On 2020/4/2 9:02, Liyongle (Fred) via Community wrote:
Hi all,
One colleague pinged me to discuss the process about review and approve. What he said was “PR的approve我们的流程是不是需要固定一下，好像感觉没有明确谁最终approve，大家都在看”. In English, it seems that many committers may have reviewed but no one approves usually, should we define who should approve?
For your information, currently, one PR will not be merged until both /lgtm and /approve are labeled. Only committers/maintainers can /lgtm and /approve.
In my opinion, it is up to each SIG to decide whether they need identify who should do /approve. In my team, we committers follow the non-written rule:
1. the PR should be reviewed, and all the comments should be handled and replied. If not, /lgtm will not be given.
2. if one committer gives the first /lgtm, the committer doesn’t give /approve usually, unless the PR is obviously an easy one. In other word, 2 committers to agree on the PR are a must.
What is your opinion or suggestion?
I totally agree with you. One PR should be reviewed by one committer, and then approved by another. BTW, it seems that there are not enough committers now. Each committer may have too many PRs to review. So I suggest whether we can open the use of /lgtm to everyone who wants to join the discussion. And /approve must be used by committers. Regards, liuzhiqiang
Fred 李永乐
_______________________________________________ Community mailing list -- community@openeuler.org To unsubscribe send an email to community-leave@openeuler.org
Thank you, Zhiqiang. Please see my comments inline. On Thu, Apr 2, 2020 at 9:33 AM Zhiqiang Liu via Community <community@openeuler.org> wrote:
On 2020/4/2 9:02, Liyongle (Fred) via Community wrote:
Hi all,
One colleague pinged me to discuss the process about review and approve. What he said was “PR的approve我们的流程是不是需要固定一下，好像感觉没有明确谁最终approve，大家都在看”. In English, it seems that many committers may have reviewed but no one approves usually, should we define who should approve?
For your information, currently, one PR will not be merged until both /lgtm and /approve are labeled. Only committers/maintainers can /lgtm and /approve.
In my opinion, it is up to each SIG to decide whether they need identify who should do /approve. In my team, we committers follow the non-written rule:
1. the PR should be reviewed, and all the comments should be handled and replied. If not, /lgtm will not be given.
2. if one committer gives the first /lgtm, the committer doesn’t give /approve usually, unless the PR is obviously an easy one. In other word, 2 committers to agree on the PR are a must.
What is your opinion or suggestion?
I totally agree with you. One PR should be reviewed by one committer, and then approved by another.
BTW, it seems that there are not enough committers now. Each committer may have too many PRs to review. So I suggest whether we can open the use of /lgtm to everyone who wants to join the discussion. And /approve must be used by committers.
Regarding the current amount of PRs, except the repo community, I don't think committers are the bottleneck. And quality always has top priority, I think adding committers shall also follow process seriously. I would keep your idea and wait for a while to see whether we can allow anyone to /lgtm. Now some committers do /approve without /lgtm, it is a little dangerous now. Additionally, or the non-committers, they can still show opinion by +1 or other words. Thank you again.
Regards, liuzhiqiang
Fred 李永乐
_______________________________________________ Community mailing list -- community@openeuler.org To unsubscribe send an email to community-leave@openeuler.org
_______________________________________________ Community mailing list -- community@openeuler.org To unsubscribe send an email to community-leave@openeuler.org
-- Regards Fred Li (李永乐)
On 2020/4/2 9:02, Liyongle (Fred) via Tc wrote:
Hi all,
One colleague pinged me to discuss the process about review and approve. What he said was “PR的approve我们的流程是不是需要固定一下，好像感觉没有明确谁最终approve，大家都在看”. In English, it seems that many committers may have reviewed but no one approves usually, should we define who should approve?
For your information, currently, one PR will not be merged until both /lgtm and /approve are labeled. Only committers/maintainers can /lgtm and /approve.
In my opinion, it is up to each SIG to decide whether they need identify who should do /approve. In my team, we committers follow the non-written rule:
1. the PR should be reviewed, and all the comments should be handled and replied. If not, /lgtm will not be given.
2. if one committer gives the first /lgtm, the committer doesn’t give /approve usually, unless the PR is obviously an easy one. In other word, 2 committers to agree on the PR are a must.
What is your opinion or suggestion?
If there are multi maintainer for one SIG, I think, - Should be a chief maintainer in the group, he needs to review/ack/nack/merge patches/pr when nobody else cared. - As you said, needs review/ack (/lgtm) from other maintainers before merged - But if nobody cared, the chief maintainer can decide to merge or not. Thanks Hanjun
participants (4)
-
Fred Li -
Hanjun Guo -
Liyongle (Fred) -
Zhiqiang Liu