cmd/juju: add --upload-tools flag to bootstrap command

cmd/juju: add --upload-tools flag to bootstrap command

Also add Storage API to environs.Environ interface.

https://code.launchpad.net/~rogpeppe/juju/go-upload-tools-flag/+merge/104257

(do not edit description out of merge proposal)

[rog, 2012-05-01 14:33:56]

Please take a look.

[rog, 2012-05-02 12:57:08]

Please take a look.

[fwereade, 2012-05-02 13:36:27]

LGTM, very clear.

https://codereview.appspot.com/6145043/diff/7001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6145043/diff/7001/environs/dummy/environs.go#n...
environs/dummy/environs.go:171: if err != nil {
Maybe squish those two lines together (er, is there a technical term for that
operation?)

[niemeyer, 2012-05-03 00:52:05]

Looks nice. I'm just hoping we can do the cmd testing in a better fashion.

https://codereview.appspot.com/6145043/diff/7001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6145043/diff/7001/cmd/juju/cmd_test.go#newcode142
cmd/juju/cmd_test.go:142: c.Check(<-opc, Equals, op(dummy.OpPutFile, "peckham"))
The mocking here is starting to feel really bad. This test is verifying that
PutFile is called somewhen, somehow, for some reason. There are zero guarantees
that it is actually related to tools being uploaded. A much better test (and
pretty simple) would be verifying that the dummy environment actually got the
file uploaded.

https://codereview.appspot.com/6145043/diff/7001/environs/jujutest/test.go
File environs/jujutest/test.go (right):

https://codereview.appspot.com/6145043/diff/7001/environs/jujutest/test.go#ne...
environs/jujutest/test.go:78: err := t.Env.Bootstrap(true)
Shouldn't this be false, at least for the moment while nothing is being done
with the toolset?

[rog, 2012-05-04 16:29:13]

oops, forgot to publish these comments

https://codereview.appspot.com/6145043/diff/7001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6145043/diff/7001/cmd/juju/cmd_test.go#newcode142
cmd/juju/cmd_test.go:142: c.Check(<-opc, Equals, op(dummy.OpPutFile, "peckham"))
On 2012/05/03 00:52:05, niemeyer wrote:
> The mocking here is starting to feel really bad. This test is verifying that
> PutFile is called somewhen, somehow, for some reason. There are zero
guarantees
> that it is actually related to tools being uploaded. A much better test (and
> pretty simple) would be verifying that the dummy environment actually got the
> file uploaded.

Ok. I thought that since we test UploadTools in more detail elsewhere, we could
assume that the presence of PutFile means that UploadTools was invoked
correctly. I wanted to avoid duplicating the file name in one more place, but
perhaps I'd be better off defining it inside the version package. I'll do that,
and see what you think.

https://codereview.appspot.com/6145043/diff/7001/environs/jujutest/test.go
File environs/jujutest/test.go (right):

https://codereview.appspot.com/6145043/diff/7001/environs/jujutest/test.go#ne...
environs/jujutest/test.go:78: err := t.Env.Bootstrap(true)
On 2012/05/03 00:52:05, niemeyer wrote:
> Shouldn't this be false, at least for the moment while nothing is being done
> with the toolset?

ok, i'll leave it as a TODO.

[rog, 2012-05-04 17:17:25]

Please take a look.

[niemeyer, 2012-05-15 20:18:29]

This is looking nice, but we've had a conversation today about how to organize
things (GetTools/PutTools, Storage, etc) that should be reflected in it.

I'm marking it as WIP for organization. Please re-propose when it's ready for
another look.

[rog, 2012-05-16 16:12:18]

Please take a look.

[niemeyer, 2012-05-17 01:08:41]

I like this a lot. There are many comments because the branch is ginormous, but
the number of them doesn't reflect my feeling about it. Thanks.

https://codereview.appspot.com/6145043/diff/25018/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6145043/diff/25018/cmd/juju/cmd_test.go#newcod...
cmd/juju/cmd_test.go:123: defer dummy.Reset(nil, false)
I'm unhappy with the concept of Reset but don't reset (clear=false). These tests
can easily be organized so that they call Reset just once when they start, so
that we can get rid of that weird flag that will require one to read the
implementation of dummy to understand.

https://codereview.appspot.com/6145043/diff/25018/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/dummy/environs.go#...
environs/dummy/environs.go:90: // If clean is true, this will be empty;
otherwise
This will be empty? That comment doesn't seem to make sense in its context, and
I'm not sure it makes sense even if we explain it further, because with clear
being false, Reset does not reset, which is pretty strange. I'd appreciate
removing this flag and simplifying the test so that they're not resetting the
environment in the middle if they don't want that, as pointed out in the other
comment.

The other changes in this file seem good, though.

https://codereview.appspot.com/6145043/diff/25018/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/ec2/ec2.go#newcode45
environs/ec2/ec2.go:45: store  storage
I suggest "storage storage". When we talk about "store" in the juju context, we
generally mean something else.

https://codereview.appspot.com/6145043/diff/25018/environs/ec2/state.go
File environs/ec2/state.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/ec2/state.go#newco...
environs/ec2/state.go:51: s := e.Storage().(*storage)
Why the type assertion here?

https://codereview.appspot.com/6145043/diff/25018/environs/ec2/storage.go
File environs/ec2/storage.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/ec2/storage.go#new...
environs/ec2/storage.go:16: b                *s3.Bucket
Can we name this as "bucket" instead?

https://codereview.appspot.com/6145043/diff/25018/environs/export_test.go
File environs/export_test.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/export_test.go#new...
environs/export_test.go:3: var ToolsPath = toolsPath
I'd prefer to avoid this file and duplicate the path in the tests. First,
because we'll need to test the selection logic soonish anyway. And then, as it
is if someone puts a typo by mistake on this (important!) path, we're screwed
and won't be aware of it until too late.

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:43: // NotFoundError is the concrete error type returned
by
s/concrete//

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:46: // error gives the underlying error.
s/gives/is/

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:50: // StorageReader gives a way to read data stored
within
These interfaces look nice. Just a few comments regarding the docs.

Here, I suggest this instead:

// A StorageReader can retrieve and list files from a storage provider.

Note the data isn't necessarily *in* the environment. Public storage may well be
outside of it (and will be, in the case of the local provider for example). We
just get a reference to the storage from the environment.

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:53: // should be *NotFoundError.
This is documenting the behavior of the method. Better to have it in the
respective method.

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:58: // after use.
Indentation is off on the first line.

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:62: // in alphabetical order.
This should also mention the behavior with "directories".

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:71: // within an environment.
// A StorageWriter adds and removes files in a storage provider.

https://codereview.appspot.com/6145043/diff/25018/environs/jujutest/tests.go
File environs/jujutest/tests.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/jujutest/tests.go#...
environs/jujutest/tests.go:87: checkList(c, store, "zzz/", []string{"zzz/aa",
"zzz/bb"})
Nice tests in this file.

https://codereview.appspot.com/6145043/diff/25018/environs/jujutest/tests.go#...
environs/jujutest/tests.go:133: c.Assert(err, FitsTypeOf,
(*environs.NotFoundError)(nil))
This generally looks nicer with a variable. E.g.:

var error *environs.NotFoundError
c.Assert(err, FitsTypeOf, error)

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode17
environs/tools.go:17: // toolsPath gives the path for the current juju tools, as
expected
// toolsPath is the storage path for the juju tools with the
// same version, OS, and arch as the currently running client.

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode19
environs/tools.go:19: var toolsPath = fmt.Sprintf("tools/%v-%s-%s.tgz",
version.Current, runtime.GOOS, runtime.GOARCH)
We need a proper file name here, and we already had one before:

"tools/juju-..."

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode120
environs/tools.go:120: func PutTools(store StorageWriter) error {
s/store/storage/

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode146
environs/tools.go:146: // and downloads them into the given directory.
s/and downloads/, downloads, and extracts/

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode147
environs/tools.go:147: func GetTools(store StorageReader, dir string) error {
s/store/storage/

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode148
environs/tools.go:148: // TODO search the store for the right tools.
s/tools/tools version/

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode171
environs/tools.go:171: // TODO (perhaps) allow subdirectories.
The TODO is not necessary. We don't need it now or in the foreseeable feature.
If we ever need it, we'll necessarily have other changes associated and the
error below will let us know nicely about the problem.

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode197
environs/tools.go:197: type emptyStorage struct {
Would you mind if we always spelled that kind of no-fields structs in a single
line:

type emptyStorage struct{}

[rog, 2012-05-17 11:43:33]

thanks a lot for the review, and sorry the branch is so big. it started off
quite a reasonable size!

https://codereview.appspot.com/6145043/diff/25018/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6145043/diff/25018/cmd/juju/cmd_test.go#newcod...
cmd/juju/cmd_test.go:123: defer dummy.Reset(nil, false)
On 2012/05/17 01:08:42, niemeyer wrote:
> I'm unhappy with the concept of Reset but don't reset (clear=false). These
tests
> can easily be organized so that they call Reset just once when they start, so
> that we can get rid of that weird flag that will require one to read the
> implementation of dummy to understand.

it's not quite that easy, unfortunately. we need to call Reset because it's
reset that closes the channel, and without the channel being closed, we're not
certain to receive all the changes deterministically.

i've factored Reset into two operations - Listen and Close, which i think may
work better.

https://codereview.appspot.com/6145043/diff/25018/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/dummy/environs.go#...
environs/dummy/environs.go:90: // If clean is true, this will be empty;
otherwise
On 2012/05/17 01:08:42, niemeyer wrote:
> This will be empty? That comment doesn't seem to make sense in its context,
and
> I'm not sure it makes sense even if we explain it further, because with clear
> being false, Reset does not reset, which is pretty strange. I'd appreciate
> removing this flag and simplifying the test so that they're not resetting the
> environment in the middle if they don't want that, as pointed out in the other
> comment.
> 
> The other changes in this file seem good, though.

will refactor as mentioned in cmd_test.go

https://codereview.appspot.com/6145043/diff/25018/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/ec2/ec2.go#newcode45
environs/ec2/ec2.go:45: store  storage
On 2012/05/17 01:08:42, niemeyer wrote:
> I suggest "storage storage". When we talk about "store" in the juju context,
we
> generally mean something else.

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/ec2/state.go
File environs/ec2/state.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/ec2/state.go#newco...
environs/ec2/state.go:51: s := e.Storage().(*storage)
On 2012/05/17 01:08:42, niemeyer wrote:
> Why the type assertion here?

ah, it's a leak from the upcoming version where we delete the bucket afterwards,
so need a handle on the private type so we can call DelBucket on s.b.

done.

https://codereview.appspot.com/6145043/diff/25018/environs/ec2/storage.go
File environs/ec2/storage.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/ec2/storage.go#new...
environs/ec2/storage.go:16: b                *s3.Bucket
On 2012/05/17 01:08:42, niemeyer wrote:
> Can we name this as "bucket" instead?

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/export_test.go
File environs/export_test.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/export_test.go#new...
environs/export_test.go:3: var ToolsPath = toolsPath
On 2012/05/17 01:08:42, niemeyer wrote:
> I'd prefer to avoid this file and duplicate the path in the tests. First,
> because we'll need to test the selection logic soonish anyway. And then, as it
> is if someone puts a typo by mistake on this (important!) path, we're screwed
> and won't be aware of it until too late.

i'm not entirely convinced that copy and pasting that line gives any significant
protection, but done anyway.

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:43: // NotFoundError is the concrete error type returned
by
On 2012/05/17 01:08:42, niemeyer wrote:
> s/concrete//

changed a little more than that. NotFoundError can be used in other places too.

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:46: // error gives the underlying error.
On 2012/05/17 01:08:42, niemeyer wrote:
> s/gives/is/

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:50: // StorageReader gives a way to read data stored
within
On 2012/05/17 01:08:42, niemeyer wrote:
> These interfaces look nice. Just a few comments regarding the docs.
> 
> Here, I suggest this instead:
> 
> // A StorageReader can retrieve and list files from a storage provider.
> 
> Note the data isn't necessarily *in* the environment. Public storage may well
be
> outside of it (and will be, in the case of the local provider for example). We
> just get a reference to the storage from the environment.

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:53: // should be *NotFoundError.
On 2012/05/17 01:08:42, niemeyer wrote:
> This is documenting the behavior of the method. Better to have it in the
> respective method.

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:58: // after use.
On 2012/05/17 01:08:42, niemeyer wrote:
> Indentation is off on the first line.

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:62: // in alphabetical order.
On 2012/05/17 01:08:42, niemeyer wrote:
> This should also mention the behavior with "directories".

i'm not sure what you mean here. there are no directories, and all behaviour
here can be understood in terms of simple names, i believe.

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:71: // within an environment.
On 2012/05/17 01:08:42, niemeyer wrote:
> // A StorageWriter adds and removes files in a storage provider.

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/jujutest/tests.go
File environs/jujutest/tests.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/jujutest/tests.go#...
environs/jujutest/tests.go:87: checkList(c, store, "zzz/", []string{"zzz/aa",
"zzz/bb"})
On 2012/05/17 01:08:42, niemeyer wrote:
> Nice tests in this file.

thanks

https://codereview.appspot.com/6145043/diff/25018/environs/jujutest/tests.go#...
environs/jujutest/tests.go:133: c.Assert(err, FitsTypeOf,
(*environs.NotFoundError)(nil))
On 2012/05/17 01:08:42, niemeyer wrote:
> This generally looks nicer with a variable. E.g.:
> 
> var error *environs.NotFoundError
> c.Assert(err, FitsTypeOf, error)

if you say so.
done.

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode17
environs/tools.go:17: // toolsPath gives the path for the current juju tools, as
expected
On 2012/05/17 01:08:42, niemeyer wrote:
> // toolsPath is the storage path for the juju tools with the
> // same version, OS, and arch as the currently running client.

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode19
environs/tools.go:19: var toolsPath = fmt.Sprintf("tools/%v-%s-%s.tgz",
version.Current, runtime.GOOS, runtime.GOARCH)
On 2012/05/17 01:08:42, niemeyer wrote:
> We need a proper file name here, and we already had one before:
> 
> "tools/juju-..."

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode120
environs/tools.go:120: func PutTools(store StorageWriter) error {
On 2012/05/17 01:08:42, niemeyer wrote:
> s/store/storage/

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode146
environs/tools.go:146: // and downloads them into the given directory.
On 2012/05/17 01:08:42, niemeyer wrote:
> s/and downloads/, downloads, and extracts/

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode147
environs/tools.go:147: func GetTools(store StorageReader, dir string) error {
On 2012/05/17 01:08:42, niemeyer wrote:
> s/store/storage/

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode148
environs/tools.go:148: // TODO search the store for the right tools.
On 2012/05/17 01:08:42, niemeyer wrote:
> s/tools/tools version/

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode171
environs/tools.go:171: // TODO (perhaps) allow subdirectories.
On 2012/05/17 01:08:42, niemeyer wrote:
> The TODO is not necessary. We don't need it now or in the foreseeable feature.
> If we ever need it, we'll necessarily have other changes associated and the
> error below will let us know nicely about the problem.

Done.

https://codereview.appspot.com/6145043/diff/25018/environs/tools.go#newcode197
environs/tools.go:197: type emptyStorage struct {
On 2012/05/17 01:08:42, niemeyer wrote:
> Would you mind if we always spelled that kind of no-fields structs in a single
> line:
> 
> type emptyStorage struct{}

Done.

[rog, 2012-05-17 11:44:51]

Please take a look.

[fwereade, 2012-05-17 11:55:32]

LGTM, I find this pleasing.

https://codereview.appspot.com/6145043/diff/30001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6145043/diff/30001/environs/interface.go#newco...
environs/interface.go:61: // TODO: URL returns a URL that can be used to access
the given
This being an interface method says to me that it's done by definition ;).

https://codereview.appspot.com/6145043/diff/21023/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6145043/diff/21023/environs/ec2/ec2.go#newcode44
environs/ec2/ec2.go:44: s3      *s3.S3
Do we need to keep the S3 around here, or can we fold it entirely into the
storage?

[rog, 2012-05-17 15:22:41]

Please take a look.

[rog, 2012-05-17 15:32:52]

Please take a look.

[rog, 2012-05-17 15:34:30]

https://codereview.appspot.com/6145043/diff/21023/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6145043/diff/21023/environs/ec2/ec2.go#newcode44
environs/ec2/ec2.go:44: s3      *s3.S3
On 2012/05/17 11:55:32, fwereade wrote:
> Do we need to keep the S3 around here, or can we fold it entirely into the
> storage?

we'll need it when we come to implement PublicStorage, i think. maybe we should
fold it in anyway, but i don't feel too strongly. i like having the ec2 and s3
members at the top level.

[niemeyer, 2012-05-17 18:25:48]

Minor gripe with the new dummy API. If you're happy with the suggested change,
the whole thing LGTM.

https://codereview.appspot.com/6145043/diff/25018/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6145043/diff/25018/cmd/juju/cmd_test.go#newcod...
cmd/juju/cmd_test.go:123: defer dummy.Reset(nil, false)
> i've factored Reset into two operations - Listen and Close, which i think may
> work better.

This looks like a good direction, but the API feels a bit awkward. dummy.Listen
resets? dummy.Close closes what? Not the dummy env, since it continues working
just fine. Please see the comments in the dummy environment for a suggested
follow up.

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:62: // in alphabetical order.
On 2012/05/17 11:43:33, rog wrote:
> On 2012/05/17 01:08:42, niemeyer wrote:
> > This should also mention the behavior with "directories".
> 
> i'm not sure what you mean here. there are no directories, and all behaviour
> here can be understood in terms of simple names, i believe.

The point is precisely to tell people what you believe, since what they believe
may not match what you believe. Let's please include this:

// The names in the storage are considered to be in a flat
// namespace, so the prefix may include slashes and what is
// returned is the full name for the matching entries.

https://codereview.appspot.com/6145043/diff/27003/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6145043/diff/27003/cmd/juju/cmd_test.go#newcod...
cmd/juju/cmd_test.go:138: defer dummy.Listen(nil)
Is this needed, considering that TearDownTest does it too?

https://codereview.appspot.com/6145043/diff/27003/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6145043/diff/27003/environs/dummy/environs.go#...
environs/dummy/environs.go:107: // cleaned without registering a channel.
As pointed out earlier, the direction seems good, but the details of how this is
working sounds more involved than necessary.

The following API would separate the two concepts apart, and clarify the call
sites:

// Reset resets the entire dummy environment and any
// registered operation listeners. All opened environments
// after Reset will share the same underlying state.
func Reset() {

// Listen closes the previously registered listener (if any),
// and if c is not nil registers it to receive notifications 
// of follow up operations in the environment.
func Listen(c chan<- Operation) {


No need for dummy.Close. The dummy environment is not being closed.

[rog, 2012-05-18 11:22:18]

*** Submitted:

cmd/juju: add --upload-tools flag to bootstrap command

Also add Storage API to environs.Environ interface.

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

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6145043/diff/25018/environs/interface.go#newco...
environs/interface.go:62: // in alphabetical order.
On 2012/05/17 18:25:48, niemeyer wrote:
> On 2012/05/17 11:43:33, rog wrote:
> > On 2012/05/17 01:08:42, niemeyer wrote:
> > > This should also mention the behavior with "directories".
> > 
> > i'm not sure what you mean here. there are no directories, and all behaviour
> > here can be understood in terms of simple names, i believe.
> 
> The point is precisely to tell people what you believe, since what they
believe
> may not match what you believe. Let's please include this:
> 
> // The names in the storage are considered to be in a flat
> // namespace, so the prefix may include slashes and what is
> // returned is the full name for the matching entries.

Done.

https://codereview.appspot.com/6145043/diff/27003/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6145043/diff/27003/cmd/juju/cmd_test.go#newcod...
cmd/juju/cmd_test.go:138: defer dummy.Listen(nil)
On 2012/05/17 18:25:49, niemeyer wrote:
> Is this needed, considering that TearDownTest does it too?

Done.

https://codereview.appspot.com/6145043/diff/27003/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6145043/diff/27003/environs/dummy/environs.go#...
environs/dummy/environs.go:107: // cleaned without registering a channel.
On 2012/05/17 18:25:49, niemeyer wrote:
> As pointed out earlier, the direction seems good, but the details of how this
is
> working sounds more involved than necessary.
> 
> The following API would separate the two concepts apart, and clarify the call
> sites:
> 
> // Reset resets the entire dummy environment and any
> // registered operation listeners. All opened environments
> // after Reset will share the same underlying state.
> func Reset() {
> 
> // Listen closes the previously registered listener (if any),
> // and if c is not nil registers it to receive notifications 
> // of follow up operations in the environment.
> func Listen(c chan<- Operation) {
> 
> 
> No need for dummy.Close. The dummy environment is not being closed.

that's definitely nicer, thanks. done.