Go Windows Azure Client Library

Merge lp://staging/~rvb/gwacl/add-network-config into lp://staging/gwacl

  1. add-network-config
  2. Merge into trunk
Proposed by Raphaël Badin
Status: Merged
Approved by: Raphaël Badin
Approved revision: 153
Merged at revision: 154
Proposed branch: lp://staging/~rvb/gwacl/add-network-config
Merge into: lp://staging/gwacl
Diff against target: 475 lines (+140/-101)
5 files modified
example/management/run.go (+19/-4)
helpers_apiobjects_test.go (+4/-11)
management_base_test.go (+3/-3)
xmlobjects.go (+57/-42)
xmlobjects_test.go (+57/-41)
To merge this branch: bzr merge lp://staging/~rvb/gwacl/add-network-config
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+172518@code.staging.launchpad.net

Commit message

Add support for configuring the network when creating a VM.

Description of the change

This branch's main goal is to allow the user to configure the network using a ConfigurationSet object. It can be used when creating the VM and (hopefully) later on when reconfiguring the VM to open/close ports.

The main trick is that the object we used to call LinuxProvisioningConfiguration is in fact a "generic" ConfigurationSet object which can contain different types of configuration. This is rather bad design on Azure's part but it's easier for us to reflect exactly the structure of Azure's objects in our class hierarchy. This is why I renamed LinuxProvisioningConfiguration into ConfigurationSet and added a utility method to create the other type of ConfigurationSet object: the network configuration object (i.e. a ConfigurationSet object with a type of "NetworkConfiguration").

I changed the DisableSSHPasswordAuthentication boolean into a string because the 'omitempty' parameter would remove the field altogether from the serialized XML when it's false. And we *have to* use 'omitempty' because, when the ConfigurationSet object will be of type NetworkConfiguration, we need that field to be omitted.

I had to tweak the XML used in tests in xmlobjects_test.go because the examples (I /think/, not my doing) where taken straight out of the documentation and are not actually valid (for instance the value for the ports where not integers).

Drive-by fixes:
    - display the host/username/password info in example/management/run.go
    - add the GeoReplicated stuff as a parameter

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Is it really worth adding GeoReplicationEnabled as a parameter to NewCreateStorageServiceInputWithLocation? Go puts you on this slippery slope from "I want optional and named parameters for my function" to "I'll use a Parameter Object" to "I need a constructor for my Parameter Object" to "I want another parameter added to the constructor" to "my constructor needs a Parameter Object."

So unless we need to set the GeoReplicationEnabled all the time, or there is something fundamentally dangerous that requires an explicit choice, I'd just make NewCreateStorageServiceInputWithLocation initialize GeoReplicationEnabled to either "false" or "true," whichever makes more sense. Or if you do make it a parameter, then IMO at least the constructor should take a boolean so that the caller can ignore the weirdness in the field's type!

Revision history for this message
Raphaël Badin (rvb) wrote :

> Is it really worth adding GeoReplicationEnabled as a parameter to
> NewCreateStorageServiceInputWithLocation? Go puts you on this slippery slope
> from "I want optional and named parameters for my function" to "I'll use a
> Parameter Object" to "I need a constructor for my Parameter Object" to "I want
> another parameter added to the constructor" to "my constructor needs a
> Parameter Object."
>
> So unless we need to set the GeoReplicationEnabled all the time, or there is
> something fundamentally dangerous that requires an explicit choice, I'd just
> make NewCreateStorageServiceInputWithLocation initialize GeoReplicationEnabled
> to either "false" or "true," whichever makes more sense. Or if you do make it
> a parameter, then IMO at least the constructor should take a boolean so that
> the caller can ignore the weirdness in the field's type!

You've right, I forgot to say *why* I thought it was important: the geo replication has a very high cost (dixit Scott) and thus I thought that it would be better to force the user to explicitly set it.

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Also, why only retrieve deployment information in the optional "pause" bit? Isn't this something you might as well do anyway, as part of the test?

Arguably it's not useful if you're not pausing, but then again the same goes for creating the virtual machine in the first place. :) If shutdown fails, for example, you might still want to have a closer look at the instance, perhaps using the information you got from GetDeployment.

Finally, would it be worth adding a few words of how-and-why-this-works documentation to ConfigurationSet?

review: Approve
lp://staging/~rvb/gwacl/add-network-config updated
152. By Raphaël Badin

Review fixes.

153. By Raphaël Badin

Remove test data.

Revision history for this message
Raphaël Badin (rvb) wrote :

> Also, why only retrieve deployment information in the optional "pause" bit?
> Isn't this something you might as well do anyway, as part of the test?
>
> Arguably it's not useful if you're not pausing, but then again the same goes
> for creating the virtual machine in the first place. :) If shutdown fails,
> for example, you might still want to have a closer look at the instance,
> perhaps using the information you got from GetDeployment.

Very true, fixed.

> Finally, would it be worth adding a few words of how-and-why-this-works
> documentation to ConfigurationSet?

Done.

Thanks for the review!

Revision history for this message
Julian Edwards (julian-edwards) wrote :

On 02/07/13 20:44, Raphaël Badin wrote:
> I changed the DisableSSHPasswordAuthentication boolean into a string because the 'omitempty' parameter would remove the field altogether from the serialized XML when it's false. And we *have to* use 'omitempty' because, when the ConfigurationSet object will be of type NetworkConfiguration, we need that field to be omitted.

FWIW, this really is shouting that LinuxProvisioningConfiguration is not
the same as a generic ConfigurationSet at all ... which is why I did it
that way to start with.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: