Hi,
> - The one who merged this pull request, just merge this pull request > without saying anything. I am not sure he mis-understand the > STATUS/READY-TO-MERGE label or not. But I think if read the context > through, one should be more cautious to merge that pull request. I'm so sorry it was me who merged that pull request. I had read through the context(actually I reviewed although I did not comment) and indeed I've misunderstood the STATUS/READY-TO-MERGE label, and I suggest put more detail about the labels in the committer guide[1] to avoid later committers making the same mistakes. Sorry again for making such mistake, I'll revert it if needed. 1. http://dubbo.apache.org/en-us/docs/developers/committer-guide/label-an-issue-guide_dev.html At 2019-03-15 16:28:16, "yuhang xiu" <[email protected]> wrote: >Hi, huxing. > >Great advice. In the previous review process, I also ignored some issues. > >Maybe we need a new label such as: `need-more-review`. When we need more >reviews, we can mark this pr to remind other committers to review it. > >In addition, I think we need to clarify the meaning of `READY-TO-MERGE`. I >think there is only one case where we will use this label, which is the >request that is approved, but this pr has not passed ci. > >In this pr merger, there are some problems, and we need to pay more >attention in the future. > >Huxing Zhang <[email protected]> 于2019年3月15日周五 下午4:03写道： > >> Hi, >> >> Looking at the reviewing process of the pull request, I can see some >> issues here: >> - No one has officially approved the pull request before it getting >> merged. By saying officially I mean this[2], which I've mentioned in >> another thread. >> - One of the reviewer has marked the milestone as 2.7.2, but applied >> it with a label called STATUS/READY-TO-MERGE. These two activities >> look contradictory but according to the context he's meaning I guess >> is to put some more time to it. >> - The one who merged this pull request, just merge this pull request >> without saying anything. I am not sure he mis-understand the >> STATUS/READY-TO-MERGE label or not. But I think if read the context >> through, one should be more cautious to merge that pull request. >> >> What I think can improve it: >> - Every pull request must be approved officially by at least one of >> the committer. (Comments such as LGTM does not mean official >> approval.) >> - If you feel uncertain about a pull request, please comment (see step >> 6 in [2]) that you need more review to come in to review or request >> review from others directly >> - If you want to merge the pull request, you must read through all the >> context. >> >> How do you think? >> >> [1] https://github.com/apache/incubator-dubbo/pull/3549 >> [2] >> https://help.github.com/en/articles/approving-a-pull-request-with-required-reviews >> >> On Thu, Mar 14, 2019 at 10:03 PM Ian Luo <[email protected]> wrote: >> > >> > Folks, >> > >> > I think we need to revisit pull request 3549 in 2.7.2. In my opinion, >> > fluent api need to be carefully resigned. >> > >> > What do you think? >> > >> > Thanks, >> > -Ian. >> >> >> >> -- >> Best Regards！ >> Huxing >>
