Tidy up jujud agents

Tidy up jujud agents

* --session-file is made obsolete by the state/presence package, and has
  been removed.
* apart from that deliberate change, functionality should be unaffected.
* agentConf became public and lost its Command methods: it now supplies
  addFlags and checkArgs methods, which are used by individual agents in
  their own Init methods. This follows the pattern established by cmd.Log
  and eliminates the slightly awkward inversion of flagset handling in the
  cmd/jujud package, at the cost of having to implement an explicit Init
  method on ProvisioningAgent.
* contents of flag.go moved into agent.go; neither was big enough to justify
  independent existence.

https://code.launchpad.net/~fwereade/juju/go-agent-cleanup/+merge/103830

(do not edit description out of merge proposal)

[fwereade, 2012-04-27 08:18:10]

Please take a look.

[rog, 2012-04-27 09:38:32]

LGTM modulo vague pondering below.

https://codereview.appspot.com/6137043/diff/1/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6137043/diff/1/cmd/jujud/agent.go#newcode73
cmd/jujud/agent.go:73: return cmd.CheckEmpty(args)
is it the case that no agent may have any arguments?
i wonder if it might be better to call this function checkFlags (taking no
arguments), and move the checkEmpty call into the agents themselves.

[fwereade, 2012-04-27 09:56:44]

https://codereview.appspot.com/6137043/diff/1/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6137043/diff/1/cmd/jujud/agent.go#newcode73
cmd/jujud/agent.go:73: return cmd.CheckEmpty(args)
On 2012/04/27 09:38:32, rog wrote:
> is it the case that no agent may have any arguments?
> i wonder if it might be better to call this function checkFlags (taking no
> arguments), and move the checkEmpty call into the agents themselves.

I pondered this as well this time, and we both did in the first version; IMO we
can stick with the original conclusion, that it *is* in fact common behaviour
across all known agents, and may as well be shared; and that it won't be hard to
change if we one day decide we need to.

[rog, 2012-04-27 11:01:38]

On 2012/04/27 09:56:44, fwereade wrote:
> https://codereview.appspot.com/6137043/diff/1/cmd/jujud/agent.go
> File cmd/jujud/agent.go (right):
> 
> https://codereview.appspot.com/6137043/diff/1/cmd/jujud/agent.go#newcode73
> cmd/jujud/agent.go:73: return cmd.CheckEmpty(args)
> On 2012/04/27 09:38:32, rog wrote:
> > is it the case that no agent may have any arguments?
> > i wonder if it might be better to call this function checkFlags (taking no
> > arguments), and move the checkEmpty call into the agents themselves.
> 
> I pondered this as well this time, and we both did in the first version; IMO
we
> can stick with the original conclusion, that it *is* in fact common behaviour
> across all known agents, and may as well be shared; and that it won't be hard
to
> change if we one day decide we need to.

i'm happy with that.

[fwereade, 2012-04-27 15:41:38]

Please take a look.

[niemeyer, 2012-05-01 07:34:19]

Feels like there are some nice improvements here, but the agent type still
doesn't feel quite right. This feels like a good chance to fix it.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go#newcode20
cmd/jujud/agent.go:20: var validAddr = regexp.MustCompile("^.*:[0-9]+$")
Is ":1234" valid?

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go#newcode22
cmd/jujud/agent.go:22: // Set splits value into zookeeper addresses.
// Set splits the comma-separated list of ZooKeeper
// addresses and stores onto v's Addrs. Addresses
// must include port numbers.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go#newcode24
cmd/jujud/agent.go:24: v.Addrs = strings.Split(value, ",")
Probably will never be an actual issue, but ideally it should only be stored in
v.Addrs if it's valid.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go#newcode33
cmd/jujud/agent.go:33: // String returns the original value passed to Set.
// String returns the list of ZooKeeper addresses joined by commas.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go#newcode47
cmd/jujud/agent.go:47: // agent provides common agent functionality.
This tastes like a base class for the known agents, with a bag of functionality
that is not well defined. Given what is being implemented below, I suggest
naming it as something like commonFlags.

I'd also get rid of the name field and the Info method, and implement the
one-liner Info in each of the commands. Note that the type here is not even
close to being a command (doesn't implement any of the methods on Command).

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/machine.go#newcode11
cmd/jujud/machine.go:11: agent
Doesn't look like this should be embedded. commonFlags is really a component of
the agent, rather than an "extended commonFlags" of some sort. For example,
machineAgent.addFlags would really not do what one expects.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/machine.go#newcode12
cmd/jujud/machine.go:12: MachineId int
Why is MachineId public, but the rest is not?

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:11: agent
Same.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/unit.go
File cmd/jujud/unit.go (right):

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/unit.go#newcode12
cmd/jujud/unit.go:12: agent
Same.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/unit.go#newcode13
cmd/jujud/unit.go:13: UnitName string
Same.

[fwereade, 2012-05-02 09:46:47]

Please take a look.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go#newcode20
cmd/jujud/agent.go:20: var validAddr = regexp.MustCompile("^.*:[0-9]+$")
On 2012/05/01 07:34:20, niemeyer wrote:
> Is ":1234" valid?

Done. Sorta. Would you prefer something tighter?

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go#newcode22
cmd/jujud/agent.go:22: // Set splits value into zookeeper addresses.
On 2012/05/01 07:34:20, niemeyer wrote:
> // Set splits the comma-separated list of ZooKeeper
> // addresses and stores onto v's Addrs. Addresses
> // must include port numbers.

Done.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go#newcode24
cmd/jujud/agent.go:24: v.Addrs = strings.Split(value, ",")
On 2012/05/01 07:34:20, niemeyer wrote:
> Probably will never be an actual issue, but ideally it should only be stored
in
> v.Addrs if it's valid.

Done.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go#newcode33
cmd/jujud/agent.go:33: // String returns the original value passed to Set.
On 2012/05/01 07:34:20, niemeyer wrote:
> // String returns the list of ZooKeeper addresses joined by commas.

Done.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go#newcode47
cmd/jujud/agent.go:47: // agent provides common agent functionality.
On 2012/05/01 07:34:20, niemeyer wrote:
> This tastes like a base class for the known agents, with a bag of
functionality
> that is not well defined. Given what is being implemented below, I suggest
> naming it as something like commonFlags.
> 
> I'd also get rid of the name field and the Info method, and implement the
> one-liner Info in each of the commands. Note that the type here is not even
> close to being a command (doesn't implement any of the methods on Command).

Excellent points all. Thanks.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/machine.go#newcode11
cmd/jujud/machine.go:11: agent
On 2012/05/01 07:34:20, niemeyer wrote:
> Doesn't look like this should be embedded. commonFlags is really a component
of
> the agent, rather than an "extended commonFlags" of some sort. For example,
> machineAgent.addFlags would really not do what one expects.

Done.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/machine.go#newcode12
cmd/jujud/machine.go:12: MachineId int
On 2012/05/01 07:34:20, niemeyer wrote:
> Why is MachineId public, but the rest is not?

The private ones were there for ease of testing given an embedded `agent`; the
consideration is no longer relevant.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:11: agent
On 2012/05/01 07:34:20, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/unit.go
File cmd/jujud/unit.go (right):

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/unit.go#newcode12
cmd/jujud/unit.go:12: agent
On 2012/05/01 07:34:20, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/unit.go#newcode13
cmd/jujud/unit.go:13: UnitName string
On 2012/05/01 07:34:20, niemeyer wrote:
> Same.

Done. (Btw, for testing's sake, I've been assuming that everything should
actually be public -- the private fields were just so I could have a sensibly
named interface in export_test (now gone))

[niemeyer, 2012-05-03 00:56:50]

LGTM, thank you.

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6137043/diff/2002/cmd/jujud/agent.go#newcode20
cmd/jujud/agent.go:20: var validAddr = regexp.MustCompile("^.*:[0-9]+$")
On 2012/05/02 09:46:47, fwereade wrote:
> Done. Sorta. Would you prefer something tighter?

I think it's fine for the moment. This is an internal command anyway.

[fwereade, 2012-05-03 08:30:31]

*** Submitted:

Tidy up jujud agents

* --session-file is made obsolete by the state/presence package, and has
  been removed.
* apart from that deliberate change, functionality should be unaffected.
* agentConf became public and lost its Command methods: it now supplies
  addFlags and checkArgs methods, which are used by individual agents in
  their own Init methods. This follows the pattern established by cmd.Log
  and eliminates the slightly awkward inversion of flagset handling in the
  cmd/jujud package, at the cost of having to implement an explicit Init
  method on ProvisioningAgent.
* contents of flag.go moved into agent.go; neither was big enough to justify
  independent existence.

R=rog, niemeyer
CC=
https://codereview.appspot.com/6137043