Merge lp:~jtv/gwacl/list-images into lp:gwacl

[Jeroen T. Vermeulen]

Support listing of OS images.

I kept XML annotations minimal: we've been listing the XML tag names but they're optional where identical to the names of their respective struct fields. I continued our tradition of upper-casing acronyms in the struct names even if Microsoft sometimes (but not always!) writes them in mixed case. I may write up a separate branch ensuring consistency, so that we'll always capitalize these fields in a predictable way — even if we're not actually consistent with Microsoft's spelling.

For testing I went with a different approach than we've been doing: one test verifies that the function makes the expected request, and another verifies the call's outcome. I think that ends up being much easier to read than a flurry of checks at the end to assert very different aspects of what happened. Concentrating the tests into big "check everything the way I usually do" functions reduces check size but makes it harder to follow what is being checked where, or how to add new checks. It also harms flexibility.

What we see at work here is the old "big tests attract more code" antipattern. It's caused by expensive test setup, and by a lack of dependency injection forcing the unit-testing into integration-style tests. Go forces these styles on us, but I reduced test setup *per test* slightly in that now I only need to rig one fake dispatcher per test, with each serving a more specific goal.

Jeroen

[Julian Edwards (julian-edwards)]

Nice change! There's quite a few niggling bits to change I think so I marked it needs fixing so I can take another look later.

8 +// ListOSImages retrieves the list of available operating system disk images
9 +// from the Azure management API.
10 +// Images are returned in the order in which Azure lists them.
11 +func (api *ManagementAPI) ListOSImages() ([]OSImage, error)

Can you please get into the habit of adding the URL to the Azure API reference in the comments for these exported API methods. It'll really help newcomers understand what it's doing, and help maintainers fix bugs quicker!

45 +func (suite *managementAPISuite) TestListOSImagesReturnsImages(c *C) {
46 + const (
47 + logicalSize = 199.0
48 + label = "osimagelabel"
49 + mediaLink = "http://storage.example.com/12345"
50 + name = "imagename"
51 + eula = "firstborn.txt"
52 + description = "Description."
53 + )

I don't find this const section useful at all, FWIW. I'd just throw the values into the string formatting args for Sprintf. At least if you're going to use consts/variables, I'd use MakeRandomString() and friends.

80 + c.Check(osimage.LogicalSizeInGB, Equals, float32(logicalSize))
81 + c.Check(osimage.Label, Equals, label)
82 + c.Check(osimage.MediaLink, Equals, mediaLink)
83 + c.Check(osimage.Name, Equals, name)
84 + c.Check(osimage.OS, Equals, "Linux")
85 + c.Check(osimage.EULA, Equals, eula)
86 + c.Check(osimage.Description, Equals, description)

You can do this in a single check if you make a single "expected" struct and do a DeepEquals, as is done for some of the storage tests. The code reads better that way and you also get a single failure listing all differences at once.

103 + c.Assert(len(listing), Equals, 3)
104 + c.Check(listing[0].Name, Equals, "image2")
105 + c.Check(listing[1].Name, Equals, "image1")
106 + c.Check(listing[2].Name, Equals, "image3")

Same here.

122 +// rigPreparedResponseDispatcher sets up a request dispatcher that returns,
123 +// for each consecutive request, the next of a series of prepared responses.

This reminds me of Mocker. Out of interest, do you prefer its record/replay mocking style over the simpler Mock?

+type OSImage struct {
154 + AffinityGroup string `xml:",omitempty"`
155 + Category string
156 + Label string
[snip]

You said you were leaving out the xml tags, but please don't do that. Gavin and I talked about this in Brisbane too, but we decided that we need a strong separation between external API names and internal code struct names - if the Azure API changes then it would either mean code changes everywhere or adding the tag anyway. Adding the tags also reinforces to the code reader that its an XML-filled property.

review: Needs Fixing

[lp:~jtv/gwacl/list-images]

92. By Jeroen T. Vermeulen on 2013-04-02

Review changes.

[Jeroen T. Vermeulen (jtv)]

> Nice change! There's quite a few niggling bits to change I think so I marked
> it needs fixing so I can take another look later.
>
> 8 +// ListOSImages retrieves the list of available operating system disk
> images
> 9 +// from the Azure management API.
> 10 +// Images are returned in the order in which Azure lists them.
> 11 +func (api *ManagementAPI) ListOSImages() ([]OSImage, error)
>
> Can you please get into the habit of adding the URL to the Azure API reference
> in the comments for these exported API methods. It'll really help newcomers
> understand what it's doing, and help maintainers fix bugs quicker!

Done.

> 45 +func (suite *managementAPISuite) TestListOSImagesReturnsImages(c *C)
> {
> 46 + const (
> 47 + logicalSize = 199.0
> 48 + label = "osimagelabel"
> 49 + mediaLink = "http://storage.example.com/12345"
> 50 + name = "imagename"
> 51 + eula = "firstborn.txt"
> 52 + description = "Description."
> 53 + )
>
> I don't find this const section useful at all, FWIW. I'd just throw the
> values into the string formatting args for Sprintf. At least if you're going
> to use consts/variables, I'd use MakeRandomString() and friends.

Well the point is of course, as you saw right after writing this, that they had to be repeated exactly.

> 80 + c.Check(osimage.LogicalSizeInGB, Equals, float32(logicalSize))
> 81 + c.Check(osimage.Label, Equals, label)
> 82 + c.Check(osimage.MediaLink, Equals, mediaLink)
> 83 + c.Check(osimage.Name, Equals, name)
> 84 + c.Check(osimage.OS, Equals, "Linux")
> 85 + c.Check(osimage.EULA, Equals, eula)
> 86 + c.Check(osimage.Description, Equals, description)
>
> You can do this in a single check if you make a single "expected" struct and
> do a DeepEquals, as is done for some of the storage tests. The code reads
> better that way and you also get a single failure listing all differences at
> once.

Done.

> 103 + c.Assert(len(listing), Equals, 3)
> 104 + c.Check(listing[0].Name, Equals, "image2")
> 105 + c.Check(listing[1].Name, Equals, "image1")
> 106 + c.Check(listing[2].Name, Equals, "image3")
>
> Same here.

Can't DeepEquals the objects here, or the error output will be completely impossible to figure out. After all what's in the objects really doesn't matter — only the ordering does. So instead I created one array with the desired names and another with the received names, and compared those.

Extracting an array of Name attributes from the array of objects does make the code long and cumbersome, of course. But such is the price of Go: don't struggle, you'll just sink faster. The more generic and data-driven I make this test, the worse it gets.

> 122 +// rigPreparedResponseDispatcher sets up a request dispatcher that
> returns,
> 123 +// for each consecutive request, the next of a series of prepared
> responses.
>
> This reminds me of Mocker. Out of interest, do you prefer its record/replay
> mocking style over the simpler Mock?

I don't think I've ever used this...

[Julian Edwards (julian-edwards)]

On Tuesday 02 Apr 2013 03:25:27 you wrote:
> Well the point is of course, as you saw right after writing this, that they
> had to be repeated exactly.
> > 80 + c.Check(osimage.LogicalSizeInGB, Equals,
> > float32(logicalSize))
> > 81 + c.Check(osimage.Label, Equals, label)
> > 82 + c.Check(osimage.MediaLink, Equals, mediaLink)
> > 83 + c.Check(osimage.Name, Equals, name)
> > 84 + c.Check(osimage.OS, Equals, "Linux")
> > 85 + c.Check(osimage.EULA, Equals, eula)
> > 86 + c.Check(osimage.Description, Equals, description)
> >
> > You can do this in a single check if you make a single "expected" struct
> > and do a DeepEquals, as is done for some of the storage tests. The code
> > reads better that way and you also get a single failure listing all
> > differences at once.
>
> Done.

What you've done is loads better now, thanks :)

>
> > 103 + c.Assert(len(listing), Equals, 3)
> > 104 + c.Check(listing[0].Name, Equals, "image2")
> > 105 + c.Check(listing[1].Name, Equals, "image1")
> > 106 + c.Check(listing[2].Name, Equals, "image3")
> >
> > Same here.
>
> Can't DeepEquals the objects here, or the error output will be completely
> impossible to figure out. After all what's in the objects really doesn't
> matter — only the ordering does. So instead I created one array with the
> desired names and another with the received names, and compared those.

Oh, arse :/

>
> Extracting an array of Name attributes from the array of objects does make
> the code long and cumbersome, of course. But such is the price of Go:
> don't struggle, you'll just sink faster. The more generic and data-driven
> I make this test, the worse it gets.

:(

Our best practices are much harder to implement in Go than we are used to with
Python.

> > 122 +// rigPreparedResponseDispatcher sets up a request dispatcher
> > that
> > returns,
> > 123 +// for each consecutive request, the next of a series of prepared
> > responses.
> >
> > This reminds me of Mocker. Out of interest, do you prefer its
> > record/replay mocking style over the simpler Mock?
>
> I don't think I've ever used this function myself, so I'm probably not the
> best person to ask. I just saw that its documentation was broken and fixed
> it.

Ok :)

> > +type OSImage struct {
> > 154 + AffinityGroup string `xml:",omitempty"`
> > 155 + Category string
> > 156 + Label string
> > [snip]
> >
> > You said you were leaving out the xml tags, but please don't do that.
> > Gavin and I talked about this in Brisbane too, but we decided that we
> > need a strong separation between external API names and internal code
> > struct names - if the Azure API changes then it would either mean code
> > changes everywhere or adding the tag anyway. Adding the tags also
> > reinforces to the code reader that its an XML-filled property.
>
> Okay, I put them in. A minor disadvantage is that this obscures the
> differences in capitalization between gwacl and Azure.

It does?

Thanks for the changes, go ahead and land!

[Julian Edwards (julian-edwards)]

review: Approve

=== modified file 'managementapi.go'
--- managementapi.go	2013-03-29 16:03:54 +0000
+++ managementapi.go	2013-04-02 03:19:21 +0000
@@ -65,6 +65,20 @@
     return nil
 }
 
+// ListOSImages retrieves the list of available operating system disk images
+// from the Azure management API.
+// Images are returned in the order in which Azure lists them.
+// http://msdn.microsoft.com/en-us/library/windowsazure/jj157191.aspx
+func (api *ManagementAPI) ListOSImages() ([]OSImage, error) {
+    response, err := api.session.get("services/images")
+    if err != nil {
+        return nil, err
+    }
+    images := Images{}
+    err = xml.Unmarshal(response.Body, &images)
+    return images.Images, err
+}
+
 // ListHostedServiceDescriptors loads a list of HostedServiceDescriptor objects from the
 // Azure management API. 
 // HostedServiceDescriptor objects contains a small subset of the fields present in
 
=== modified file 'managementapi_test.go'
--- managementapi_test.go	2013-03-29 16:03:54 +0000
+++ managementapi_test.go	2013-04-02 03:19:21 +0000
@@ -191,6 +191,89 @@
     c.Check(err, ErrorMatches, ".*no operation header.*")
 }
 
+func (suite *managementAPISuite) TestListOSImagesRequestsListing(c *C) {
+    api := makeAPI(c)
+    rigFixedResponseDispatcher(&x509Response{StatusCode: http.StatusOK, Body: []byte("<Images></Images>")})
+    requests := make([]*x509Request, 0)
+    rigRecordingDispatcher(&requests)
+
+    _, err := api.ListOSImages()
+    c.Assert(err, IsNil)
+
+    c.Assert(len(requests), Equals, 1)
+    c.Check(requests[0].URL, Equals, api.session.composeURL("services/images"))
+}
+
+func (suite *managementAPISuite) TestListOSImagesReturnsImages(c *C) {
+    expectedImage := OSImage{
+        LogicalSizeInGB:   199.0,
+        Label:             MakeRandomString(10),
+        MediaLink:         "http://storage.example.com/" + MakeRandomString(10),
+        Name:              MakeRandomString(10),
+        OS:                "Linux",
+        EULA:              "http://eula.example.com/" + MakeRandomString(10),
+        Description:       MakeRandomString(10),
+        RecommendedVMSize: "Medium",
+    }
+    body := fmt.Sprintf(`
+        <Images>
+            <OSImage>
+                <LogicalSizeInGB>%f</LogicalSizeInGB>
+                <Label>%s</Label>
+                <MediaLink>%s</MediaLink>
+                <Name>%s</Name>
+                <OS>%s</OS>
+                <Eula>%s</Eula>
+                <Description>%s</Description>
+                <RecommendedVMSize>%s</RecommendedVMSize>
+            </OSImage>
+        </Images>
+    `,
+        expectedImage.LogicalSizeInGB, expectedImage.Label,
+        expectedImage.MediaLink, expectedImage.Name, expectedImage.OS,
+        expectedImage.EULA, expectedImage.Description,
+        expectedImage.RecommendedVMSize)
+    api := makeAPI(c)
+    rigFixedResponseDispatcher(&x509Response{
+        StatusCode: http.StatusOK,
+        Body:       []byte(body),
+    })
+
+    listing, err := api.ListOSImages()
+    c.Assert(err, IsNil)
+
+    c.Assert(len(listing), Equals, 1)
+    c.Check(listing[0], DeepEquals, expectedImage)
+}
+
+func (suite *managementAPISuite) TestListOSImagesPreservesOrdering(c *C) {
+    imageNames := []string{
+        MakeRandomString(5),
+        MakeRandomString(5),
+        MakeRandomString(5),
+    }
+    body := fmt.Sprintf(`
+            <Images>
+                <OSImage><Name>%s</Name></OSImage>
+                <OSImage><Name>%s</Name></OSImage>
+                <OSImage><Name>%s</Name></OSImage>
+            </Images>
+        `,
+        imageNames[0], imageNames[1], imageNames[2])
+    api := makeAPI(c)
+    rigFixedResponseDispatcher(&x509Response{StatusCode: http.StatusOK, Body: []byte(body)})
+
+    listing, err := api.ListOSImages()
+    c.Assert(err, IsNil)
+
+    c.Assert(len(listing), Equals, 3)
+    receivedNames := make([]string, 3)
+    for index := range listing {
+        receivedNames[index] = listing[index].Name
+    }
+    c.Check(receivedNames, DeepEquals, imageNames)
+}
+
 func (suite *managementAPISuite) TestListHostedServiceDescriptors(c *C) {
     api := makeAPI(c)
     fixedResponse := x509Response{
 
=== modified file 'test_helpers.go'
--- test_helpers.go	2013-03-29 01:27:37 +0000
+++ test_helpers.go	2013-04-02 03:19:21 +0000
@@ -217,8 +217,8 @@
     errorObject error
 }
 
-// rigPreparedResponseDispatcher sets up a request dispatcher that all the provided
-// reponses, one after the other each time the dispatcher is called.
+// rigPreparedResponseDispatcher sets up a request dispatcher that returns,
+// for each consecutive request, the next of a series of prepared responses.
 func rigPreparedResponseDispatcher(responses []DispatcherResponse) {
     index := 0
     _X509Dispatcher = func(*x509Session, *x509Request) (*x509Response, error) {
 
=== modified file 'xmlobjects.go'
--- xmlobjects.go	2013-03-29 10:23:07 +0000
+++ xmlobjects.go	2013-04-02 03:19:21 +0000
@@ -74,6 +74,48 @@
 }
 
 //
+// Images bits
+//
+
+// Images is a series of OSImages.
+type Images struct {
+    Images []OSImage `xml:"OSImage"`
+}
+
+func (i *Images) Deserialize(data []byte) error {
+    return xml.Unmarshal(data, i)
+}
+
+//
+// OSImage bits
+//
+
+// OSImage represents a disk image containing an operating system.
+// Confusingly, the Azure API documentation also calls it a VM image.
+type OSImage struct {
+    AffinityGroup     string  `xml:"AffinityGroup,omitempty"`
+    Category          string  `xml:"Category"`
+    Label             string  `xml:"Label"`
+    Location          string  `xml:"Location,omitempty"`
+    LogicalSizeInGB   float32 `xml:"LogicalSizeInGB"`
+    MediaLink         string  `xml:"MediaLink"`
+    Name              string  `xml:"Name"`
+    OS                string  `xml:"OS"`
+    EULA              string  `xml:"Eula,omitempty"`
+    Description       string  `xml:"Description,omitempty"`
+    ImageFamily       string  `xml:"ImageFamily,omitempty"`
+    PublishedDate     string  `xml:"PublishedDate,omitempty"`
+    IsPremium         string  `xml:"IsPremium,omitempty"` // Reserved for future use.  Seriously.  In XML.
+    PrivacyURI        string  `xml:"PrivacyUri,omitempty"`
+    RecommendedVMSize string  `xml:"RecommendedVMSize,omitempty"`
+    PublisherName     string  `xml:"PublisherName"`
+}
+
+func (i *OSImage) Deserialize(data []byte) error {
+    return xml.Unmarshal(data, i)
+}
+
+//
 // OSVirtualHardDisk bits
 //