Load configuration and system properties from external file.

Hi Tuomas

Thank you for the patch. Why not add a property constant such as cargo.properties.file directly that would be defined as a constant in org.codehaus.cargo.container.property.GeneralPropertySet and loaded directly in org.codehaus.cargo.container.spi.configuration.AbstractConfiguration's getPropertyValue method?

Thank you

Hello Ali,

I was figuring out the approach you suggested and realized couple of things.

It would also feel as a hack for custom cargo users to use 'cargo.properties.file' because they have the total freedom to choose how to fill up the properties. ANT people have used to http://ant.apache.org/manual/Tasks/property.html style of file referencing and mojo's seem to use surefire's approach so this feels like a poor compromise. I think that the strong point of cargo is that it generalizes different containers without generalizing the tooling (those mojo's that are made using ANT task's give usually no extra value compared to using antrun plugin directly).

I noticed also what you proposed would't cover system properties and for them a similar solution would require also patching of AbstractInstalledLocalContainer which would increase the complexity even further. The embedding is done at mojo level so serious refactoring would have to be done in order to push that down to API level as well.

Maven has also a notion of user properties that is those under -D switch. I didn't include to the patch but the idea is that although cargo container might be interested to system properties of maven execution, the delegation would need to restrict on explicitly defined system properties to avoid situation similar to SUREFIRE-491. This is easy to implement at the mojo but would increase complexity if it's delegated to API level as well.

I'd hope you reconsider the patch but in the for me it's quite same how to get the job done. I could even do it at my custom config but this approach just felt smarter.

–
Tuomas

Hi Tuomas

Thank you for these explanations, indeed having the possibility of using the same mechanism for <properties> and <systemProperties> is a great added value.

Could you please let me know about your username on Codehaus Xircles, so I add you to the list of commiters?

Once you've commited, please make sure https://docs.codehaus.org/display/CARGO/Maven2+Plugin+Reference+Guide is also up to date

Cheers

Hello Ali,

Thanks for the invitation. I didn't realize until now that my jira username wasn't linked with xircles account but now everything should be in order (username is 'tuomas_kiviaho').

Tuomas Kiviaho (tuomas_kiviaho) accepted as a Developer

Enjoy

Reattaching fixed patch. In CARGO-1053 case the reused container got new configuration but it did not receive logger from pre-existing container thus replacing MavenLogger with NullLogger.

Hi Tuomas

You've attached the JAR file, and not the patch

Can you please check?

Thank you

Thanks for noticing Ali,

Change in container's logger setup was essential because originally the logger was not yet present at the time property files were read. I should have left the AbstractCargoMojo#setupLogger() TODO notification removal at later stage but now that's taken care of as well.

Hi Tuomas

The patch is looking perfect -good job Please go ahead and:

1. Commit the changes
2. Check the Continous Integration: http://bamboo.ci.codehaus.org/browse/CARGO-TRUNKM3JDK16
3. Once the CI is also OK, close this ticket with fix versions 1.2.3 and 1.3.0
4. Update https://docs.codehaus.org/display/CARGO/Maven2+Plugin+Reference+Guide

Thank you

Sorry I haven't noticed this ticket until now. Normally, having Maven configuration outside of the pom is not the Maven way. Furthermore, I believe there could be some practical problems using this option in the configuration of the plugin in a parent and then inheriting. You then CANNOT have the file in the parent but need to have it in EVERY child. If you configure it the normal Maven way through pom param instead it will be inherited.
I was surprised to see this option in the surefire plugin. I have never seen a good use case to use it.

Hi Andres,

I stumbled upon the same situation with my patch that handles this scenario in the same way as surefire does. Surefire doesn't treat missing file as unrecoverable situation but instead just prints out a warning message (which is why I needed to adjust the logger initialization)

I agree that using a property file is not ideal but in cases where convention over configuration turns to configuration over convention cluttering the pom.xml it is quite common to externalize the non-maven plugin parts of the configuration. For instance maven-bundle-plugin is quite often used by configuring it partly though an external .bnd file rather than adding multiple lines of instructions to the plugin configuration. I guess same applies for jar and source plugins when configuring excessive amounts of manifest entries.

PS. I managed to commit the change already just prior your comment

Sure, there might be scenarios where this could be used. But I'm always worried of introducing things in Maven plugins that will trick people into thinking that using it is the "Maven way". And then they will run into other issues, which they will try to solve with even worse Maven solutions.

Documentation updated.

Hi Ali,

Thanks for the documentation update but I have to return this one to the table once more for a minor fixup. I noticed that the warning message when container property file is missing is going to NullLogger although the warning from declared but missing system.properties is printed nicely. This is due to late binding of logger to the container so here's a patch to solve that one as well

This patch takes given logger into account already when creating the configuration so that possible warnings do not pass by unnoticed.

Hi Tuomas

The patch looks good. Please check it in and re-close this ticket with fix versions 1.2.4 and 1.3.0.

Thank you