state/api: Deployer client-side facade.

state/api: Deployer client-side facade.

Implemented all the necessary API calls for the
deployer worker. This is the last step needed
before the deployer can be refactored to use
only the API to talk to state.

Also fixed the StringsWatcher client implementation
(I initially got it wrong, but since nobody was using
it yet it got undetected).

Few other minor issues discovered and fixed in state,
adding tests as needed.

https://code.launchpad.net/~dimitern/juju-core/070-deployer-client-facade/+merge/174973

(do not edit description out of merge proposal)

[dimitern, 2013-07-16 10:59:05]

Please take a look.

[mue, 2013-07-16 12:20:37]

LGTM, only some comments.

https://codereview.appspot.com/11342043/diff/1/state/api/deployer/deployer_te...
File state/api/deployer/deployer_test.go (right):

https://codereview.appspot.com/11342043/diff/1/state/api/deployer/deployer_te...
state/api/deployer/deployer_test.go:53: s.stateAPI = s.OpenAPIAs(c,
s.machine.Tag(), "test-password")
Stumbled upon this for the first time. You call it "login" in the comment. So
maybe LoginAsToAPI() would be better. But that's only an idea.

https://codereview.appspot.com/11342043/diff/1/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (right):

https://codereview.appspot.com/11342043/diff/1/state/api/watcher/watcher.go#n...
state/api/watcher/watcher.go:179: stringWatcherId string
It's StringsWatcher and StringsWatchId, so plural strings. Would name the field
here stringsWatcherId too.

https://codereview.appspot.com/11342043/diff/1/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/11342043/diff/1/state/apiserver/root.go#newcode89
state/apiserver/root.go:89: // TODO: There is no direct test for this
// TODO(dimitern) ...

https://codereview.appspot.com/11342043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/11342043/diff/1/state/machine.go#newcode163
state/machine.go:163: if !strings.HasPrefix(tag, machineTagPrefix) {
How does code calling this function react when "" is returned? Maybe it's better
to return an error too.

MachineIdFromTag(tag string) (string, error)

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

https://codereview.appspot.com/11342043/diff/1/state/unit.go#newcode635
state/unit.go:635: if !strings.HasPrefix(tag, unitTagPrefix) {
Same as said for machine here.

[fwereade, 2013-07-16 13:00:06]

LGTM. I can see arguments both ways for the *FromTag functions... I think it's
probably good enough that they return things that are never valid ids/names.

https://codereview.appspot.com/11342043/diff/1/state/api/deployer/deployer_te...
File state/api/deployer/deployer_test.go (right):

https://codereview.appspot.com/11342043/diff/1/state/api/deployer/deployer_te...
state/api/deployer/deployer_test.go:135: c.Assert(err, gc.IsNil)
Removal shouldn't be necessary to see the change, fwiw.

https://codereview.appspot.com/11342043/diff/1/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (right):

https://codereview.appspot.com/11342043/diff/1/state/api/watcher/watcher.go#n...
state/api/watcher/watcher.go:179: stringWatcherId string
On 2013/07/16 12:20:37, mue wrote:
> It's StringsWatcher and StringsWatchId, so plural strings. Would name the
field
> here stringsWatcherId too.

+1

[dimitern, 2013-07-16 14:20:14]

Please take a look.

https://codereview.appspot.com/11342043/diff/1/state/api/deployer/deployer_te...
File state/api/deployer/deployer_test.go (right):

https://codereview.appspot.com/11342043/diff/1/state/api/deployer/deployer_te...
state/api/deployer/deployer_test.go:53: s.stateAPI = s.OpenAPIAs(c,
s.machine.Tag(), "test-password")
On 2013/07/16 12:20:37, mue wrote:
> Stumbled upon this for the first time. You call it "login" in the comment. So
> maybe LoginAsToAPI() would be better. But that's only an idea.

The idea AFAIU is that you always need to login when you open a connection the
the API server, hence the name of the function.

https://codereview.appspot.com/11342043/diff/1/state/api/deployer/deployer_te...
state/api/deployer/deployer_test.go:135: c.Assert(err, gc.IsNil)
On 2013/07/16 13:00:06, fwereade wrote:
> Removal shouldn't be necessary to see the change, fwiw.

Good point, will leave only the EnsureDead call.

https://codereview.appspot.com/11342043/diff/1/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (right):

https://codereview.appspot.com/11342043/diff/1/state/api/watcher/watcher.go#n...
state/api/watcher/watcher.go:179: stringWatcherId string
On 2013/07/16 12:20:37, mue wrote:
> It's StringsWatcher and StringsWatchId, so plural strings. Would name the
field
> here stringsWatcherId too.

Done.

https://codereview.appspot.com/11342043/diff/1/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/11342043/diff/1/state/apiserver/root.go#newcode89
state/apiserver/root.go:89: // TODO: There is no direct test for this
On 2013/07/16 12:20:37, mue wrote:
> // TODO(dimitern) ...

Done.

https://codereview.appspot.com/11342043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/11342043/diff/1/state/machine.go#newcode163
state/machine.go:163: if !strings.HasPrefix(tag, machineTagPrefix) {
On 2013/07/16 12:20:37, mue wrote:
> How does code calling this function react when "" is returned? Maybe it's
better
> to return an error too.
> 
> MachineIdFromTag(tag string) (string, error)

I didn't want to change the signature, but I added a test for that. I'll add a
TODO for your suggestion as a possible future improvement.

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

https://codereview.appspot.com/11342043/diff/1/state/unit.go#newcode635
state/unit.go:635: if !strings.HasPrefix(tag, unitTagPrefix) {
On 2013/07/16 12:20:37, mue wrote:
> Same as said for machine here.

Same answer :)

[fwereade, 2013-07-17 07:57:29]

FWIW, I don't think you fixed the potential event-skipping in StringsWatcher.
Followup please, though, rather than adding to this.