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

Revision history for this message
Ian Booth (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.

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

« Back to merge proposal