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:j...@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 j...@zonker.net Twitter: @jzb http://www.dissociatedpress.net/
