> 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 function myself, so I'm probably not the best person to ask.  I just saw that its documentation was broken and fixed it.


> +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.