[Engine-devel] Development process wiki page
Hey all, I've wrote a wiki page on the oVirt development process. It contains mostly information on the patch review process, patch submission and some git guidelines. http://www.ovirt.org/wiki/DevProcess I added a link to it from the main wiki page. Your comments are welcome. Thank you, Oved
We should add some guideline for the review process as well to ease the process for the reviewers. When pushing a newer version of a patch to gerrit we can distinguish between a patch which was already reviewed to non-reviewed patch: 1. A reviewed patch: the previous patch-set contains reviewer comments, therefore the expected change should be described there, as part of the conversation around a specific piece of code (either as a reply or using the 'Done' button). 2. A new reviewed patch: A general message should be added on the patch level describing what have changed since previous patch-set. Thoughts ? On 01/30/2012 09:45 AM, Oved Ourfalli wrote:
Hey all,
I've wrote a wiki page on the oVirt development process. It contains mostly information on the patch review process, patch submission and some git guidelines.
http://www.ovirt.org/wiki/DevProcess
I added a link to it from the main wiki page.
Your comments are welcome.
Thank you, Oved _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
----- Original Message -----
From: "Moti Asayag" <masayag@redhat.com> To: engine-devel@ovirt.org Sent: Monday, January 30, 2012 10:30:01 AM Subject: Re: [Engine-devel] Development process wiki page
We should add some guideline for the review process as well to ease the process for the reviewers.
When pushing a newer version of a patch to gerrit we can distinguish between a patch which was already reviewed to non-reviewed patch:
1. A reviewed patch: the previous patch-set contains reviewer comments, therefore the expected change should be described there, as part of the conversation around a specific piece of code (either as a reply or using the 'Done' button).
2. A new reviewed patch: A general message should be added on the patch level describing what have changed since previous patch-set.
Thoughts ? Sounds good to me. In case a newer patch version is needed due to gerrit asking you to fetch+rebase, it adds such a general comment message on its own, so in that case one won't need to do it explicitly.
I'll update the oVirt gerrit wiki page with these details (will wait a while for other people to comment...).
On 01/30/2012 09:45 AM, Oved Ourfalli wrote:
Hey all,
I've wrote a wiki page on the oVirt development process. It contains mostly information on the patch review process, patch submission and some git guidelines.
http://www.ovirt.org/wiki/DevProcess
I added a link to it from the main wiki page.
Your comments are welcome.
Thank you, Oved _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
Not sure if this is the place for it, but I think we should have some guideline regarding what is an acceptable change, e.g. - new logic is tested with a unit test, a cobertura report can prove that no branches were missed except catching and rethrowing, every public method is javadoced, etc. ----- Original Message -----
From: "Oved Ourfalli" <ovedo@redhat.com> To: engine-devel@ovirt.org Sent: Monday, January 30, 2012 9:45:47 AM Subject: [Engine-devel] Development process wiki page
Hey all,
I've wrote a wiki page on the oVirt development process. It contains mostly information on the patch review process, patch submission and some git guidelines.
http://www.ovirt.org/wiki/DevProcess
I added a link to it from the main wiki page.
Your comments are welcome.
Thank you, Oved _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
----- Original Message -----
From: "Oved Ourfalli" <ovedo@redhat.com> To: engine-devel@ovirt.org Sent: Monday, January 30, 2012 9:45:47 AM Subject: [Engine-devel] Development process wiki page
Hey all,
I've wrote a wiki page on the oVirt development process. It contains mostly information on the patch review process, patch submission and some git guidelines.
http://www.ovirt.org/wiki/DevProcess
I added a link to it from the main wiki page.
Your comments are welcome.
my 2 cents : (obvious , but should be in the wiki) 1) Regarding patch separation, still each commit should not break the build 2) When having more than one patch set due to comments and applied fixes add to the commit message V[n] : <changes description> where [n] is the patch set number
Thank you, Oved _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
On 01/31/2012 03:03 AM, Eli Mesika wrote:
----- Original Message -----
From: "Oved Ourfalli"<ovedo@redhat.com> To: engine-devel@ovirt.org Sent: Monday, January 30, 2012 9:45:47 AM Subject: [Engine-devel] Development process wiki page
Hey all,
I've wrote a wiki page on the oVirt development process. It contains mostly information on the patch review process, patch submission and some git guidelines.
http://www.ovirt.org/wiki/DevProcess
I added a link to it from the main wiki page.
Your comments are welcome.
my 2 cents : (obvious , but should be in the wiki)
1) Regarding patch separation, still each commit should not break the build 2) When having more than one patch set due to comments and applied fixes add to the commit message V[n] :<changes description> where [n] is the patch set number
I agree we need this, but the problem is we are lacking the [0/nnn] cover letter email you get for such commentary when using gerrit. and there is a concept of the commit itself should not include the history of the patch being reviewed. so we need to adapt this to gerrit. maybe a convention that when uploading a new patch set, contributor also adds a review comment of the changes from previous version (similar to the cover letter). I wonder if the git/gerrit utility some adopted from openstack can help with such a step to allow adding a comment when pushing the patch for review.
----- Original Message -----
On 01/31/2012 03:03 AM, Eli Mesika wrote:
----- Original Message -----
From: "Oved Ourfalli"<ovedo@redhat.com> To: engine-devel@ovirt.org Sent: Monday, January 30, 2012 9:45:47 AM Subject: [Engine-devel] Development process wiki page
Hey all,
I've wrote a wiki page on the oVirt development process. It contains mostly information on the patch review process, patch submission and some git guidelines.
http://www.ovirt.org/wiki/DevProcess
I added a link to it from the main wiki page.
Your comments are welcome.
my 2 cents : (obvious , but should be in the wiki)
1) Regarding patch separation, still each commit should not break the build 2) When having more than one patch set due to comments and applied fixes add to the commit message V[n] :<changes description> where [n] is the patch set number
I agree we need this, but the problem is we are lacking the [0/nnn] cover letter email you get for such commentary when using gerrit. and there is a concept of the commit itself should not include the history of the patch being reviewed. so we need to adapt this to gerrit. maybe a convention that when uploading a new patch set, contributor also adds a review comment of the changes from previous version (similar to the cover letter).
I think if the commit history already exists in gerrit, and the commit itself is pushed through gerrit, then this will be redundant.
I wonder if the git/gerrit utility some adopted from openstack can help with such a step to allow adding a comment when pushing the patch for review. _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
On 01/31/2012 12:04 PM, Mike Kolesnik wrote:
----- Original Message -----
On 01/31/2012 03:03 AM, Eli Mesika wrote:
----- Original Message -----
From: "Oved Ourfalli"<ovedo@redhat.com> To: engine-devel@ovirt.org Sent: Monday, January 30, 2012 9:45:47 AM Subject: [Engine-devel] Development process wiki page
Hey all,
I've wrote a wiki page on the oVirt development process. It contains mostly information on the patch review process, patch submission and some git guidelines.
http://www.ovirt.org/wiki/DevProcess
I added a link to it from the main wiki page.
Your comments are welcome.
my 2 cents : (obvious , but should be in the wiki)
1) Regarding patch separation, still each commit should not break the build 2) When having more than one patch set due to comments and applied fixes add to the commit message V[n] :<changes description> where [n] is the patch set number
I agree we need this, but the problem is we are lacking the [0/nnn] cover letter email you get for such commentary when using gerrit. and there is a concept of the commit itself should not include the history of the patch being reviewed. so we need to adapt this to gerrit. maybe a convention that when uploading a new patch set, contributor also adds a review comment of the changes from previous version (similar to the cover letter).
I think if the commit history already exists in gerrit, and the commit itself is pushed through gerrit, then this will be redundant.
this is not about the patch history, it is about providing reviewers a focus point as to what changed in this new version of the patch set.
I wonder if the git/gerrit utility some adopted from openstack can help with such a step to allow adding a comment when pushing the patch for review. _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
On 01/31/2012 01:31 PM, Itamar Heim wrote:
On 01/31/2012 12:04 PM, Mike Kolesnik wrote:
----- Original Message -----
On 01/31/2012 03:03 AM, Eli Mesika wrote:
----- Original Message -----
From: "Oved Ourfalli"<ovedo@redhat.com> To: engine-devel@ovirt.org Sent: Monday, January 30, 2012 9:45:47 AM Subject: [Engine-devel] Development process wiki page
Hey all,
I've wrote a wiki page on the oVirt development process. It contains mostly information on the patch review process, patch submission and some git guidelines.
http://www.ovirt.org/wiki/DevProcess
I added a link to it from the main wiki page.
Your comments are welcome.
my 2 cents : (obvious , but should be in the wiki)
1) Regarding patch separation, still each commit should not break the build 2) When having more than one patch set due to comments and applied fixes add to the commit message V[n] :<changes description> where [n] is the patch set number
I agree we need this, but the problem is we are lacking the [0/nnn] cover letter email you get for such commentary when using gerrit. and there is a concept of the commit itself should not include the history of the patch being reviewed. so we need to adapt this to gerrit. maybe a convention that when uploading a new patch set, contributor also adds a review comment of the changes from previous version (similar to the cover letter).
I think if the commit history already exists in gerrit, and the commit itself is pushed through gerrit, then this will be redundant.
this is not about the patch history, it is about providing reviewers a focus point as to what changed in this new version of the patch set.
However for the +2 the entire patch should be reviewed again as there might be changes after rebasing the code (even for a new file, e.g. db upgrade scripts with already used version in the file name). Following the small patches convention should ease the review, combining with the short message after the push. However many times the newer version of a patch is just a rebase, so it seems redundant adding just the "patch was rebased" message.
I wonder if the git/gerrit utility some adopted from openstack can help with such a step to allow adding a comment when pushing the patch for review. _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
On 02/01/2012 12:56 AM, Moti Asayag wrote:
On 01/31/2012 01:31 PM, Itamar Heim wrote:
On 01/31/2012 12:04 PM, Mike Kolesnik wrote:
----- Original Message -----
On 01/31/2012 03:03 AM, Eli Mesika wrote:
----- Original Message -----
From: "Oved Ourfalli"<ovedo@redhat.com> To: engine-devel@ovirt.org Sent: Monday, January 30, 2012 9:45:47 AM Subject: [Engine-devel] Development process wiki page
Hey all,
I've wrote a wiki page on the oVirt development process. It contains mostly information on the patch review process, patch submission and some git guidelines.
http://www.ovirt.org/wiki/DevProcess
I added a link to it from the main wiki page.
Your comments are welcome.
my 2 cents : (obvious , but should be in the wiki)
1) Regarding patch separation, still each commit should not break the build 2) When having more than one patch set due to comments and applied fixes add to the commit message V[n] :<changes description> where [n] is the patch set number
I agree we need this, but the problem is we are lacking the [0/nnn] cover letter email you get for such commentary when using gerrit. and there is a concept of the commit itself should not include the history of the patch being reviewed. so we need to adapt this to gerrit. maybe a convention that when uploading a new patch set, contributor also adds a review comment of the changes from previous version (similar to the cover letter).
I think if the commit history already exists in gerrit, and the commit itself is pushed through gerrit, then this will be redundant.
this is not about the patch history, it is about providing reviewers a focus point as to what changed in this new version of the patch set.
However for the +2 the entire patch should be reviewed again as there might be changes after rebasing the code (even for a new file, e.g. db upgrade scripts with already used version in the file name).
Following the small patches convention should ease the review, combining with the short message after the push.
However many times the newer version of a patch is just a rebase, so it seems redundant adding just the "patch was rebased" message.
why - wouldn't it save a lot of time for the reviewers to know this is the only change in this patchset, rather than trying to see there are no changes in it? I agree message shouldn't be in the commit, but a review comment by the patch submitter would go a long way helping reviewers.
On 02/01/2012 01:04 AM, Itamar Heim wrote:
On 02/01/2012 12:56 AM, Moti Asayag wrote:
On 01/31/2012 01:31 PM, Itamar Heim wrote:
On 01/31/2012 12:04 PM, Mike Kolesnik wrote:
----- Original Message -----
On 01/31/2012 03:03 AM, Eli Mesika wrote:
----- Original Message ----- > From: "Oved Ourfalli"<ovedo@redhat.com> > To: engine-devel@ovirt.org > Sent: Monday, January 30, 2012 9:45:47 AM > Subject: [Engine-devel] Development process wiki page > > Hey all, > > I've wrote a wiki page on the oVirt development process. > It contains mostly information on the patch review process, patch > submission and some git guidelines. > > http://www.ovirt.org/wiki/DevProcess > > I added a link to it from the main wiki page. > > Your comments are welcome.
my 2 cents : (obvious , but should be in the wiki)
1) Regarding patch separation, still each commit should not break the build 2) When having more than one patch set due to comments and applied fixes add to the commit message V[n] :<changes description> where [n] is the patch set number
I agree we need this, but the problem is we are lacking the [0/nnn] cover letter email you get for such commentary when using gerrit. and there is a concept of the commit itself should not include the history of the patch being reviewed. so we need to adapt this to gerrit. maybe a convention that when uploading a new patch set, contributor also adds a review comment of the changes from previous version (similar to the cover letter).
I think if the commit history already exists in gerrit, and the commit itself is pushed through gerrit, then this will be redundant.
this is not about the patch history, it is about providing reviewers a focus point as to what changed in this new version of the patch set.
However for the +2 the entire patch should be reviewed again as there might be changes after rebasing the code (even for a new file, e.g. db upgrade scripts with already used version in the file name).
Following the small patches convention should ease the review, combining with the short message after the push.
However many times the newer version of a patch is just a rebase, so it seems redundant adding just the "patch was rebased" message.
why - wouldn't it save a lot of time for the reviewers to know this is the only change in this patchset, rather than trying to see there are no changes in it? I agree message shouldn't be in the commit, but a review comment by the patch submitter would go a long way helping reviewers.
If those patches are about to be submitted, there is a need to go all over them. If it is 'review-in-process' it will surely save time. Just thinking on multi-patchset (series of dependent patches) where each push to gerrit for review requires going over all of the patches.
participants (6)
-
Allon Mureinik -
Eli Mesika -
Itamar Heim -
Mike Kolesnik -
Moti Asayag -
Oved Ourfalli