Sorry if i am ignoring some arguments in this thread, (@andrija, @sven) but here are some reactions of mine.
On Tue, Oct 15, 2019 at 7:51 AM Rohit Yadav <rohit.ya...@shapeblue.com> wrote: ... Agree with requirements, let me share my view and summarise them: > > > * 2x LGTM, with regression test and at least one confirmation > * the confirmation and the description of the test performed either > by the LGTM/reviewer or collaborators/users who can confirm the fix > * The confirmation can also be based on a new specific test (unit > test or marvin), screenshot etc. in that PR > * Non CloudStack-code specific changes, for example to text files > (readme etc), scripts or changes not part of regression tests (for example > the appliance/systemvmtemplate build etc) may not require regression tests > * No outstanding comment/requests or objections (in which case, it may > be brought to dev@ for discussion/voting) > * Committer can squash merge a PR when requirements are met (squash > merged is preferred as it makes it easier to investigate git history and > track changes) > * After a freeze is called only the RM or co-RMs may merge a PR > So I read in this a confirmation of my initial proposal, or am i missing something? > > Discuss - should we accept an explicit confirmation from the author of a > PR? Let's say a PR author sent a PR claiming they've fixed an issue and > tested it? I think if they have an esoteric environment/condition that > others don't have or are difficult to set up, should we accept a PR > author's claim? > Good point about the esoteric environments. I think we should accept to merge if the author accepts to b e maintainer of that part of the code base. There is a recent PR for support of MACOS specific checnges where I did that [1] ________________________________ > From: Paul Angus <paul.an...@shapeblue.com> > ... > 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. > exactly > _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. > see Rohit's last point. [1] https://github.com/apache/cloudstack/pull/3580 -----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 > -- Daan
