api/uniter: Remaining unit operations

api/uniter: Remaining unit operations

Implemented what's left of the client-side uniter API
operations on units:
* ConfigSettings
* WatchConfigSettings
* OpenPort
* ClosePort
* IsPrincipal
* SubordinateNames
* Destroy
* Resolved
* ClearResolved
* CharmURL
* SetCharmURL
* PublicAddress
* SetPublicAddress
* PrivateAddress
* SetPrivateAddress

https://code.launchpad.net/~dimitern/juju-core/112-api-uniter-unit-ops2/+merge/183412

(do not edit description out of merge proposal)

[dimitern, 2013-09-02 10:16:27]

Please take a look.

[fwereade, 2013-09-02 10:48:57]

Looking very good, just a couple of questions.

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/unit.go
File state/api/uniter/unit.go (right):

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/unit.go#newco...
state/api/uniter/unit.go:150: return convertSettings(result.Settings), nil
Aren't these both map[string]interface{}?

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/unit.go#newco...
state/api/uniter/unit.go:315: return result.Result, result.Ok, nil
It's not worth fixing now, because we hope to drop this code, but fwiw: (result,
bool) is a special case of (result, error), used when there's only one possible
error. When that's no longer true, the information encoded in the bool should be
made into an explicit error.

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/unit.go#newco...
state/api/uniter/unit.go:401: return curl, result.Ok, nil
Ehh, similarly.

And, hmm, I'm starting to think we *should* fix this before we do a release with
the uniter API. But it should definitely be a new CL.

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/uniter_test.go
File state/api/uniter/uniter_test.go (right):

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/uniter_test.g...
state/api/uniter/uniter_test.go:142: c.Assert(err, gc.ErrorMatches, `unit
"wordpress/0" not found`)
This is surprising to me. A unit shouldn't be able to destroy *itself*, should
it? Only its subordinates...

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/uniter_test.g...
state/api/uniter/uniter_test.go:203: c.Assert(mode, gc.Equals,
params.ResolvedNone)
in general, s.stateUnit and apiUnit might be more helpful names.

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/uniter_test.g...
state/api/uniter/uniter_test.go:426: // NOTE: This test is not as exhaustive as
the one it state,
s/ it / in /

[fwereade, 2013-09-02 11:14:52]

Discussed online -- (1) a unit does need to be able to destroy itself, (2) it
also needs to destroy its subordinates, (3) fixing that is out of scope.

So, LGTM, so long as we get an imminent followup that fixes Destroy server-side.

[dimitern, 2013-09-02 11:22:19]

Please take a look.

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/unit.go
File state/api/uniter/unit.go (right):

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/unit.go#newco...
state/api/uniter/unit.go:150: return convertSettings(result.Settings), nil
On 2013/09/02 10:48:57, fwereade wrote:
> Aren't these both map[string]interface{}?

No, params.Settings is map[string]string, as we agreed before, and at
server-side we do a map[string]interface{} -> map[string]string conversion for
sending over the wire. Here, we need the opposite conversion for that matter.

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/unit.go#newco...
state/api/uniter/unit.go:315: return result.Result, result.Ok, nil
On 2013/09/02 10:48:57, fwereade wrote:
> It's not worth fixing now, because we hope to drop this code, but fwiw:
(result,
> bool) is a special case of (result, error), used when there's only one
possible
> error. When that's no longer true, the information encoded in the bool should
be
> made into an explicit error.

Agreed. My intention here was to change the method signature as little as
possible, while taking into account the error we can get from the API call. But
as you said, it's something that may or may not remain, so I'll leave it for
now.

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/unit.go#newco...
state/api/uniter/unit.go:401: return curl, result.Ok, nil
On 2013/09/02 10:48:57, fwereade wrote:
> Ehh, similarly.
> 
> And, hmm, I'm starting to think we *should* fix this before we do a release
with
> the uniter API. But it should definitely be a new CL.

Will do.

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/uniter_test.go
File state/api/uniter/uniter_test.go (right):

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/uniter_test.g...
state/api/uniter/uniter_test.go:142: c.Assert(err, gc.ErrorMatches, `unit
"wordpress/0" not found`)
On 2013/09/02 10:48:57, fwereade wrote:
> This is surprising to me. A unit shouldn't be able to destroy *itself*, should
> it? Only its subordinates...

As discussed online, there are 2 cases where the uniter calls unit.Destroy() on
a non-subordinate (filter.go:453 and uniter.go:498), but I'll add a test about
destroying a subordinate.

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/uniter_test.g...
state/api/uniter/uniter_test.go:203: c.Assert(mode, gc.Equals,
params.ResolvedNone)
On 2013/09/02 10:48:57, fwereade wrote:
> in general, s.stateUnit and apiUnit might be more helpful names.

I like the suggestion, but if you don't mind, I'd like to change that in a
follow-up.

https://codereview.appspot.com/13272045/diff/1/state/api/uniter/uniter_test.g...
state/api/uniter/uniter_test.go:426: // NOTE: This test is not as exhaustive as
the one it state,
On 2013/09/02 10:48:57, fwereade wrote:
> s/ it / in /

Done.