Merge lp://staging/~allenap/gwacl/destroy-hosted-service into lp://staging/gwacl

Please wait a bit to review and land this… we've got a problem with the underlying DestroyDeployment() method (I've tested it using example/management/run.go and it does not work [blows up trying to stop the VMs]… I thought the only problem is that we should ignore the error we get when stopping the VMs but it turns out it's a bit more involved).

I think we should fix DestroyDeployment by doing this first: https://code.launchpad.net/~rvb/gwacl/fix-destroydeploy2/+merge/173095

Since the deployment used in the tests does not contain VMs, this branch will probably work as is, even with that change.

Confirmed, this can be reviewed (and landed) independently from my "fix-destroydeploy2" branch.

Looks good but please consider my suggestions below:

[0]

We've had trouble lately with using these DestroyDeployment() so I'd advice to do this: now that my "fix-destroydeploy2" branch has landed, merge trunk and then update the example/management/run.go file to call DestroyHostedService() (instead of DestroyDeployment() and the defered which cleans up the hosted service) from there and make sure this works fine when talking to the Azure server.

[1]

126 +var exampleHostedService = &HostedService{
127 + Deployments: []Deployment{
128 + {Name: "one"}, {Name: "two"},
129 + },
130 +}

Having that as a global variable is, I think, an anti-pattern. This saves a bit of duplicate to have that as a global variable and not repeat it in each and every test. But a side effect is that the tests are not "self-contained" anymore and it makes it them much more difficult to understand and reason with.

For instance, the reasoning behind these checks:
159 + assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
160 + request.ServiceName, "one"}, record[1])
161 + assertDeleteDeploymentRequest(c, api, request.ServiceName, "one",
162 + record[2])
163 + // The fourth and fifth requests are a repaat of the previous two, but for
164 + // deployment "two".
165 + assertGetDeploymentRequest(c, api, &GetDeploymentRequest{
166 + request.ServiceName, "two"}, record[3])
167 + assertDeleteDeploymentRequest(c, api, request.ServiceName, "two",
168 + record[4])

is inherently linked to the structure of the tested object… and one has to navigate in the file to find the exampleHostedService's definition to understand why these assertions are meaningful.

I'd suggest:
- creating a tiny makeHostedService method that would take a slice of Deployment objects and return a *HostedService
- calling that method from every test so that the initialization of the tested object is not far from the checks.

Even better, maybe use variables instead of duplicating the strings/objects on which the checks are performed:
deployName1 := "one"
deployName2 := "two"
exampleHostedService = makeHostedService([]Deployment{{Name: deployName1}, {Name: deployName2}})

then the checks can be along the lines of:
assertDeleteDeploymentRequest(c, api, request.ServiceName, deployName2, record[4])

Thanks for the review.

> [0]
>
> We've had trouble lately with using these DestroyDeployment() so I'd advice to
> do this: now that my "fix-destroydeploy2"  branch has landed, merge trunk and
> then update the example/management/run.go file to call DestroyHostedService()
> (instead of DestroyDeployment() and the defered which cleans up the hosted
> service) from there and make sure this works fine when talking to the Azure
> server.

Done.

>
> [1]
>
> 126     +var exampleHostedService = &HostedService{
> 127     +    Deployments: []Deployment{
> 128     +        {Name: "one"}, {Name: "two"},
> 129     +    },
> 130     +}
>
> Having that as a global variable is, I think, an anti-pattern.  This saves a
> bit of duplicate to have that as a global variable and not repeat it in each
> and every test.  But a side effect is that the tests are not "self-contained"
> anymore and it makes it them much more difficult to understand and reason
> with.
...

I've changed some of these example bits to be functions instead, and
I'm not sure it makes it easier to reason about. I've noticed with
some of the test refactoring going on - extracting things into
functions and whatnot - that:

- The loss of locality of what I'm looking at is more harmful than
  having a few extra lines of code in my test.

- The indirection of having all test data constructed at runtime from
  parameters is more harmful than having static test data.

- Using strings like "disk1" is easier to read and grok than either
  deployments[0].RoleList[1].OSVirtualHardDisk[0].DiskName, or storing
  the names of everything in lots of local variables.

- Using functions for creating things in Go is painful because of the
  very limited calling conventions available. We are going to end up
  with 100s of factory functions for creating objects when it would
  simpler and more expressive to create them near the point of use.

Even so, I'll land the few changes that I did make.

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.