Code review comment for lp:~wallyworld/juju-core/improve-image-metadata-command

On 2013/10/16 02:57:49, wallyworld wrote:
> Please take a look.

https://codereview.appspot.com/14502059/diff/1/cmd/plugins/juju-metadata/imagemetadata.go
> File cmd/plugins/juju-metadata/imagemetadata.go (right):

https://codereview.appspot.com/14502059/diff/1/cmd/plugins/juju-metadata/imagemetadata.go#newcode72
> cmd/plugins/juju-metadata/imagemetadata.go:72: return
fmt.Errorf("environment %q
> cannot provide region and endpoint", environ.Name())
> On 2013/10/15 07:08:34, axw wrote:
> > What if the user has specified -s, -r, and -e on the command line?
Is this
> > really an error then?
> >

> Hmmm. I thought yes when I wrote this. But I changed it so that it
works more as
> expected.

> > Just a thought: perhaps ImageMetadataCommand could implement
HasRegion. If
> > environ doesn't implement HasRegion, fall back to
ImageMetadataCommand.

> See my changes - I think it's ok as is.

https://codereview.appspot.com/14502059/diff/1/cmd/plugins/juju-metadata/imagemetadata_test.go
> File cmd/plugins/juju-metadata/imagemetadata_test.go (right):

https://codereview.appspot.com/14502059/diff/1/cmd/plugins/juju-metadata/imagemetadata_test.go#newcode20
> cmd/plugins/juju-metadata/imagemetadata_test.go:20: "path/filepath"
> On 2013/10/15 07:08:34, axw wrote:
> > Move me please

> Done.

https://codereview.appspot.com/14502059/diff/1/cmd/plugins/juju-metadata/imagemetadata_test.go#newcode42
> cmd/plugins/juju-metadata/imagemetadata_test.go:42: restore :=
> testbase.PatchEnvironment("AWS_ACCESS_KEY_ID", "access")
> On 2013/10/15 07:08:34, axw wrote:
> > You can just use s.PatchEnvironment here, which wraps
> testbase.PatchEnvironment
> > & AddCleanup.

> Done.

Thanks, LGTM

https://codereview.appspot.com/14502059/