Code review comment for lp:~jtv/gwacl/list-images

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.