You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by David Sidrane <da...@nscdg.com> on 2022/02/14 17:28:41 UTC
[DISCUSS]: Self merge and Single company/organization merge gating
I am opening this discussion to widen the audience for feedback on the
rules for merging. The original interexchange was in [1]
1 ) Given the geographical and time differences should we consider that
once a review with approval* has been done the PR’s author can merge it?
2) Should we consider requiring more than one organization/company to
approve a PR before it is merged.
Background:
If only one company does a review, that can lead to undo influence as noted
as the "The Enemies"
<https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md> of
the INVIOLABLES [2]:
“Focus on the values of the organization, not the values of the Open Source
project. Need to support both.”
*The change would define PR approval as requiring a review/approval from
more than the members of one company or organization.
I am in favor of both these changes.
David
[1]
https://github.com/apache/incubator-nuttx/pull/5320#issuecomment-1020379240
[2] https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md
Re: [DISCUSS]: Self merge and Single company/organization merge gating
Posted by Petro Karashchenko <pe...@gmail.com>.
Hi Alin,
The original discussion came from
https://github.com/apache/incubator-nuttx/pull/5320#issuecomment-1020379240
after I merged my PR that was approved my Alan. Me and Alan do not
represent the same organization, so probably we need to clarify what the
"self merge" is.
Best regards,
Petro
On Fri, Feb 18, 2022, 10:29 AM Alin Jerpelea <je...@gmail.com> wrote:
> Hi Petro,
>
> for me self merge meant also without review from someone else
>
> In the mentioned cases the Author had at least 1 review from someone else
> (even from another company)
>
> Best regards
> Alin
>
>
> On Fri, Feb 18, 2022 at 8:52 AM Petro Karashchenko <
> petro.karashchenko@gmail.com> wrote:
>
> > Hi,
> >
> > I agree that auto-merge should not be used.
> >
> > But I disagree that "as it is now since almost all patches follow the
> > rule and seldom someone self-merges a patch". Here is a list of
> > patches that were self merged last 12 days:
> > https://github.com/apache/incubator-nuttx/pull/5474
> > https://github.com/apache/incubator-nuttx/pull/5445
> > https://github.com/apache/incubator-nuttx/pull/5444
> > https://github.com/apache/incubator-nuttx/pull/5428
> > https://github.com/apache/incubator-nuttx/pull/5425
> > https://github.com/apache/incubator-nuttx/pull/5508
> >
> > All of the PRs have relatively low complexity and do not touch the
> > core functionality so I'm ok with self-merging in such cases.
> >
> > Best regards,
> > Petro
> >
> > пт, 18 лют. 2022 р. о 08:35 Alin.Jerpelea@sony.com
> > <Al...@sony.com> пише:
> > >
> > > Hi all
> > >
> > > In my opinion we should not use the auto merge functionality since most
> > of the time there is at least 1 of us active at any time and the amount
> of
> > patches is not comparable to EX: Google.
> > >
> > > I think that the merge policy is fine as it is now since almost all
> > patches follow the rule and seldom someone self-merges a patch.
> > >
> > > Also we should note that in case some patches land accidentally in the
> > master branch we can always revert them if it is necessary
> > >
> > > Best regards
> > > Alin
> > >
> > > -----Original Message-----
> > > From: David Sidrane <da...@nscdg.com>
> > > Sent: den 17 februari 2022 22:31
> > > To: dev@nuttx.apache.org
> > > Subject: RE: [DISCUSS]: Self merge and Single company/organization
> merge
> > gating
> > >
> > > On Self merge:
> > >
> > > As Nathan pointed out, it is more about time zones then merge velocity.
> > > However, using a backport only methodology requires an upstream merge
> > before the work can be backported with least effort and adds a serial
> > delay. It would be ideal to reduces the CI quantum delay this as much as
> we
> > can.
> > >
> > > GH has a setting to merge on successful CI after approval. It is lit by
> > the approver. This removes the polling for completion of CI.
> > > If this can be configured it reduces the polling for both approver and
> > author. If it can not be configured in our repos, then self merge is the
> > next best thing.
> > >
> > > I am not trying to circumvent the review process at all - just remove
> > the idle time imposed by the process that is sampling related.
> > >
> > > > an approval from outside of the company/organization then the author
> > > > can do the merge. For complex changes the person outside the
> > > > organization should perform the merge even if there are more than 1
> > > > approval from inside the company/organization.
> > >
> > > I agree.
> > >
> > > David
> > >
> > > -----Original Message-----
> > > From: Petro Karashchenko <pe...@gmail.com>
> > > Sent: Thursday, February 17, 2022 1:01 PM
> > > To: dev@nuttx.apache.org
> > > Subject: Re: [DISCUSS]: Self merge and Single company/organization
> merge
> > gating
> > >
> > > Hello,
> > >
> > > Regarding PRs megre by the author: I think that if the changes are
> > relatively simple (again that is very subjective, but I hope that people
> > with merge rights have more or less the same common sense of
> > > it) and there is an approval from outside of the company/organization
> > then the author can do the merge. For complex changes the person outside
> > the organization should perform the merge even if there are more than 1
> > approval from inside the company/organization.
> > >
> > > In this way reviewers can perform reviews with better quality and if
> > someone "forget" to press the "rebase & merge" button because for example
> > CI is still running and that is the end of working day, then the author
> can
> > press that button and not do extra tagging in PRs. I vote to make that
> > process usable for people and sacrifice bureaucracy in the places where
> it
> > is possible.
> > >
> > > Best regards,
> > > Petro
> > >
> > > вт, 15 лют. 2022 р. о 18:26 Nathan Hartman <ha...@gmail.com>
> > пише:
> > > >
> > > > On Mon, Feb 14, 2022 at 2:01 PM Brennan Ashton
> > > > <ba...@brennanashton.com> wrote:
> > > > > > Background:
> > > > > I am generally opposed to both of these. It is quite rare that we
> > > > > need a crazy fast merge turn around on a PR. And if something is
> > > > > approved and straight up broken in master that needs to get in then
> > > > > I think forgiveness can be used to self merge.
> > > > >
> > > > >
> > > > > I also generally do not have a big issue about people from the same
> > > > > company reviewing and merging. I could see the arguments for shared
> > > > > code but then I
> > > > > think we are nitpicking. I prefer the velocity with a few oops
> that
> > > > > can
> > > > > be reverted along the way if needed. There is also parts of the
> > > > > code base where the best people to review are on the same company.
> > > > >
> > > > >
> > > > > I think most of the concerns here are best addressed not by process
> > > > > but increasing the number of contributors who can participate.
> (more
> > > > > committers and PPMC)
> > > >
> > > > Feel free to correct me if I'm mistaken, but I think David is
> bringing
> > > > this up because of time zones.
> > > >
> > > > Indeed, most of the PR merging activity seems to occur during what I
> > > > would call nighttime or early morning, and I think that might be more
> > > > pronounced in David's time zone.
> > > >
> > > > Still, I think things have been working well, more or less, and I
> > > > don't think we need to make up any new rules right now.
> > > >
> > > > Instead, I would only urge committers to give complex PRs 12-24 hours
> > > > to percolate, even if there's an approving review, so other time
> zones
> > > > have a chance to look at them.
> > > >
> > > > Obviously that doesn't apply if it's urgent. For example, if the
> build
> > > > is broken and people can't get work done, or a serious error was
> > > > merged and needs to be reverted ASAP, don't wait, do it!
> > > >
> > > > Also, it's not necessary to delay for trivial PRs.
> > > >
> > > > What are the definitions of "complex," "trivial," "urgent," etc? I
> > > > say, committers should just use their best judgment and try to find a
> > > > good balance. Don't rush too much, don't delay too much. :-)
> > > >
> > > > David brings up a good point about time zones and we do have to
> > > > remember that NuttX is a global project, and I think that's the main
> > > > point to keep in mind.
> > > >
> > > > To Brennan's last point: as we grow the committer base, we are likely
> > > > to have more people in more time zones and more PR reviewers, so this
> > > > should become less of a concern.
> > > >
> > > > Nathan
> >
>
Re: [DISCUSS]: Self merge and Single company/organization merge gating
Posted by Alin Jerpelea <je...@gmail.com>.
Hi Petro,
for me self merge meant also without review from someone else
In the mentioned cases the Author had at least 1 review from someone else
(even from another company)
Best regards
Alin
On Fri, Feb 18, 2022 at 8:52 AM Petro Karashchenko <
petro.karashchenko@gmail.com> wrote:
> Hi,
>
> I agree that auto-merge should not be used.
>
> But I disagree that "as it is now since almost all patches follow the
> rule and seldom someone self-merges a patch". Here is a list of
> patches that were self merged last 12 days:
> https://github.com/apache/incubator-nuttx/pull/5474
> https://github.com/apache/incubator-nuttx/pull/5445
> https://github.com/apache/incubator-nuttx/pull/5444
> https://github.com/apache/incubator-nuttx/pull/5428
> https://github.com/apache/incubator-nuttx/pull/5425
> https://github.com/apache/incubator-nuttx/pull/5508
>
> All of the PRs have relatively low complexity and do not touch the
> core functionality so I'm ok with self-merging in such cases.
>
> Best regards,
> Petro
>
> пт, 18 лют. 2022 р. о 08:35 Alin.Jerpelea@sony.com
> <Al...@sony.com> пише:
> >
> > Hi all
> >
> > In my opinion we should not use the auto merge functionality since most
> of the time there is at least 1 of us active at any time and the amount of
> patches is not comparable to EX: Google.
> >
> > I think that the merge policy is fine as it is now since almost all
> patches follow the rule and seldom someone self-merges a patch.
> >
> > Also we should note that in case some patches land accidentally in the
> master branch we can always revert them if it is necessary
> >
> > Best regards
> > Alin
> >
> > -----Original Message-----
> > From: David Sidrane <da...@nscdg.com>
> > Sent: den 17 februari 2022 22:31
> > To: dev@nuttx.apache.org
> > Subject: RE: [DISCUSS]: Self merge and Single company/organization merge
> gating
> >
> > On Self merge:
> >
> > As Nathan pointed out, it is more about time zones then merge velocity.
> > However, using a backport only methodology requires an upstream merge
> before the work can be backported with least effort and adds a serial
> delay. It would be ideal to reduces the CI quantum delay this as much as we
> can.
> >
> > GH has a setting to merge on successful CI after approval. It is lit by
> the approver. This removes the polling for completion of CI.
> > If this can be configured it reduces the polling for both approver and
> author. If it can not be configured in our repos, then self merge is the
> next best thing.
> >
> > I am not trying to circumvent the review process at all - just remove
> the idle time imposed by the process that is sampling related.
> >
> > > an approval from outside of the company/organization then the author
> > > can do the merge. For complex changes the person outside the
> > > organization should perform the merge even if there are more than 1
> > > approval from inside the company/organization.
> >
> > I agree.
> >
> > David
> >
> > -----Original Message-----
> > From: Petro Karashchenko <pe...@gmail.com>
> > Sent: Thursday, February 17, 2022 1:01 PM
> > To: dev@nuttx.apache.org
> > Subject: Re: [DISCUSS]: Self merge and Single company/organization merge
> gating
> >
> > Hello,
> >
> > Regarding PRs megre by the author: I think that if the changes are
> relatively simple (again that is very subjective, but I hope that people
> with merge rights have more or less the same common sense of
> > it) and there is an approval from outside of the company/organization
> then the author can do the merge. For complex changes the person outside
> the organization should perform the merge even if there are more than 1
> approval from inside the company/organization.
> >
> > In this way reviewers can perform reviews with better quality and if
> someone "forget" to press the "rebase & merge" button because for example
> CI is still running and that is the end of working day, then the author can
> press that button and not do extra tagging in PRs. I vote to make that
> process usable for people and sacrifice bureaucracy in the places where it
> is possible.
> >
> > Best regards,
> > Petro
> >
> > вт, 15 лют. 2022 р. о 18:26 Nathan Hartman <ha...@gmail.com>
> пише:
> > >
> > > On Mon, Feb 14, 2022 at 2:01 PM Brennan Ashton
> > > <ba...@brennanashton.com> wrote:
> > > > > Background:
> > > > I am generally opposed to both of these. It is quite rare that we
> > > > need a crazy fast merge turn around on a PR. And if something is
> > > > approved and straight up broken in master that needs to get in then
> > > > I think forgiveness can be used to self merge.
> > > >
> > > >
> > > > I also generally do not have a big issue about people from the same
> > > > company reviewing and merging. I could see the arguments for shared
> > > > code but then I
> > > > think we are nitpicking. I prefer the velocity with a few oops that
> > > > can
> > > > be reverted along the way if needed. There is also parts of the
> > > > code base where the best people to review are on the same company.
> > > >
> > > >
> > > > I think most of the concerns here are best addressed not by process
> > > > but increasing the number of contributors who can participate. (more
> > > > committers and PPMC)
> > >
> > > Feel free to correct me if I'm mistaken, but I think David is bringing
> > > this up because of time zones.
> > >
> > > Indeed, most of the PR merging activity seems to occur during what I
> > > would call nighttime or early morning, and I think that might be more
> > > pronounced in David's time zone.
> > >
> > > Still, I think things have been working well, more or less, and I
> > > don't think we need to make up any new rules right now.
> > >
> > > Instead, I would only urge committers to give complex PRs 12-24 hours
> > > to percolate, even if there's an approving review, so other time zones
> > > have a chance to look at them.
> > >
> > > Obviously that doesn't apply if it's urgent. For example, if the build
> > > is broken and people can't get work done, or a serious error was
> > > merged and needs to be reverted ASAP, don't wait, do it!
> > >
> > > Also, it's not necessary to delay for trivial PRs.
> > >
> > > What are the definitions of "complex," "trivial," "urgent," etc? I
> > > say, committers should just use their best judgment and try to find a
> > > good balance. Don't rush too much, don't delay too much. :-)
> > >
> > > David brings up a good point about time zones and we do have to
> > > remember that NuttX is a global project, and I think that's the main
> > > point to keep in mind.
> > >
> > > To Brennan's last point: as we grow the committer base, we are likely
> > > to have more people in more time zones and more PR reviewers, so this
> > > should become less of a concern.
> > >
> > > Nathan
>
Re: [DISCUSS]: Self merge and Single company/organization merge gating
Posted by Nathan Hartman <ha...@gmail.com>.
On Fri, Feb 18, 2022 at 5:27 AM Xiang Xiao <xi...@gmail.com> wrote:
>
> On Fri, Feb 18, 2022 at 3:52 PM Petro Karashchenko <
> petro.karashchenko@gmail.com> wrote:
>
> > Hi,
> >
> > I agree that auto-merge should not be used.
> >
> > But I disagree that "as it is now since almost all patches follow the
> > rule and seldom someone self-merges a patch". Here is a list of
> > patches that were self merged last 12 days:
> > https://github.com/apache/incubator-nuttx/pull/5474
> > https://github.com/apache/incubator-nuttx/pull/5445
> > https://github.com/apache/incubator-nuttx/pull/5444
> > https://github.com/apache/incubator-nuttx/pull/5428
> > https://github.com/apache/incubator-nuttx/pull/5425
> > https://github.com/apache/incubator-nuttx/pull/5508
> >
> > All of the PRs have relatively low complexity and do not touch the
> > core functionality so I'm ok with self-merging in such cases.
> >
> >
> All come from me, I think:(. All these patch meet the following criteria:
>
> 1. Approved by some people
> 2. Either the change is very minor
> 3. Or the PR was sent a few days ago.
I think that part of the pressure to merge PRs quickly might be
related to old open PRs that are lingering in the list.
We have about 25 open PRs that have not been updated in 6 months or
more (almost 50% of open PRs).
Perhaps it would be helpful if there was some PR cleanup to remove old
PRs that meet some criteria such as:
1. Not updated for 6 months or more.
2. Obsolete, incorrect, or unmergeable.
3. Not important work that should be preserved/revived.
I suggest that authors of old PRs please look through your PRs and see
if you want to revive them or if it's better to close them.
Cheers,
Nathan
Re: [DISCUSS]: Self merge and Single company/organization merge gating
Posted by Xiang Xiao <xi...@gmail.com>.
On Fri, Feb 18, 2022 at 3:52 PM Petro Karashchenko <
petro.karashchenko@gmail.com> wrote:
> Hi,
>
> I agree that auto-merge should not be used.
>
> But I disagree that "as it is now since almost all patches follow the
> rule and seldom someone self-merges a patch". Here is a list of
> patches that were self merged last 12 days:
> https://github.com/apache/incubator-nuttx/pull/5474
> https://github.com/apache/incubator-nuttx/pull/5445
> https://github.com/apache/incubator-nuttx/pull/5444
> https://github.com/apache/incubator-nuttx/pull/5428
> https://github.com/apache/incubator-nuttx/pull/5425
> https://github.com/apache/incubator-nuttx/pull/5508
>
> All of the PRs have relatively low complexity and do not touch the
> core functionality so I'm ok with self-merging in such cases.
>
>
All come from me, I think:(. All these patch meet the following criteria:
1. Approved by some people
2. Either the change is very minor
3. Or the PR was sent a few days ago.
> Best regards,
> Petro
>
> пт, 18 лют. 2022 р. о 08:35 Alin.Jerpelea@sony.com
> <Al...@sony.com> пише:
> >
> > Hi all
> >
> > In my opinion we should not use the auto merge functionality since most
> of the time there is at least 1 of us active at any time and the amount of
> patches is not comparable to EX: Google.
> >
> > I think that the merge policy is fine as it is now since almost all
> patches follow the rule and seldom someone self-merges a patch.
> >
> > Also we should note that in case some patches land accidentally in the
> master branch we can always revert them if it is necessary
> >
> > Best regards
> > Alin
> >
> > -----Original Message-----
> > From: David Sidrane <da...@nscdg.com>
> > Sent: den 17 februari 2022 22:31
> > To: dev@nuttx.apache.org
> > Subject: RE: [DISCUSS]: Self merge and Single company/organization merge
> gating
> >
> > On Self merge:
> >
> > As Nathan pointed out, it is more about time zones then merge velocity.
> > However, using a backport only methodology requires an upstream merge
> before the work can be backported with least effort and adds a serial
> delay. It would be ideal to reduces the CI quantum delay this as much as we
> can.
> >
> > GH has a setting to merge on successful CI after approval. It is lit by
> the approver. This removes the polling for completion of CI.
> > If this can be configured it reduces the polling for both approver and
> author. If it can not be configured in our repos, then self merge is the
> next best thing.
> >
> > I am not trying to circumvent the review process at all - just remove
> the idle time imposed by the process that is sampling related.
> >
> > > an approval from outside of the company/organization then the author
> > > can do the merge. For complex changes the person outside the
> > > organization should perform the merge even if there are more than 1
> > > approval from inside the company/organization.
> >
> > I agree.
> >
> > David
> >
> > -----Original Message-----
> > From: Petro Karashchenko <pe...@gmail.com>
> > Sent: Thursday, February 17, 2022 1:01 PM
> > To: dev@nuttx.apache.org
> > Subject: Re: [DISCUSS]: Self merge and Single company/organization merge
> gating
> >
> > Hello,
> >
> > Regarding PRs megre by the author: I think that if the changes are
> relatively simple (again that is very subjective, but I hope that people
> with merge rights have more or less the same common sense of
> > it) and there is an approval from outside of the company/organization
> then the author can do the merge. For complex changes the person outside
> the organization should perform the merge even if there are more than 1
> approval from inside the company/organization.
> >
> > In this way reviewers can perform reviews with better quality and if
> someone "forget" to press the "rebase & merge" button because for example
> CI is still running and that is the end of working day, then the author can
> press that button and not do extra tagging in PRs. I vote to make that
> process usable for people and sacrifice bureaucracy in the places where it
> is possible.
> >
> > Best regards,
> > Petro
> >
> > вт, 15 лют. 2022 р. о 18:26 Nathan Hartman <ha...@gmail.com>
> пише:
> > >
> > > On Mon, Feb 14, 2022 at 2:01 PM Brennan Ashton
> > > <ba...@brennanashton.com> wrote:
> > > > > Background:
> > > > I am generally opposed to both of these. It is quite rare that we
> > > > need a crazy fast merge turn around on a PR. And if something is
> > > > approved and straight up broken in master that needs to get in then
> > > > I think forgiveness can be used to self merge.
> > > >
> > > >
> > > > I also generally do not have a big issue about people from the same
> > > > company reviewing and merging. I could see the arguments for shared
> > > > code but then I
> > > > think we are nitpicking. I prefer the velocity with a few oops that
> > > > can
> > > > be reverted along the way if needed. There is also parts of the
> > > > code base where the best people to review are on the same company.
> > > >
> > > >
> > > > I think most of the concerns here are best addressed not by process
> > > > but increasing the number of contributors who can participate. (more
> > > > committers and PPMC)
> > >
> > > Feel free to correct me if I'm mistaken, but I think David is bringing
> > > this up because of time zones.
> > >
> > > Indeed, most of the PR merging activity seems to occur during what I
> > > would call nighttime or early morning, and I think that might be more
> > > pronounced in David's time zone.
> > >
> > > Still, I think things have been working well, more or less, and I
> > > don't think we need to make up any new rules right now.
> > >
> > > Instead, I would only urge committers to give complex PRs 12-24 hours
> > > to percolate, even if there's an approving review, so other time zones
> > > have a chance to look at them.
> > >
> > > Obviously that doesn't apply if it's urgent. For example, if the build
> > > is broken and people can't get work done, or a serious error was
> > > merged and needs to be reverted ASAP, don't wait, do it!
> > >
> > > Also, it's not necessary to delay for trivial PRs.
> > >
> > > What are the definitions of "complex," "trivial," "urgent," etc? I
> > > say, committers should just use their best judgment and try to find a
> > > good balance. Don't rush too much, don't delay too much. :-)
> > >
> > > David brings up a good point about time zones and we do have to
> > > remember that NuttX is a global project, and I think that's the main
> > > point to keep in mind.
> > >
> > > To Brennan's last point: as we grow the committer base, we are likely
> > > to have more people in more time zones and more PR reviewers, so this
> > > should become less of a concern.
> > >
> > > Nathan
>
Re: [DISCUSS]: Self merge and Single company/organization merge gating
Posted by Petro Karashchenko <pe...@gmail.com>.
Hi,
I agree that auto-merge should not be used.
But I disagree that "as it is now since almost all patches follow the
rule and seldom someone self-merges a patch". Here is a list of
patches that were self merged last 12 days:
https://github.com/apache/incubator-nuttx/pull/5474
https://github.com/apache/incubator-nuttx/pull/5445
https://github.com/apache/incubator-nuttx/pull/5444
https://github.com/apache/incubator-nuttx/pull/5428
https://github.com/apache/incubator-nuttx/pull/5425
https://github.com/apache/incubator-nuttx/pull/5508
All of the PRs have relatively low complexity and do not touch the
core functionality so I'm ok with self-merging in such cases.
Best regards,
Petro
пт, 18 лют. 2022 р. о 08:35 Alin.Jerpelea@sony.com
<Al...@sony.com> пише:
>
> Hi all
>
> In my opinion we should not use the auto merge functionality since most of the time there is at least 1 of us active at any time and the amount of patches is not comparable to EX: Google.
>
> I think that the merge policy is fine as it is now since almost all patches follow the rule and seldom someone self-merges a patch.
>
> Also we should note that in case some patches land accidentally in the master branch we can always revert them if it is necessary
>
> Best regards
> Alin
>
> -----Original Message-----
> From: David Sidrane <da...@nscdg.com>
> Sent: den 17 februari 2022 22:31
> To: dev@nuttx.apache.org
> Subject: RE: [DISCUSS]: Self merge and Single company/organization merge gating
>
> On Self merge:
>
> As Nathan pointed out, it is more about time zones then merge velocity.
> However, using a backport only methodology requires an upstream merge before the work can be backported with least effort and adds a serial delay. It would be ideal to reduces the CI quantum delay this as much as we can.
>
> GH has a setting to merge on successful CI after approval. It is lit by the approver. This removes the polling for completion of CI.
> If this can be configured it reduces the polling for both approver and author. If it can not be configured in our repos, then self merge is the next best thing.
>
> I am not trying to circumvent the review process at all - just remove the idle time imposed by the process that is sampling related.
>
> > an approval from outside of the company/organization then the author
> > can do the merge. For complex changes the person outside the
> > organization should perform the merge even if there are more than 1
> > approval from inside the company/organization.
>
> I agree.
>
> David
>
> -----Original Message-----
> From: Petro Karashchenko <pe...@gmail.com>
> Sent: Thursday, February 17, 2022 1:01 PM
> To: dev@nuttx.apache.org
> Subject: Re: [DISCUSS]: Self merge and Single company/organization merge gating
>
> Hello,
>
> Regarding PRs megre by the author: I think that if the changes are relatively simple (again that is very subjective, but I hope that people with merge rights have more or less the same common sense of
> it) and there is an approval from outside of the company/organization then the author can do the merge. For complex changes the person outside the organization should perform the merge even if there are more than 1 approval from inside the company/organization.
>
> In this way reviewers can perform reviews with better quality and if someone "forget" to press the "rebase & merge" button because for example CI is still running and that is the end of working day, then the author can press that button and not do extra tagging in PRs. I vote to make that process usable for people and sacrifice bureaucracy in the places where it is possible.
>
> Best regards,
> Petro
>
> вт, 15 лют. 2022 р. о 18:26 Nathan Hartman <ha...@gmail.com> пише:
> >
> > On Mon, Feb 14, 2022 at 2:01 PM Brennan Ashton
> > <ba...@brennanashton.com> wrote:
> > > > Background:
> > > I am generally opposed to both of these. It is quite rare that we
> > > need a crazy fast merge turn around on a PR. And if something is
> > > approved and straight up broken in master that needs to get in then
> > > I think forgiveness can be used to self merge.
> > >
> > >
> > > I also generally do not have a big issue about people from the same
> > > company reviewing and merging. I could see the arguments for shared
> > > code but then I
> > > think we are nitpicking. I prefer the velocity with a few oops that
> > > can
> > > be reverted along the way if needed. There is also parts of the
> > > code base where the best people to review are on the same company.
> > >
> > >
> > > I think most of the concerns here are best addressed not by process
> > > but increasing the number of contributors who can participate. (more
> > > committers and PPMC)
> >
> > Feel free to correct me if I'm mistaken, but I think David is bringing
> > this up because of time zones.
> >
> > Indeed, most of the PR merging activity seems to occur during what I
> > would call nighttime or early morning, and I think that might be more
> > pronounced in David's time zone.
> >
> > Still, I think things have been working well, more or less, and I
> > don't think we need to make up any new rules right now.
> >
> > Instead, I would only urge committers to give complex PRs 12-24 hours
> > to percolate, even if there's an approving review, so other time zones
> > have a chance to look at them.
> >
> > Obviously that doesn't apply if it's urgent. For example, if the build
> > is broken and people can't get work done, or a serious error was
> > merged and needs to be reverted ASAP, don't wait, do it!
> >
> > Also, it's not necessary to delay for trivial PRs.
> >
> > What are the definitions of "complex," "trivial," "urgent," etc? I
> > say, committers should just use their best judgment and try to find a
> > good balance. Don't rush too much, don't delay too much. :-)
> >
> > David brings up a good point about time zones and we do have to
> > remember that NuttX is a global project, and I think that's the main
> > point to keep in mind.
> >
> > To Brennan's last point: as we grow the committer base, we are likely
> > to have more people in more time zones and more PR reviewers, so this
> > should become less of a concern.
> >
> > Nathan
RE: [DISCUSS]: Self merge and Single company/organization merge gating
Posted by "Alin.Jerpelea@sony.com" <Al...@sony.com>.
Hi all
In my opinion we should not use the auto merge functionality since most of the time there is at least 1 of us active at any time and the amount of patches is not comparable to EX: Google.
I think that the merge policy is fine as it is now since almost all patches follow the rule and seldom someone self-merges a patch.
Also we should note that in case some patches land accidentally in the master branch we can always revert them if it is necessary
Best regards
Alin
-----Original Message-----
From: David Sidrane <da...@nscdg.com>
Sent: den 17 februari 2022 22:31
To: dev@nuttx.apache.org
Subject: RE: [DISCUSS]: Self merge and Single company/organization merge gating
On Self merge:
As Nathan pointed out, it is more about time zones then merge velocity.
However, using a backport only methodology requires an upstream merge before the work can be backported with least effort and adds a serial delay. It would be ideal to reduces the CI quantum delay this as much as we can.
GH has a setting to merge on successful CI after approval. It is lit by the approver. This removes the polling for completion of CI.
If this can be configured it reduces the polling for both approver and author. If it can not be configured in our repos, then self merge is the next best thing.
I am not trying to circumvent the review process at all - just remove the idle time imposed by the process that is sampling related.
> an approval from outside of the company/organization then the author
> can do the merge. For complex changes the person outside the
> organization should perform the merge even if there are more than 1
> approval from inside the company/organization.
I agree.
David
-----Original Message-----
From: Petro Karashchenko <pe...@gmail.com>
Sent: Thursday, February 17, 2022 1:01 PM
To: dev@nuttx.apache.org
Subject: Re: [DISCUSS]: Self merge and Single company/organization merge gating
Hello,
Regarding PRs megre by the author: I think that if the changes are relatively simple (again that is very subjective, but I hope that people with merge rights have more or less the same common sense of
it) and there is an approval from outside of the company/organization then the author can do the merge. For complex changes the person outside the organization should perform the merge even if there are more than 1 approval from inside the company/organization.
In this way reviewers can perform reviews with better quality and if someone "forget" to press the "rebase & merge" button because for example CI is still running and that is the end of working day, then the author can press that button and not do extra tagging in PRs. I vote to make that process usable for people and sacrifice bureaucracy in the places where it is possible.
Best regards,
Petro
вт, 15 лют. 2022 р. о 18:26 Nathan Hartman <ha...@gmail.com> пише:
>
> On Mon, Feb 14, 2022 at 2:01 PM Brennan Ashton
> <ba...@brennanashton.com> wrote:
> > > Background:
> > I am generally opposed to both of these. It is quite rare that we
> > need a crazy fast merge turn around on a PR. And if something is
> > approved and straight up broken in master that needs to get in then
> > I think forgiveness can be used to self merge.
> >
> >
> > I also generally do not have a big issue about people from the same
> > company reviewing and merging. I could see the arguments for shared
> > code but then I
> > think we are nitpicking. I prefer the velocity with a few oops that
> > can
> > be reverted along the way if needed. There is also parts of the
> > code base where the best people to review are on the same company.
> >
> >
> > I think most of the concerns here are best addressed not by process
> > but increasing the number of contributors who can participate. (more
> > committers and PPMC)
>
> Feel free to correct me if I'm mistaken, but I think David is bringing
> this up because of time zones.
>
> Indeed, most of the PR merging activity seems to occur during what I
> would call nighttime or early morning, and I think that might be more
> pronounced in David's time zone.
>
> Still, I think things have been working well, more or less, and I
> don't think we need to make up any new rules right now.
>
> Instead, I would only urge committers to give complex PRs 12-24 hours
> to percolate, even if there's an approving review, so other time zones
> have a chance to look at them.
>
> Obviously that doesn't apply if it's urgent. For example, if the build
> is broken and people can't get work done, or a serious error was
> merged and needs to be reverted ASAP, don't wait, do it!
>
> Also, it's not necessary to delay for trivial PRs.
>
> What are the definitions of "complex," "trivial," "urgent," etc? I
> say, committers should just use their best judgment and try to find a
> good balance. Don't rush too much, don't delay too much. :-)
>
> David brings up a good point about time zones and we do have to
> remember that NuttX is a global project, and I think that's the main
> point to keep in mind.
>
> To Brennan's last point: as we grow the committer base, we are likely
> to have more people in more time zones and more PR reviewers, so this
> should become less of a concern.
>
> Nathan
Re: RE: [DISCUSS]: Self merge and Single company/organization merge gating
Posted by Jukka Laitinen <ju...@iki.fi>.
Just my two cents,
I like the self-merge policy. I am using it in all the repos where I can decide.
My take on it is that it leaves the final responsibility for the change to the PR creator. And at the same time it doesn't take away the authority of the approvers.
While the responsibility is left to the original PR creator, it actually adds one more decision point; the creator of the PR has one more chance to think again. I don't like the automatic merge on approval, I always want that the creator of the code presses the button, hence assuming responsibility.
And as a + side, it can also speed up the merging.
There is one thing that must be forced though; if the PR is changed after approval, a new approval must be required before self-merge can be done.
Br,
Jukka
David Sidrane kirjoitti torstai 17. helmikuuta 2022:
> On Self merge:
>
> As Nathan pointed out, it is more about time zones then merge velocity.
> However, using a backport only methodology requires an upstream merge before
> the work can be backported with least effort and adds a serial delay. It
> would be ideal to reduces the CI quantum delay this as much as we can.
>
> GH has a setting to merge on successful CI after approval. It is lit by the
> approver. This removes the polling for completion of CI.
> If this can be configured it reduces the polling for both approver and
> author. If it can not be configured in our repos, then self merge is the
> next best thing.
>
> I am not trying to circumvent the review process at all - just remove the
> idle time imposed by the process that is sampling related.
>
> > an approval from outside of the company/organization then the author can
> > do the merge. For complex changes the person outside the organization
> > should perform the merge even if there are more than 1 approval from
> > inside the company/organization.
>
> I agree.
>
> David
>
> -----Original Message-----
> From: Petro Karashchenko <pe...@gmail.com>
> Sent: Thursday, February 17, 2022 1:01 PM
> To: dev@nuttx.apache.org
> Subject: Re: [DISCUSS]: Self merge and Single company/organization merge
> gating
>
> Hello,
>
> Regarding PRs megre by the author: I think that if the changes are
> relatively simple (again that is very subjective, but I hope that people
> with merge rights have more or less the same common sense of
> it) and there is an approval from outside of the company/organization then
> the author can do the merge. For complex changes the person outside the
> organization should perform the merge even if there are more than 1 approval
> from inside the company/organization.
>
> In this way reviewers can perform reviews with better quality and if someone
> "forget" to press the "rebase & merge" button because for example CI is
> still running and that is the end of working day, then the author can press
> that button and not do extra tagging in PRs. I vote to make that process
> usable for people and sacrifice bureaucracy in the places where it is
> possible.
>
> Best regards,
> Petro
>
> вт, 15 лют. 2022 р. о 18:26 Nathan Hartman <ha...@gmail.com> пише:
> >
> > On Mon, Feb 14, 2022 at 2:01 PM Brennan Ashton
> > <ba...@brennanashton.com> wrote:
> > > > Background:
> > > I am generally opposed to both of these. It is quite rare that we
> > > need a crazy fast merge turn around on a PR. And if something is
> > > approved and straight up broken in master that needs to get in then
> > > I think forgiveness can be used to self merge.
> > >
> > >
> > > I also generally do not have a big issue about people from the same
> > > company reviewing and merging. I could see the arguments for shared code
> > > but then I
> > > think we are nitpicking. I prefer the velocity with a few oops that
> > > can
> > > be reverted along the way if needed. There is also parts of the
> > > code base where the best people to review are on the same company.
> > >
> > >
> > > I think most of the concerns here are best addressed not by process
> > > but increasing the number of contributors who can participate. (more
> > > committers and PPMC)
> >
> > Feel free to correct me if I'm mistaken, but I think David is bringing
> > this up because of time zones.
> >
> > Indeed, most of the PR merging activity seems to occur during what I
> > would call nighttime or early morning, and I think that might be more
> > pronounced in David's time zone.
> >
> > Still, I think things have been working well, more or less, and I
> > don't think we need to make up any new rules right now.
> >
> > Instead, I would only urge committers to give complex PRs 12-24 hours
> > to percolate, even if there's an approving review, so other time zones
> > have a chance to look at them.
> >
> > Obviously that doesn't apply if it's urgent. For example, if the build
> > is broken and people can't get work done, or a serious error was
> > merged and needs to be reverted ASAP, don't wait, do it!
> >
> > Also, it's not necessary to delay for trivial PRs.
> >
> > What are the definitions of "complex," "trivial," "urgent," etc? I
> > say, committers should just use their best judgment and try to find a
> > good balance. Don't rush too much, don't delay too much. :-)
> >
> > David brings up a good point about time zones and we do have to
> > remember that NuttX is a global project, and I think that's the main
> > point to keep in mind.
> >
> > To Brennan's last point: as we grow the committer base, we are likely
> > to have more people in more time zones and more PR reviewers, so this
> > should become less of a concern.
> >
> > Nathan
>
RE: [DISCUSS]: Self merge and Single company/organization merge gating
Posted by David Sidrane <da...@nscdg.com>.
On Self merge:
As Nathan pointed out, it is more about time zones then merge velocity.
However, using a backport only methodology requires an upstream merge before
the work can be backported with least effort and adds a serial delay. It
would be ideal to reduces the CI quantum delay this as much as we can.
GH has a setting to merge on successful CI after approval. It is lit by the
approver. This removes the polling for completion of CI.
If this can be configured it reduces the polling for both approver and
author. If it can not be configured in our repos, then self merge is the
next best thing.
I am not trying to circumvent the review process at all - just remove the
idle time imposed by the process that is sampling related.
> an approval from outside of the company/organization then the author can
> do the merge. For complex changes the person outside the organization
> should perform the merge even if there are more than 1 approval from
> inside the company/organization.
I agree.
David
-----Original Message-----
From: Petro Karashchenko <pe...@gmail.com>
Sent: Thursday, February 17, 2022 1:01 PM
To: dev@nuttx.apache.org
Subject: Re: [DISCUSS]: Self merge and Single company/organization merge
gating
Hello,
Regarding PRs megre by the author: I think that if the changes are
relatively simple (again that is very subjective, but I hope that people
with merge rights have more or less the same common sense of
it) and there is an approval from outside of the company/organization then
the author can do the merge. For complex changes the person outside the
organization should perform the merge even if there are more than 1 approval
from inside the company/organization.
In this way reviewers can perform reviews with better quality and if someone
"forget" to press the "rebase & merge" button because for example CI is
still running and that is the end of working day, then the author can press
that button and not do extra tagging in PRs. I vote to make that process
usable for people and sacrifice bureaucracy in the places where it is
possible.
Best regards,
Petro
вт, 15 лют. 2022 р. о 18:26 Nathan Hartman <ha...@gmail.com> пише:
>
> On Mon, Feb 14, 2022 at 2:01 PM Brennan Ashton
> <ba...@brennanashton.com> wrote:
> > > Background:
> > I am generally opposed to both of these. It is quite rare that we
> > need a crazy fast merge turn around on a PR. And if something is
> > approved and straight up broken in master that needs to get in then
> > I think forgiveness can be used to self merge.
> >
> >
> > I also generally do not have a big issue about people from the same
> > company reviewing and merging. I could see the arguments for shared code
> > but then I
> > think we are nitpicking. I prefer the velocity with a few oops that
> > can
> > be reverted along the way if needed. There is also parts of the
> > code base where the best people to review are on the same company.
> >
> >
> > I think most of the concerns here are best addressed not by process
> > but increasing the number of contributors who can participate. (more
> > committers and PPMC)
>
> Feel free to correct me if I'm mistaken, but I think David is bringing
> this up because of time zones.
>
> Indeed, most of the PR merging activity seems to occur during what I
> would call nighttime or early morning, and I think that might be more
> pronounced in David's time zone.
>
> Still, I think things have been working well, more or less, and I
> don't think we need to make up any new rules right now.
>
> Instead, I would only urge committers to give complex PRs 12-24 hours
> to percolate, even if there's an approving review, so other time zones
> have a chance to look at them.
>
> Obviously that doesn't apply if it's urgent. For example, if the build
> is broken and people can't get work done, or a serious error was
> merged and needs to be reverted ASAP, don't wait, do it!
>
> Also, it's not necessary to delay for trivial PRs.
>
> What are the definitions of "complex," "trivial," "urgent," etc? I
> say, committers should just use their best judgment and try to find a
> good balance. Don't rush too much, don't delay too much. :-)
>
> David brings up a good point about time zones and we do have to
> remember that NuttX is a global project, and I think that's the main
> point to keep in mind.
>
> To Brennan's last point: as we grow the committer base, we are likely
> to have more people in more time zones and more PR reviewers, so this
> should become less of a concern.
>
> Nathan
Re: [DISCUSS]: Self merge and Single company/organization merge gating
Posted by Petro Karashchenko <pe...@gmail.com>.
Hello,
Regarding PRs megre by the author: I think that if the changes are
relatively simple (again that is very subjective, but I hope that
people with merge rights have more or less the same common sense of
it) and there is an approval from outside of the company/organization
then the author can do the merge. For complex changes the person
outside the organization should perform the merge even if there are
more than 1 approval from inside the company/organization.
In this way reviewers can perform reviews with better quality and if
someone "forget" to press the "rebase & merge" button because for
example CI is still running and that is the end of working day, then
the author can press that button and not do extra tagging in PRs. I
vote to make that process usable for people and sacrifice bureaucracy
in the places where it is possible.
Best regards,
Petro
вт, 15 лют. 2022 р. о 18:26 Nathan Hartman <ha...@gmail.com> пише:
>
> On Mon, Feb 14, 2022 at 2:01 PM Brennan Ashton
> <ba...@brennanashton.com> wrote:
> > > Background:
> > I am generally opposed to both of these. It is quite rare that we need a
> > crazy fast merge turn around on a PR. And if something is approved and
> > straight up broken in master that needs to get in then I think forgiveness
> > can be used to self merge.
> >
> >
> > I also generally do not have a big issue about people from the same company
> > reviewing and merging. I could see the arguments for shared code but then I
> > think we are nitpicking. I prefer the velocity with a few oops that can
> > be reverted along the way if needed. There is also parts of the code base
> > where the best people to review are on the same company.
> >
> >
> > I think most of the concerns here are best addressed not by process but
> > increasing the number of contributors who can participate. (more committers
> > and PPMC)
>
> Feel free to correct me if I'm mistaken, but I think David is bringing
> this up because of time zones.
>
> Indeed, most of the PR merging activity seems to occur during what I
> would call nighttime or early morning, and I think that might be more
> pronounced in David's time zone.
>
> Still, I think things have been working well, more or less, and I
> don't think we need to make up any new rules right now.
>
> Instead, I would only urge committers to give complex PRs 12-24 hours
> to percolate, even if there's an approving review, so other time zones
> have a chance to look at them.
>
> Obviously that doesn't apply if it's urgent. For example, if the build
> is broken and people can't get work done, or a serious error was
> merged and needs to be reverted ASAP, don't wait, do it!
>
> Also, it's not necessary to delay for trivial PRs.
>
> What are the definitions of "complex," "trivial," "urgent," etc? I
> say, committers should just use their best judgment and try to find a
> good balance. Don't rush too much, don't delay too much. :-)
>
> David brings up a good point about time zones and we do have to
> remember that NuttX is a global project, and I think that's the main
> point to keep in mind.
>
> To Brennan's last point: as we grow the committer base, we are likely
> to have more people in more time zones and more PR reviewers, so this
> should become less of a concern.
>
> Nathan
Re: [DISCUSS]: Self merge and Single company/organization merge gating
Posted by Nathan Hartman <ha...@gmail.com>.
On Mon, Feb 14, 2022 at 2:01 PM Brennan Ashton
<ba...@brennanashton.com> wrote:
> > Background:
> I am generally opposed to both of these. It is quite rare that we need a
> crazy fast merge turn around on a PR. And if something is approved and
> straight up broken in master that needs to get in then I think forgiveness
> can be used to self merge.
>
>
> I also generally do not have a big issue about people from the same company
> reviewing and merging. I could see the arguments for shared code but then I
> think we are nitpicking. I prefer the velocity with a few oops that can
> be reverted along the way if needed. There is also parts of the code base
> where the best people to review are on the same company.
>
>
> I think most of the concerns here are best addressed not by process but
> increasing the number of contributors who can participate. (more committers
> and PPMC)
Feel free to correct me if I'm mistaken, but I think David is bringing
this up because of time zones.
Indeed, most of the PR merging activity seems to occur during what I
would call nighttime or early morning, and I think that might be more
pronounced in David's time zone.
Still, I think things have been working well, more or less, and I
don't think we need to make up any new rules right now.
Instead, I would only urge committers to give complex PRs 12-24 hours
to percolate, even if there's an approving review, so other time zones
have a chance to look at them.
Obviously that doesn't apply if it's urgent. For example, if the build
is broken and people can't get work done, or a serious error was
merged and needs to be reverted ASAP, don't wait, do it!
Also, it's not necessary to delay for trivial PRs.
What are the definitions of "complex," "trivial," "urgent," etc? I
say, committers should just use their best judgment and try to find a
good balance. Don't rush too much, don't delay too much. :-)
David brings up a good point about time zones and we do have to
remember that NuttX is a global project, and I think that's the main
point to keep in mind.
To Brennan's last point: as we grow the committer base, we are likely
to have more people in more time zones and more PR reviewers, so this
should become less of a concern.
Nathan
Re: [DISCUSS]: Self merge and Single company/organization merge gating
Posted by Brennan Ashton <ba...@brennanashton.com>.
On Mon, Feb 14, 2022, 9:28 AM David Sidrane <da...@nscdg.com> wrote:
> I am opening this discussion to widen the audience for feedback on the
> rules for merging. The original interexchange was in [1]
>
>
>
> 1 ) Given the geographical and time differences should we consider that
> once a review with approval* has been done the PR’s author can merge it?
>
>
>
> 2) Should we consider requiring more than one organization/company to
> approve a PR before it is merged.
>
>
>
> Background:
>
>
>
> If only one company does a review, that can lead to undo influence as noted
> as the "The Enemies"
> <https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md> of
> the INVIOLABLES [2]:
>
> “Focus on the values of the organization, not the values of the Open Source
> project. Need to support both.”
>
> *The change would define PR approval as requiring a review/approval from
> more than the members of one company or organization.
>
>
>
>
>
> I am in favor of both these changes.
>
>
>
> David
>
>
>
> [1]
> https://github.com/apache/incubator-nuttx/pull/5320#issuecomment-1020379240
>
> [2] https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md
I am generally opposed to both of these. It is quite rare that we need a
crazy fast merge turn around on a PR. And if something is approved and
straight up broken in master that needs to get in then I think forgiveness
can be used to self merge.
I also generally do not have a big issue about people from the same company
reviewing and merging. I could see the arguments for shared code but then I
think we are nitpicking. I prefer the velocity with a few oops that can
be reverted along the way if needed. There is also parts of the code base
where the best people to review are on the same company.
I think most of the concerns here are best addressed not by process but
increasing the number of contributors who can participate. (more committers
and PPMC)
--Brennan