Please take a look.


https://codereview.appspot.com/7204055/diff/1/testservices/service.go
File testservices/service.go (right):

https://codereview.appspot.com/7204055/diff/1/testservices/service.go#newcode65
testservices/service.go:65: // }
On 2013/01/28 06:13:58, jameinel wrote:
> I can't help but feel that something like "nil" would be clearer than
the empty
> string. But I think we're stuck with what we have.


Welcome to Go :-(

> Alternatively, if most of the time we're just going to be grabbing the
function
> name, maybe we want to make that the obvious one, and add a separate
function
> when we have to?

I thought about it (and cursed and lamented the lack of default
parameters in Go). The Go Way seems to be though to stick to a single
method and pass in default parameter values all the time, from what I
can tell.

https://codereview.appspot.com/7204055/diff/1/testservices/service_test.go
File testservices/service_test.go (right):

https://codereview.appspot.com/7204055/diff/1/testservices/service_test.go#newcode50
testservices/service_test.go:50: func nonFunctionControlHook(s
ServiceControl, args ...interface{}) error {
On 2013/01/28 06:13:58, jameinel wrote:
> I don't quite understand why this is "nonFunctionControlHook", is it
meant to be
> "named" hook, or something like that?

"namedControlHook" is much nicer, will use that

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_test.go
File tools/secgroup-delete-all/main_test.go (left):

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_test.go#oldcode47
tools/secgroup-delete-all/main_test.go:47: openstack :=
openstack.New(creds)
On 2013/01/28 06:13:58, jameinel wrote:
> I'd like us to generally avoid using variable names that match module
names. It
> gets confusing to figure out which "openstack" thing is being operated
on.

> Not that you added this, but I've seen it in a couple places, and was
a bit
> confused by it.

No, it was me who added it in a previous branch from memory.
I'll change the variable name. Perhaps I should have called the package
"testopenstack" but then there's already a "testservices" in the import
path and it seems redundant. We call the nova double "novaservice" but
"openstackservice" seems too long. But in the juju-core code, the
production openstack provider package is just called "openstack" and so
disambiguation is needed there. So just "openstack" in this case does
seem like it is not the best choice in hindsight.

I'll change it to "openstackservice" and see if I get agreement on that.

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_test.go
File tools/secgroup-delete-all/main_test.go (right):

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_test.go#newcode75
tools/secgroup-delete-all/main_test.go:75: if groupId == 2 {
On 2013/01/28 06:13:58, jameinel wrote:
> This seems a bit risky to have it always work (comparing an
interface{} to an
> integer).

I've added a cast. Note though that my expectation here is that the
author of the hook is expected to know the signature of the function
being processed and hence what the arguments will be.

> Also, it would be nicer if we at least compared to the group name,
rather than
> to an integer id. Is it possible to get more context for the hook? I
realize
> this is what we have today, but it seems like it would be really easy
to
> accidentally break (changing the numbering of the groups, adding a
default
> group, etc.)

Fair point. I've adopted the solution you suggested below.

https://codereview.appspot.com/7204055/diff/1/tools/secgroup-delete-all/main_test.go#newcode84
tools/secgroup-delete-all/main_test.go:84:
nova.CreateSecurityGroup("group-b", "Another group")
On 2013/01/28 06:13:58, jameinel wrote:

> I also wonder if it would be nice to have
os.Nova.RegisterControlPoint() return
> a callable that you can use to cleanup. So it would look like:

> cleanup := os.Nova.RegisterControlPoint("removeSecurityGroup",
deleteError)
> defer cleanup()

> It's nice in python, I'm not sure if it is nice in go.

I like this suggestion and implemented it.

https://codereview.appspot.com/7204055/