Code review comment for lp:~fwereade/juju-core/bootstrap-constraints-3a

*** Submitted:

environs/ec2: instance-type logic and data

Not hooked up to anything else yet, no change in functionality.

R=dimitern, jameinel, dfc, thumper
CC=
https://codereview.appspot.com/7693048

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/export_test.go
File environs/ec2/export_test.go (right):

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/export_test.go#newcode91
environs/ec2/export_test.go:91: var TestInstanceTypeContent =
map[string]float64{
On 2013/03/19 01:15:06, thumper wrote:
> Would be good to have a comment here as to what the map is a map to.
The
> numbers by themselves are not obvious.

Done.

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/export_test.go#newcode103
environs/ec2/export_test.go:103: delete(allRegionCosts, "test")
On 2013/03/18 12:21:41, dimitern wrote:
> deleting from a nil map is ok? didn't know this.

It's not nil.

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go
File environs/ec2/instancetype.go (right):

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go#newcode29
environs/ec2/instancetype.go:29: return nothing, false
On 2013/03/18 23:37:52, dfc wrote:
> nice name

Cheers :)

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go#newcode50
environs/ec2/instancetype.go:50: continue outer
On 2013/03/18 23:37:52, dfc wrote:
> i don't think this needs to be this complicated, why not just break,
which will
> break the outer loop, right ? (i always find labeled breaks and
continues
> confusing)

D'oh. Sorry.

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go#newcode67
environs/ec2/instancetype.go:67: cons.CpuPower = &defaultCpuPower
On 2013/03/19 01:39:38, thumper wrote:
> On 2013/03/19 01:20:15, dfc wrote:
> > On 2013/03/19 01:15:06, thumper wrote:
> > > Taking the address of a locally defined variable feels wrong.
> >
> > Its not a locally defined var, it's a package level var. This is
safe in Go.

> This is safe in C++ too, but it doesn't mean it is a good idea.
cons.CpuPower
> can now change the value of the package level variable. If this is
the expected
> behaviour, I'd prefer a comment.

Better to just use a fresh copy, I think. Thanks all.

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go#newcode116
environs/ec2/instancetype.go:116: var allInstanceTypes = []instanceType{
On 2013/03/18 23:37:52, dfc wrote:
> On 2013/03/18 12:21:41, dimitern wrote:
> > it good to think about having these externally and filling them in
initially,
> > using some parsing.

> +1, i'd like to see this move to goamz, someday.

Long-term, I think there's no question that we should be maintaining
this list in a central location. However, I don't think that's the best
use of my attention in the current context. I hope that in oakland we'll
be able to use the examples of maas, openstack, and ec2 to have a useful
discussion about a common instance-data format and write a shared
implementation.

https://codereview.appspot.com/7693048/diff/3001/environs/ec2/instancetype.go#newcode244
environs/ec2/instancetype.go:244: "ap-northeast-1": { // Tokyo.
On 2013/03/19 00:03:18, thumper wrote:
> On 2013/03/18 23:37:52, dfc wrote:
> > If these are costs they should be integer cents, or 10ths of a cent.
Floats
> > should not be used to represent money.

> If we were charging people, I'd certainly agree.

> If however the costs are only being used to order options, I think
floats
> representing dollar values are fine.

I think dave's point is good -- it makes me froth when people calculate
money with floats, and by storing a money value as a float I am only
enabling and encouraging such behaviour in the future. And we can sort
by int costs just as easily as floats :).

https://codereview.appspot.com/7693048/