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 <jerpe...@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.karashche...@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.jerpe...@sony.com > > <alin.jerpe...@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 <david.sidr...@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 <petro.karashche...@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 <hartman.nat...@gmail.com> > > пише: > > > > > > > > On Mon, Feb 14, 2022 at 2:01 PM Brennan Ashton > > > > <bash...@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 > > >
