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. 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. 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. 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, Livnat
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.
<snip>
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
thanks
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...
looks good
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.
I see it now, yep.
All applied to the web site.
Thanks, how about changing the "Adding a sub-project" to "Adding a project"? i am fine with keeping it sub-project, just wanted to make sure it did not simply sleep. Livnat
On 09/16/2011 01:10 AM, Livnat Peer wrote:
Thanks, how about changing the "Adding a sub-project" to "Adding a project"? i am fine with keeping it sub-project, just wanted to make sure it did not simply sleep.
Thanks, I have been trying to find all instances on sub-project and kill them. I search again and see which ones I find Carl.
-----Original Message----- From: project-planning-bounces@ovirt.org [mailto:project-planning-bounces@ovirt.org] On Behalf Of Livnat Peer Sent: Thursday, September 15, 2011 11:17 AM To: project-planning@ovirt.org Cc: Carl Trieloff Subject: oVirt site
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.
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.
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'd extend on both points: - patch verification is the responsibility of the submitter, but the actual verification can be done by 3rd party if relevant (user complains on a bug. patch is sent for user to verify solves the problem. Patch can be submitted with note of verified by X). - need to decide if patch must be verified before submitting, or verification could be done as part of patch review (for example, gerrit supports verification as part of flow. Some projects may prefer this approach). - (using gerrit terminology) anyone can +1 a patch. only maintainer can +2 the patch. a +1 would help a maintainer do the +2. - going back to my earlier point on committer != maintainer: in a large project, we'd still like committer to have right to commit to entire git to make things simple, but may have maintainers (or call them sub maintainers) per module in the project. A committer would expect (trust would suffice I hope) the relevant maintainer[1] to give the +2 before committing the patch[2] [1] could be multiple maintainers, if patch touches multiple modules (backend/api/ui/etc.) [2] maintainer may push themselves, or leave to patch submitter if has commit rights
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, Livnat _______________________________________________ Project-planning mailing list Project-planning@ovirt.org http://lists.ovirt.org/mailman/listinfo/project-planning
On 09/15/2011 12:17 PM, Itamar Heim wrote:
- need to decide if patch must be verified before submitting, or verification could be done as part of patch review (for example, gerrit supports verification as part of flow. Some projects may prefer this approach). - (using gerrit terminology) anyone can +1 a patch. only maintainer can +2 the patch. a +1 would help a maintainer do the +2. - going back to my earlier point on committer != maintainer: in a large project, we'd still like committer to have right to commit to entire git to make things simple, but may have maintainers (or call them sub maintainers) per module in the project. A committer would expect (trust would suffice I hope) the relevant maintainer[1] to give the +2 before committing the patch[2]
Itamar, Do you want to explicitly add the +2 for projects that use gerrit? I can do that. Carl.
On 09/15/2011 07:40 PM, Carl Trieloff wrote:
On 09/15/2011 12:17 PM, Itamar Heim wrote:
- need to decide if patch must be verified before submitting, or verification could be done as part of patch review (for example, gerrit supports verification as part of flow. Some projects may prefer this approach). - (using gerrit terminology) anyone can +1 a patch. only maintainer can +2 the patch. a +1 would help a maintainer do the +2. - going back to my earlier point on committer != maintainer: in a large project, we'd still like committer to have right to commit to entire git to make things simple, but may have maintainers (or call them sub maintainers) per module in the project. A committer would expect (trust would suffice I hope) the relevant maintainer[1] to give the +2 before committing the patch[2]
Itamar,
Do you want to explicitly add the +2 for projects that use gerrit? I can do that.
I don't think we should use gerrit terms, gerrit is only the tool we use to apply the guidelines we are phrasing now. I think we do need to add the trust model Itamar mentioned in his mail. Livnat
Carl.
-----Original Message----- From: Carl Trieloff [mailto:cctrieloff@redhat.com] Sent: Thursday, September 15, 2011 19:41 PM To: Itamar Heim Cc: 'Livnat Peer'; project-planning@ovirt.org; 'Carl Trieloff' Subject: Re: oVirt site
On 09/15/2011 12:17 PM, Itamar Heim wrote:
- need to decide if patch must be verified before submitting, or verification could be done as part of patch review (for example, gerrit supports verification as part of flow. Some projects may prefer this approach). - (using gerrit terminology) anyone can +1 a patch. only maintainer can +2 the patch. a +1 would help a maintainer do the +2. - going back to my earlier point on committer != maintainer: in a large project, we'd still like committer to have right to commit to entire git to make things simple, but may have maintainers (or call them sub maintainers) per module in the project. A committer would expect (trust would suffice I hope) the relevant maintainer[1] to give the +2 before committing the patch[2]
Itamar,
Do you want to explicitly add the +2 for projects that use gerrit? I can do that.
I agree with livnat we don't need it specifically. A +2 in gerrit is the actual ack which is mentioned as +1. (anyone can give a +1, maintainers can give a +2. The +1 helps the maintainers with giving the +2 of course). We can clarify this when we set it up with details on the process for the projects using gerrit.
Carl.
participants (3)
-
Carl Trieloff -
Itamar Heim -
Livnat Peer