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ć
