geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Jencks <david_jen...@yahoo.com>
Subject Re: [RTC] m2-plugins deployment plugin: PLEASE VOTE
Date Tue, 13 Jun 2006 18:53:12 GMT
Last (and only, depending on how you count) +1 was 4 days ago, any  
chance 2 more people are willing to review this?

thanks
david jencks



On Jun 9, 2006, at 7:51 AM, Guillaume Nodet wrote:

> +1
>
> Cheers,
> Guillaume Nodet
>
> David Jencks wrote:
>
>> I've uploaded
>>
>> http://issues.apache.org/jira/secure/attachment/12335128/geronimo-  
>> deployment-plugin-RTC-VOTE.2.patch
>>
>> incorporating most of your comments, see below.
>>
>> Thanks for the review!
>>
>> david jencks
>>
>> On Jun 6, 2006, at 11:57 PM, Jacek Laskowski wrote:
>>
>>> On 6/6/06, David Jencks <david_jencks@yahoo.com> wrote:
>>>
>>>> Prasad has been working for a long time on an m2 deployment plugin.
>>>> I've cleaned up his latest patch a little bit and think its  
>>>> ready to
>>>> commit.
>>>>
>>>> Please review http://issues.apache.org/jira/secure/attachment/
>>>> 12335116/geronimo-deployment-plugin-RTC-VOTE.patch
>>>
>>>
>>> I have taken a look at the patch and the comments are as follows
>>> (they're rather style-centric nor technical, but still valid I  
>>> hope).
>>> No testing was performed.
>>>
>>> 1/ I was scared to read: "May not have been tested." in one of  
>>> the  classes
>>
>>
>> You want me to lie :-) ? I don't think anyone has ever used the in- 
>> vm  startServer command in the m1 plugin, so I doubt prasad has  
>> tested  this one either.  I still think its worth including as a  
>> starting  point in case some one wants to try it out.
>>
>>>
>>> 2/ Copyright 2004-2006 - shouldn't it be Copyright 2006 only?
>>
>>
>> These are basically copies + modifications of the m1 deployment   
>> plugin, copyright 2004.  I don't know if any changes happened in   
>> 2005, but 2004 and 2006 are definitely needed.
>>
>>>
>>> 3/ The javadoc of classes should be consistent. It means that it   
>>> should read:
>>>
>>>   /**
>>>    * @goal undeploy (if appropriate)
>>>    *
>>>    * @version $Rev$ $Date$
>>>    */
>>>
>>> whereas some contain
>>>
>>>   @version $Revision$ $Date$
>>>
>>> or no version at all.
>>
>>
>> They should all have the @version, but only mojos should have the   
>> @goal in order to not confuse maven.  I've fixed the @version tags  
>> as  well as I can find them.
>>
>>>
>>> 4/ Geronimo :: Maven Deployment Plugin using m2 -> Geronimo ::  
>>> Maven 2
>>> Deployment Plugin or alternatively Geronimo :: Maven Deployment  
>>> Plugin
>>> for Maven 2, but I'd prefer the former.
>>
>>
>> fixed
>>
>>>
>>> 5/ geronimo-deployment-plugin/pom.xml has no ASF license header.
>>
>> fixed.  I think its better to include  the asf license in the  
>> source  even if maven removes it during processing.
>>
>>
>>>
>>> So, unless it's corrected I'm -1. If you're swampped with your other
>>> work, I can take care of it and propose the patch corrected again.
>>>
>>>> Here's my +1.
>>>
>>>
>>> It doesn't count, though ;-)
>>
>>
>> Why not?  Because I edited prasad's patch for formatting and  
>> removed  unused cruft?  Ken's directive requires 3 +1 votes from  
>> committers  other than the proposer (who apparently does not need  
>> to be a  committer): although the document he appears to have  
>> adapted states  only PMC member votes count, that is not in his  
>> directive.  Since he  hasn't responded to requests for  
>> clarification I think we have to  take him at his word.
>>
>> thanks
>> david jencks
>>
>>
>>
>>>
>>>> david jencks
>>>
>>>
>>> Jacek
>>>
>>> -- 
>>> Jacek Laskowski
>>> http://www.laskowski.net.pl
>>
>>
>>
>>


Mime
View raw message