Implement the client part for the logger worker.

Implement the client part for the logger worker.

A bit of cut and paste here, which is why we have a type
called State (all the api client endpoints are called State),
and instead of bucking the trend, I followed blindly along.

Also the
 // TODO: not directly tested
lines are also in all the other client endpoints. Consistency
for the win.

Some test code is also refactored out.

https://code.launchpad.net/~thumper/juju-core/logger-api-client/+merge/185404

(do not edit description out of merge proposal)

[thumper, 2013-09-13 02:33:12]

Please take a look.

[wallyworld, 2013-09-13 02:50:33]

LGTM with the testing issues fixed

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

https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger.go#new...
state/api/logger/logger.go:25: func (st *State) LoggingConfig(tag string)
(string, error) {
Doc string please. At first glance, I don't know what tag is.

https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger.go#new...
state/api/logger/logger.go:46: func (st *State) WatchLoggingConfig(agentTag
string) (watcher.NotifyWatcher, error) {
Doc string again. Also, why agentTag here and tag above?

https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger_test.go
File state/api/logger/logger_test.go (right):

https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger_test.g...
state/api/logger/logger_test.go:66: wc.AssertOneChange()
Do we need to test the content of the event?

https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger_test.g...
state/api/logger/logger_test.go:71: wc.AssertOneChange()
Again, we should test the content of the event matches the change we just made

https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger_test.g...
state/api/logger/logger_test.go:78: wc.AssertOneChange()
ditto

[thumper, 2013-09-13 03:34:56]

On 2013/09/13 02:50:33, wallyworld wrote:
> LGTM with the testing issues fixed
> 
> https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger.go
> File state/api/logger/logger.go (right):
> 
>
https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger.go#new...
> state/api/logger/logger.go:25: func (st *State) LoggingConfig(tag string)
> (string, error) {
> Doc string please. At first glance, I don't know what tag is.
> 
>
https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger.go#new...
> state/api/logger/logger.go:46: func (st *State) WatchLoggingConfig(agentTag
> string) (watcher.NotifyWatcher, error) {
> Doc string again. Also, why agentTag here and tag above?

Will do.

> https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger_test.go
> File state/api/logger/logger_test.go (right):
> 
>
https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger_test.g...
> state/api/logger/logger_test.go:66: wc.AssertOneChange()
> Do we need to test the content of the event?
> 
>
https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger_test.g...
> state/api/logger/logger_test.go:71: wc.AssertOneChange()
> Again, we should test the content of the event matches the change we just made
> 
>
https://codereview.appspot.com/13691043/diff/1/state/api/logger/logger_test.g...
> state/api/logger/logger_test.go:78: wc.AssertOneChange()
> ditto

There is no content to check.  It is a notify watcher, so the event is struct{}.

[rog, 2013-09-13 06:56:58]

LGTM with one suggestion below.

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

https://codereview.appspot.com/13691043/diff/1/state/apiserver/logger/logger....
state/apiserver/logger/logger.go:19: // endpoint because our rpc mechanism
panics.  However, I still feel that this
Interesting; that's a bug in the rpc package.
Fix proposed at https://codereview.appspot.com/13300050

But I'm not sure I agree with the principle - how is an
interface better documentation than the LoggerAPI type
itself? I'd move the first sentence of this comment onto that type
and delete the interface.