Merge lp:~julian-edwards/gwacl/affinity-group into lp:gwacl

[Julian Edwards]

Add CreateAffinityGroup, UpdateAffinityGroup and DeleteAffinityGroup management api calls.

It turns out that this is needed by vnets, so add all the bumf for AffinityGroup creation, update and deletion.

[Raphaël Badin (rvb)]

Looks good!

[0]

59 + var err error
60 + url := "affinitygroups/" + request.Name

and

78 + var err error
79 + url := "affinitygroups/" + request.Name

You need to use checkPathComponents() to make sure request.Name can be safely used in the url.

[1]

279 +}
280 func (suite *xmlSuite) TestDeployment(c *C) {

Missing an empty line here I think… did you run 'make format'?

[2]

You might want to do this in another branch or in here since it's really tiny but anyway, I'm mentioning it here so that we don't forget: NewCreateHostedServiceWithLocation() needs to be updated to take an affinity group name (AffinityGroup is already a field in the CreateHostedService struct). Then we need to update the call to NewCreateHostedServiceWithLocation() in example/management/run.go to take the name of the newly created affinity group. In this branch as it is now, you're creating the affinity group in example/management/run.go but the created hosted service is not linked to it.

[3]

36 +type CreateAffinityGroupRequest struct {
37 + CreateAffinityGroup *CreateAffinityGroup
38 +}

Not sure if it's really useful but I guess it's more coherent with the other calls to have this intermediate CreateAffinityGroupRequest structure…?

[4]

9 + affinityGroupName := gwacl.MakeRandomHostname("affinitygroup")

Why did you use MakeRandomHostname() here? AFAIK this is not a hostname is it? Hum, looking at names.go we might want to expose makeRandomIdentifier() and use that directly.

[5]

165 +//
166 +type CreateAffinityGroup struct {

Maybe include the link to the API documentation in the "docstring"…? Here and in a couple of other places.

review: Approve

[Julian Edwards (julian-edwards)]

Thank you for the review.

On 10/07/13 18:16, Raphaël Badin wrote:
> Review: Approve
>
> Looks good!
>
> [0]
>
> 59 + var err error
> 60 + url := "affinitygroups/" + request.Name
>
> and
>
> 78 + var err error
> 79 + url := "affinitygroups/" + request.Name
>
> You need to use checkPathComponents() to make sure request.Name can be safely used in the url.

Gnargh, I forgot. Again.

There has to be a better way of dealing with this.

>
> [1]
>
> 279 +}
> 280 func (suite *xmlSuite) TestDeployment(c *C) {
>
> Missing an empty line here I think… did you run 'make format'?

I did - maybe it's a merge error...

>
> [2]
>
> You might want to do this in another branch or in here since it's really tiny but anyway, I'm mentioning it here so that we don't forget: NewCreateHostedServiceWithLocation() needs to be updated to take an affinity group name (AffinityGroup is already a field in the CreateHostedService struct). Then we need to update the call to NewCreateHostedServiceWithLocation() in example/management/run.go to take the name of the newly created affinity group. In this branch as it is now, you're creating the affinity group in example/management/run.go but the created hosted service is not linked to it.

Separate branch, yes :)

>
> [3]
>
> 36 +type CreateAffinityGroupRequest struct {
> 37 + CreateAffinityGroup *CreateAffinityGroup
> 38 +}
>
> Not sure if it's really useful but I guess it's more coherent with the other calls to have this intermediate CreateAffinityGroupRequest structure…?

Consistency always wins. Besides, we may want to add more parameters
later as previously discussed.

>
> [4]
>
> 9 + affinityGroupName := gwacl.MakeRandomHostname("affinitygroup")
>
> Why did you use MakeRandomHostname() here? AFAIK this is not a hostname is it? Hum, looking at names.go we might want to expose makeRandomIdentifier() and use that directly.

Because it does exactly what I need. We can rename it as necessary I
suppose.

>
> [5]
>
> 165 +//
> 166 +type CreateAffinityGroup struct {
>
> Maybe include the link to the API documentation in the "docstring"…? Here and in a couple of other places.
>

Yes - despite me telling others to do the same thing, and mostly
remembering to do so myself, I always miss one!

Cheers.

[Gavin Panella (allenap)]

> [2]
>
> You might want to do this in another branch or in here since it's
> really tiny but anyway, I'm mentioning it here so that we don't
> forget: NewCreateHostedServiceWithLocation() needs to be updated to
> take an affinity group name (AffinityGroup is already a field in the
> CreateHostedService struct). Then we need to update the call to
> NewCreateHostedServiceWithLocation() in example/management/run.go to
> take the name of the newly created affinity group. In this branch
> as it is now, you're creating the affinity group in
> example/management/run.go but the created hosted service is not
> linked to it.

Perhaps we ought to have a NewCreateHostedServiceWithLocationParams
struct so that we can pass optional arguments into the constructor?
</black-humour>

I'm coming to the opinion that the NewBlah() pattern for constructors
is actually an antipattern. Calling conventions for Go's functions
have an impedance mismatch with constructing a struct directly, so it
always feels like hammering a square peg into the proverbial.

I'm wondering if something like the following might be better:

  type Thing struct {
      ...
  }

  func (t *Thing) Init() *Thing {
      // Initialise, e.g. set default fields if they're nil.
      ...
      return t
  }

  var thing *Thing = &Thing{...}.Init()

This still supports multiple constructors, any of which /could/ also
take args. The struct itself has to be exported for use by other
packages, which may be an undesirable side-effect in some cases.

[Julian Edwards (julian-edwards)]

Attempt to merge into lp:gwacl failed due to conflicts:

text conflict in management_base.go
text conflict in management_base_test.go
text conflict in xmlobjects_test.go

[Gavin Panella (allenap)]

> [2]
>
> You might want to do this in another branch or in here since it's
> really tiny but anyway, I'm mentioning it here so that we don't
> forget: NewCreateHostedServiceWithLocation() needs to be updated to
> take an affinity group name (AffinityGroup is already a field in the
> CreateHostedService struct).  Then we need to update the call to
> NewCreateHostedServiceWithLocation() in example/management/run.go to
> take the name of the newly created affinity group.  In this branch
> as it is now, you're creating the affinity group in
> example/management/run.go but the created hosted service is not
> linked to it.

Perhaps we ought to have a NewCreateHostedServiceWithLocationParams
struct so that we can pass optional arguments into the constructor?
</black-humour>

I'm coming to the opinion that the NewBlah() pattern for constructors
is actually an antipattern. Calling conventions for Go's functions
have an impedance mismatch with constructing a struct directly, so it
always feels like hammering a square peg into the proverbial.

I'm wondering if something like the following might be better:

  type Thing struct {
      ...
  }

  func (t *Thing) Init() *Thing {
      // Initialise, e.g. set default fields if they're nil.
      ...
      return t
  }

  var thing *Thing = &Thing{...}.Init()

This still supports multiple constructors, any of which /could/ also
take args. The struct itself has to be exported for use by other
packages, which may be an undesirable side-effect in some cases.

[Julian Edwards (julian-edwards)]

I too have felt uneasy about some of these "constructors" as it's just
as easy to set the fields directly.

The only time it's particularly useful is when you have the need for a
non-Go default value (like our XLMNS).

=== modified file 'example/management/run.go'
--- example/management/run.go	2013-07-09 20:17:57 +0000
+++ example/management/run.go	2013-07-10 09:03:28 +0000
@@ -87,6 +87,7 @@
 }
 
 func ExerciseHostedServicesAPI(api *gwacl.ManagementAPI) {
+    var err error
     networkConfig, err := api.GetNetworkConfiguration()
     checkError(err)
     if networkConfig == nil {
@@ -100,6 +101,21 @@
     location := "West US"
     release := "13.04"
 
+    affinityGroupName := gwacl.MakeRandomHostname("affinitygroup")
+    fmt.Printf("Creating an affinity group...")
+    cag := gwacl.NewCreateAffinityGroup(affinityGroupName, "affinity-label", "affinity-description", location)
+    err = api.CreateAffinityGroup(&gwacl.CreateAffinityGroupRequest{
+        CreateAffinityGroup: cag})
+    checkError(err)
+    fmt.Println("created %s", affinityGroupName)
+
+    defer func() {
+        fmt.Println("Deleting affinity group %s", affinityGroupName)
+        api.DeleteAffinityGroup(&gwacl.DeleteAffinityGroupRequest{
+            Name: affinityGroupName})
+        fmt.Println("Done deleting affinity group %s", affinityGroupName)
+    }()
+
     fmt.Printf("Getting OS Image for release '%s' and location '%s'...\n", release, location)
     images, err := api.ListOSImages()
     checkError(err)

=== modified file 'management_base.go'
--- management_base.go	2013-07-09 20:11:22 +0000
+++ management_base.go	2013-07-10 09:03:28 +0000
@@ -358,6 +358,62 @@
     return &role, nil
 }
 
+type CreateAffinityGroupRequest struct {
+    CreateAffinityGroup *CreateAffinityGroup
+}
+
+// CreateAffinityGroup sends a request to make a new affinity group.
+func (api *ManagementAPI) CreateAffinityGroup(request *CreateAffinityGroupRequest) error {
+    var err error
+    url := "affinitygroups"
+    body, err := request.CreateAffinityGroup.Serialize()
+    if err != nil {
+        return err
+    }
+    response, err := api.session.post(url, []byte(body), "application/xml")
+    return api.blockUntilCompleted(response)
+}
+
+type UpdateAffinityGroupRequest struct {
+    Name                string
+    UpdateAffinityGroup *UpdateAffinityGroup
+}
+
+// UpdateAffinityGroup sends a request to update the named affinity group.
+func (api *ManagementAPI) UpdateAffinityGroup(request *UpdateAffinityGroupRequest) error {
+    var err error
+    checkPathComponents(request.Name)
+    url := "affinitygroups/" + request.Name
+    body, err := request.UpdateAffinityGroup.Serialize()
+    if err != nil {
+        return err
+    }
+    response, err := api.session.put(url, []byte(body))
+    if err != nil {
+        return err
+    }
+    return api.blockUntilCompleted(response)
+}
+
+type DeleteAffinityGroupRequest struct {
+    Name string
+}
+
+// DeleteAffinityGroup requests a deletion of the named affinity group.
+func (api *ManagementAPI) DeleteAffinityGroup(request *DeleteAffinityGroupRequest) error {
+    var err error
+    checkPathComponents(request.Name)
+    url := "affinitygroups/" + request.Name
+    if err != nil {
+        return err
+    }
+    response, err := api.session.delete(url)
+    if err != nil {
+        return err
+    }
+    return api.blockUntilCompleted(response)
+}
+
 // GetNetworkConfiguration gets the network configuration for this
 // subscription. If there is no network configuration the configuration will
 // be nil.

=== modified file 'management_base_test.go'
--- management_base_test.go	2013-07-09 20:11:22 +0000
+++ management_base_test.go	2013-07-10 09:03:28 +0000
@@ -848,6 +848,65 @@
     c.Check(role.RoleName, Equals, "rolename")
 }
 
+func (suite *managementBaseAPISuite) TestCreateAffinityGroup(c *C) {
+    api := makeAPI(c)
+    cag := NewCreateAffinityGroup(
+        "name", "label", "description", "location")
+    request := CreateAffinityGroupRequest{
+        CreateAffinityGroup: cag}
+    fixedResponse := x509Response{
+        StatusCode: http.StatusCreated}
+    rigFixedResponseDispatcher(&fixedResponse)
+    recordedRequests := make([]*X509Request, 0)
+    rigRecordingDispatcher(&recordedRequests)
+
+    err := api.CreateAffinityGroup(&request)
+    c.Assert(err, IsNil)
+
+    expectedURL := AZURE_URL + api.session.subscriptionId + "/affinitygroups"
+    expectedBody, _ := cag.Serialize()
+    checkOneRequest(c, &recordedRequests, expectedURL, []byte(expectedBody), "POST")
+}
+
+func (suite *managementBaseAPISuite) TestUpdateAffinityGroup(c *C) {
+    api := makeAPI(c)
+    uag := NewUpdateAffinityGroup("label", "description")
+    request := UpdateAffinityGroupRequest{
+        Name:                "groupname",
+        UpdateAffinityGroup: uag}
+    fixedResponse := x509Response{
+        StatusCode: http.StatusCreated}
+    rigFixedResponseDispatcher(&fixedResponse)
+    recordedRequests := make([]*X509Request, 0)
+    rigRecordingDispatcher(&recordedRequests)
+
+    err := api.UpdateAffinityGroup(&request)
+    c.Assert(err, IsNil)
+
+    expectedURL := (AZURE_URL + api.session.subscriptionId +
+        "/affinitygroups/" + request.Name)
+    expectedBody, _ := uag.Serialize()
+    checkOneRequest(c, &recordedRequests, expectedURL, []byte(expectedBody), "PUT")
+}
+
+func (suite *managementBaseAPISuite) TestDeleteAffinityGroup(c *C) {
+    api := makeAPI(c)
+    request := DeleteAffinityGroupRequest{
+        Name: "groupname"}
+    fixedResponse := x509Response{
+        StatusCode: http.StatusCreated}
+    rigFixedResponseDispatcher(&fixedResponse)
+    recordedRequests := make([]*X509Request, 0)
+    rigRecordingDispatcher(&recordedRequests)
+
+    err := api.DeleteAffinityGroup(&request)
+    c.Assert(err, IsNil)
+
+    expectedURL := (AZURE_URL + api.session.subscriptionId +
+        "/affinitygroups/" + request.Name)
+    checkOneRequest(c, &recordedRequests, expectedURL, nil, "DELETE")
+}
+
 func makeNetworkConfiguration() *NetworkConfiguration {
     return &NetworkConfiguration{
         XMLNS: XMLNS_NC,

=== modified file 'xmlobjects.go'
--- xmlobjects.go	2013-07-10 08:33:49 +0000
+++ xmlobjects.go	2013-07-10 09:03:28 +0000
@@ -634,6 +634,54 @@
 }
 
 //
+// Affinity Group
+//
+
+// See http://msdn.microsoft.com/en-us/library/windowsazure/gg715317.aspx
+type CreateAffinityGroup struct {
+    XMLNS       string `xml:"xmlns,attr"`
+    Name        string `xml:"Name"`
+    Label       string `xml:"Label"` // Must be base64 encoded.
+    Description string `xml:"Description",omitempty`
+    Location    string `xml:"Location"` // Value comes from ListLocations.
+}
+
+func (c *CreateAffinityGroup) Serialize() (string, error) {
+    return toxml(c)
+}
+
+func NewCreateAffinityGroup(name, label, description, location string) *CreateAffinityGroup {
+    base64label := base64.StdEncoding.EncodeToString([]byte(label))
+    return &CreateAffinityGroup{
+        XMLNS:       XMLNS,
+        Name:        name,
+        Label:       base64label,
+        Description: description,
+        Location:    location,
+    }
+}
+
+// See http://msdn.microsoft.com/en-us/library/windowsazure/gg715316.aspx
+type UpdateAffinityGroup struct {
+    XMLNS       string `xml:"xmlns,attr"`
+    Label       string `xml:"Label"` // Must be base64 encoded.
+    Description string `xml:"Description",omitempty`
+}
+
+func (u *UpdateAffinityGroup) Serialize() (string, error) {
+    return toxml(u)
+}
+
+func NewUpdateAffinityGroup(label, description string) *UpdateAffinityGroup {
+    base64label := base64.StdEncoding.EncodeToString([]byte(label))
+    return &UpdateAffinityGroup{
+        XMLNS:       XMLNS,
+        Label:       base64label,
+        Description: description,
+    }
+}
+
+//
 // Storage Services bits
 //
 

=== modified file 'xmlobjects_test.go'
--- xmlobjects_test.go	2013-07-10 08:41:26 +0000
+++ xmlobjects_test.go	2013-07-10 09:03:28 +0000
@@ -363,6 +363,67 @@
     c.Assert(observed, Equals, expected)
 }
 
+func (suite *xmlSuite) TestCreateAffinityGroup(c *C) {
+    expected := dedent.Dedent(`
+        <CreateAffinityGroup xmlns="http://schemas.microsoft.com/windowsazure">
+          <Name>affinity-group-name</Name>
+          <Label>base64-encoded-affinity-group-label</Label>
+          <Description>affinity-group-description</Description>
+          <Location>location</Location>
+        </CreateAffinityGroup>`)
+
+    input := CreateAffinityGroup{
+        XMLNS:       XMLNS,
+        Name:        "affinity-group-name",
+        Label:       "base64-encoded-affinity-group-label",
+        Description: "affinity-group-description",
+        Location:    "location"}
+
+    observed, err := input.Serialize()
+    c.Assert(err, IsNil)
+    c.Assert(observed, Equals, expected)
+}
+
+func (suite *xmlSuite) TestNewCreateAffinityGroup(c *C) {
+    name := "name"
+    label := "label"
+    description := "description"
+    location := "location"
+    ag := NewCreateAffinityGroup(name, label, description, location)
+    base64label := base64.StdEncoding.EncodeToString([]byte(label))
+    c.Check(ag.XMLNS, Equals, XMLNS)
+    c.Check(ag.Name, Equals, name)
+    c.Check(ag.Label, Equals, base64label)
+    c.Check(ag.Description, Equals, description)
+    c.Check(ag.Location, Equals, location)
+}
+
+func (suite *xmlSuite) TestUpdateAffinityGroup(c *C) {
+    expected := dedent.Dedent(`
+        <UpdateAffinityGroup xmlns="http://schemas.microsoft.com/windowsazure">
+          <Label>base64-encoded-affinity-group-label</Label>
+          <Description>affinity-group-description</Description>
+        </UpdateAffinityGroup>`)
+    input := UpdateAffinityGroup{
+        XMLNS:       XMLNS,
+        Label:       "base64-encoded-affinity-group-label",
+        Description: "affinity-group-description"}
+
+    observed, err := input.Serialize()
+    c.Assert(err, IsNil)
+    c.Assert(observed, Equals, expected)
+}
+
+func (suite *xmlSuite) TestNewUpdateAffinityGroup(c *C) {
+    label := "label"
+    description := "description"
+    ag := NewUpdateAffinityGroup(label, description)
+    base64label := base64.StdEncoding.EncodeToString([]byte(label))
+    c.Check(ag.XMLNS, Equals, XMLNS)
+    c.Check(ag.Label, Equals, base64label)
+    c.Check(ag.Description, Equals, description)
+}
+
 func (suite *xmlSuite) TestNetworkConfigurationDeserialize(c *C) {
     // Template from
     // http://msdn.microsoft.com/en-us/library/windowsazure/jj157196.aspx