11:17 a.m.
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
7:10 p.m.
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.
8:10 a.m.
<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
4:42 p.m.
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.
7:17 p.m.
-----Original Message-----
From: project-planning-bounces(a)ovirt.org
[mailto:project-planning-bounces@ovirt.org] On Behalf Of Livnat
Peer
Sent: Thursday, September 15, 2011 11:17 AM
To: project-planning(a)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(a)ovirt.org
http://lists.ovirt.org/mailman/listinfo/project-planning
7:40 p.m.
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.
8:17 a.m.
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.
6:18 a.m.
-----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(a)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.
4809
days inactive
4814
days old
7 comments
3 participants
participants (3)
-
Carl Trieloff -
Itamar Heim -
Livnat Peer