Fail when run against Python juju environment

Fail when run against Python juju environment

Introduce a CheckEnvironment call that checks if bootstrap-verify contains content as produced by juju-core Bootstrap (actually, VerifyStorage) call, or that there is no 'bootstrap-verify' file (which holds for earlier versions of juju-core).

If the file has bad content, then it assumes it's not a juju-core environment and outputs an error message.

It has been tested with ec2, and livetests are still being fine-tuned to work properly:

  $ juju status
  error: Environment is not a juju-core environment

Perhaps error message should suggest ensuring that it's not a Python environment? Any wording suggestions.

https://code.launchpad.net/~danilo/juju-core/python-env-fails/+merge/171997

(do not edit description out of merge proposal)

[Danilo, 2013-06-28 11:52:44]

Please take a look.

[Danilo, 2013-06-28 12:04:33]

I am having trouble with a live test: http://paste.ubuntu.com/5807576/
Only the BadVerificationFile one fails (consistently) on

  	c.Assert(err, Equals, environs.InvalidEnvironmentError)

line.  I suppose I can't compare errors like this, so I'll be fixing this as
well.

[gz, 2013-06-28 14:46:52]

On 2013/06/28 12:04:33, Danilo wrote:
> I am having trouble with a live test: http://paste.ubuntu.com/5807576/
> Only the BadVerificationFile one fails (consistently) on
> 
>   	c.Assert(err, Equals, environs.InvalidEnvironmentError)
> 
> line.  I suppose I can't compare errors like this, so I'll be fixing this as
> well.

The issue is the line below. You're calling conn.Close(), after asserting that
actually an error was returned. NewConn returns nil for conn in the error case,
hence the panic. Instead remove that line, or better make it condtional, before
the assert, on NewConn *not* returning an err.

[Danilo, 2013-06-28 14:53:37]

Figured this out and now doing a final run of livetest: one can't defer a
conn.Close() when connection opening has failed ;)

[fwereade, 2013-06-28 14:56:10]

looking good, but the tests need isolation

https://codereview.appspot.com/10733044/diff/1/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/10733044/diff/1/environs/jujutest/livetests.go...
environs/jujutest/livetests.go:489: // Absence of a verification file means it
is.
s/./ a juju-core environment with an older version, which is fine./

or something

https://codereview.appspot.com/10733044/diff/1/environs/open_test.go
File environs/open_test.go (right):

https://codereview.appspot.com/10733044/diff/1/environs/open_test.go#newcode108
environs/open_test.go:108: // it will fail because storage will already be
poisoned!!!
do a dummy.Reset() in SetUpTest; maybe in TearDownTest too for maximum
protection.

[Danilo, 2013-06-28 15:35:05]

Please take a look.

https://codereview.appspot.com/10733044/diff/1/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/10733044/diff/1/environs/jujutest/livetests.go...
environs/jujutest/livetests.go:489: // Absence of a verification file means it
is.
Improved as suggested.

https://codereview.appspot.com/10733044/diff/1/environs/open_test.go
File environs/open_test.go (right):

https://codereview.appspot.com/10733044/diff/1/environs/open_test.go#newcode108
environs/open_test.go:108: // it will fail because storage will already be
poisoned!!!
Only done it in a TearDownTest: I am hesistant to do it unnecessarily in the
set-up. I prefer tests that know what they are doing (even though I might not
know what I am doing).  I've also added the same to the verifyStorageSuite so
that two test suites that could step on each other toes are at the very least
safe.

[fwereade, 2013-06-28 15:51:19]

just a couple more tweaks I just spotted:

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:476: func
restoreBootstrapVerificationFile(storage environs.Storage) error {
I'd accept a C and Assert(err, IsNil) in here rather than return an error, I
think.

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:507: defer
restoreBootstrapVerificationFile(storage)
maybe better to defer the restore one line later?

https://codereview.appspot.com/10733044/diff/8002/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/open.go#newcode85
environs/open.go:85: return InvalidEnvironmentError
I'd prefer to distinguish between "couldn't read file" and "bad content", if
possible. eg

if reader, err := storage.Get(verificationFilename); errors.IsNotFoundError(err)
{
    return nil
} else if err != nil {
    return err
} else if content, err := ioutil.ReadAll(reader); err != nil {
    return err
} else if content != verificationContent {
    return InvalidEnvironmentError
}
return nil

...although some would prefer that the ladder be split up a bit and I'd be fine
with that too.

https://codereview.appspot.com/10733044/diff/8002/environs/open_test.go
File environs/open_test.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/open_test.go#newco...
environs/open_test.go:87: // VerifyStorage is sufficient for our tests and much
simpler
I really appreciate blank lines before comments for readability's sake.

https://codereview.appspot.com/10733044/diff/8002/environs/open_test.go#newco...
environs/open_test.go:104: err = environs.VerifyStorage(storage)
c.Assert(err, IsNil)

https://codereview.appspot.com/10733044/diff/8002/environs/open_test.go#newco...
environs/open_test.go:122: err = environs.VerifyStorage(storage)
c.Assert(err, IsNil)

[dimitern, 2013-06-28 16:34:08]

Looks good in general, but I have a few minor suggestions.

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:476: func
restoreBootstrapVerificationFile(storage environs.Storage) error {
On 2013/06/28 15:51:19, fwereade wrote:
> I'd accept a C and Assert(err, IsNil) in here rather than return an error, I
> think.

+1

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:494: defer conn.Close()
conn.Close()

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:507: defer
restoreBootstrapVerificationFile(storage)
On 2013/06/28 15:51:19, fwereade wrote:
> maybe better to defer the restore one line later?

yeah, it might have failed, hence after the assert is better.

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:512: defer conn.Close()
just conn.Close() is better i think - it's the last line anyway.

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:528: err := storage.Put("bootstrap-verify",
reader,
if you're splitting lines, I personally prefer this:
err := storage.Put(
    "bootstrap-verify",
    reader,
    int64(len(....),
)

https://codereview.appspot.com/10733044/diff/8002/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/open.go#newcode62
environs/open.go:62: // Checks if an environment has a bootstrap-verify that is
written by
// CheckEnvironment checks ...

https://codereview.appspot.com/10733044/diff/8002/environs/open_test.go
File environs/open_test.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/open_test.go#newco...
environs/open_test.go:87: // VerifyStorage is sufficient for our tests and much
simpler
On 2013/06/28 15:51:19, fwereade wrote:
> I really appreciate blank lines before comments for readability's sake.

+10

https://codereview.appspot.com/10733044/diff/8002/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/storage.go#newcode31
environs/storage.go:31: "Environment is not a juju-core environment")
i'd prefer "environment ...", also perhaps put these 2 in a var ( .. ) block

[dimitern, 2013-06-28 16:34:08]

Looks good in general, but I have a few minor suggestions.

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:476: func
restoreBootstrapVerificationFile(storage environs.Storage) error {
On 2013/06/28 15:51:19, fwereade wrote:
> I'd accept a C and Assert(err, IsNil) in here rather than return an error, I
> think.

+1

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:494: defer conn.Close()
conn.Close()

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:507: defer
restoreBootstrapVerificationFile(storage)
On 2013/06/28 15:51:19, fwereade wrote:
> maybe better to defer the restore one line later?

yeah, it might have failed, hence after the assert is better.

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:512: defer conn.Close()
just conn.Close() is better i think - it's the last line anyway.

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:528: err := storage.Put("bootstrap-verify",
reader,
if you're splitting lines, I personally prefer this:
err := storage.Put(
    "bootstrap-verify",
    reader,
    int64(len(....),
)

https://codereview.appspot.com/10733044/diff/8002/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/open.go#newcode62
environs/open.go:62: // Checks if an environment has a bootstrap-verify that is
written by
// CheckEnvironment checks ...

https://codereview.appspot.com/10733044/diff/8002/environs/open_test.go
File environs/open_test.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/open_test.go#newco...
environs/open_test.go:87: // VerifyStorage is sufficient for our tests and much
simpler
On 2013/06/28 15:51:19, fwereade wrote:
> I really appreciate blank lines before comments for readability's sake.

+10

https://codereview.appspot.com/10733044/diff/8002/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/storage.go#newcode31
environs/storage.go:31: "Environment is not a juju-core environment")
i'd prefer "environment ...", also perhaps put these 2 in a var ( .. ) block

[Danilo, 2013-06-29 03:03:11]

Please take a look.

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:476: func
restoreBootstrapVerificationFile(storage environs.Storage) error {
On 2013/06/28 15:51:19, fwereade wrote:
> I'd accept a C and Assert(err, IsNil) in here rather than return an error, I
> think.

Ok, done.

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:494: defer conn.Close()
On 2013/06/28 16:34:08, dimitern wrote:
> conn.Close()

Fixed.

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:507: defer
restoreBootstrapVerificationFile(storage)
On 2013/06/28 15:51:19, fwereade wrote:
> maybe better to defer the restore one line later?

Sure, just an omission on my part. Fixed.

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:512: defer conn.Close()
On 2013/06/28 16:34:08, dimitern wrote:
> just conn.Close() is better i think - it's the last line anyway.

Yeah, fixed.

https://codereview.appspot.com/10733044/diff/8002/environs/jujutest/livetests...
environs/jujutest/livetests.go:528: err := storage.Put("bootstrap-verify",
reader,
On 2013/06/28 16:34:08, dimitern wrote:
> if you're splitting lines, I personally prefer this:
> err := storage.Put(
>     "bootstrap-verify",
>     reader,
>     int64(len(....),
> )

Ok, done (though I wouldn't prefer it like this: this is something that would
benefit from a set coding style guide, since people _will_ have different
opinions).

https://codereview.appspot.com/10733044/diff/8002/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/open.go#newcode62
environs/open.go:62: // Checks if an environment has a bootstrap-verify that is
written by
On 2013/06/28 16:34:08, dimitern wrote:
> // CheckEnvironment checks ...

Fixed.

https://codereview.appspot.com/10733044/diff/8002/environs/open.go#newcode85
environs/open.go:85: return InvalidEnvironmentError
On 2013/06/28 15:51:19, fwereade wrote:
> I'd prefer to distinguish between "couldn't read file" and "bad content", if
> possible. eg
> 
> if reader, err := storage.Get(verificationFilename);
errors.IsNotFoundError(err)
> {
>     return nil
> } else if err != nil {
>     return err
> } else if content, err := ioutil.ReadAll(reader); err != nil {
>     return err
> } else if content != verificationContent {
>     return InvalidEnvironmentError
> }
> return nil
> 
> ...although some would prefer that the ladder be split up a bit and I'd be
fine
> with that too.

Done it mostly like this (though I very much dislike "if x := expression; x"
syntax for readability purposes).

Also, the reasoning I was not propagating the error from internal subcalls is
your comment on https://codereview.appspot.com/9876043/#msg13 — I understood it
to mean a better pattern is to simply log any internal errors we hit (which,
granted, I did not do here), and return a single error message from the
function. This is something that'd hugely benefit from a coding style guide
recommendation, to avoid any unnecessary back-and-forth.

https://codereview.appspot.com/10733044/diff/8002/environs/open_test.go
File environs/open_test.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/open_test.go#newco...
environs/open_test.go:87: // VerifyStorage is sufficient for our tests and much
simpler
On 2013/06/28 15:51:19, fwereade wrote:
> I really appreciate blank lines before comments for readability's sake.

I am a big fan of readability, but, with syntax highlighting, I do not see this
as a problem. Anyway, still done in all the places I could catch.

https://codereview.appspot.com/10733044/diff/8002/environs/open_test.go#newco...
environs/open_test.go:104: err = environs.VerifyStorage(storage)
On 2013/06/28 15:51:19, fwereade wrote:
> c.Assert(err, IsNil)

Fixed.

https://codereview.appspot.com/10733044/diff/8002/environs/open_test.go#newco...
environs/open_test.go:122: err = environs.VerifyStorage(storage)
On 2013/06/28 15:51:19, fwereade wrote:
> c.Assert(err, IsNil)

Fixed.

https://codereview.appspot.com/10733044/diff/8002/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/10733044/diff/8002/environs/storage.go#newcode31
environs/storage.go:31: "Environment is not a juju-core environment")
On 2013/06/28 16:34:08, dimitern wrote:
> i'd prefer "environment ...", also perhaps put these 2 in a var ( .. ) block

Fixed (and moved to open.go where it belongs).

[fwereade, 2013-07-02 13:01:40]

LGTM

[gz, 2013-07-02 14:11:27]

LGTM,passes tests merged into trunk.