Re: [vdsm] Patch review process
- Original Message - From: Ryan Harper ry...@us.ibm.com To: Alon Bar-Lev alo...@redhat.com Cc: Ryan Harper ry...@us.ibm.com, Ryan Harper ry...@linux.vnet.ibm.com, Anthony Liguori aligu...@linux.vnet.ibm.com, vdsm-devel@lists.fedorahosted.org, Adam Litke a...@us.ibm.com Sent: Monday, September 10, 2012 9:41:14 PM Subject: Re: [vdsm] Patch review process * Alon Bar-Lev alo...@redhat.com [2012-09-10 12:44]: - Original Message - From: Ryan Harper ry...@us.ibm.com To: Alon Bar-Lev alo...@redhat.com Cc: Ryan Harper ry...@us.ibm.com, Ryan Harper ry...@linux.vnet.ibm.com, Anthony Liguori aligu...@linux.vnet.ibm.com, vdsm-devel@lists.fedorahosted.org, Adam Litke a...@us.ibm.com Sent: Monday, September 10, 2012 8:33:53 PM Subject: Re: [vdsm] Patch review process * Alon Bar-Lev alo...@redhat.com [2012-09-10 12:22]: - Original Message - From: Ryan Harper ry...@us.ibm.com To: Adam Litke a...@us.ibm.com Cc: Ryan Harper ry...@linux.vnet.ibm.com, Anthony Liguori aligu...@linux.vnet.ibm.com, vdsm-devel@lists.fedorahosted.org Sent: Monday, September 10, 2012 7:07:56 PM Subject: Re: [vdsm] Patch review process * Adam Litke a...@us.ibm.com [2012-09-09 12:29]: snip I'm certainly willing to review any patches that show up on the mailing list directly, so if folks want to submit patches first for review before pushing into gerrit; I'm quite happy with reviewing those. Why won't you subscribe to vdsm-patches[1] and comment within gerrit? I am subscribed. Commenting within gerrit is much slower for myself. I'm always in my email client, so sending an email back involves far less work for myself and I don't have to write lots of words in a small text box without a proper text editor. Gerrit comment workflow: 1) follow link from vdsm-patches email to gerrit patch in browser 2) sign-in via openid 3) find which patch file I want to comment upon 4) select the line I want to comment upon to bring up text widget 5) write up comment in a browswer gui box without any support from text editor 6) repeat (3-5) if there are multiple files touched 7) go back to top 8) hit publish Email comment workflow after we know have inline patches. 1) Reply to email 2) find lines in email for comments 3) write up responses in editor 4) close file and mail. This method is superior as the past/present/future comments and review I disagree here. process is managed within productivity application, from birth to merge or death. Much like an email thread with inline patches that's archived for everyone to read and review. Each comment within gerrit is going to the list as if sent directly to the mailing list, so you can track the activity. What's the point of going to the list if not to be able to respond to email? You see the benefit of you as a reviewer, while there are other reviewers (past and future, members and guests) and there is the patch owner. While it may indeed be easier for you to just send a message, for the other people who are involved in the process it may not be as productive as managing the discussion within productivity application which supports the process quite well. Sure; There is a fixed set of folks who already have been working with gerrit for some time and I'm sure are quite comfortable with the process. Part of opening the community up is figuring out how best to attract and maintain additional developers. When growing a community, reducing the barrier for entry is a must. I'll posit that if contributions to gerrit-based communities require the additional openid, web-based commented/editing, some-what undocumented process for obtain review and approval then we're not going to see tremendous growth given the additional overhead of working with gerrit as a contributor. Only recently did we get gerrit to push the code out after submission, meaning that if someone wanted to lurk and read the code, it was always through the web-based viewing; some comments showup on the page of the changeset, the other in-line comments are only available if you know which file the comment was made against. All comments go to the vdsm-patches, but this is a one-way avenue; you don't see discussion happening in response; and if you want to reply, you have to sign-in to gerrit and hunt down the right file. None of this is *bad*; it's just different. Any one of these extra steps could be the excuse that keeps developers from participating. It is not that gerrit is perfect, it is far from being perfect, however has advantages over plain list. I'd really like to hear what advantage
Re: [vdsm] Patch review process
On Sun, Sep 09, 2012 at 02:33:00PM -0400, Alon Bar-Lev wrote: - Original Message - From: Adam Litke a...@us.ibm.com To: vdsm-devel@lists.fedorahosted.org Cc: Ryan Harper ry...@linux.vnet.ibm.com, Anthony Liguori aligu...@linux.vnet.ibm.com Sent: Sunday, September 9, 2012 8:27:30 PM Subject: [vdsm] Patch review process While discussing gerrit recently, I learned that some people use gerrit simply to host work-in-progress patches and they don't intend for those to be reviewed. How can a reviewer recognize this and skip those patches when choosing what to review? Is there a way to mark certain patches as more important and others as drafts? Yes. See [1]. $ git push upstream HEAD:refs/drafts/master/description Thanks for pointing it out. It would be nice if we could get people to start pushing WIP patches to drafts now that we have this feature. [1] http://gerrit-documentation.googlecode.com/svn/ReleaseNotes/ReleaseNotes-2.3.html -- Adam Litke a...@us.ibm.com IBM Linux Technology Center ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
On Sun, Sep 09, 2012 at 12:27:30PM -0500, Adam Litke wrote: Hi, I want to open up a discussion about patch reviews in the vdsm project. I believe everyone will agree that more code review needs to happen for the betterment of the project. I want to ask everyone some questions and also make some observations. I hope to gain some insights and improve my own workflow. And I hope the same for everyone else too. How much time in a week do you spend reviewing patches? For me it is a couple of hours every day... We have a lot of open patches in gerrit. When deciding to review some code, how do you select a patch to review. I have heard people say that they only select patches which have named them specifically as a reviewer. How does a new contributor know who to ask? I think that `git blame` on the relevant code area would take him a long way. I personally add people I trust as reviewers to patches written by third parties. Does anyone have a workflow (or gerrit query) to select recent unreviewed patches? Are you looking for something like http://gerrit.ovirt.org/#/q/status:open+project:vdsm+-codereview%253E%253D1+-codereview%253C%253D-1+-age:1w,n,z ? I cannot say that I use anything like that. But I do search regularly for changes that have all the acks, and wait for my submitting them. While discussing gerrit recently, I learned that some people use gerrit simply to host work-in-progress patches and they don't intend for those to be reviewed. How can a reviewer recognize this and skip those patches when choosing what to review? Is there a way to mark certain patches as more important and others as drafts? Thanks for taking the time to share your thoughts. Regards, Dan. ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
* Adam Litke a...@us.ibm.com [2012-09-09 12:29]: Hi, I want to open up a discussion about patch reviews in the vdsm project. I believe everyone will agree that more code review needs to happen for the betterment of the project. I want to ask everyone some questions and also make some observations. I hope to gain some insights and improve my own workflow. And I hope the same for everyone else too. How much time in a week do you spend reviewing patches? We have a lot of open patches in gerrit. When deciding to review some code, how do you select a patch to review. I have heard people say that they only select patches which have named them specifically as a reviewer. How does a new contributor know who to ask? Does anyone have a workflow (or gerrit query) to select recent unreviewed patches? In non-gerrit communities, it's also common for patches to not get reviewed. The general approach has been to resubmit the patches to the mailing list for followup. I know in the past I've emailed this list for review requests; and that seemed to help somewhat, but I do worry that it's not obvious to developers how exactly this should be approached. If you've received some reviews (say even a +1) but are lacking the additional +1 then it seems email request is the best option since a resubmit will remove any previous reviews (+1). So, it's not clear to me what the best method (or even the preferred method of the community) is when soliciting review. I'm certainly willing to review any patches that show up on the mailing list directly, so if folks want to submit patches first for review before pushing into gerrit; I'm quite happy with reviewing those. While discussing gerrit recently, I learned that some people use gerrit simply to host work-in-progress patches and they don't intend for those to be reviewed. How can a reviewer recognize this and skip those patches when choosing what to review? Is there a way to mark certain patches as more important and others as drafts? Thanks for taking the time to share your thoughts. -- Adam Litke a...@us.ibm.com IBM Linux Technology Center ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ry...@us.ibm.com ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
- Original Message - From: Ryan Harper ry...@us.ibm.com To: Adam Litke a...@us.ibm.com Cc: Ryan Harper ry...@linux.vnet.ibm.com, Anthony Liguori aligu...@linux.vnet.ibm.com, vdsm-devel@lists.fedorahosted.org Sent: Monday, September 10, 2012 7:07:56 PM Subject: Re: [vdsm] Patch review process * Adam Litke a...@us.ibm.com [2012-09-09 12:29]: snip I'm certainly willing to review any patches that show up on the mailing list directly, so if folks want to submit patches first for review before pushing into gerrit; I'm quite happy with reviewing those. Why won't you subscribe to vdsm-patches[1] and comment within gerrit? This method is superior as the past/present/future comments and review process is managed within productivity application, from birth to merge or death. Each comment within gerrit is going to the list as if sent directly to the mailing list, so you can track the activity. Alon. [1] https://lists.fedorahosted.org/pipermail/vdsm-patches/ ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
* Alon Bar-Lev alo...@redhat.com [2012-09-10 12:22]: - Original Message - From: Ryan Harper ry...@us.ibm.com To: Adam Litke a...@us.ibm.com Cc: Ryan Harper ry...@linux.vnet.ibm.com, Anthony Liguori aligu...@linux.vnet.ibm.com, vdsm-devel@lists.fedorahosted.org Sent: Monday, September 10, 2012 7:07:56 PM Subject: Re: [vdsm] Patch review process * Adam Litke a...@us.ibm.com [2012-09-09 12:29]: snip I'm certainly willing to review any patches that show up on the mailing list directly, so if folks want to submit patches first for review before pushing into gerrit; I'm quite happy with reviewing those. Why won't you subscribe to vdsm-patches[1] and comment within gerrit? I am subscribed. Commenting within gerrit is much slower for myself. I'm always in my email client, so sending an email back involves far less work for myself and I don't have to write lots of words in a small text box without a proper text editor. Gerrit comment workflow: 1) follow link from vdsm-patches email to gerrit patch in browser 2) sign-in via openid 3) find which patch file I want to comment upon 4) select the line I want to comment upon to bring up text widget 5) write up comment in a browswer gui box without any support from text editor 6) repeat (3-5) if there are multiple files touched 7) go back to top 8) hit publish Email comment workflow after we know have inline patches. 1) Reply to email 2) find lines in email for comments 3) write up responses in editor 4) close file and mail. This method is superior as the past/present/future comments and review I disagree here. process is managed within productivity application, from birth to merge or death. Much like an email thread with inline patches that's archived for everyone to read and review. Each comment within gerrit is going to the list as if sent directly to the mailing list, so you can track the activity. What's the point of going to the list if not to be able to respond to email? Alon. [1] https://lists.fedorahosted.org/pipermail/vdsm-patches/ -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ry...@us.ibm.com ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
On 09/10/2012 08:33 PM, Ryan Harper wrote: What's the point of going to the list if not to be able to respond to email? to be able to see what's going on in bulk, in offline, via mail client. but go on gerrit to reply/discuss, or some of your comments will get lost from the patch activity. if you comment via email, other reviewers may not see your comments when they review the patch in gerrit. ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
* Itamar Heim ih...@redhat.com [2012-09-10 12:43]: On 09/10/2012 08:33 PM, Ryan Harper wrote: What's the point of going to the list if not to be able to respond to email? to be able to see what's going on in bulk, in offline, via mail client. but go on gerrit to reply/discuss, or some of your comments will get lost from the patch activity. yes, I've been asking for replies to show up as gerrit comments; but I know that's not happing any time soon. if you comment via email, other reviewers may not see your comments when they review the patch in gerrit. I always cc vdsm-devel; this small group discussion of patches makes things not like a community. I like having everyone see the discussions about a patch, rather than just the reviewer. yes, I know all comments are pushed to the -patches list, but the whole point is to have code + comments + discussion in a single thread. -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ry...@us.ibm.com ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
* Alon Bar-Lev alo...@redhat.com [2012-09-10 12:44]: - Original Message - From: Ryan Harper ry...@us.ibm.com To: Alon Bar-Lev alo...@redhat.com Cc: Ryan Harper ry...@us.ibm.com, Ryan Harper ry...@linux.vnet.ibm.com, Anthony Liguori aligu...@linux.vnet.ibm.com, vdsm-devel@lists.fedorahosted.org, Adam Litke a...@us.ibm.com Sent: Monday, September 10, 2012 8:33:53 PM Subject: Re: [vdsm] Patch review process * Alon Bar-Lev alo...@redhat.com [2012-09-10 12:22]: - Original Message - From: Ryan Harper ry...@us.ibm.com To: Adam Litke a...@us.ibm.com Cc: Ryan Harper ry...@linux.vnet.ibm.com, Anthony Liguori aligu...@linux.vnet.ibm.com, vdsm-devel@lists.fedorahosted.org Sent: Monday, September 10, 2012 7:07:56 PM Subject: Re: [vdsm] Patch review process * Adam Litke a...@us.ibm.com [2012-09-09 12:29]: snip I'm certainly willing to review any patches that show up on the mailing list directly, so if folks want to submit patches first for review before pushing into gerrit; I'm quite happy with reviewing those. Why won't you subscribe to vdsm-patches[1] and comment within gerrit? I am subscribed. Commenting within gerrit is much slower for myself. I'm always in my email client, so sending an email back involves far less work for myself and I don't have to write lots of words in a small text box without a proper text editor. Gerrit comment workflow: 1) follow link from vdsm-patches email to gerrit patch in browser 2) sign-in via openid 3) find which patch file I want to comment upon 4) select the line I want to comment upon to bring up text widget 5) write up comment in a browswer gui box without any support from text editor 6) repeat (3-5) if there are multiple files touched 7) go back to top 8) hit publish Email comment workflow after we know have inline patches. 1) Reply to email 2) find lines in email for comments 3) write up responses in editor 4) close file and mail. This method is superior as the past/present/future comments and review I disagree here. process is managed within productivity application, from birth to merge or death. Much like an email thread with inline patches that's archived for everyone to read and review. Each comment within gerrit is going to the list as if sent directly to the mailing list, so you can track the activity. What's the point of going to the list if not to be able to respond to email? You see the benefit of you as a reviewer, while there are other reviewers (past and future, members and guests) and there is the patch owner. While it may indeed be easier for you to just send a message, for the other people who are involved in the process it may not be as productive as managing the discussion within productivity application which supports the process quite well. Sure; There is a fixed set of folks who already have been working with gerrit for some time and I'm sure are quite comfortable with the process. Part of opening the community up is figuring out how best to attract and maintain additional developers. When growing a community, reducing the barrier for entry is a must. I'll posit that if contributions to gerrit-based communities require the additional openid, web-based commented/editing, some-what undocumented process for obtain review and approval then we're not going to see tremendous growth given the additional overhead of working with gerrit as a contributor. Only recently did we get gerrit to push the code out after submission, meaning that if someone wanted to lurk and read the code, it was always through the web-based viewing; some comments showup on the page of the changeset, the other in-line comments are only available if you know which file the comment was made against. All comments go to the vdsm-patches, but this is a one-way avenue; you don't see discussion happening in response; and if you want to reply, you have to sign-in to gerrit and hunt down the right file. None of this is *bad*; it's just different. Any one of these extra steps could be the excuse that keeps developers from participating. It is not that gerrit is perfect, it is far from being perfect, however has advantages over plain list. I'd really like to hear what advantage gerrit provides over a simple mailing list with archives and threading for developers. I'm struggling to find what problem it solves and how its use is helping grow the development community around vdsm, etc. You wrote initially that this discussion already took place, is there any new factor from previous discussion that should be taken into consideration? I don't recall exactly what you're referring to here, so point me at that comment/dicussion and I'll be happy to update/reply if things have changed since then. Regards, Alon. -- Ryan
Re: [vdsm] Patch review process
- Original Message - From: Adam Litke a...@us.ibm.com To: vdsm-devel@lists.fedorahosted.org Cc: Ryan Harper ry...@linux.vnet.ibm.com, Anthony Liguori aligu...@linux.vnet.ibm.com Sent: Sunday, September 9, 2012 8:27:30 PM Subject: [vdsm] Patch review process While discussing gerrit recently, I learned that some people use gerrit simply to host work-in-progress patches and they don't intend for those to be reviewed. How can a reviewer recognize this and skip those patches when choosing what to review? Is there a way to mark certain patches as more important and others as drafts? Yes. See [1]. $ git push upstream HEAD:refs/drafts/master/description [1] http://gerrit-documentation.googlecode.com/svn/ReleaseNotes/ReleaseNotes-2.3.html ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
