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
Date Wed, 07 Jun 2006 07:59:11 GMT
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