Code review comment for lp:~rvb/gwacl/extract-imageos

Nice branch! I have a few niggles so it's needs-fixing for now, but nothing big.

95 // baseAPIVersion is the default Azure API version to use.
96 -const baseAPIVersion = "2012-03-01"
97 +const baseAPIVersion = "2013-03-01"

As I said on the review of jtv's branch, I think it's a bad idea to have a blanket default API version if different API calls can have different versions (there's no good default for that). If this is not the case for the management API then ignore this, otherwise I strongly urge you to think about placing explicit versions in each part of the code that does an Azure API call.

102 === modified file 'xmlobjects.go'

I can see why you did it, but I don't think the stuff you added here should go in this file. It was intended to be just a file of struct definitions; I think it would be better to make your changes in something like 'osimages.go'. In fact I think it's time to consider splitting up that xmlobjects.go file, it's getting rather unwieldy. But, later.

146 +// Less sort the images according to their 'PublishedDate'. Most recently published
147 +// first.

This description really confused me. I first thought s/sort/sorts/ but then having looked at what the function is actually doing, I find the claim that it's sorting even more confusing! I think this is what you need, including a change of function name:

// IsNewer returns true if the image at index i is newer than the one at index j, comparing by PublishedDate.
func (images *Images) IsNewer(i, j int) bool {

NB - I just read further down the code and realised to my horror that this name is part of the sort.Sort() contract isn't it? Sigh :/ In that case:

// Less returns true if the image at index i is newer than the one at index j, comparing by PublishedDate.
// This function is used by sort.Sort().
func (images *Images) Less(i, j int) bool {

148 +func (images *Images) Less(i, j int) bool {
149 + dateStringI := images.Images[i].PublishedDate
150 + dateI, err := time.Parse(time.RFC3339, dateStringI)
151 + if err != nil {
152 + panic(fmt.Errorf("Failed to parse image's 'PublishedDate': %s", dateStringI))
153 + }
154 + dateStringJ := images.Images[j].PublishedDate
155 + dateJ, err := time.Parse(time.RFC3339, dateStringJ)
156 + if err != nil {
157 + panic(fmt.Errorf("Failed to parse image's 'PublishedDate': %s", dateStringJ))

There is no catch for the panic statements here, and we should not leak the panic out of the library. Please return an error instead. You can use the exact same string, that's fine but I'd also include any context info about the image to help identify which one cannot be parsed.

164 +func (images *Images) GetLatestUbuntuImage(release string, location string) (*OSImage, error) {
165 + version, present := releaseVersions[release]
166 + if !present {
167 + return nil, fmt.Errorf("Unsupported release: %s", release)
168 + }

It's bad form to refer to released Ubuntu series by the development codename. The releaseVersions map is unnecessary, we should just expect people to pass "12.04", "12.10" etc as those are the official release names.

178 + sort.Sort(&matchingImages)
179 + if matchingImages.Len() == 0 {
180 + return nil, fmt.Errorf("No matching images found")
181 + }

You can probably switch these two statements. Also - Azure really returns images in an indeterminate order? :/

208 +func (image *OSImage) HasLocation(location string) bool {

I don't think this needs to be exported, it's only used internally.

209 + locations := strings.Split(image.Location, ";")
210 + for _, loc := range locations {
211 + if loc == location {
212 + return true
213 + }
214 + }
215 + return false
216 +}

Would strings.Contains() be sufficient here?

238 +func (suite *xmlSuite) TestOSImageHasLocation(c *C) {
239 + input := `
240 +<?xml version="1.0"?>
241 + <OSImage>
242 + <Category>Canonical</Category>
243 + <Label>Ubuntu Server 12.04.2 LTS</Label>
244 + <Location>East Asia;Southeast Asia;North Europe;West Europe;East US;West US</Location>
245 + <LogicalSizeInGB>30</LogicalSizeInGB>
246 + <Name>b39f27a8b8c64d52b05eac6a62ebad85__Ubuntu-12_04_2-LTS-amd64-server-20130415-en-us-30GB</Name>
247 + <OS>Linux</OS>
248 + <ImageFamily>Ubuntu Server 12.04 LTS</ImageFamily>
249 + <PublishedDate>2013-04-15T00:00:00Z</PublishedDate>
250 + <IsPremium>false</IsPremium>
251 + <PublisherName>Canonical</PublisherName>
252 + </OSImage>`
253 + image := &OSImage{}
254 + err := image.Deserialize([]byte(input))
255 + c.Assert(err, IsNil)

Do you need to test Deserialize here? In the other tests we just started defining the struct directly (you can also omit info you don't need) and I think it reads better. See xmlobjects_test.go/TestContainer's "expected" variable.

Same with the other tests you added.

377 + for _, test := range testValues {
378 + image, err := images.GetLatestUbuntuImage(test.series, test.location)
379 + c.Assert(err, DeepEquals, test.expectedError)
380 + if image != nil {
381 + c.Check(image.Label, Equals, test.expectedLabel)
382 + }

The Assert could be a Check so the loop doesn't terminate early and it can then compare all the results.