ping, keepalive... On Tue, 15 Oct 2019 at 12:10, Daan Hoogland <daan.hoogl...@gmail.com> wrote:
> 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 > -- Andrija Panić
