Adds get/setManagementNodeUri to ManagementContext interface

[sjcorbett]

I'm seeking feedback rather than expecting this to be merged.

The setter on the interface isn't great. How about just having getManagementNodeUri on the interface and a set.. on AbstractManagementContext. Anything that wants to set (only BrooklynWebServer?) has to cast.

[ahgittin]

+1 to needing to cast in order to set

we'll also need a way to set the URL explicitly, as hostname detection can be brittle in clouds. either brooklyn.properties or a CLI argument. i think i favour the latter as the former is sometimes copied and pasted between machines. @aledsage @andreaturl @grkvlt wdyt?

also probably worth describing any injected URL as the "broadcast" URL to prevent confusion with a bind URL.

[ahgittin]

commits so far are good to merge - code looks good and confirmed working in conjunction with #1438 .

@sjcorbett do you prefer to leave this open to address the comments above? or merge now so we can at least use this?

[sjcorbett]

Let's leave this open. I'll address the comments in an hour or two.

[buildhive]

Brooklyn Central » brooklyn #2397 SUCCESS This pull request looks good (what's this?)

[sjcorbett]

@ahgittin Can we just use the --bindAddress flag for the injected property?

@Option(name = { "-b", "--bindAddress" },
        description = "Specifies the IP address of the NIC to bind the Brooklyn Management Console to")
public String bindAddress = null;

[buildhive]

Brooklyn Central » brooklyn #2403 FAILURE Looks like there's a problem with this pull request (what's this?)

[buildhive]

Brooklyn Central » brooklyn #2404 SUCCESS This pull request looks good (what's this?)

[buildhive]

Brooklyn Central » brooklyn #2405 SUCCESS This pull request looks good (what's this?)

[buildhive]

Brooklyn Central » brooklyn #2409 SUCCESS This pull request looks good (what's this?)

[ahgittin]

in the absence of a --broadcastAddress then makes sense to use --bindAddress but the former should be a separate configurable property as bindAddress must be IP known on the box which may be different from publicly known IP or hostname

[ahgittin]

just checking, these two tasks are still outstanding, correct? :

• remove interface method, use cast in order to set
• add broadcast address to CLI

[sjcorbett]

No, only the second task is outstanding.

[aledsage]

I think we should definitely not use --bindAddress - that could be 0.0.0.0. How about adding --advertisedAddress rather than broadcastAddress (in case someone thinks we are broadcasting).

The value for advertisedAddress could default to the value of bindAddress, unless it is 0.0.0.0 - we could treat that as a special case, and default to hostname?

[ahgittin]

--publicAddress ? that feels even better in case internal broadcasting addresses become necessary, plus nobody likes advertising :P On 9 Jun 2014 07:26, "Aled Sage" notifications@github.com wrote:

I think we should definitely not use --bindAddress - that could be 0.0.0.0. How about adding --advertisedAddress rather than broadcastAddress (in case someone thinks we are broadcasting).

The value for advertisedAddress could default to the value of bindAddress, unless it is 0.0.0.0 - we could treat that as a special case, and default to hostname?

— Reply to this email directly or view it on GitHub https://github.com/brooklyncentral/brooklyn/pull/1439#issuecomment-45464507 .

[sjcorbett]

Comments resolved? I went for --publicAddress.

[buildhive]

Brooklyn Central » brooklyn #2463 FAILURE Looks like there's a problem with this pull request (what's this?)

[buildhive]

Brooklyn Central » brooklyn #2464 SUCCESS This pull request looks good (what's this?)

[buildhive]

Brooklyn Central » brooklyn #2465 FAILURE Looks like there's a problem with this pull request (what's this?)

[buildhive]

Brooklyn Central » brooklyn #2466 SUCCESS This pull request looks good (what's this?)

[sjcorbett]

@buildhive Is a bit of a hassle at the moment..

[ahgittin]

looks great, runs great. nice work. have made tiny tweaks and fixed merge conflicts, will push shortly.