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ć
