Replace agent.Conf with an interface

Replace agent.Conf with an interface

This was supposed to be smallish, but turned epic.
I also unexported the Conf structure so construction
now has to be through one of the two New* methods or
the Read method.

A lot of the changes are making the call sites work
with the interface instead of directly poking at the
structure.

Quite a few of the tests in agent were testing for
situations that can no longer occur.  Those have
been removed.

https://code.launchpad.net/~thumper/juju-core/agent-conf-interface/+merge/181450

(do not edit description out of merge proposal)

[thumper, 2013-08-22 04:23:21]

Please take a look.

[fwereade, 2013-08-22 10:35:43]

LGTM, this is awesome. Just a few trivial notes:

https://codereview.appspot.com/13164043/diff/1/agent/agent_test.go
File agent/agent_test.go (left):

https://codereview.appspot.com/13164043/diff/1/agent/agent_test.go#oldcode25
agent/agent_test.go:25: var _ = gc.Suite(suite{})
Please always register suites as pointers -- if/when someone adds test methods
that expect a pointer receiver they'll silently not get run. Wouldn't matter if
everyone was perfect and always ran a failing test first, but... ;)

https://codereview.appspot.com/13164043/diff/1/agent/agent_test.go
File agent/agent_test.go (right):

https://codereview.appspot.com/13164043/diff/1/agent/agent_test.go#newcode198
agent/agent_test.go:198: // method.
Nice.

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

https://codereview.appspot.com/13164043/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:75: err = m.SetPassword(initialMachinePassword)
Doesn't SetPassword also SetMongoPassword? (might well not, just thought it did)

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

https://codereview.appspot.com/13164043/diff/1/cmd/jujud/unit_test.go#newcode47
cmd/jujud/unit_test.go:47: err = unit.SetPassword(initialUnitPassword)
Ditto.

https://codereview.appspot.com/13164043/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/13164043/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:249: Password:       password,
FWIW this is, I think, a bootstrap compatibility break (until we have ian's
pick-matching-bootstrap-tools fix in). Please confer with him and dave; I guess
it doesn't matter if we release a dev version with this in, but once this lands
we *cannot* do a new stable release without his change in too.

https://codereview.appspot.com/13164043/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:254: },
This looks identical to...

https://codereview.appspot.com/13164043/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:270: })
...this. Would be nice to pull out into a var.

https://codereview.appspot.com/13164043/diff/1/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/13164043/diff/1/provider/local/environ.go#newc...
provider/local/environ.go:513: CACert:         info.CACert,
I guess it's not practical to consolidate this with the bits in cloudinit at
this stage, but there's a little bit of a smell here I think.

https://codereview.appspot.com/13164043/diff/1/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/13164043/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:106: Nonce:          "unused",
slight raised eyebrow... I expect we'll be passing this field up in the shared
agent connection code sooner or later, and we'll need the api server to expect
"unused"?

[jameinel, 2013-08-22 11:17:05]

Some thoughts I wanted to make sure to get out since William did the review.

https://codereview.appspot.com/13164043/diff/1/agent/agent.go
File agent/agent.go (left):

https://codereview.appspot.com/13164043/diff/1/agent/agent.go#oldcode59
agent/agent.go:59: }
Is the goal that 'conf' "is a" Config? In which case, we probably want the:

var _ Config = (*Conf)(nil)

Statement to solidify that (as a declaration to people reading the code, rather
than a constraint that the compiler will find as soon as you return one of them
from a function).

https://codereview.appspot.com/13164043/diff/1/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/13164043/diff/1/agent/agent.go#newcode42
agent/agent.go:42: 
A bunch of the following functions feel like they should be either free-form
functions that accept the interface, or on a different interface that helps the
Config object do stuff.

It may not be something we want to change right now, but it does highlight that
*Config* was doing a bit more than seems correct.

A different breakdown would be:
an object that handles reading and writing itself to disk.
a function that takes the abstract in-memory content and uses it to open an API
connection
a function that takes the in-memory content and opens a DB connection
etc.

https://codereview.appspot.com/13164043/diff/1/agent/agent.go#newcode171
agent/agent.go:171: 
I think we eventually will have API server machines that *don't* host the Mongo
DB (State). So we might want to split this a bit further. We can argue that we
should delay until we actually need it, though I'd like us to be sure we can get
there from here.

[thumper, 2013-08-22 21:39:12]

https://codereview.appspot.com/13164043/diff/1/agent/agent.go
File agent/agent.go (left):

https://codereview.appspot.com/13164043/diff/1/agent/agent.go#oldcode59
agent/agent.go:59: }
On 2013/08/22 11:17:05, jameinel wrote:
> Is the goal that 'conf' "is a" Config? In which case, we probably want the:
> 
> var _ Config = (*Conf)(nil)
> 
> Statement to solidify that (as a declaration to people reading the code,
rather
> than a constraint that the compiler will find as soon as you return one of
them
> from a function).
> 

Done.

https://codereview.appspot.com/13164043/diff/1/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/13164043/diff/1/agent/agent.go#newcode42
agent/agent.go:42: 
On 2013/08/22 11:17:05, jameinel wrote:
> A bunch of the following functions feel like they should be either free-form
> functions that accept the interface, or on a different interface that helps
the
> Config object do stuff.
> 
> It may not be something we want to change right now, but it does highlight
that
> *Config* was doing a bit more than seems correct.
> 
> A different breakdown would be:
> an object that handles reading and writing itself to disk.
> a function that takes the abstract in-memory content and uses it to open an
API
> connection
> a function that takes the in-memory content and opens a DB connection
> etc.

I agree here.  I would like to defer that to a later branch.

https://codereview.appspot.com/13164043/diff/1/agent/agent.go#newcode171
agent/agent.go:171: 
On 2013/08/22 11:17:05, jameinel wrote:
> I think we eventually will have API server machines that *don't* host the
Mongo
> DB (State). So we might want to split this a bit further. We can argue that we
> should delay until we actually need it, though I'd like us to be sure we can
get
> there from here.

I don't see any reason why we can't get here from there.  It is just more work
:-)  A first thing to do would be to add a machine job type for being an api
server.  We'd probably then want to have extra configs.  I'm being a bit vague
here as until we need it, it is hard to break apart.  I do have a feeling that
the StatePort isn't actually used.

https://codereview.appspot.com/13164043/diff/1/agent/agent_test.go
File agent/agent_test.go (left):

https://codereview.appspot.com/13164043/diff/1/agent/agent_test.go#oldcode25
agent/agent_test.go:25: var _ = gc.Suite(suite{})
On 2013/08/22 10:35:44, fwereade wrote:
> Please always register suites as pointers -- if/when someone adds test methods
> that expect a pointer receiver they'll silently not get run. Wouldn't matter
if
> everyone was perfect and always ran a failing test first, but... ;)

Done.

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

https://codereview.appspot.com/13164043/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:75: err = m.SetPassword(initialMachinePassword)
On 2013/08/22 10:35:44, fwereade wrote:
> Doesn't SetPassword also SetMongoPassword? (might well not, just thought it
did)

No.  Although these two methods are almost always done in pairs.

I felt it was outside the scope of this change to fix that.

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

https://codereview.appspot.com/13164043/diff/1/cmd/jujud/unit_test.go#newcode47
cmd/jujud/unit_test.go:47: err = unit.SetPassword(initialUnitPassword)
On 2013/08/22 10:35:44, fwereade wrote:
> Ditto.

The api SetPassword does both, but not the machine one.

https://codereview.appspot.com/13164043/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/13164043/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:249: Password:       password,
On 2013/08/22 10:35:44, fwereade wrote:
> FWIW this is, I think, a bootstrap compatibility break (until we have ian's
> pick-matching-bootstrap-tools fix in). Please confer with him and dave; I
guess
> it doesn't matter if we release a dev version with this in, but once this
lands
> we *cannot* do a new stable release without his change in too.

What is the break here?

https://codereview.appspot.com/13164043/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:270: })
On 2013/08/22 10:35:44, fwereade wrote:
> ...this. Would be nice to pull out into a var.

Done now.  The param objects were added later in the game, and the commonality
missed here.

https://codereview.appspot.com/13164043/diff/1/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/13164043/diff/1/provider/local/environ.go#newc...
provider/local/environ.go:513: CACert:         info.CACert,
On 2013/08/22 10:35:44, fwereade wrote:
> I guess it's not practical to consolidate this with the bits in cloudinit at
> this stage, but there's a little bit of a smell here I think.

There is always going to be some overlap in the cloudinit stuff and the local
provider (and likely manual bootstrap) due to the similarity of what they are
trying to do, but in a slightly different way.

https://codereview.appspot.com/13164043/diff/1/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/13164043/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:106: Nonce:          "unused",
On 2013/08/22 10:35:44, fwereade wrote:
> slight raised eyebrow... I expect we'll be passing this field up in the shared
> agent connection code sooner or later, and we'll need the api server to expect
> "unused"?

It is unused in this test, or in any codepath for this test, but I made the
NewAgentConfig be strict on requiring a non-empty nonce, which is why it is now
added here.

When we start passing it up with the API, this test should break and then be
fixed, no?

[thumper, 2013-08-22 22:28:45]

Please take a look.

[rog, 2013-08-23 09:13:02]

I very much like it that this branch
hides some of the grungy details of the agent
configuration behind appropriate methods.
Thanks a lot for working on this.

I have some thoughts about some other
aspects of this though, outlined
below.

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go#newcode27
agent/agent.go:27: type Config interface {
Doc comment, please.

And I'm a little puzzled as to what we gain by
making this an interface rather than simply
unexporting the component fields and defining
methods on the type. It doesn't reduce
coupling AFAICS, and it's not like the concrete
type is difficult or expensive to mock.

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go#newcode29
agent/agent.go:29: // containing the configuration files.
s/the/its/ ?

And I think this is wrong - the data dir is shared between all
agents, otherwise we wouldn't need a distincton between DataDir
and Dir below.

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go#newcode50
agent/agent.go:50: // the given Conf.
What given Conf?

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go#newcode53
agent/agent.go:53: // Write writes the agent configuration.
... to the agent's directory. ?

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go#newcode61
agent/agent.go:61: // GenerateNewPassword creates a new random password and
saves this.  The
This doesn't make it very clear what's going on.
Perhaps:

// GenerateNewPassword changes the Config's password
// to be a new random password and saves the
// new configuration by calling Config.Write.
// It returns the new password.

?

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go#newcode69
agent/agent.go:69: // APIServerDetails returns the details needed to run an API
server.
This could do with a little more documentation. It does
not say what port, cert or key are. The original documentation
for StateServerCert and StateServerKey was more clear.

Perhaps consider splitting this into two methods, one
to return the port and one to return the cert and key.

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go#newcode74
agent/agent.go:74: type conf struct {
"config" might be a better name here.

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go#newcode114
agent/agent.go:114: type AgentConfigParams struct {
Doc comment please. And on the fields too. The data
that is passed in here is the principal reason
for the existence of this type.

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go#newcode138
agent/agent.go:138: // blank.  This is by design.
The conflation of credentials and location is the underlying
problem here, I think (and the central problem with state.Info
as a concept). I'd love to see a branch that fixes that.

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go#newcode175
agent/agent.go:175: type StateMachineConfigParams struct {
Doc comment please.

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go#newcode434
agent/agent.go:434: func InitialStateConfiguration(agentConfig Config, cfg
*config.Config, timeout state.DialOpts) (*state.State, error) {
This isn't a great name - it sounds like it's returning a configuration
where actually it's highly active: it initializes the database.

I'd be tempted to call it InitializeState (or perhaps BootstrapState,
given "bootstrapUsers" below)

And it really needs a doc comment.

https://codereview.appspot.com/13164043/diff/12001/agent/agent.go#newcode435
agent/agent.go:435: c := agentConfig.(*conf)
This is a smell. Why are we using an interface at all
if we blithely assume that it always has the same static
type?

https://codereview.appspot.com/13164043/diff/12001/worker/deployer/simple_tes...
File worker/deployer/simple_test.go (right):

https://codereview.appspot.com/13164043/diff/12001/worker/deployer/simple_tes...
worker/deployer/simple_test.go:132: testing.LoggingSuite
I'm not wild about this. It breaks the
current pattern where we always call the
SetUpSuite/TearDownSuite methods of a
fixture as well as its SetUpTest/TearDownTest
methods.

Also, the LoggingSuite here is also redundant
in deployerSuite, which already includes it
by virtue of including JujuConnSuite.

I'd prefer to directly include LoggingSuite
into SimpleContextSuite.

Aside: at some point, I'd love to consistently move
away from calling our fixtures "Suites"
towards a model more like this SimpleToolsFixture
(or, better perhaps, a model where we always return a
function to undo the setup)

https://codereview.appspot.com/13164043/diff/12001/worker/deployer/simple_tes...
worker/deployer/simple_test.go:246: c.Assert(conf.DataDir(), gc.Equals,
fix.dataDir)
We've lost test coverage here, I think. We should still be checking
that the right arguments have been passed into NewAgentConfig.