All,
Thanks for starting this Daan.
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
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?
Regards,
Rohit Yadav
Software Architect, ShapeBlue
https://www.shapeblue.com
________________________________
From: Paul Angus <[email protected]>
Sent: Thursday, October 10, 2019 13:47
To: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: RE: [DISCUSS][PROPOSAL]merge policy ratification
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.
[email protected]
www.shapeblue.com<http://www.shapeblue.com>
Amadeus House, Floral Street, London WC2E 9DPUK
@shapeblue
[email protected]
www.shapeblue.com
Amadeus House, Floral Street, London WC2E 9DPUK
@shapeblue
-----Original Message-----
From: Daan Hoogland <[email protected]>
Sent: 02 October 2019 12:37
To: dev <[email protected]>
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
