environs/ec2: use default-series on Bootstrap

environs/ec2: use default-series on Bootstrap



https://code.launchpad.net/~niemeyer/juju-core/ec2-bootstrap-default-series/+merge/138156

(do not edit description out of merge proposal)

[niemeyer, 2012-12-05 13:55:14]

Please take a look.

[rog, 2012-12-05 14:56:23]

another 5 minutes more to run live tests. wouldn't it be nice if we could run
'em in parallel?

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

https://codereview.appspot.com/6868070/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:694: _, err = environs.PutTools(storage, &other)
let's hope the tools really do work across different series...

https://codereview.appspot.com/6868070/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:698: err = environs.Bootstrap(env, false,
panicWrite)
you need to call t.Destroy earlier, otherwise this test doesn't interact nicely
with the other live tests.

https://codereview.appspot.com/6868070/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:707: // machine and ensure it deployed the proper
series.
does this actually check that? it checks that the tools with the named series
have been deployed, but i think that they'd report the same thing (the tools
version you've forced) regardless of the actual series running on the deployed
machine.

[niemeyer, 2012-12-05 15:48:22]

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

https://codereview.appspot.com/6868070/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:694: _, err = environs.PutTools(storage, &other)
On 2012/12/05 14:56:23, rog wrote:
> let's hope the tools really do work across different series...

Yeah, alternatively we could change version.Current, and have other as the
current series. That's perhaps better.

https://codereview.appspot.com/6868070/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:698: err = environs.Bootstrap(env, false,
panicWrite)
On 2012/12/05 14:56:23, rog wrote:
> you need to call t.Destroy earlier, otherwise this test doesn't interact
nicely
> with the other live tests.

I'd prefer to not kill the running environment for no good reason, while other
tests are still able to use it. Renaming the environment should theoretically
work well, I guess?

https://codereview.appspot.com/6868070/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:707: // machine and ensure it deployed the proper
series.
On 2012/12/05 14:56:23, rog wrote:
> does this actually check that? it checks that the tools with the named series
> have been deployed, but i think that they'd report the same thing (the tools
> version you've forced) regardless of the actual series running on the deployed
> machine.

See the comment below. Does it answer the question? Any suggestion for how to
easily fix it now as well?

[rog, 2012-12-05 16:03:22]

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

https://codereview.appspot.com/6868070/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:694: _, err = environs.PutTools(storage, &other)
On 2012/12/05 15:48:23, niemeyer wrote:
> On 2012/12/05 14:56:23, rog wrote:
> > let's hope the tools really do work across different series...
> 
> Yeah, alternatively we could change version.Current, and have other as the
> current series. That's perhaps better.

i'm not sure that's really checking cross-series deployment, but it would
probably exercise the appropriate code paths.

https://codereview.appspot.com/6868070/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:698: err = environs.Bootstrap(env, false,
panicWrite)
On 2012/12/05 15:48:23, niemeyer wrote:
> On 2012/12/05 14:56:23, rog wrote:
> > you need to call t.Destroy earlier, otherwise this test doesn't interact
> nicely
> > with the other live tests.
> 
> I'd prefer to not kill the running environment for no good reason, while other
> tests are still able to use it. Renaming the environment should theoretically
> work well, I guess?

yes, that could work.

https://codereview.appspot.com/6868070/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:707: // machine and ensure it deployed the proper
series.
On 2012/12/05 15:48:23, niemeyer wrote:
> On 2012/12/05 14:56:23, rog wrote:
> > does this actually check that? it checks that the tools with the named
series
> > have been deployed, but i think that they'd report the same thing (the tools
> > version you've forced) regardless of the actual series running on the
deployed
> > machine.
> 
> See the comment below. Does it answer the question? Any suggestion for how to
> easily fix it now as well?

ah, yes, i'd skimmed that comment too quickly.
i don't think this is a problem for anything apart from tests, because the
version is only forced in tests.

one possibility would be to allow forcing the version number only, and have
version.Current always report the current series.

[niemeyer, 2012-12-12 14:08:16]

Please take a look.

[rog, 2012-12-12 14:55:28]

On 2012/12/12 14:08:16, niemeyer wrote:
> Please take a look.

LGTM

[rog, 2012-12-12 14:55:36]

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

https://codereview.appspot.com/6868070/diff/5001/environs/jujutest/livetests....
environs/jujutest/livetests.go:710: _, err = environs.PutTools(dummyStorage,
&current.Number)
nice

https://codereview.appspot.com/6868070/diff/5001/environs/jujutest/livetests....
environs/jujutest/livetests.go:729: // Note that
Note that ... ?

[fwereade, 2012-12-12 15:21:41]

LGTM modulo rog's missing "Note that"

[niemeyer, 2012-12-12 16:52:07]

*** Submitted:

environs/ec2: use default-series on Bootstrap

R=rog, fwereade
CC=
https://codereview.appspot.com/6868070