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

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