environs/ec2: rename ImageSpec to instanceSpec

environs/ec2: rename ImageSpec to instanceSpec

also unexport ImageConstraint.

https://code.launchpad.net/~rogpeppe/juju/go-ec2-instance-spec/+merge/105384

Requires: https://code.launchpad.net/~rogpeppe/juju/go-ec2-find-tools/+merge/105382

(do not edit description out of merge proposal)

[rog, 2012-05-22 13:29:55]

Please take a look.

[fwereade, 2012-05-23 09:19:22]

I'm not quite sure about this; if we're tweaking it to fit the emerging context
better (which is great) I'd prefer that we drop the unwanted weirdnesses sooner
rather than later, rather than just changing the name of a type that is
moderately ill-conceived in the first place.

https://codereview.appspot.com/6188068/diff/2006/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/6188068/diff/2006/environs/ec2/image.go#newcode20
environs/ec2/image.go:20: Desktop           bool
PersistentStorage/Daily/Desktop are entirely redundant AFAICT... if we're
touching this type, can we please just drop them?

https://codereview.appspot.com/6188068/diff/2006/environs/ec2/image.go#newcode25
environs/ec2/image.go:25: Arch:              "i386",
And (again because we're touching this): oneiric/i386 is just not a sensible
value in the first place; furthermore, we should not be specifying a default
default series at all, and this just looks like a hack to avoid having to
specify it where it ought to be (in env config).

[fwereade, 2012-05-23 09:37:26]

Further to IRC discussion: LGTM so long as my concerns are addressed in a future
branch. ++small-branches ;).

[rog, 2012-05-23 12:50:23]

Please take a look.

[niemeyer, 2012-05-23 20:32:01]

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/ec2.go#newcode212
environs/ec2/ec2.go:212: image, err :=
FindInstanceSpec(DefaultInstanceConstraint)
Variable names and error messages probably need updating too.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode10
environs/ec2/image.go:10: // TODO implement constraints properly.
This comment isn't helpful. It should either be improved or dropped.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode12
environs/ec2/image.go:12: // InstanceConstraint specifies a range of possible
instances
The reorg is going in a nice direction, but we need better docs to clarify
what's going on. This type doesn't specify a *range of instances* nor a *range
of images*. I can't take a value of that type and iterate over either of those
ranges.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode32
environs/ec2/image.go:32: // InstanceSpec specifies a particular machine type
and the Ubuntu
Also sounds poor. The definition shouldn't be merely stating what the fields it
contains are. I can see that below. What *is* an InstanceSpec? Why should I use
it? What's the difference between an InstanceSpec and InstanceConstraints? I
can't tell any of that from these docs.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode37
environs/ec2/image.go:37: Series  string // The Ubuntu series the image will run
on.
The inline comments are also bogus now that we're calling it an *Instance*Spec.
We can probably drop them, since they're somewhat clear given their name.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode55
environs/ec2/image.go:55: // + more that we don't care about.
If there's more, the value below is wrong. If nothing else, the comment above
should be moved to after max, since that's where the additional content lies.

[niemeyer, 2012-05-23 20:32:01]

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/ec2.go#newcode212
environs/ec2/ec2.go:212: image, err :=
FindInstanceSpec(DefaultInstanceConstraint)
Variable names and error messages probably need updating too.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode10
environs/ec2/image.go:10: // TODO implement constraints properly.
This comment isn't helpful. It should either be improved or dropped.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode12
environs/ec2/image.go:12: // InstanceConstraint specifies a range of possible
instances
The reorg is going in a nice direction, but we need better docs to clarify
what's going on. This type doesn't specify a *range of instances* nor a *range
of images*. I can't take a value of that type and iterate over either of those
ranges.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode32
environs/ec2/image.go:32: // InstanceSpec specifies a particular machine type
and the Ubuntu
Also sounds poor. The definition shouldn't be merely stating what the fields it
contains are. I can see that below. What *is* an InstanceSpec? Why should I use
it? What's the difference between an InstanceSpec and InstanceConstraints? I
can't tell any of that from these docs.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode37
environs/ec2/image.go:37: Series  string // The Ubuntu series the image will run
on.
The inline comments are also bogus now that we're calling it an *Instance*Spec.
We can probably drop them, since they're somewhat clear given their name.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode55
environs/ec2/image.go:55: // + more that we don't care about.
If there's more, the value below is wrong. If nothing else, the comment above
should be moved to after max, since that's where the additional content lies.

[rog, 2012-05-24 10:14:03]

Please take a look.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/ec2.go#newcode212
environs/ec2/ec2.go:212: image, err :=
FindInstanceSpec(DefaultInstanceConstraint)
On 2012/05/23 20:32:01, niemeyer wrote:
> Variable names and error messages probably need updating too.

Done.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode10
environs/ec2/image.go:10: // TODO implement constraints properly.
On 2012/05/23 20:32:01, niemeyer wrote:
> This comment isn't helpful. It should either be improved or dropped.

Done.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode12
environs/ec2/image.go:12: // InstanceConstraint specifies a range of possible
instances
On 2012/05/23 20:32:01, niemeyer wrote:
> The reorg is going in a nice direction, but we need better docs to clarify
> what's going on. This type doesn't specify a *range of instances* nor a *range
> of images*. I can't take a value of that type and iterate over either of those
> ranges.

Done.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode32
environs/ec2/image.go:32: // InstanceSpec specifies a particular machine type
and the Ubuntu
On 2012/05/23 20:32:01, niemeyer wrote:
> Also sounds poor. The definition shouldn't be merely stating what the fields
it
> contains are. I can see that below. What *is* an InstanceSpec? Why should I
use
> it? What's the difference between an InstanceSpec and InstanceConstraints? I
> can't tell any of that from these docs.

I've hopefully improved the comment. I've also unexported the types, because
they aren't useful outside the package. At some point in the future, some
derivation of imageConstraint will end up in the environs package, I think, but
until then, private is good I think.

https://codereview.appspot.com/6188068/diff/3014/environs/ec2/image.go#newcode55
environs/ec2/image.go:55: // + more that we don't care about.
On 2012/05/23 20:32:01, niemeyer wrote:
> If there's more, the value below is wrong. If nothing else, the comment above
> should be moved to after max, since that's where the additional content lies.

argh, this change is only showing because i forgot to re-merge the prereq.

done anyway.

[niemeyer, 2012-05-24 14:20:12]

LGTM

https://codereview.appspot.com/6188068/diff/20001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6188068/diff/20001/environs/ec2/ec2.go#newcode214
environs/ec2/ec2.go:214: return nil, fmt.Errorf("cannot find instance: %v", err)
s/instance/an instance satisfying the constraints/?

https://codereview.appspot.com/6188068/diff/20001/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/6188068/diff/20001/environs/ec2/image.go#newco...
environs/ec2/image.go:12: type instanceConstraint struct {
Feels like going in the opposite direction we should go. I'd prefer to have this
moved to environs now, rather than renaming all fields and fixing code after
that.

You said you want to do it in a follow up branch. That's fine by me too.

https://codereview.appspot.com/6188068/diff/20001/environs/ec2/image.go#newco...
environs/ec2/image.go:46: coldaily
The names here should be consistent among themselves. It was right before.

https://codereview.appspot.com/6188068/diff/20001/environs/ec2/image.go#newco...
environs/ec2/image.go:57: // the specified constraints.
s/specified constraints/provided constraints/; just to avoid the "specification
given the specified" feeling.

[rog, 2012-05-24 14:27:51]

*** Submitted:

environs/ec2: rename ImageSpec to instanceSpec

also unexport ImageConstraint.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6188068

https://codereview.appspot.com/6188068/diff/20001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6188068/diff/20001/environs/ec2/ec2.go#newcode214
environs/ec2/ec2.go:214: return nil, fmt.Errorf("cannot find instance: %v", err)
On 2012/05/24 14:20:12, niemeyer wrote:
> s/instance/an instance satisfying the constraints/?

Done.

https://codereview.appspot.com/6188068/diff/20001/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/6188068/diff/20001/environs/ec2/image.go#newco...
environs/ec2/image.go:12: type instanceConstraint struct {
On 2012/05/24 14:20:12, niemeyer wrote:
> Feels like going in the opposite direction we should go. I'd prefer to have
this
> moved to environs now, rather than renaming all fields and fixing code after
> that.
> 
> You said you want to do it in a follow up branch. That's fine by me too.

will do.

https://codereview.appspot.com/6188068/diff/20001/environs/ec2/image.go#newco...
environs/ec2/image.go:46: coldaily
On 2012/05/24 14:20:12, niemeyer wrote:
> The names here should be consistent among themselves. It was right before.

Done.

https://codereview.appspot.com/6188068/diff/20001/environs/ec2/image.go#newco...
environs/ec2/image.go:57: // the specified constraints.
On 2012/05/24 14:20:12, niemeyer wrote:
> s/specified constraints/provided constraints/; just to avoid the
"specification
> given the specified" feeling.

Done.