geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jason Dillon <ja...@planet57.com>
Subject Re: [RTC] Merge m2migration (functional m2 build) to trunk
Date Tue, 25 Jul 2006 18:22:40 GMT
Comments below... get ready to take your grain of salt...

On Jul 25, 2006, at 6:50 AM, anita kulshreshtha wrote:
> Jason,
>     you aksed me on to make a patch for the branch on July 10th. I
> looked at the code and saw that you made the following changes to the
> packaging plugin:
> 1. Renamed the plugin
> 2. Changed the package structure

Yes I merged the 2 plugins into one so they can share the same  
codebase.  These plugins work with CAR files... and there is  
absolutely no reason whatsoever that they should or need to be  
separated.  There are however good reasons to merge them.


> 3. Changed the directory layout to standard M2 layout, even though the
> rest of the project and the geronimo-deployment _plugin_ still use the
> old layout!

There are several modules in the project that use the new standard M2  
layout.  I moved all modules that were not used by the m1 build to  
use the new layout and others will follow shortly.

I don't really see how this matters though. It should be easy enough  
for the author of the patch to simply copy and paste the correct bits  
in the right place.  The code has not changed so much that the  
original logic was removed.


> 4. Used non geronimo coding style.
>     In the past subjective changes like this were discussed on the  
> list
> so people can provide their input.

Please don't tell me about coding style... the classes in these  
plugins were a mix of styles, a mix indentation, grossly abused  
System.out.* and did not properly handle exceptions.  I normalized them.

I also make may plexus-style changes, which better use the plexus  
component container's ability to configure the mojos, that and to  
take more advantage of existing plexus bits like utils to delete  
directories instead of your custom version.


> 5. Removed plexus component.xml (you have reverted this change now)

At no point did I remove plexus/components.xml.  I am very well aware  
of the purpose of this file.

If you are going to publicly list reasons you don't like what I have  
done, please do not invent reasons that do not exist, or substantiate  
them.


>    I quickly realized that it was going to require lot more effort to
> create the patch. If I had the time, I would have created the patch  
> for
> you. July is not the most productive time for me. The schools are off
> and I must attend to other children's activities. IMO, the fastest way
> to get this working will be for you to add the code from the
> m2-plugins.patch to your code. If you need the java files, I will be
> happy to upload them.

I have been reviewing that patch and have started to incorporate the  
parts which I believe are relevant.  I saw you added comments about  
the use-case to a JIRA which I will also review.


>    Once again I will request you to use ${j2eeJettyServer} etc for
> deploymentConfig. If it was up to me I would choose ease of use over
> Maven's preferred ways. This plugin is used by many users who  
> really do
> not care to know about the deployers let alone their ordering. As far
> as they are concerned they want to specify which custom server they  
> are
> packaging for. As we provide more custom servers we will be adding new
> properties.

If it was up to me, I'd remove the need for these plugins altogether  
and deploy xml directly to the server and hide all of this the CAR  
non-sense from our users.

Anyways, I disagree that in the previous ${...} state that users  
don't have to worry about the ordering... since they need to get that  
property from somewhere.  In either case, something is going to get  
copy-pasted.  If it really turns out to be a huge user burden then I  
would rather investigate a cleaner way to handle this configuration  
which hides more of the complexity from users.

--jason


Mime
View raw message