Merge lp://staging/~wallyworld/juju-core/ec2-live-tests-fix into lp://staging/~juju/juju-core/trunk

Reviewers: mp+160004_code.launchpad.net,

Message:
Please take a look.

Description:
Fix EC2 live tests - use correct series

When setting up the environment for running live tests on EC2, no
default
series was specified. This meant that "precise" was used as a fallback
to start
the bootstrap node. But later on, test asserts were being done with
version.CurrentSeries(),
which in my case was "raring" since I'm running on the latest Ubuntu. So
the tests failed.

I've tweaked the live test setup for EC2 to explicitly specify
default-series as
version.Current(). I think this makes sense, since I'd prefer we use the
series from the
current running Ubuntu when testing. If we want to test with "precise" I
think that's best
done as part of setting up the landing bot.

https://code.launchpad.net/~wallyworld/juju-core/ec2-live-tests-fix/+merge/160004

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8904043/

Affected files:
   A [revision details]
   M environs/ec2/live_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:
<email address hidden>
+New revision: <email address hidden>

Index: environs/ec2/live_test.go
=== modified file 'environs/ec2/live_test.go'
--- environs/ec2/live_test.go 2013-04-10 00:11:24 +0000
+++ environs/ec2/live_test.go 2013-04-22 04:38:01 +0000
@@ -16,6 +16,7 @@
   "launchpad.net/juju-core/juju/testing"
   "launchpad.net/juju-core/state"
   coretesting "launchpad.net/juju-core/testing"
+ "launchpad.net/juju-core/version"
   "strings"
  )

@@ -46,6 +47,7 @@
    "control-bucket": "juju-test-" + uniqueName,
    "public-bucket": "juju-public-test-" + uniqueName,
    "admin-secret": "for real",
+ "default-series": version.CurrentSeries(),
    "ca-cert": coretesting.CACert,
    "ca-private-key": coretesting.CAKey,
   }

NOT LGTM

I've probably not been clear in communicating this, but the default
series, across the board, is *precise*. The charm ecosystem is such that
there is very little reason to deploy to any other series: the
assertions are wrong, not the setup.

https://codereview.appspot.com/8904043/

We'd better hope that Go binaries compiled on other series really
do run on precise then, because that's the way that Quantal
and Raring users will *always* be running the live tests if we leave
default-series as precise.

If we're going to test like that, why honestly are we bothering
with different binaries for different series?

Personally I think it's probably ok that running live tests
on series X tests instances running series X, but I realise
there's no right answer here.

On 22 April 2013 09:38, William Reade <email address hidden> wrote:
> NOT LGTM
>
> I've probably not been clear in communicating this, but the default
> series, across the board, is *precise*. The charm ecosystem is such that
> there is very little reason to deploy to any other series: the
> assertions are wrong, not the setup.
>
> https://codereview.appspot.com/8904043/
>
> --
> https://code.launchpad.net/~wallyworld/juju-core/ec2-live-tests-fix/+merge/160004
> Your team juju hackers is requested to review the proposed merge of lp:~wallyworld/juju-core/ec2-live-tests-fix into lp:juju-core.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2013-04-22 14:34, Roger Peppe wrote:
> We'd better hope that Go binaries compiled on other series really
> do run on precise then, because that's the way that Quantal and
> Raring users will *always* be running the live tests if we leave
> default-series as precise.
>
> If we're going to test like that, why honestly are we bothering
> with different binaries for different series?
>
> Personally I think it's probably ok that running live tests on
> series X tests instances running series X, but I realise there's no
> right answer here.

Right, for *live* tests, you need to match the client series, because
that is the one that was just compiled and uploaded.

Either that or we actually just say "one set of binaries" which could
simplify the tool infrastructure a lot.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlF1JGsACgkQJdeBCYSNAAPtDACfdYqIxi+M3YYt4kczMPPJA7/d
X7AAoMm1u8jnZz+0WR+GVKKdynjx8BP6
=zrR3
-----END PGP SIGNATURE-----

On 22 April 2013 12:52, John Arbash Meinel <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 2013-04-22 14:34, Roger Peppe wrote:
>> We'd better hope that Go binaries compiled on other series really
>> do run on precise then, because that's the way that Quantal and
>> Raring users will *always* be running the live tests if we leave
>> default-series as precise.
>>
>> If we're going to test like that, why honestly are we bothering
>> with different binaries for different series?
>>
>> Personally I think it's probably ok that running live tests on
>> series X tests instances running series X, but I realise there's no
>> right answer here.
>
> Right, for *live* tests, you need to match the client series, because
> that is the one that was just compiled and uploaded.

I believe that's what Ian's change makes it do; I'm not
that William agrees about the need, but I may be
misinterpreting.

> Either that or we actually just say "one set of binaries" which could
> simplify the tool infrastructure a lot.

Indeed.

Please take a look.

https://codereview.appspot.com/8904043/

Something seems a bit fishy about the tools.Upload line, but otherwise
LGTM

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

https://codereview.appspot.com/8904043/diff/4001/environs/jujutest/livetests.go#newcode557
environs/jujutest/livetests.go:557: upgradeTools, err :=
tools.Upload(t.Env.Storage(), &newVersion.Number, newVersion.Series)
How was this not just a compile error before? You're passing a new
parameter, but there are no other files touched.

https://codereview.appspot.com/8904043/

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

https://codereview.appspot.com/8904043/diff/4001/environs/jujutest/livetests.go#newcode557
environs/jujutest/livetests.go:557: upgradeTools, err :=
tools.Upload(t.Env.Storage(), &newVersion.Number, newVersion.Series)
On 2013/05/16 05:57:05, jameinel wrote:
> How was this not just a compile error before? You're passing a new
parameter,
> but there are no other files touched.

The signature of tools.Upload() is
func Upload(storage URLPutter, forceVersion *version.Number, fakeSeries
...string)

Hence fakeSeries can be nothing or something.

https://codereview.appspot.com/8904043/

LGTM, thanks.

https://codereview.appspot.com/8904043/

*** Submitted:

Fix live tests

When setting up the environment for running live tests (on EC2), no
default
series was specified. This meant that "precise" was used as a fallback
to start
the bootstrap node. But later on, test asserts were being done with
version.CurrentSeries(),
which in my case was "raring" since I'm running on the latest Ubuntu. So
the tests failed.

Since we want series to be precise if not otherwise specified, I've
changed the asserts in
some tests to check for version with series = config.DefaultSeries (ie
"precise"). Another
change is than in the checkUpgrade() tests, the tools.Upload() was
returning tools for
the series on which the tests are running. But we are only interested in
the version
number. Rather than do a heavy weight solution to this like
FORCE-VERSION, I just tweaked
the test code to set the series of the Upload() return value to match
the expected series.
I think this is ok, but would like a 2nd opinon as I am not 100% sure of
the exact
rational behind the FORCE-VERSION code.

I've also changed the TestBootstrapWithDefaultSeries test to account for
the fact that
"precise" is the default series if not otherwise specified. I tweaked it
so that whether
runnung on precise or otherwise, the default-series env parameter is
always set to
something other than the series on which the tests are run.

R=fwereade, jameinel
CC=
https://codereview.appspot.com/8904043

https://codereview.appspot.com/8904043/