Some remarks:

• We would need some tests. Unit tests would be fine I think.
• We'll need some documentation on the wiki too
• You need to put the copyright notice at the top of the file
• The class javadoc should not have any @author element but it should have a @version tag
• The ModuleStartMojo.java has a startSingleDeployable() method but the debug comment says it's stopping the deployable...
• Some typos: "appliaction" for ex
• Wrong comment: "// Stop all deployables defined in the deployer config element (if any)"
• I think "Starting deployable..." would be better than "Starting application" as it's more Cargo-centric
• As ModuleStartMojo and ModuleStopMojo are 90% similar I'd recommend having an AbstractModuleMojo class for common code
• The DeployMojo and UndeployMojo classes need to be renamed to ModuleDeployMojo and ModuleUndeployMojo to be consistent with the 2 new mojos. And they should probably share the AbstractModuleMojo class too

Some remarks from my side as well:

We would need some tests. Unit tests would be fine I think.
> What these tests should verify?
We'll need some documentation on the wiki too
>How to do this?
You need to put the copyright notice at the top of the file
>Done
The class javadoc should not have any @author element but it should have a @version tag
>Done
The ModuleStartMojo.java has a startSingleDeployable() method but the debug comment says it's stopping the deployable...
>Fixed
Some typos: "appliaction" for ex
>Fixed
Wrong comment: "// Stop all deployables defined in the deployer config element (if any)"
>Fixed
I think "Starting deployable..." would be better than "Starting application" as it's more Cargo-centric
>Fixed
As ModuleStartMojo and ModuleStopMojo are 90% similar I'd recommend having an AbstractModuleMojo class for common code
>Done
The DeployMojo and UndeployMojo classes need to be renamed to ModuleDeployMojo and ModuleUndeployMojo to be consistent with the 2 new mojos. And they should probably share the AbstractModuleMojo class too
>How to commit renamed files ? Just also commiting it here and yes, sure if you want I can do sharing for this mojos as well and also I see DeployMojo can use then URL as additional argument.

All updated files are attached.

We have a ContainerStartMojoTest class. We need something similar for the deploy mojos.

>How to commit renamed files ? Just also commiting it here and yes, sure if you want I can do sharing for this mojos as well and also I see DeployMojo can use then URL as additional argument.

You need to create a patch. Almost all SVN clients have a "create patch" feature. For new files you need to "svn add" them before creating the patch or they won't be in the patch.

I'll have a look at the new files you've committed later today, thanks.

Best regards,
Juri.

• The patch is not created from the right directory. For example in the patch file it says:

D:/workspace/cargo-svn/extensions/maven2/src/main/java/org/codehaus/cargo/maven2/ModuleStopMojo.java

The root of the patch should be extensions/. I've fixed this manually for now but please make sure this is fine for the next patches you send.

• The copyright year should be the year when the file was created till the last time it was modified. Thus new files should have a copyright of 2006.

• The @version is not correct. It must be: "@version $Id: $" so that the SVN client replaces the value when the file is committed. For example you'll get:

@version $Id: AbstractCargoMojo.java 940 2006-03-17 21:29:46Z vmassol $

• Your patch should include deleted files too. It did not include deletion of currnet DeployMojo/UndeployMojo

• There are several blank lines that shouldn't exist as in:

private void deploySingleDeployable(org.codehaus.cargo.container.deployer.Deployer deployer, 
        org.codehaus.cargo.container.deployable.Deployable deployable, URL pingURL)
    {
        
        
        getLog().debug("Deploying [" + deployable.getFile() + "]"
            + ((pingURL == null) ? "..." : " using ping URL [" + pingURL + "]"));
                
        
        if (pingURL != null)
        {
            deployer.deploy(deployable, createDeployableMonitor(pingURL, deployable));
        }
        else
        {
            deployer.deploy(deployable);
        }
        
    }

(in this example you can remove 4 lines).

• Method shoud be verbs. I've replaced moduleAction() by performDeployerAction(). (Note: I make this mistake myself but this doesn't mean someone shouldn't correct me! )

• I've replaced all Module names with Deployer instead so that it's easier to relate code to the Cargo API. Module is a term that doesn't exist in cargo. Right now as I've suggetsed on the mailing list (I don't think anyone answered) I had suggested deployer-start, deployer-deploy, deployer-undeploy, deployer-stop, which I think is better than module-start, etc.

• Added some comments to the code

• Refactored more (execute in parent class, removed duplications in deploy and undeploy mojos). The pingURL should be passed to all. In the future there should be APIs in the Deployer interface for using them when starting and stopping deployables too.

• Add redeploy mojo to fully implement the Deployer interface

• Changed goal name. I'm happy to change it back to what you defined or whatever we all agree on. I've sent again a mail on the cargo dev list for feedback.

Once more thanks a lot for your patch!