Code review comment for lp:~wallyworld/juju-core/watcher-address-issues

> Hi Ian, this branch looks good with some comments below.
>
> As I mentioned in the bug, not sure why LXC addresses have empty scopes.
> I'll try to describe my concerns: API clients are often interested in the
> public address of a unit/machine.
> When using cloud providers, fetching them is trivial:
> - just pick the first "ipv4" address with "public" NetworkScope.
> When using local envs, currently is more tricky, and I am not sure about the
> logic: the scope is empty so,
> the logic above might become:
> - try to find an "ipv4" address with "public" NetworkScope
> - if none is found, pick the first "ipv4" address with an empty scope, it
> must be the public one.
> Is this correct? If so, we can surely make quickstart/GUI work like that.
> Anyway, it does not seem the best client story we can have.
>

The current logic to put public/private address on the unit item in the mega watcher calls Pubic/PrivateAddress() on state.Unit. These methods look at the addresses gathered by the instance poller and stored on the machhne and return the ones considered to be most public/private respectively. For the local provider, I'm not sure what the instance poller does, I'd have to check.

> 8 + // As of 1.18, provider addresses are not gathered anymore so
> this field
> 9 + // will not be populated. It is retained for backwards
> compatibility.
>
> This does not seem to reflect reality. AFAICT provider addresses are still
> used and populated for top level machines.
>

I think there's crossed wires here. My interpretation of "provider addresses" is the address getters on the provider itself and these have been removed. Maybe that's not what you mean. I'll remove the comment.

> 25 + addresses := make([]instance.Address, len(merged))
> 26 + for _, address := range merged {
> 27 + addresses = append(addresses, address)
> 28 + }
>
> It seems we are creating the addresses with a specific length but then we
> append to it. This way we have len(merged) empty addresses at the beginning of
> the slice. I guess we should either assign by index or start with a 0 length
> slice.
>

Fixed.

> 37 +// Note: As of 1.18, provider addresses are no longer populated.
>
> Same as above.

Removed comment.