
On 09/15/2011 04:17 AM, Livnat Peer wrote:
Hi,
I reviewed the documents on the site and it looks great.
Here are some comments i have on the different documents: -----------------------------------------------------------------------
Becoming a maintainer -
A very good document.
typos: - "(More on joining the board here …)" - add a link to the board doc - " to some projects that to others " - s/that/than - "or to be become a maintainer" - remove the 'be'
-----------------------------------------------------------------------
Adding a sub-project -
- I think the title should be "Adding a project" to be consistent with the rest of the docs on the site.
typo: - "In addition the board my vote" - s/my/may - "theoVirt" - the oVirt
-----------------------------------------------------------------------
Voting -
1. If the R-T-C policy is in effect the document says that +1 means ‘I have tested this patch myself, and found it good.’.
I find this requirement to be too strict. I think that testing a patch is the responsibility of the patch submitter and reviewing a patch should not require actual testing.
changed it to reviewed/tested
I suggest the following instead -
For me reviewing a patch (+1 for a patch) means: - The patch make sense - The patch follows the project standards (e.g. code format) - The patch takes into account the different aspects/components of the project - The patch does not changes any core behavior or introduces new concepts that were not discussed in the mailing list beforehand.
I've incorporated this, take a look and shout if you don't like how I did it...
If the reviewer wants to test the patch or thinks additional testing is needed then he better do so but i would not go and set that as a requirement.
2. The document says that a three +1 votes are required for a pass. In the context of patch approval that requirement might be an overkill.
I am looking at the engine project as an example there are a lot of patches a day, large part of them are not too complicated and one ack can be enough for validation. Requiring 3 acks can slow the development and i am not sure it is justified/needed.
I suggest that by default a patch should require only one ack. If a patch introduces a major change or new concepts the patch submitter should mark the patch as such and ask for 3 acks. In addition if a patch reviewer thinks a patch has some level of complexity he can also ask the patch to have 3 acks before approval.
This approach can also contribute for Developing and Maturing projects which do not have the needed resources to deal with 3 acks for each patch.
I thought I covered this, you want 3 for anything big, but a simple ack for minor stuff is fine. I re-read and added a bit to make it clearer.
typos: - "majour" - s/majour/major - The title "Vetos" is lost in the Votes on releases section.
-----------------------------------------------------------------------
Board -
typo: - "with 10 or more resource" - s/resource/resources
-----------------------------------------------------------------------
Thanks, All applied to the web site. Carl.