search

typesafehub / sbt-conductr

Typesafe ConductR plugin for sbt
Other
29 stars 22 forks source link

Issue # 60. Could not load bundle. Due to the fact that the ConductrCont... #65

Closed dsugden closed 9 years ago

dsugden commented 9 years ago

The ConductrController Actor is created onload, with the control server URL as a parameter. This PR adds a message to it's protocol allowing the plugin to change this URL after plugin loading.

hseeberger commented 9 years ago

@dsugden, thanks a lot for the contribution! Please take a look at my comments.

dsugden commented 9 years ago

@hseeberger Great, Thanks!

I think this PR was premature, I was excited. I did want feedback on this.

I would like to move the IPAddress parsing into the sbt.complete.Parser block in TypesafeConductRPlugin.Parsers as, currently, with this PR, an invalid IP address causes sys error, but this should be caught be parsers.

wrt your first comment above: 

s/uri/host

I'm not clear on what you intended to tell me.. ? arg naming?

hseeberger commented 9 years ago

Great, I already thought it might be better to use sbt parsers instead of Scala ones.

Yes, replace uri with host.

dsugden commented 9 years ago

Issue # 60. 1) removed scala parser combinators: using sbt.Parser, wrote a naive ipAdress parser that should be sufficient for syntax parsing 2) changed tab completion for controlServer from http://IP:PORT to IP:PORT 3) still returning a string (hopefully better than "Success") from ConductRController on setting the URL. The ask pattern necessitates this. 4) Added tests for ipAddress parser: changed visibility of Parsers to private[sbt] for the tests

hseeberger commented 9 years ago

@dsugden, thanks a lot!!!

huntc commented 9 years ago

Thanks Dave!

huntc commented 9 years ago

Hmmm. I'm a little concerned with why the previous logic had started to fail...

huntc commented 9 years ago

Ah, now I get it - when the setConductr input task was introduced it upset the approach we were using, which was to pull the ConductR url from global settings. Given that the previous conductr command was a command and not a task, the sbt session was updated.

@dsugden I may look at reverting this change, not because it isn't up to scratch or anything; it is great to receive the fix. However I would prefer to get things working the way they were. Instead of a setConductr input task I want to revert to the command approach and have set-conductr-ip and set-conductr-port. These are also analogous to the Python CLI's CONDUCTR_IP and CONDUCTR_PORT env vars.

dsugden commented 9 years ago

All good, this fix was fun for me, my first foray into plugins. Revert away :)

I'm enjoying this, let me know how I can help.

huntc commented 9 years ago

Thanks again @dsugden - there's a new PR to replace this: #66

dsugden commented 9 years ago

@huntc no worries, just looked at it, I'm intrigued by the sse connection to conductR. This is an interesting plugin. Looking forward to witnessing this :)

huntc commented 9 years ago

The SSE connection is something already handled by the bridge between ConductR and HAProxy (conductr-haproxy). You can subscribe to control protocol events. The Visualizer also uses SSE to update its view of the world. Check out the source in the browser! :-)