Merge lp://staging/~allenap/gwacl/add-virtual-network into lp://staging/gwacl

Looks generally good but I've got a question (see [0]).

[0]

76 + for _, existingSite := range *networkConfig.VirtualNetworkSites {
77 + if existingSite.Name == site.Name {
78 + // Network already defined.
79 + return nil
80 + }

That's a weird behavior don't you think? Say I try adding a network with the same name but a different affinity group; the method will return with a nil error so I'll think that everything is ok and it will be a lie :). I /think/ returning an error (maybe only if the network you're trying to add has exactly the same characteristics as the one with the same name which already exists) is more appropriate here. What do you think?

[1]

102 + for _, existingSite := range *networkConfig.VirtualNetworkSites {
103 + if existingSite.Name != siteName {
104 + virtualNetworkSites = append(virtualNetworkSites, existingSite)
105 + }

We cannot just remove the network in question from networkConfig.VirtualNetworkSites can we :/

[2]

208 + virtualNetwork := &VirtualNetworkSite{Name: MakeRandomVirtualNetworkName("test-")}
209 + err := api.AddVirtualNetworkSite(virtualNetwork)
210 + c.Assert(err, IsNil)
211 + c.Check(record, HasLen, 2)

It's a detail but that's something Jeroen and I have tried to do in tests, especially when the tests are long: put a empty line between the initialization code and the method you're testing, and another after that. This way, we have 3 visually well-separated blocks: initialization, method call, checks.

[3]

You could add a check in example/management/run.go to make sure that the created machine effectively has an IP in the virtual network.

> [0]
>
> 76      +    for _, existingSite := range *networkConfig.VirtualNetworkSites {
> 77      +        if existingSite.Name == site.Name {
> 78      +            // Network already defined.
> 79      +            return nil
> 80      +        }
>
> That's a weird behavior don't you think?  Say I try adding a network with the
> same name but a different affinity group; the method will return with a nil
> error so I'll think that everything is ok and it will be a lie :).  I /think/
> returning an error (maybe only if the network you're trying to add has exactly
> the same characteristics as the one with the same name which already exists)
> is more appropriate here.  What do you think?

Good point. I've changed it to return an error when there's a
preexisting network with the same name.

>
> [1]
>
> 102     +    for _, existingSite := range *networkConfig.VirtualNetworkSites {
> 103     +        if existingSite.Name != siteName {
> 104     +            virtualNetworkSites = append(virtualNetworkSites,
> existingSite)
> 105     +        }
>
> We cannot just remove the network in question from
> networkConfig.VirtualNetworkSites can we :/

Is there a reason why we can't?

Ah, do you mean why can't we delete the entry directly from the slice?
Because Go.

Seriously, if you know how to do this nicely, please let me know.

>
> [2]
>
> 208     +    virtualNetwork := &VirtualNetworkSite{Name:
> MakeRandomVirtualNetworkName("test-")}
> 209     +    err := api.AddVirtualNetworkSite(virtualNetwork)
> 210     +    c.Assert(err, IsNil)
> 211     +    c.Check(record, HasLen, 2)
>
> It's a detail but that's something Jeroen and I have tried to do in tests,
> especially when the tests are long: put a empty line between the
> initialization code and the method you're testing, and another after that.
> This way, we have 3 visually well-separated blocks: initialization, method
> call, checks.

It's a pretty inconsistently followed pattern, but what the hell;
fixed.

Fwiw, I've been avoiding blank lines because gof*t only adds a single
line between functions instead of the two I'm used to.

>
> [3]
>
> You could add a check in example/management/run.go to make sure that the
> created machine effectively has an IP in the virtual network.

I'll give this a go.

> > [3]
> >
> > You could add a check in example/management/run.go to make sure that the
> > created machine effectively has an IP in the virtual network.
>
> I'll give this a go.

I've just realised there's a good reason why it doesn't do this: VMs
are not yet deployed into the virtual network that's created. That's
something for a subsequent branch.

> > > [3]
> > >
> > > You could add a check in example/management/run.go to make sure that the
> > > created machine effectively has an IP in the virtual network.
> >
> > I'll give this a go.
>
> I've just realised there's a good reason why it doesn't do this: VMs
> are not yet deployed into the virtual network that's created. That's
> something for a subsequent branch.

Fair enough… it should be as simple as giving the name of the vnet to gwacl.NewDeploymentForCreateVMDeployment()… and maybe adding the objects with the right affinity group in a couple of places.

> > Ah, do you mean why can't we delete the entry directly from the slice?
> Because Go.

Yeah, because Go :/

> Seriously, if you know how to do this nicely, please let me know.

I don't… that's a shame.

Thanks for the fixes.

Thanks for the review!