We are here talking about the one who presses the trigger (merge), not the one submitting PR/code/etc - merge action has to be considered if it is OK to merge or not.
On Thu, 10 Oct 2019 at 13:53, Sven Vogel <s.vo...@ewerk.com> wrote: > > Catching inefficient or poorly written code is important, but no one > will care that the code is beautifully written, if the SSVM deletes a > users' VMs because what looked good in theory didn't work in practice. > > Agree Paul. > > --trust > > maybe I was misunderstood. if a person has a lot of commits there is more > trust more then a person who has the first commit. I know that mistakes are > the normal course of life and experienced developer and committer can also > produce mistakes. :) > > Sanity check if possible, code check, manually testing is the best. > > Cheers > Sven > > > > > __ > > Sven Vogel > Teamlead Platform > > EWERK DIGITAL GmbH > Brühl 24, D-04109 Leipzig > P +49 341 42649 - 99 > F +49 341 42649 - 98 > s.vo...@ewerk.com > www.ewerk.com > > Geschäftsführer: > Dr. Erik Wende, Hendrik Schubert, Frank Richter > Registergericht: Leipzig HRB 9065 > > Zertifiziert nach: > ISO/IEC 27001:2013 > DIN EN ISO 9001:2015 > DIN ISO/IEC 20000-1:2011 > > EWERK-Blog | LinkedIn | Xing | Twitter | Facebook > > Auskünfte und Angebote per Mail sind freibleibend und unverbindlich. > > Disclaimer Privacy: > Der Inhalt dieser E-Mail (einschließlich etwaiger beigefügter Dateien) ist > vertraulich und nur für den Empfänger bestimmt. Sollten Sie nicht der > bestimmungsgemäße Empfänger sein, ist Ihnen jegliche Offenlegung, > Vervielfältigung, Weitergabe oder Nutzung des Inhalts untersagt. Bitte > informieren Sie in diesem Fall unverzüglich den Absender und löschen Sie > die E-Mail (einschließlich etwaiger beigefügter Dateien) von Ihrem System. > Vielen Dank. > > The contents of this e-mail (including any attachments) are confidential > and may be legally privileged. If you are not the intended recipient of > this e-mail, any disclosure, copying, distribution or use of its contents > is strictly prohibited, and you should please notify the sender immediately > and then delete it (including any attachments) from your system. Thank you. > > Am 10.10.2019 um 12:49 schrieb Paul Angus <paul.an...@shapeblue.com>: > > > > Yes, the 'independent verification' could come from one of the LGTM > committers, although an explanation (+'proof') would still be required for > sanity checking. Catching inefficient or poorly written code is > important, but no one will care that the code is beautifully written, if > the SSVM deletes a users' VMs because what looked good in theory didn't > work in practice. > > > > It's not about trust, people make mistakes, have bad days, or even get > lazy occasionally, we have to catch mistakes where we can to make > CloudStack as good as we can to keep people using it. > > > > > > paul.an...@shapeblue.com > > www.shapeblue.com > > Amadeus House, Floral Street, London WC2E 9DPUK > > @shapeblue > > > > > > > > > > -----Original Message----- > > From: Sven Vogel <s.vo...@ewerk.com> > > Sent: 10 October 2019 10:48 > > To: dev@cloudstack.apache.org > > Cc: priv...@cloudstack.apache.org > > Subject: Re: [DISCUSS][PROPOSAL]merge policy ratification > > > > I think 2 LGTM should be ok if anyone of them has tested it. > > > > The main thing is you need to trust anyway that the person if he has > tested it E.g. manually. > > > > That’s live and trust from committers I think. Apache is also a chain of > trust. > > > > Von meinem iPhone gesendet > > > > > > __ > > > > Sven Vogel > > Teamlead Platform > > > > EWERK DIGITAL GmbH > > Brühl 24, D-04109 Leipzig > > P +49 341 42649 - 99 > > F +49 341 42649 - 98 > > s.vo...@ewerk.com > > www.ewerk.com > > > > Geschäftsführer: > > Dr. Erik Wende, Hendrik Schubert, Frank Richter > > Registergericht: Leipzig HRB 9065 > > > > Zertifiziert nach: > > ISO/IEC 27001:2013 > > DIN EN ISO 9001:2015 > > DIN ISO/IEC 20000-1:2011 > > > > EWERK-Blog | LinkedIn | Xing | Twitter | Facebook > > > > Auskünfte und Angebote per Mail sind freibleibend und unverbindlich. > > > > Disclaimer Privacy: > > Der Inhalt dieser E-Mail (einschließlich etwaiger beigefügter Dateien) > ist vertraulich und nur für den Empfänger bestimmt. Sollten Sie nicht der > bestimmungsgemäße Empfänger sein, ist Ihnen jegliche Offenlegung, > Vervielfältigung, Weitergabe oder Nutzung des Inhalts untersagt. Bitte > informieren Sie in diesem Fall unverzüglich den Absender und löschen Sie > die E-Mail (einschließlich etwaiger beigefügter Dateien) von Ihrem System. > Vielen Dank. > > > > The contents of this e-mail (including any attachments) are confidential > and may be legally privileged. If you are not the intended recipient of > this e-mail, any disclosure, copying, distribution or use of its contents > is strictly prohibited, and you should please notify the sender immediately > and then delete it (including any attachments) from your system. Thank you. > >> Am 10.10.2019 um 11:43 schrieb Andrija Panic <andrija.pa...@gmail.com>: > >> > >> Is there a way to "force" some text into the PR description - i.e. > >> like we do for ISSUES (there is some predefined text when creating one). > >> My idea ^^^ is to make those rules visible in every PR - besides > >> having rules more explicit, they need to be more visible (as a > >> reminder) to humans pulling the merge trigger. > >> > >> (I also like the idea of regression-test pass being considered just a > >> starting point for any further testing of the PR) > >> > >> My objection for some observed PRs: > >> 2 x LGTM from purely code review should NOT be enough for merging > >> (assuming regression testing was fine) - perhaps in some realy edge > >> cases where it's 100% clear there is no reason for manual testing > >> (typos fix, simple GUI changes etc). > >> At least one LGTM needs to come from the person who did explicit > >> testing - "worksOnMyComputer" (from the submitter of the PR) is not > enough IMHO. > >> > >> Best, > >> Andrija > >> > >> > >> > >>> On Thu, 10 Oct 2019 at 10:25, Paul Angus <paul.an...@shapeblue.com> > wrote: > >>> > >>> BUMP. > >>> > >>> Hey Guys, > >>> > >>> We have a lot of new people in the community these days; this seems > >>> like an important exercise to ensure that we're all on the same page, > >>> whether that ends up simply re-signing up to the existing practices > >>> or evolving them. > >>> > >>> _personally_ I'd like the conditions to be more explicit that there > >>> needs to be some independent verification that the change does what > >>> it's supposed to (and doesn't do anything that it's not supposed to). > >>> It looks to me that sometimes passing regression tests is seen as the > >>> change has been tested. IMO regression test passing is a > >>> prerequisite of a PR being ready for anyone other than the author(s) > to start reviewing the PR. > >>> > >>> Cheers > >>> > >>> > >>> Paul. > >>> > >>> > >>> > >>> paul.an...@shapeblue.com > >>> www.shapeblue.com > >>> Amadeus House, Floral Street, London WC2E 9DPUK @shapeblue > >>> > >>> > >>> > >>> > >>> -----Original Message----- > >>> From: Daan Hoogland <daan.hoogl...@gmail.com> > >>> Sent: 02 October 2019 12:37 > >>> To: dev <dev@cloudstack.apache.org> > >>> Subject: [DISCUSS][PROPOSAL]merge policy ratification > >>> > >>> LS, > >>> in the past we had set a set of rules in the community under which PR > >>> could be merged. I want to reiterate them here as it seems we are kind > of > >>> slacking. Please chime in if there are any issues or omissions: > >>> > >>> For a PR to be merged it has to adhere to the following conditions: > >>> - In any case > >>> -- A PR has to have had two approving reviews > >>> -- A PR has to have no outstanding requests for changes. A request for > >>> changes is regarded no longer outstanding if the requester stops > responding > >>> on the PR discussions. > >>> -- A PR has to have a review with verification description. Depending > on > >>> the type of PR this can be a test description, an automated test > included, > >>> screenshots in case of UI changes. If it is a tetual change it must be > >>> verified to not apply to logs or events. > >>> - any commiter can merge a PR if it adheres to those conditions > >>> -- unless a freeze has been called by a the branch it is to be merged > on > >>> by a community appointed release manager for that branch > >>> > >>> hope this is short and complete enough at the same time. It has been > >>> agreed upon in the past but I am too lazy to find the mail thread in > the > >>> archives. > >>> If anyone disagrees we'll have to go there. They seem reasonable and > >>> self-evident to me. I am also not sure if these should be stated in > bylaws > >>> or on github, so comments in that respect are welcome as well. Let's > first > >>> again agree on them. > >>> > >>> regards, > >>> > >>> -- > >>> Daan > >>> > >> > >> > >> -- > >> > >> Andrija Panić > > -- Andrija Panić
