state/api/worker: use Tools in params

state/api/worker: use Tools in params

This simplifies the SetAgentTools call generally.

https://code.launchpad.net/~rogpeppe/juju-core/348-upgrader-api-use-tools-type/+merge/176883

(do not edit description out of merge proposal)

[rog, 2013-07-25 08:02:50]

Please take a look.

[dimitern, 2013-07-25 09:14:33]

LGTM with a couple of suggestions below

https://codereview.appspot.com/11800044/diff/5001/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/11800044/diff/5001/state/api/params/internal.g...
state/api/params/internal.go:93: // SetAgent tools specifies tools to be set for
an agent with the
// SetAgentTools specifies...

https://codereview.appspot.com/11800044/diff/5001/state/api/upgrader/upgrader.go
File state/api/upgrader/upgrader.go (right):

https://codereview.appspot.com/11800044/diff/5001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader.go:26: func (st *State) SetTools(tag string, tools
*tools.Tools) error {
doc comment?

[fwereade, 2013-07-25 09:42:29]

This looks excellent, just one question.

https://codereview.appspot.com/11800044/diff/5001/state/apiserver/upgrader/up...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/11800044/diff/5001/state/apiserver/upgrader/up...
state/apiserver/upgrader/upgrader.go:129: type setAgentTooler interface {
YMMV, but such things have tended to be implemented as interfaces in state. I'd
really prefer consistency here -- are there other considerations in play?

[fwereade, 2013-07-25 09:46:17]

LGTM with the interface move.

[rog, 2013-07-25 11:15:10]

Please take a look.

[rog, 2013-07-25 11:15:22]

https://codereview.appspot.com/11800044/diff/5001/state/apiserver/upgrader/up...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/11800044/diff/5001/state/apiserver/upgrader/up...
state/apiserver/upgrader/upgrader.go:129: type setAgentTooler interface {
On 2013/07/25 09:42:29, fwereade wrote:
> YMMV, but such things have tended to be implemented as interfaces in state.
I'd
> really prefer consistency here -- are there other considerations in play?

Done.

[rog, 2013-07-25 11:18:34]

Please take a look.

https://codereview.appspot.com/11800044/diff/5001/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/11800044/diff/5001/state/api/params/internal.g...
state/api/params/internal.go:93: // SetAgent tools specifies tools to be set for
an agent with the
On 2013/07/25 09:14:33, dimitern wrote:
> // SetAgentTools specifies...

Done.

https://codereview.appspot.com/11800044/diff/5001/state/api/upgrader/upgrader.go
File state/api/upgrader/upgrader.go (right):

https://codereview.appspot.com/11800044/diff/5001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader.go:26: func (st *State) SetTools(tag string, tools
*tools.Tools) error {
On 2013/07/25 09:14:33, dimitern wrote:
> doc comment?

Done.

[jameinel, 2013-07-27 21:59:03]

When submitting this sort of thing for review, it would be probably good to
highlight that this is an API break. (The bytes on the wire for AgentTools is
different, I do believe.)

I'm a little hesitant on having state/api/params depend on anything, given we've
managed to avoid dependencies so far. Though I understand the desire to avoid
duplication.

[rog, 2013-07-29 08:28:08]

On 27 July 2013 22:59,  <john.meinel@canonical.com> wrote:
> When submitting this sort of thing for review, it would be probably good
> to highlight that this is an API break. (The bytes on the wire for
> AgentTools is different, I do believe.)

If anything that was currently hooked in was sending a tools
description on the wire,
I would not have changed it. As it is, I think the only thing that
does is the upgrader
which isn't yet committed.

> I'm a little hesitant on having state/api/params depend on anything,
> given we've managed to avoid dependencies so far.

state/api/params already depends on a few external things:
instance.Port, charm.Meta, constraints.Value. I agree we should
avoid adding unnecessary dependencies, but this one seems ok to me.
It would be nice to have some automated way of determining which
types are used in the API so that we can have alarm bells go off when
they're changed.

  cheers,
    rog.