Remove env cert creation from environs.Bootstrap

Remove env cert creation from environs.Bootstrap

The generation of the environment certificates is moved to the Bootstrap
command.  Tests updated.

https://code.launchpad.net/~thumper/juju-core/move-cert-gen/+merge/153279

(do not edit description out of merge proposal)

[thumper, 2013-03-14 03:05:44]

Please take a look.

[dimitern, 2013-03-14 08:43:42]

LGTM with some trivials.

https://codereview.appspot.com/7809043/diff/1/cmd/builddb/main.go
File cmd/builddb/main.go (right):

https://codereview.appspot.com/7809043/diff/1/cmd/builddb/main.go#newcode31
cmd/builddb/main.go:31: // TODO: what about environments without certs here?
what about them?

https://codereview.appspot.com/7809043/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/7809043/diff/1/cmd/juju/bootstrap.go#newcode78
cmd/juju/bootstrap.go:78: if hasCACert && !hasCAKey {
you're already checking for both up there, so !hasCAKey should be enough here?

https://codereview.appspot.com/7809043/diff/1/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/7809043/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:74: err = checkCertificate(env)
move that with the assert below?

https://codereview.appspot.com/7809043/diff/1/environs/bootstrap_test.go
File environs/bootstrap_test.go (right):

https://codereview.appspot.com/7809043/diff/1/environs/bootstrap_test.go#newc...
environs/bootstrap_test.go:38: c.Assert(err, ErrorMatches, "environment
configuration missing CA certificate")
how about checking for the missing key error?

[rog, 2013-03-14 11:46:52]

this is reasonable, but please leave the certificate generation logic inside
environs, or at least outside of cmd/juju.

https://codereview.appspot.com/7809043/diff/1/cmd/builddb/main.go
File cmd/builddb/main.go (right):

https://codereview.appspot.com/7809043/diff/1/cmd/builddb/main.go#newcode31
cmd/builddb/main.go:31: // TODO: what about environments without certs here?
this issue was the main consideration behind the current design of
environs.Bootstrap - it was considered important that it be easy to bootstrap
from Go itself and the API was designed accordingly. this change is a
significant regression IMHO.

that said, in my opinion builddb should not be bootstrapping anyway, so one fix
would be to simply remove that line from here, which makes it possible to use
builddb on an existing environment.

https://codereview.appspot.com/7809043/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/7809043/diff/1/cmd/juju/bootstrap.go#newcode102
cmd/juju/bootstrap.go:102: err = checkCertificate(environ)
i'm not happy that this functionality is only available in the command now. i
don't mind it being factored out of environs.Bootstrap itself, but i think it's
important that any Go code that wants to bootstrap can do so without reproducing
the above logic or shelling out to juju bootstrap.

https://codereview.appspot.com/7809043/diff/1/environs/bootstrap_test.go
File environs/bootstrap_test.go (right):

https://codereview.appspot.com/7809043/diff/1/environs/bootstrap_test.go#newc...
environs/bootstrap_test.go:48: // is tested elsewhere.
i think i'd check the error anyway, as the connection between environs.Bootstrap
and $HOME/.juju/foo-cert.pem is not *that* strong, and if someone breaks that,
the error here will be obscure.

[thumper, 2013-03-15 00:28:20]

Please take a look.

[thumper, 2013-03-15 01:07:01]

Just going to push up the latest few changes.

https://codereview.appspot.com/7809043/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/7809043/diff/1/cmd/juju/bootstrap.go#newcode78
cmd/juju/bootstrap.go:78: if hasCACert && !hasCAKey {
On 2013/03/14 08:43:42, dimitern wrote:
> you're already checking for both up there, so !hasCAKey should be enough here?

No... because if we are just checking for one being missing, if both are missing
we still hit the error state, which is wrong.

https://codereview.appspot.com/7809043/diff/1/cmd/juju/bootstrap.go#newcode102
cmd/juju/bootstrap.go:102: err = checkCertificate(environ)
On 2013/03/14 11:46:52, rog wrote:
> i'm not happy that this functionality is only available in the command now. i
> don't mind it being factored out of environs.Bootstrap itself, but i think
it's
> important that any Go code that wants to bootstrap can do so without
reproducing
> the above logic or shelling out to juju bootstrap.

Moved this out to environs/cert.go in the next diffset.

https://codereview.appspot.com/7809043/diff/1/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/7809043/diff/1/cmd/juju/bootstrap_test.go#newc...
cmd/juju/bootstrap_test.go:74: err = checkCertificate(env)
On 2013/03/14 08:43:42, dimitern wrote:
> move that with the assert below?

Done.

https://codereview.appspot.com/7809043/diff/1/environs/bootstrap_test.go
File environs/bootstrap_test.go (right):

https://codereview.appspot.com/7809043/diff/1/environs/bootstrap_test.go#newc...
environs/bootstrap_test.go:38: c.Assert(err, ErrorMatches, "environment
configuration missing CA certificate")
On 2013/03/14 08:43:42, dimitern wrote:
> how about checking for the missing key error?

I think that is now tested in TestEnsureCertificateMissingKey

https://codereview.appspot.com/7809043/diff/1/environs/bootstrap_test.go#newc...
environs/bootstrap_test.go:48: // is tested elsewhere.
On 2013/03/14 11:46:52, rog wrote:
> i think i'd check the error anyway, as the connection between
environs.Bootstrap
> and $HOME/.juju/foo-cert.pem is not *that* strong, and if someone breaks that,
> the error here will be obscure.

Done.

[thumper, 2013-03-15 01:08:36]

Please take a look.

[dave_cheney.net, 2013-03-15 04:42:37]

LGTM. Have you run the live tests ? did they pass ?

https://codereview.appspot.com/7809043/diff/13001/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/bootstrap.go#newco...
environs/bootstrap.go:22: if !hasCACert {
why does cfg.CACert return both a []byte and a bool to indicate success ? why
not just return nil indicating no cert?

https://codereview.appspot.com/7809043/diff/13001/environs/cert.go
File environs/cert.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/cert.go#newcode25
environs/cert.go:25: if err := ioutil.WriteFile(path+"-private-key.pem", key,
0600); err != nil {
can just 

return ioutil.WriteFile(path+"-private-key.pem", key, 0600) ...

[rog, 2013-03-15 09:31:23]

much better, but not there quite yet.
mostly trivial remarks, but i think that EnsureCertificate
is more important.

https://codereview.appspot.com/7809043/diff/13001/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/bootstrap.go#newco...
environs/bootstrap.go:22: if !hasCACert {
On 2013/03/15 04:42:37, dfc wrote:
> why does cfg.CACert return both a []byte and a bool to indicate success ? why
> not just return nil indicating no cert?

the same question applies to state.Machine.InstanceId.
you'd have to ask someone else ;-)

https://codereview.appspot.com/7809043/diff/13001/environs/cert.go
File environs/cert.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/cert.go#newcode56
environs/cert.go:56: func EnsureCertificate(environ Environ) (CreatedCert,
error) {
i'm sorry, i don't think this is quite right.

i believe that it's important that we can perform all the usual juju operations
without needing to write to disk. that's why the original Bootstrap function
took a function as an argument, so if (just for instance) we had some daemon
that was responsible for bootstrapping many juju environments, it could as
easily write the certs to an arbitrary persistent data store as to $HOME.

AFAIK if this gets committed, it's the only place in juju where we require a
home directory, or writable disk storage at all.

FWIW it would also make it easier for the juju command to log something when a
certificate gets created.

https://codereview.appspot.com/7809043/diff/13001/environs/cert_blackbox_test.go
File environs/cert_blackbox_test.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/cert_blackbox_test...
environs/cert_blackbox_test.go:10: type EnvironsCertBlackBoxSuite struct {
rather than calling this "BlackBox" as if that was something unusual (when it's
actually the way that almost all of our tests are written), how about naming the
other file cert_internal_test.go, and the suite inside EnvironsCertInternalSuite
(or just EnvironsCertSuite again - there's no clash) ?

that's the route i've been taking with my recent changes in the state package
and it seems to fit ok.

we should at least agree to be consistent one way or the other :-)

one other remark: i think it's worth at least including testing.LoggingSuite in
any suite, so that log messages get included in the testing output rather than
being discarded.

https://codereview.appspot.com/7809043/diff/13001/environs/cert_test.go
File environs/cert_test.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/cert_test.go#newco...
environs/cert_test.go:12: }
i'd embed testing.LoggingSuite here.

https://codereview.appspot.com/7809043/diff/13001/testing/environ.go
File testing/environ.go (right):

https://codereview.appspot.com/7809043/diff/13001/testing/environ.go#newcode43
testing/environ.go:43: const PeckhamConfig = `
i think that having one default environment here may be ok (although perhaps
better living in juju/testing), but i'm not sure about including all three
environments - the second and third are really only here for a very few tests in
cmd/juju AFAICS and seem very arbitrary to include in this package which is used
by all tests.

[jameinel, 2013-03-17 11:49:23]

https://codereview.appspot.com/7809043/diff/13001/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/bootstrap.go#newco...
environs/bootstrap.go:22: if !hasCACert {
When I asked that question in the past (we were discussing whether one of them
should return an error or a bool), it came down to a belief that these APIs
might have been a network call (in which case you could have more ways of
failing than just the local "I didn't find it on my in-memory structure")

I think we can leave it returning a bool for now, but flag the whole type as
something we might want to revisit.

https://codereview.appspot.com/7809043/diff/13001/environs/cert.go
File environs/cert.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/cert.go#newcode56
environs/cert.go:56: func EnsureCertificate(environ Environ) (CreatedCert,
error) {
On 2013/03/15 09:31:24, rog wrote:
> i'm sorry, i don't think this is quite right.
> 
> i believe that it's important that we can perform all the usual juju
operations
> without needing to write to disk. that's why the original Bootstrap function
> took a function as an argument, so if (just for instance) we had some daemon
> that was responsible for bootstrapping many juju environments, it could as
> easily write the certs to an arbitrary persistent data store as to $HOME.

That makes it sound like you want a CertStore sort of interface, so that you can
plug in different ones (which honestly sounds like a great thing for testing,
etc). Then the test suite could just have a canned "InMemoryCertStore" and not
have to worry about $HOME either.

That said, it could certainly be outside the scope of Tim's patch.

https://codereview.appspot.com/7809043/diff/13001/environs/cert_blackbox_test.go
File environs/cert_blackbox_test.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/cert_blackbox_test...
environs/cert_blackbox_test.go:10: type EnvironsCertBlackBoxSuite struct {
I agree we should be consistent, but that sounds like a 'juju-dev@' discussion,
rather than just on a review. Other projects I've worked with certainly
defaulted to unittests/whitebox testing, so blackbox was the special snowflake.
:)

https://codereview.appspot.com/7809043/diff/13001/environs/cert_test.go
File environs/cert_test.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/cert_test.go#newco...
environs/cert_test.go:12: }
On 2013/03/15 09:31:24, rog wrote:
> i'd embed testing.LoggingSuite here.

Thanks for the pointer. I wonder if this should be in the equivalent of HACKING.

[rog, 2013-03-17 18:49:48]

https://codereview.appspot.com/7809043/diff/13001/environs/cert.go
File environs/cert.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/cert.go#newcode56
environs/cert.go:56: func EnsureCertificate(environ Environ) (CreatedCert,
error) {
On 2013/03/17 11:49:23, jameinel wrote:
> On 2013/03/15 09:31:24, rog wrote:
> > i'm sorry, i don't think this is quite right.
> > 
> > i believe that it's important that we can perform all the usual juju
> operations
> > without needing to write to disk. that's why the original Bootstrap function
> > took a function as an argument, so if (just for instance) we had some daemon
> > that was responsible for bootstrapping many juju environments, it could as
> > easily write the certs to an arbitrary persistent data store as to $HOME.
> 
> That makes it sound like you want a CertStore sort of interface, so that you
can
> plug in different ones (which honestly sounds like a great thing for testing,
> etc). Then the test suite could just have a canned "InMemoryCertStore" and not
> have to worry about $HOME either.
> 
> That said, it could certainly be outside the scope of Tim's patch.

We had this functionality *before* this patch (via the function pointer arg to
Bootstrap). I would prefer to avoid regression - there was a fair amount of
discussion that went into the design of the original interface, and although I
don't mind it changing (the allocation of responsibilities is nicer now IMO) I
would prefer if the original design goals were still met.

[thumper, 2013-03-18 01:34:28]

message: On 2013/03/15 09:31:23, rog wrote:
> https://codereview.appspot.com/7809043/diff/13001/environs/cert.go
> File environs/cert.go (right):
> 
> https://codereview.appspot.com/7809043/diff/13001/environs/cert.go#newcode56
> environs/cert.go:56: func EnsureCertificate(environ Environ) (CreatedCert,
> error) {
> i'm sorry, i don't think this is quite right.
> 
> i believe that it's important that we can perform all the usual juju
operations
> without needing to write to disk. that's why the original Bootstrap function
> took a function as an argument, so if (just for instance) we had some daemon
> that was responsible for bootstrapping many juju environments, it could as
> easily write the certs to an arbitrary persistent data store as to $HOME.

Sorry, but I call YAGNI on this.  What's more, if we do find that we need it,
add the
CertStore interface that John mentioned below.  But don't design for what you
think
you might need, design for what you do need, then stop.

"Prediction is very difficult, especially if it's about the future."
  - Niels Bohr 

> AFAIK if this gets committed, it's the only place in juju where we require a
> home directory, or writable disk storage at all.

Sorry, but this is bollocks.

I've not added any functionality at all, just refactored it.  The code to write
to the home directory was in environs/bootstrap.go before, and all I've done
is decoupled the writing of the certs from the bootstrap command itself.

>
https://codereview.appspot.com/7809043/diff/13001/environs/cert_blackbox_test...
> environs/cert_blackbox_test.go:10: type EnvironsCertBlackBoxSuite struct {
> rather than calling this "BlackBox" as if that was something unusual (when
it's
> actually the way that almost all of our tests are written), how about naming
the
> other file cert_internal_test.go, and the suite inside
EnvironsCertInternalSuite
> (or just EnvironsCertSuite again - there's no clash) ?
> 
> that's the route i've been taking with my recent changes in the state package
> and it seems to fit ok.
> 
> we should at least agree to be consistent one way or the other :-)

I've taken this to the juju-dev list.

> one other remark: i think it's worth at least including testing.LoggingSuite
in
> any suite, so that log messages get included in the testing output rather than
> being discarded.
> 
> https://codereview.appspot.com/7809043/diff/13001/environs/cert_test.go
> File environs/cert_test.go (right):
> 
>
https://codereview.appspot.com/7809043/diff/13001/environs/cert_test.go#newco...
> environs/cert_test.go:12: }
> i'd embed testing.LoggingSuite here.

Done.

> https://codereview.appspot.com/7809043/diff/13001/testing/environ.go
> File testing/environ.go (right):
> 
> https://codereview.appspot.com/7809043/diff/13001/testing/environ.go#newcode43
> testing/environ.go:43: const PeckhamConfig = `
> i think that having one default environment here may be ok (although perhaps
> better living in juju/testing), but i'm not sure about including all three
> environments - the second and third are really only here for a very few tests
in
> cmd/juju AFAICS and seem very arbitrary to include in this package which is
used
> by all tests.

I've moved it into one central place because having multiple copies of it around
is worse than having it in one place.

If the tests are using a poor environment config, then we should fix the tests,
but not here, and not now.

[rog, 2013-03-18 09:23:35]

On 2013/03/18 01:34:28, thumper wrote:
> message: On 2013/03/15 09:31:23, rog wrote:
> > https://codereview.appspot.com/7809043/diff/13001/environs/cert.go
> > File environs/cert.go (right):
> > 
> > https://codereview.appspot.com/7809043/diff/13001/environs/cert.go#newcode56
> > environs/cert.go:56: func EnsureCertificate(environ Environ) (CreatedCert,
> > error) {
> > i'm sorry, i don't think this is quite right.
> > 
> > i believe that it's important that we can perform all the usual juju
> operations
> > without needing to write to disk. that's why the original Bootstrap function
> > took a function as an argument, so if (just for instance) we had some daemon
> > that was responsible for bootstrapping many juju environments, it could as
> > easily write the certs to an arbitrary persistent data store as to $HOME.
> 
> Sorry, but I call YAGNI on this.  What's more, if we do find that we need it,
> add the
> CertStore interface that John mentioned below.  But don't design for what you
> think
> you might need, design for what you do need, then stop.
> 
> "Prediction is very difficult, especially if it's about the future."
>   - Niels Bohr 

I'm not trying to *predict* the future - I'm hoping that we can keep things
regular and minimise dependencies so that the system as a whole is
more flexible and easier to change regardless of what the future
might hold.

> > AFAIK if this gets committed, it's the only place in juju where we require a
> > home directory, or writable disk storage at all.
> 
> Sorry, but this is bollocks.
> 
> I've not added any functionality at all, just refactored it.  The code to
write
> to the home directory was in environs/bootstrap.go before, and all I've done
> is decoupled the writing of the certs from the bootstrap command itself.

The code to write to the home directory *was* in environs/bootstrap.go before,
but it was only invoked by default, when the writeCertAndKey argument to
Bootstrap was nil. All I'm asking is that we keep that argument, even
though it will now be an argument to EnsureCertificate not Bootstrap.

> > https://codereview.appspot.com/7809043/diff/13001/testing/environ.go
> > File testing/environ.go (right):
> > 
> >
https://codereview.appspot.com/7809043/diff/13001/testing/environ.go#newcode43
> > testing/environ.go:43: const PeckhamConfig = `
> > i think that having one default environment here may be ok (although perhaps
> > better living in juju/testing), but i'm not sure about including all three
> > environments - the second and third are really only here for a very few
tests
> in
> > cmd/juju AFAICS and seem very arbitrary to include in this package which is
> used
> > by all tests.
> 
> I've moved it into one central place because having multiple copies of it
around
> is worse than having it in one place.

the "peckham" environment *was* only defined in one place previously.
there's no reason, AFAICS that the new tests in environs need to use
that specific environment (the "walthamstow" and "brokenenv" sections are there
just for specific tests in cmd/juju).

i *do* think it's a good idea to have a stock environ config around (i count
around
34 different environment configurations scattered around the tests and they're
a bloody pain when changing config-related matters)

but we already have two environ configs in testing/environs, both
of which with deliberately arbitrary names that look as if they've been factored
out from some random test, and indeed each is directly used in only a single
test.

i'd love it if this stuff could be rationalised rather than factored
out in what seems like an ad hoc way - the arbitrary names, "peckham",
"erewemos-2" etc are fine when they really are arbitrary as part of a single
test, but we're making them part of the testing contract and i'm not
sure that's good.

if something really *is* used in multiple places, then let's please
factor it out. but pulling stuff that's there specifically for
a single test away from the test that it's for seems like it's
making things harder to understand and more fragile.

> If the tests are using a poor environment config, then we should fix the
tests,
> but not here, and not now.

the environment config used by the tests is fine - it was designed
*for* those tests. but factoring that particular config out and then
using it as if it was designed to be used in other contexts is not great IMHO.

[niemeyer, 2013-03-18 13:22:41]

On Sun, Mar 17, 2013 at 10:37 PM, Tim Penhey <tim.penhey@canonical.com> wrote:
> But don't design for what you think you might need, design
> for what you do need, then stop.

That's only a good advice if there's relevant cost in designing something
obviously better, otherwise this is simply an excuse to do a poor job.

You're implementing both the software and the library that other projects can
use to interact with that system, and it doesn't make sense to depend on a $HOME
directory when you're using it as library. I can provide a number of painful
examples of softwares that ignore that, starting with ssh. Roger is simply
asking you to preserve that trivial logic, and apparently in a pretty reasonable
and polite way.

> "Prediction is very difficult, especially if it's about the future."
>    - Niels Bohr

I suggest sticking to reasoning about what is being achieved
without invoking bullocks, YAGNIs, and Niels Bohr. It makes for
a more pleasant, fruitful, and less heated conversation.


gustavo @ http://niemeyer.net

[fwereade, 2013-03-18 18:18:37]

Reasonable points all round; all contributions appreciated.

Tim, I think you can maintain the structure of your solution alongside the
functionality originally present, without perpetrating serious ugliness. Would
that work for you?

https://codereview.appspot.com/7809043/diff/13001/environs/cert.go
File environs/cert.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/cert.go#newcode56
environs/cert.go:56: func EnsureCertificate(environ Environ) (CreatedCert,
error) {
On 2013/03/17 18:49:48, rog wrote:
> We had this functionality *before* this patch (via the function pointer arg to
> Bootstrap). I would prefer to avoid regression - there was a fair amount of
> discussion that went into the design of the original interface, and although I
> don't mind it changing (the allocation of responsibilities is nicer now IMO) I
> would prefer if the original design goals were still met.

I think this is the clincher for me; it's a feature we had that made a
dependency configurable, and I don't think the change requires that the
functionality be dropped. Tim, would you just make the WriteCertAndKeyToHome
func public, and maybe make it an explicit parameter to EnsureCertificate? A nil
will then panic just as well as those panicWrites did, when we want that; and we
get to maintain everything about your change apart from one parameter and one
newly exported func.

https://codereview.appspot.com/7809043/diff/13001/environs/cert_blackbox_test.go
File environs/cert_blackbox_test.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/cert_blackbox_test...
environs/cert_blackbox_test.go:10: type EnvironsCertBlackBoxSuite struct {
On 2013/03/17 11:49:23, jameinel wrote:
> I agree we should be consistent, but that sounds like a 'juju-dev@'
discussion,
> rather than just on a review. Other projects I've worked with certainly
> defaulted to unittests/whitebox testing, so blackbox was the special
snowflake.
> :)

And I vaguely feel that the ideal is to expose units small enough to be amenable
to clean blackbox testing. Regardless, I think the fact it's in a different
package is enough to render the BlackBox redundant.

https://codereview.appspot.com/7809043/diff/13001/environs/cert_test.go
File environs/cert_test.go (right):

https://codereview.appspot.com/7809043/diff/13001/environs/cert_test.go#newco...
environs/cert_test.go:12: }
On 2013/03/17 11:49:23, jameinel wrote:
> On 2013/03/15 09:31:24, rog wrote:
> > i'd embed testing.LoggingSuite here.
> 
> Thanks for the pointer. I wonder if this should be in the equivalent of
HACKING.

+1

[thumper, 2013-03-18 20:59:06]

On 19/03/13 02:22, n13m3y3r@gmail.com wrote:
> On Sun, Mar 17, 2013 at 10:37 PM, Tim Penhey <tim.penhey@canonical.com>
> wrote:
>> But don't design for what you think you might need, design
>> for what you do need, then stop.
> 
> That's only a good advice if there's relevant cost in designing
> something obviously better, otherwise this is simply an excuse to do a
> poor job.

There is a big difference between Simple and Simplistic, and this isn't
the time to go into it.  A simple solution isn't necessarily a poor job,
just like an overly complicated solution isn't necessarily a good job.

> You're implementing both the software and the library that other
> projects can use to interact with that system, and it doesn't make sense
> to depend on a $HOME directory when you're using it as library. I can
> provide a number of painful examples of softwares that ignore that,
> starting with ssh. Roger is simply asking you to preserve that trivial
> logic, and apparently in a pretty reasonable and polite way.

I'm willing to put the writing function back in, but I still think it it
pointless.

You are making assumptions about how the code might be used in the
future.  Sure the existing code had this complexity, but there were only
two uses: write to the home directory; panic if it tried.  The logic has
been changed so the writing of the certs is done independently.  That
means that EnsureCertificates is called with one, and only one meaning,
which is write certs to the home dir if they don't exist.

By adding the function pointer to write the files you are adding
complexity for no useful reason.

My main suggestion is don't add the complexity until you have shown the
need for it.

I find no benefit in continuing this argument.  I'll add the function
pointer back in.

>> "Prediction is very difficult, especially if it's about the future."
>>     - Niels Bohr
> 
> I suggest sticking to reasoning about what is being achieved
> without invoking bullocks, YAGNIs, and Niels Bohr. It makes for
> a more pleasant, fruitful, and less heated conversation.

Fair call.

[thumper, 2013-03-18 23:16:23]

*** Submitted:

Remove env cert creation from environs.Bootstrap

The generation of the environment certificates is moved to the Bootstrap
command.  Tests updated.

R=
CC=
https://codereview.appspot.com/7809043

[thumper, 2013-03-18 23:19:38]

On 19/03/13 12:16, tim.penhey@canonical.com wrote:
> *** Submitted:
> 
> Remove env cert creation from environs.Bootstrap
> 
> The generation of the environment certificates is moved to the Bootstrap
> command.  Tests updated.
> 
> R=
> CC=
> https://codereview.appspot.com/7809043
> 
> 
> https://codereview.appspot.com/7809043/

OK, obviously something was a little suspect here.

By the R= nothing.

Why did it choose noone?  Was it because it was proposed again after the
LGTM comments?

Tim

[dave_cheney.net, 2013-03-18 23:51:59]

Dunno, i can't see a pattern from other reviews.