You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Radhika Puthiyetath <ra...@citrix.com> on 2012/12/13 06:47:01 UTC
[Discussion] Doc Reviews on Apache Review Board [WAS RE: Review
Request: Documentation for the new features]
A great discussion. I am copying the community.
In the past, I had to resubmit the patches multiple times just because the files are overwritten by someone else before they were getting reviewed/merged. I had to wait for weeks to get my patches reviewed/ submitted.
What I feel is that we should not delay the reviews for more than a week. We cannot expect the person, who commits the patch, to test his or her patch every day to check whether it breaks something.
Correct me if I am wrong. Suggest me the best way to avoid such issues in future.
Thanks
-Radhika
-----Original Message-----
From: Pranav Saxena
Sent: Thursday, December 13, 2012 11:07 AM
To: Jessica Tomechak; Radhika Puthiyetath; Joe Brockmeier
Subject: RE: Review Request: Documentation for the new features: Optional Public IP assignment for EIP with Basic Zone and User-provided hostname in vCenter
Usually , I believe this should not happen . However , if one thinks that his/her patch has a dependency on something which exists in the codebase and he/she happens to notice that before her patch got merged , the dependency got changed/deleted by somebody else , then yes , you should be observing and modifying your patch and submitting a new diff I guess . Because in a such a large community , I believe , It's quite possible that such kind of events might happen time and again and the person who has created the patch should be testing it with the current latest code . I also agree that once a person has created a patch , his responsibility is just to make amendments as per the reviews but as I pointed out the case where there are dependencies , one has to take the pain of updating the patch himself/herself since you are the best judge of what your patch is going to do.
Regards,
Pranav
-----Original Message-----
From: Jessica Tomechak
Sent: Wednesday, December 12, 2012 9:04 PM
To: Pranav Saxena; Radhika Puthiyetath; Joe Brockmeier
Subject: RE: Review Request: Documentation for the new features: Optional Public IP assignment for EIP with Basic Zone and User-provided hostname in vCenter
Ok, this is a process question, then. Radhika and I both run Publican every so often while working...certainly at least daily, more likely hourly. That's just an ingrained habit now. The problem arises, as you rightly point out, when there's a delay between the review request and the "Ship it!" moment. The delay could be due to no committers paying attention sooner, or often will be because reviewers have technical questions that take time to resolve before the docs can be approved.
So once a reviewer clicks "Ship it!" is it then the responsibility of the person who submitted the patch to pull the latest code and re-test it, before a committer merges the new code in? In this case, Radhika isn't a committer yet, so she can't do the final step herself. Joe went ahead (it appears) to try to merge in the code, and he saw the failure. Should Joe have waited for Radhika to notice the "Ship it!" event, re-test her code, and then post some sort of remark on Reviewboard saying "tested on today's ASF repo and still compiles"? Or is it acceptable for things to follow exactly the course they did for this patch: whoever tries to merge it discovers the error, and alerts the patch author that fixes might be needed. In this case, I think the fixes might be required in files outside Radhika's original patch. Then what? Add those files to the patch?
Jessica T.
CloudStack Tech Pubs
-----Original Message-----
From: Pranav Saxena
Sent: Wednesday, December 12, 2012 5:06 PM
To: Radhika Puthiyetath; Jessica Tomechak
Subject: RE: Review Request: Documentation for the new features: Optional Public IP assignment for EIP with Basic Zone and User-provided hostname in vCenter
Well , you cannot expect that there would be no code check-ins for 1-2 weeks for docs files in a large open source community. Who gets his patch reviewed first is entitled to get it merged . Perhaps , if your patch was reviewed earlier and merged , this problem might not have been there !
-----Original Message-----
From: Radhika Puthiyetath
Sent: Wednesday, December 12, 2012 5:04 PM
To: Pranav Saxena; Jessica Tomechak
Subject: RE: Review Request: Documentation for the new features: Optional Public IP assignment for EIP with Basic Zone and User-provided hostname in vCenter
How can we test it every day :-(
-----Original Message-----
From: Pranav Saxena
Sent: Thursday, December 13, 2012 6:33 AM
To: Radhika Puthiyetath; Jessica Tomechak
Subject: RE: Review Request: Documentation for the new features: Optional Public IP assignment for EIP with Basic Zone and User-provided hostname in vCenter
Trying applying and compiling your patch on the latest asf/master code. There could be a possibility that somebody else made some checkings to docs files and modified something on which your code had a dependency.
-----Original Message-----
From: Radhika Puthiyetath
Sent: Wednesday, December 12, 2012 5:01 PM
To: Pranav Saxena; Jessica Tomechak
Subject: RE: Review Request: Documentation for the new features: Optional Public IP assignment for EIP with Basic Zone and User-provided hostname in vCenter
When I tested it 1-2 weeks back, it was all fine ! strange...
-----Original Message-----
From: Pranav Saxena
Sent: Thursday, December 13, 2012 5:10 AM
To: Jessica Tomechak
Cc: Radhika Puthiyetath
Subject: FW: Review Request: Documentation for the new features: Optional Public IP assignment for EIP with Basic Zone and User-provided hostname in vCenter
Importance: High
-----Original Message-----
From: Joe Brockmeier [mailto:jzb@zonker.net]
Sent: Wednesday, December 12, 2012 3:38 PM
To: CloudStack Developers
Subject: Re: Review Request: Documentation for the new features: Optional Public IP assignment for EIP with Basic Zone and User-provided hostname in vCenter
Does this compile? I get an validity error when building the install guide on the elastic-ip.xml file. We need to be sure to verify any doc changes by using Publican to build the docs before committing. (Mistakes happen, I know... but let's be sure to test w/Publican first.)
On Wed, Dec 12, 2012, at 04:39 PM, Pranav Saxena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8180/#review14398
> -----------------------------------------------------------
>
>
> The patch has been reviewed and merged with asf/master. It applied
> cleanly without any trailing white space errors.Please mark the ticket
> as "Submitted".
> Thanks !
>
> - Pranav Saxena
>
>
> On Nov. 26, 2012, 4:54 a.m., Radhika PC wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/8180/
> > -----------------------------------------------------------
> >
> > (Updated Nov. 26, 2012, 4:54 a.m.)
> >
> >
> > Review request for cloudstack, Chip Childers, Venkata Siva Vijayendra Bhamidipati, Jessica Tomechak, Murali Reddy, and Joe Brockmeier.
> >
> >
> > Description
> > -------
> >
> > This review request is for the documentation written for the following features:
> >
> > Optional Public IP assignment for EIP with Basic Zone User-provided
> > host name in vCenter
> >
> >
> > Diffs
> > -----
> >
> > docs/en-US/about-working-with-vms.xml 47153e2
> > docs/en-US/append-displayname-vms.xml PRE-CREATION
> > docs/en-US/creating-network-offerings.xml ab56920
> > docs/en-US/elastic-ip.xml PRE-CREATION
> > docs/en-US/networks.xml a7b9ea1
> > docs/en-US/virtual-machines.xml 7c74932
> >
> > Diff: https://reviews.apache.org/r/8180/diff/
> >
> >
> > Testing
> > -------
> >
> > Patch cleanly applies. doc is reviewed by QA
> >
> >
> > Thanks,
> >
> > Radhika PC
> >
> >
>
Best,
jzb
--
Joe Brockmeier
jzb@zonker.net
Twitter: @jzb
http://www.dissociatedpress.net/
RE: [Discussion] Doc Reviews on Apache Review Board [WAS RE: Review
Request: Documentation for the new features]
Posted by Radhika Puthiyetath <ra...@citrix.com>.
Yes. I could. I have included elastic -ip.xml file in the networks.xml file. Also, creating-network-offerings.xml is already part of the book.
-----Original Message-----
From: Radhika Puthiyetath [mailto:radhika.puthiyetath@citrix.com]
Sent: Thursday, December 13, 2012 1:42 PM
To: cloudstack-dev@incubator.apache.org
Subject: RE: [Discussion] Doc Reviews on Apache Review Board [WAS RE: Review Request: Documentation for the new features]
Yes. I could. I have included elastic -ip.xml file in the networks.xml file.
-----Original Message-----
From: David Nalley [mailto:david@gnsa.us]
Sent: Thursday, December 13, 2012 12:30 PM
To: cloudstack-dev@incubator.apache.org
Subject: Re: [Discussion] Doc Reviews on Apache Review Board [WAS RE: Review Request: Documentation for the new features]
On Thu, Dec 13, 2012 at 12:47 AM, Radhika Puthiyetath <ra...@citrix.com> wrote:
> A great discussion. I am copying the community.
>
> In the past, I had to resubmit the patches multiple times just because the files are overwritten by someone else before they were getting reviewed/merged. I had to wait for weeks to get my patches reviewed/ submitted.
>
> What I feel is that we should not delay the reviews for more than a week. We cannot expect the person, who commits the patch, to test his or her patch every day to check whether it breaks something.
>
> Correct me if I am wrong. Suggest me the best way to avoid such issues in future.
I personally think that a week is a great timeframe for reviewing patches.
That said, I don't think that was the problem in this specific case.
One of the files you added had a xref to a section that doesn't appear to be included in either the admin or install guide, and thus broke both of those documents.
The specific error on build is:
Validation failed:
elastic-ip.xml:65: validity error : IDREF attribute linkend references an unknown ID "creating-network-offerings"
Were you able to get the admin and install guide to build locally?
--David
RE: [Discussion] Doc Reviews on Apache Review Board [WAS RE: Review
Request: Documentation for the new features]
Posted by Radhika Puthiyetath <ra...@citrix.com>.
Yes. I could. I have included elastic -ip.xml file in the networks.xml file.
-----Original Message-----
From: David Nalley [mailto:david@gnsa.us]
Sent: Thursday, December 13, 2012 12:30 PM
To: cloudstack-dev@incubator.apache.org
Subject: Re: [Discussion] Doc Reviews on Apache Review Board [WAS RE: Review Request: Documentation for the new features]
On Thu, Dec 13, 2012 at 12:47 AM, Radhika Puthiyetath <ra...@citrix.com> wrote:
> A great discussion. I am copying the community.
>
> In the past, I had to resubmit the patches multiple times just because the files are overwritten by someone else before they were getting reviewed/merged. I had to wait for weeks to get my patches reviewed/ submitted.
>
> What I feel is that we should not delay the reviews for more than a week. We cannot expect the person, who commits the patch, to test his or her patch every day to check whether it breaks something.
>
> Correct me if I am wrong. Suggest me the best way to avoid such issues in future.
I personally think that a week is a great timeframe for reviewing patches.
That said, I don't think that was the problem in this specific case.
One of the files you added had a xref to a section that doesn't appear to be included in either the admin or install guide, and thus broke both of those documents.
The specific error on build is:
Validation failed:
elastic-ip.xml:65: validity error : IDREF attribute linkend references an unknown ID "creating-network-offerings"
Were you able to get the admin and install guide to build locally?
--David
Re: [Discussion] Doc Reviews on Apache Review Board [WAS RE: Review
Request: Documentation for the new features]
Posted by David Nalley <da...@gnsa.us>.
On Thu, Dec 13, 2012 at 12:47 AM, Radhika Puthiyetath
<ra...@citrix.com> wrote:
> A great discussion. I am copying the community.
>
> In the past, I had to resubmit the patches multiple times just because the files are overwritten by someone else before they were getting reviewed/merged. I had to wait for weeks to get my patches reviewed/ submitted.
>
> What I feel is that we should not delay the reviews for more than a week. We cannot expect the person, who commits the patch, to test his or her patch every day to check whether it breaks something.
>
> Correct me if I am wrong. Suggest me the best way to avoid such issues in future.
I personally think that a week is a great timeframe for reviewing patches.
That said, I don't think that was the problem in this specific case.
One of the files you added had a xref to a section that doesn't appear
to be included in either the admin or install guide, and thus broke
both of those documents.
The specific error on build is:
Validation failed:
elastic-ip.xml:65: validity error : IDREF attribute linkend references
an unknown ID "creating-network-offerings"
Were you able to get the admin and install guide to build locally?
--David