You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@dubbo.apache.org by jun liu <ke...@gmail.com> on 2019/04/03 03:13:50 UTC
Re: Pull request merged without approval
> In some critical cases, one can approve the pull request by
> himself/herself with proper comments or commit directly.
I think the concern from Mark is quite reasonable, but this particular approach Huxing suggests could solve it while at the same it gives the community a technical rule to obey.
Jun
> On Mar 20, 2019, at 10:45 PM, Huxing Zhang <hu...@apache.org> wrote:
>
> Hi,
>
> On Wed, Mar 20, 2019 at 6:08 PM Mark Thomas <markt@apache.org <ma...@apache.org>> wrote:
>>
>> I recommend that you enforce this socially rather than technically.
>>
>> There are times when a committer needs to get something committed (CI is
>> broken, security fix, correct an obvious bug that is blocking something
>> else, etc.) and having the flexibility to just commit it can be very
>> helpful.
>
> I think the discussion here does not prevent some one to commit
> directly. It is just about if a pull request is raised, it must be
> approved.
> In some critical cases, one can approve the pull request by
> himself/herself with proper comments or commit directly.
>
>>
>> Mark
>>
>>
>> On 20/03/2019 06:37, yuhang xiu wrote:
>>> agree. I think this is a very good way to help us standardize the pr merge.
>>>
>>> YunKun Huang <hy...@apache.org> 于2019年3月20日周三 上午11:41写道：
>>>
>>>> Agree, we should have at least one approval before merging
>>>>
>>>> On 2019/03/20 03:00:51, Huxing Zhang <hu...@apache.org> wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>> On Wed, Mar 20, 2019 at 10:55 AM YunKun Huang <hy...@apache.org>
>>>> wrote:
>>>>>>
>>>>>>
>>>>>> I guess you are talking about "Require pull request reviews before
>>>> merging" feature [1]
>>>>>
>>>>> Cool! That is what I want.
>>>>> I am +1 to support enabling this and set the number of required approval
>>>> to 1.
>>>>> How do others think?
>>>>>
>>>>>>
>>>>>> [1]
>>>> https://help.github.com/en/articles/enabling-required-reviews-for-pull-requests
>>>>>>
>>>>>> On 2019/03/20 02:39:43, Huxing Zhang <hu...@apache.org> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> This pull request[1] is merged without any comments, or following any
>>>>>>> review process.
>>>>>>> Any ideas how to avoid this?
>>>>>>> Can Github support the feature that disable the merge button until a
>>>>>>> committer has approved the pull request?
>>>>>>>
>>>>>>> [1] https://github.com/apache/incubator-dubbo/pull/3693
>>>>>>> --
>>>>>>> Best Regards！
>>>>>>> Huxing
>>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Best Regards！
>>>>> Huxing
>>>>>
>>>>
>>>
>>
>
>
> --
> Best Regards！
> Huxing
Re: Pull request merged without approval
Posted by Ian Luo <ia...@gmail.com>.
If we cannot find a perfect way, then I think we should do it in the social
way as Mark suggested.
Regards,
-Ian.
On Wed, Apr 3, 2019 at 1:09 PM Huxing Zhang <hu...@apache.org> wrote:
> Hi All,
>
> On Wed, Apr 3, 2019 at 11:14 AM jun liu <ke...@gmail.com> wrote:
> >
> > > In some critical cases, one can approve the pull request by
> > > himself/herself with proper comments or commit directly.
> >
> >
> > I think the concern from Mark is quite reasonable, but this particular
> approach Huxing suggests could solve it while at the same it gives the
> community a technical rule to obey.
>
> Actually I found this does not work as expected. I found that:
> - one cannot push to master branch directly
> - The one who raise the pull request can not merge the pull request by
> oneself.
> This will introduce the issue that Mark has pointed. If some critical
> cases, a second committer's approval is required.
> So do you think we should revert the changes? Or just keep it for a while?
>
> >
> > Jun
> >
> > > On Mar 20, 2019, at 10:45 PM, Huxing Zhang <hu...@apache.org> wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Mar 20, 2019 at 6:08 PM Mark Thomas <markt@apache.org <mailto:
> markt@apache.org>> wrote:
> > >>
> > >> I recommend that you enforce this socially rather than technically.
> > >>
> > >> There are times when a committer needs to get something committed (CI
> is
> > >> broken, security fix, correct an obvious bug that is blocking
> something
> > >> else, etc.) and having the flexibility to just commit it can be very
> > >> helpful.
> > >
> > > I think the discussion here does not prevent some one to commit
> > > directly. It is just about if a pull request is raised, it must be
> > > approved.
> > > In some critical cases, one can approve the pull request by
> > > himself/herself with proper comments or commit directly.
> > >
> > >>
> > >> Mark
> > >>
> > >>
> > >> On 20/03/2019 06:37, yuhang xiu wrote:
> > >>> agree. I think this is a very good way to help us standardize the pr
> merge.
> > >>>
> > >>> YunKun Huang <hy...@apache.org> 于2019年3月20日周三 上午11:41写道：
> > >>>
> > >>>> Agree, we should have at least one approval before merging
> > >>>>
> > >>>> On 2019/03/20 03:00:51, Huxing Zhang <hu...@apache.org> wrote:
> > >>>>> Hi,
> > >>>>>
> > >>>>>
> > >>>>> On Wed, Mar 20, 2019 at 10:55 AM YunKun Huang <hy...@apache.org>
> > >>>> wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>> I guess you are talking about "Require pull request reviews before
> > >>>> merging" feature [1]
> > >>>>>
> > >>>>> Cool! That is what I want.
> > >>>>> I am +1 to support enabling this and set the number of required
> approval
> > >>>> to 1.
> > >>>>> How do others think?
> > >>>>>
> > >>>>>>
> > >>>>>> [1]
> > >>>>
> https://help.github.com/en/articles/enabling-required-reviews-for-pull-requests
> > >>>>>>
> > >>>>>> On 2019/03/20 02:39:43, Huxing Zhang <hu...@apache.org> wrote:
> > >>>>>>> Hi,
> > >>>>>>>
> > >>>>>>> This pull request[1] is merged without any comments, or
> following any
> > >>>>>>> review process.
> > >>>>>>> Any ideas how to avoid this?
> > >>>>>>> Can Github support the feature that disable the merge button
> until a
> > >>>>>>> committer has approved the pull request?
> > >>>>>>>
> > >>>>>>> [1] https://github.com/apache/incubator-dubbo/pull/3693
> > >>>>>>> --
> > >>>>>>> Best Regards！
> > >>>>>>> Huxing
> > >>>>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> --
> > >>>>> Best Regards！
> > >>>>> Huxing
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >
> > >
> > > --
> > > Best Regards！
> > > Huxing
> >
>
>
> --
> Best Regards！
> Huxing
>
Re: Pull request merged without approval
Posted by Huxing Zhang <hu...@apache.org>.
Hi All,
On Wed, Apr 3, 2019 at 11:14 AM jun liu <ke...@gmail.com> wrote:
>
> > In some critical cases, one can approve the pull request by
> > himself/herself with proper comments or commit directly.
>
>
> I think the concern from Mark is quite reasonable, but this particular approach Huxing suggests could solve it while at the same it gives the community a technical rule to obey.
Actually I found this does not work as expected. I found that:
- one cannot push to master branch directly
- The one who raise the pull request can not merge the pull request by oneself.
This will introduce the issue that Mark has pointed. If some critical
cases, a second committer's approval is required.
So do you think we should revert the changes? Or just keep it for a while?
>
> Jun
>
> > On Mar 20, 2019, at 10:45 PM, Huxing Zhang <hu...@apache.org> wrote:
> >
> > Hi,
> >
> > On Wed, Mar 20, 2019 at 6:08 PM Mark Thomas <markt@apache.org <ma...@apache.org>> wrote:
> >>
> >> I recommend that you enforce this socially rather than technically.
> >>
> >> There are times when a committer needs to get something committed (CI is
> >> broken, security fix, correct an obvious bug that is blocking something
> >> else, etc.) and having the flexibility to just commit it can be very
> >> helpful.
> >
> > I think the discussion here does not prevent some one to commit
> > directly. It is just about if a pull request is raised, it must be
> > approved.
> > In some critical cases, one can approve the pull request by
> > himself/herself with proper comments or commit directly.
> >
> >>
> >> Mark
> >>
> >>
> >> On 20/03/2019 06:37, yuhang xiu wrote:
> >>> agree. I think this is a very good way to help us standardize the pr merge.
> >>>
> >>> YunKun Huang <hy...@apache.org> 于2019年3月20日周三 上午11:41写道：
> >>>
> >>>> Agree, we should have at least one approval before merging
> >>>>
> >>>> On 2019/03/20 03:00:51, Huxing Zhang <hu...@apache.org> wrote:
> >>>>> Hi,
> >>>>>
> >>>>>
> >>>>> On Wed, Mar 20, 2019 at 10:55 AM YunKun Huang <hy...@apache.org>
> >>>> wrote:
> >>>>>>
> >>>>>>
> >>>>>> I guess you are talking about "Require pull request reviews before
> >>>> merging" feature [1]
> >>>>>
> >>>>> Cool! That is what I want.
> >>>>> I am +1 to support enabling this and set the number of required approval
> >>>> to 1.
> >>>>> How do others think?
> >>>>>
> >>>>>>
> >>>>>> [1]
> >>>> https://help.github.com/en/articles/enabling-required-reviews-for-pull-requests
> >>>>>>
> >>>>>> On 2019/03/20 02:39:43, Huxing Zhang <hu...@apache.org> wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> This pull request[1] is merged without any comments, or following any
> >>>>>>> review process.
> >>>>>>> Any ideas how to avoid this?
> >>>>>>> Can Github support the feature that disable the merge button until a
> >>>>>>> committer has approved the pull request?
> >>>>>>>
> >>>>>>> [1] https://github.com/apache/incubator-dubbo/pull/3693
> >>>>>>> --
> >>>>>>> Best Regards！
> >>>>>>> Huxing
> >>>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> --
> >>>>> Best Regards！
> >>>>> Huxing
> >>>>>
> >>>>
> >>>
> >>
> >
> >
> > --
> > Best Regards！
> > Huxing
>
--
Best Regards！
Huxing