Code review - Issue 6868070: environs/ec2: use default-series on Bootstraphttps://codereview.appspot.com/2012-12-12T16:52:07+00:00rietveld
Message from unknown
2012-12-05T13:55:09+00:00niemeyerurn:md5:610856ea9e566775f29cea01be640655
Message from n13m3y3r@gmail.com
2012-12-05T13:55:14+00:00niemeyerurn:md5:84da1296582d8da042e022b35be041c7
Please take a look.
Message from rogpeppe@gmail.com
2012-12-05T14:56:23+00:00rogurn:md5:641aa2b8a870068a7a8c7b45fba0dc22
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#newcode694
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#newcode698
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#newcode707
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.
Message from n13m3y3r@gmail.com
2012-12-05T15:48:22+00:00niemeyerurn:md5:5ecbce585d50b5b9528428b7d129bf56
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#newcode694
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#newcode698
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#newcode707
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?
Message from rogpeppe@gmail.com
2012-12-05T16:03:22+00:00rogurn:md5:9357d52c580ee5adeef3bc8e08d3e0ba
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#newcode694
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#newcode698
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#newcode707
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.
Message from unknown
2012-12-12T14:08:13+00:00niemeyerurn:md5:7d9e960dbdb899a9c857bfbdee50902a
Message from n13m3y3r@gmail.com
2012-12-12T14:08:16+00:00niemeyerurn:md5:4ee9abd8316d92e1e00107e27c2baa89
Please take a look.
Message from rogpeppe@gmail.com
2012-12-12T14:55:28+00:00rogurn:md5:2ea31ca853302e0bd0c09e6377b202bf
On 2012/12/12 14:08:16, niemeyer wrote:
> Please take a look.
LGTM
Message from rogpeppe@gmail.com
2012-12-12T14:55:36+00:00rogurn:md5:d6679830e9aaf7a695b5cc93bf07a14b
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.go#newcode710
environs/jujutest/livetests.go:710: _, err = environs.PutTools(dummyStorage, &current.Number)
nice
https://codereview.appspot.com/6868070/diff/5001/environs/jujutest/livetests.go#newcode729
environs/jujutest/livetests.go:729: // Note that
Note that ... ?
Message from fwereade@gmail.com
2012-12-12T15:21:41+00:00fwereadeurn:md5:95636a44f9f4c0cb192920d3a1507579
LGTM modulo rog's missing "Note that"
Message from unknown
2012-12-12T16:51:03+00:00niemeyerurn:md5:906f7ce5d059d9d781c7973bf5fd1d58
Message from n13m3y3r@gmail.com
2012-12-12T16:52:07+00:00niemeyerurn:md5:dd4a6161d20dce7a9a50c66bee80773e
*** Submitted:
environs/ec2: use default-series on Bootstrap
R=rog, fwereade
CC=
https://codereview.appspot.com/6868070