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

Thanks for the review!

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

You're right, let's do this in another branch this.

> 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 {

This is indeed needed by the sort.Sort() contract. Doc fixed.

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

I cannot do that as Less() is part of the srot.Sort() contract so I've changed the caller to recover from panic errors "raised" from Less().

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

I'm really not sure about this:
- distro-info, which is the official source of release names, returns the codename as default. There is an option to get the full name but really, the codename seems to be the "canonical" name for a series. (http://paste.ubuntu.com/5652819/)
- we've always used codenames in MAAS
- Andres confirms that codenames are *the* "canonical" name for series

>
> 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? :/

It does not say, so I figured: better safe than sorry.

> 208 +func (image *OSImage) HasLocation(location string) bool {
>
> I don't think this needs to be exported, it's only used internally.

Good point, done.

> 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?

I think it's simpler to do it this way, because using strings.Contains("") would force you do deal manually with the separator. Imagine there is a region called "US East" and another "US East New York", if the only available location is "USE East New York" and you use Contain(), if you check for "US East" you will get a "false positive". Of course we could work around that but it becomes clumsy and I think the solution I proposed is simpler and safer.

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

Ok, I've refactored this test. I'd like to keep that last 2 tests as they are though because I used real-world XML and that's also part of the test.

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

Good point, done.