search

typesafehub / sbt-lagom-bundle

Other
0 stars 5 forks source link

lagom10-conductr-bundle-lib should be added to bundle, but not the project's classpath #16

Closed dotta closed 8 years ago

dotta commented 8 years ago

The lagom10-conductr-bundle-lib library is needed to run Lagom services in production, but it has no use within the Lagom development environment. The fact that the lagom10-conductr-bundle-lib library is added to a Lagom project classpath can even be problematic, as https://github.com/lagom/lagom/issues/42 demonstrates.

https://github.com/typesafehub/conductr-lib/pull/95 is an OK fix to the problem, but if you'd really want to do it properly, then the lagom10-conductr-bundle-lib should be added to the classpath only when producing a ConductR bundle. By doing so, you will also be able to remove the conditional logic added in https://github.com/typesafehub/conductr-lib/pull/95, because when running in production only the ConductR service locator will be bound to the Lagom ServiceLocator interface.

However, touching the classpath carries a complexity cost that needs to be considered. It also adds magic, since a library will get "dynamically" injected when producing a bundle. These are aspects worth considering, given that the solution implemented in https://github.com/typesafehub/conductr-lib/pull/95 is 1) very simple, 2) easy to understand for everyone, and 3) it gets the job done.

dotta commented 8 years ago

If needed, https://github.com/lagom/lagom/pull/53 can be used as a source of inspiration for how to use sbt config to dynamically inject jars in a classpath.

dotta commented 8 years ago

runAll on chirper now prints the following

[info] application - Signalled start to ConductR
[info] application - Signalled start to ConductR
[info] application - Signalled start to ConductR
[info] application - Signalled start to ConductR

Which I find quite annoying. Quick fix is to change the logging to [debug], but I wanted to pointed out as it shows there may be value in fixing this ticket.

huntc commented 8 years ago

We shouldn't change the logging level - it is useful to see this event in the logs when running via ConductR. However we should only output it if the service is started by ConductR. ConductR-bundle-lib can and should detect and output accordingly.

On 19 Mar 2016, at 18:22, Mirco Dotta notifications@github.com wrote:

runAll on chirper now prints the following

[info] application - Signalled start to ConductR [info] application - Signalled start to ConductR [info] application - Signalled start to ConductR [info] application - Signalled start to ConductR Which I find quite annoying. Quick fix is to change the logging to [debug], but I wanted to pointed out as it shows there may be value in fixing this ticket.

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub

markusjura commented 8 years ago

The logging issue has been resolved with PR https://github.com/typesafehub/conductr-lib/pull/100.

markusjura commented 8 years ago

@dotta After having a chat with @huntc we think it is not worth spending a decent amount of time adding the ConductR Service Locator to the classpath only if ConductR is running.

The current behavior doesn't lead into an issue and it follows the normal sbt approach. Whenever you add a plugin all the libraries are part of the classpath, at least for global plugin such as sbt-conductr-sandbox.

Therefore I'll close this issue.