Properly implement SupportedArchitectures()

Properly implement SupportedArchitectures()

Query the simplestreams image metadata to extract all the
architectures for the images defined for the environment's
region. These are the architectures we are intersted in.

Also refactor the azure tests to clean them up.

https://code.launchpad.net/~wallyworld/juju-core/provider-supported-architectures/+merge/211858

(do not edit description out of merge proposal)

[wallyworld, 2014-03-20 03:26:26]

Please take a look.

[wallyworld, 2014-03-20 03:28:20]

Please take a look.

[thumper, 2014-03-20 04:27:14]

https://codereview.appspot.com/78030045/diff/20001/environs/imagemetadata/mar...
File environs/imagemetadata/marshal.go (right):

https://codereview.appspot.com/78030045/diff/20001/environs/imagemetadata/mar...
environs/imagemetadata/marshal.go:76: VType:      t.VType,
VType isn't very descriptive or helpful here.

https://codereview.appspot.com/78030045/diff/20001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/78030045/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:364: machineType, err :=
selectMachineType(gwacl.RoleSizes,
defaultToBaselineSpec(constraint.Constraints))
you pass in a pointer and dereference it without checking

why pass in by pointer?

https://codereview.appspot.com/78030045/diff/20001/provider/azure/environ_tes...
File provider/azure/environ_test.go (right):

https://codereview.appspot.com/78030045/diff/20001/provider/azure/environ_tes...
provider/azure/environ_test.go:71: getEndpoint("West US"),
I'm assuming these are patched out for testing and aren't actually trying to hit
the remote endpoints?

https://codereview.appspot.com/78030045/diff/20001/provider/azure/instancetyp...
File provider/azure/instancetype_test.go (right):

https://codereview.appspot.com/78030045/diff/20001/provider/azure/instancetyp...
provider/azure/instancetype_test.go:332: expectation[index].Arches =
[]string{"amd64"}
why not use arch.AMD64?

https://codereview.appspot.com/78030045/diff/20001/provider/azure/instancetyp...
provider/azure/instancetype_test.go:371: Arches: []string{"amd64"},
ditto

https://codereview.appspot.com/78030045/diff/20001/provider/azure/instancetyp...
provider/azure/instancetype_test.go:397: Arches: []string{"amd64"},
and again

https://codereview.appspot.com/78030045/diff/20001/provider/common/supporteda...
File provider/common/supportedarchitectures_test.go (right):

https://codereview.appspot.com/78030045/diff/20001/provider/common/supporteda...
provider/common/supportedarchitectures_test.go:66: env, cloudSpec :=
s.setupMetadata(c, []string{"ppc64"})
You went to the trouble of defining constants for all the arches, we should use
them.

[wallyworld, 2014-03-20 05:05:34]

Please take a look.

https://codereview.appspot.com/78030045/diff/20001/environs/imagemetadata/mar...
File environs/imagemetadata/marshal.go (right):

https://codereview.appspot.com/78030045/diff/20001/environs/imagemetadata/mar...
environs/imagemetadata/marshal.go:76: VType:      t.VType,
On 2014/03/20 04:27:15, thumper wrote:
> VType isn't very descriptive or helpful here.

I renamed to VirtType. Still concise but hopefully clearer.

https://codereview.appspot.com/78030045/diff/20001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/78030045/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:364: machineType, err :=
selectMachineType(gwacl.RoleSizes,
defaultToBaselineSpec(constraint.Constraints))
On 2014/03/20 04:27:15, thumper wrote:
> you pass in a pointer and dereference it without checking
> 
> why pass in by pointer?

The underlying simplestreams fetch methods take pointers.

https://codereview.appspot.com/78030045/diff/20001/provider/azure/environ_tes...
File provider/azure/environ_test.go (right):

https://codereview.appspot.com/78030045/diff/20001/provider/azure/environ_tes...
provider/azure/environ_test.go:71: getEndpoint("West US"),
On 2014/03/20 04:27:15, thumper wrote:
> I'm assuming these are patched out for testing and aren't actually trying to
hit
> the remote endpoints?

getEndpoint is a function that just returns a string. It doesn't do anything
else

https://codereview.appspot.com/78030045/diff/20001/provider/azure/instancetyp...
File provider/azure/instancetype_test.go (right):

https://codereview.appspot.com/78030045/diff/20001/provider/azure/instancetyp...
provider/azure/instancetype_test.go:332: expectation[index].Arches =
[]string{"amd64"}
On 2014/03/20 04:27:15, thumper wrote:
> why not use arch.AMD64?

I prefer to use "amd64" etc in tests since if the constants change, the tests
will break and it forces the author to double check everything is ok across the
board.

https://codereview.appspot.com/78030045/diff/20001/provider/common/supporteda...
File provider/common/supportedarchitectures_test.go (right):

https://codereview.appspot.com/78030045/diff/20001/provider/common/supporteda...
provider/common/supportedarchitectures_test.go:66: env, cloudSpec :=
s.setupMetadata(c, []string{"ppc64"})
On 2014/03/20 04:27:15, thumper wrote:
> You went to the trouble of defining constants for all the arches, we should
use
> them.

See previous comment about tests

[fwereade, 2014-03-20 11:10:07]

LGTM modulo eyebrow-raises, let's chat live quickly

https://codereview.appspot.com/78030045/diff/40001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/78030045/diff/40001/provider/azure/environ.go#...
provider/azure/environ.go:64: archMutex sync.Mutex
it's common to put the guarded field under the mutex

https://codereview.appspot.com/78030045/diff/40001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/78030045/diff/40001/provider/ec2/ec2.go#newcod...
provider/ec2/ec2.go:356: })
This bit feels a bit repetitive, having looked at azure. I'm essentially willing
to trust your judgment, which I believe is that the payoff from the caching in
different env types is sufficient benefit to outweigh the cost of the repetition
(vs using the common interface to get the sources), but it does raise a bit of
an eyebrow -- how many times do we call this per process? (OTOH, maybe we
already grab the simplestreams data too damn many times already..?)

https://codereview.appspot.com/78030045/diff/40001/provider/ec2/ec2.go#newcod...
provider/ec2/ec2.go:358: return e.supportedArchitectures, err
return common.SupportedArchitectures(...)?

https://codereview.appspot.com/78030045/diff/40001/provider/joyent/environ.go
File provider/joyent/environ.go (right):

https://codereview.appspot.com/78030045/diff/40001/provider/joyent/environ.go...
provider/joyent/environ.go:66: //		Endpoint: "",
make sure dstroppa knows about this please :)

[wallyworld, 2014-03-20 11:40:29]

Please take a look.

https://codereview.appspot.com/78030045/diff/40001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/78030045/diff/40001/provider/azure/environ.go#...
provider/azure/environ.go:64: archMutex sync.Mutex
On 2014/03/20 11:10:08, fwereade wrote:
> it's common to put the guarded field under the mutex

Done.