Go Windows Azure Client Library

Merge lp:~rvb/gwacl/extract-imageos into lp:gwacl

extract-imageos
Merge into trunk

Proposed by Raphaël Badin on 2013-04-26

Status:	Merged
Approved by:	Raphaël Badin on 2013-05-15
Approved revision:	102
Merged at revision:	105
Proposed branch:	lp:~rvb/gwacl/extract-imageos
Merge into:	lp:gwacl
Diff against target:	364 lines (+229/-16) 6 files modified example/management/run.go (+14/-3) managementapi.go (+2/-2) managementapi_test.go (+5/-5) x509dispatcher.go (+1/-1) xmlobjects.go (+78/-5) xmlobjects_test.go (+129/-0)
To merge this branch:	bzr merge lp:~rvb/gwacl/extract-imageos
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Julian Edwards (community)		2013-04-26	Approve on 2013-05-13
Review via email: mp+161171@code.launchpad.net

Commit message

Add method to extract the OS image for a particular series.

Description of the change

The main change in this branch is to add the method GetLatestUbuntuImage() used to extract, from the list of available OS Images, the one corresponding to a particular series and available in a particular location.

Note that I bumped the api version to get the new variables on the OS Images objects (in particular, PublishedDate was not available in the old version).

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2013-04-29:

Download full text (5.2 KiB)

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

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.

review: Needs Fixing

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2013-04-29:

I forgot to mention, thank you for adding context to errors instead of blindly propagating them up the stack. This is something that we should always be doing so that errors can be traced to source, and not just a "wtf" moment.

Revision history for this message

Raphaël Badin (rvb) wrote on 2013-05-10:

Download full text (6.0 KiB)

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

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.

lp:~rvb/gwacl/extract-imageos updated on 2013-05-10

101. By Raphaël Badin on 2013-05-10: Review fixes.

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2013-05-13:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

review: approve

Hi,

I still want the release name thing fixed (see below), but nothing
else is of concern so please feel free to land it once that's addressed.

On 11/05/13 09:47, Raphaël Badin wrote:
> Thanks for the review!

My pleasure.

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

No problem.

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

Ah jeez, ok! So Go does have exceptions after all :)

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

distro-info is intended for use where things require the code name,
such as in a sources.list.

> - Andres confirms that codenames are *the* "canonical" name for
> series

Andres is wrong then. They are used internally, yes, but not
externally. Externally, the release name is always the NN.NN form.

Before an Ubuntu series is released it is indeed always called by its
code name. However, once released it is called by its release name,
such as "12.04". This is reflected in how the Launchpad UI works in
the Soyuz pages, of which I have plenty of experience :)

I am happy to compromise and make the function take either form, but
we should never rely entirely on internal release names, and if MAAS
is doing that we need to fix MAAS as well.

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

Wise :)

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlGQP7QACgkQWhGlTF8G/HdVlQCdHp7AyUzyUsoDTsCD6MvxuD3V
IEoAnAoGk/kGYNcxcwhjw9Kk3aBJKscv
=C7Yk
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

review: approve

Hi,

I still want the release name thing fixed (see below), but nothing
else is of concern so please feel free to land it once that's addressed.

On 11/05/13 09:47, Raphaël Badin wrote:
> Thanks for the review!

My pleasure.

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

No problem.

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

Ah jeez, ok!  So Go does have exceptions after all :)

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

distro-info is intended for use where things require the code name,
such as in a sources.list.

> - Andres confirms that codenames are *the* "canonical" name for
> series

Andres is wrong then.  They are used internally, yes, but not
externally.  Externally, the release name is always the NN.NN form.

Before an Ubuntu series is released it is indeed always called by its
code name.  However, once released it is called by its release name,
such as "12.04".  This is reflected in how the Launchpad UI works in
the Soyuz pages, of which I have plenty of experience :)

I am happy to compromise and make the function take either form, but
we should never rely entirely on internal release names, and if MAAS
is doing that we need to fix MAAS as well.

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

Wise :)

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlGQP7QACgkQWhGlTF8G/HdVlQCdHp7AyUzyUsoDTsCD6MvxuD3V
IEoAnAoGk/kGYNcxcwhjw9Kk3aBJKscv
=C7Yk
-----END PGP SIGNATURE-----

review: Approve

lp:~rvb/gwacl/extract-imageos updated on 2013-05-15

102. By Raphaël Badin on 2013-05-15: Review fixes (2).

Revision history for this message

Raphaël Badin (rvb) wrote on 2013-05-15:

> I still want the release name thing fixed (see below), but nothing
> else is of concern so please feel free to land it once that's addressed.

All right. I've fixed the release name thing as advised but I still think we need to have a look at that because MAAS and Juju use the codename everywhere (I /think/).

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2013-05-15:

On Wednesday 15 May 2013 07:43:26 you wrote:
> > I still want the release name thing fixed (see below), but nothing
> > else is of concern so please feel free to land it once that's addressed.
>
> All right. I've fixed the release name thing as advised but I still think
> we need to have a look at that because MAAS and Juju use the codename
> everywhere (I /think/).

I agree.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

GWACL Hackers

Raphaël Badin

 === modified file 'example/management/run.go'
 --- example/management/run.go	2013-04-25 15:06:21 +0000
 +++ example/management/run.go	2013-05-15 07:42:26 +0000
@@ -85,11 +85,23 @@
+ }
  func ExerciseHostedServicesAPI(api *gwacl.ManagementAPI) {
++    location := "East US"
++    series := "precise"
++
++    fmt.Printf("Getting OS Image for series '%s' and location '%s'...\n", series, location)
++    images, err := api.ListOSImages()
++    checkError(err)
++    preciseImage, err := images.GetLatestUbuntuImage(series, location)
++    checkError(err)
++    sourceImageName := preciseImage.Name
++    fmt.Printf("Got image named '%s'\n", sourceImageName)
++    fmt.Println("Done getting OS Image\n")
++
      // A hosted-service name must be between 3 and 24 characters long.
      hostServiceName := makeRandomIdentifier("gwacl", 24)
      fmt.Printf("Creating a hosted service: '%s'...\n", hostServiceName)
--    createHostedService := gwacl.NewCreateHostedServiceWithLocation(hostServiceName, "testLabel", "East US")
--    err := api.AddHostedService(createHostedService)
++    createHostedService := gwacl.NewCreateHostedServiceWithLocation(hostServiceName, "testLabel", location)
++    err = api.AddHostedService(createHostedService)
      checkError(err)
      fmt.Println("Done creating a hosted service\n")
@@ -120,7 +132,6 @@
      fmt.Println("Done requesting storage account\n")
      mediaLink := fmt.Sprintf("http://%s.blob.core.windows.net/vhds/%s.vhd", storageAccount, vhdName)
--    sourceImageName := "b39f27a8b8c64d52b05eac6a62ebad85__Ubuntu-12_04_2-LTS-amd64-server-20130325-en-us-30GB"
      diskName := makeRandomIdentifier("gwacldisk", 16)
      diskLabel := makeRandomIdentifier("gwacl", 64)
      OSVirtualHardDisk := gwacl.NewOSVirtualHardDisk("", diskLabel, diskName, mediaLink, sourceImageName)
 === modified file 'managementapi.go'
 --- managementapi.go	2013-04-16 13:29:25 +0000
 +++ managementapi.go	2013-05-15 07:42:26 +0000
@@ -96,14 +96,14 @@
  // 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) {
++func (api *ManagementAPI) ListOSImages() (*Images, 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
++    return &images, err
+ }
  // ListHostedServiceDescriptors loads a list of HostedServiceDescriptor objects from the
 === modified file 'managementapi_test.go'
 --- managementapi_test.go	2013-04-16 13:09:54 +0000
 +++ managementapi_test.go	2013-05-15 07:42:26 +0000
@@ -318,8 +318,8 @@
      listing, err := api.ListOSImages()
      c.Assert(err, IsNil)
--    c.Assert(len(listing), Equals, 1)
--    c.Check(listing[0], DeepEquals, expectedImage)
++    c.Assert(len(listing.Images), Equals, 1)
++    c.Check(listing.Images[0], DeepEquals, expectedImage)
+ }
  func (suite *managementAPISuite) TestListOSImagesPreservesOrdering(c *C) {
@@ -342,10 +342,10 @@
      listing, err := api.ListOSImages()
      c.Assert(err, IsNil)
--    c.Assert(len(listing), Equals, 3)
++    c.Assert(len(listing.Images), Equals, 3)
      receivedNames := make([]string, 3)
--    for index := range listing {
--        receivedNames[index] = listing[index].Name
++    for index := range listing.Images {
++        receivedNames[index] = listing.Images[index].Name
+     }
      c.Check(receivedNames, DeepEquals, imageNames)
+ }
 === modified file 'x509dispatcher.go'
 --- x509dispatcher.go	2013-04-22 16:20:00 +0000
 +++ x509dispatcher.go	2013-05-15 07:42:26 +0000
@@ -26,7 +26,7 @@
+ }
  // baseAPIVersion is the default Azure API version to use.
--const baseAPIVersion = "2012-03-01"
++const baseAPIVersion = "2013-03-01"
  // newX509RequestGET initializes an x509Request for a GET.  You may still need
  // to set further values.
 === modified file 'xmlobjects.go'
 --- xmlobjects.go	2013-04-29 07:19:57 +0000
 +++ xmlobjects.go	2013-05-15 07:42:26 +0000
@@ -3,9 +3,15 @@
  package gwacl
--import "encoding/xml"
--import "encoding/base64"
--import "fmt"
++import (
++    "encoding/base64"
++    "encoding/xml"
++    "fmt"
++    "regexp"
++    "sort"
++    "strings"
++    "time"
++)
  // It's impossible to have any kind of common method inherited into all the
  // various serializable objects because the receiver of the method is the
@@ -86,6 +92,63 @@
      return xml.Unmarshal(data, i)
+ }
++var canonicalPublisherName = "Canonical"
++var imageFamilyFormatRegexp = "^Ubuntu Server %s.*$"
++
++func (images *Images) Len() int {
++    return len(images.Images)
++}
++
++func (images *Images) Swap(i, j int) {
++    images.Images[i], images.Images[j] = images.Images[j], images.Images[i]
++}
++
++// 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 {
++    // We need to implement the sort interface so Less cannot return an error.  We panic if
++    // one of the dates cannot be parse and the calling method will recover this.
++    dateStringI := images.Images[i].PublishedDate
++    dateI, err := time.Parse(time.RFC3339, dateStringI)
++    if err != nil {
++        panic(fmt.Errorf("Failed to parse image's 'PublishedDate': %s", dateStringI))
++    }
++    dateStringJ := images.Images[j].PublishedDate
++    dateJ, err := time.Parse(time.RFC3339, dateStringJ)
++    if err != nil {
++        panic(fmt.Errorf("Failed to parse image's 'PublishedDate': %s", dateStringJ))
++    }
++    return dateI.After(dateJ)
++}
++
++// GetLatestUbuntuImage returns the most recent available OSImage, for the given release name
++// and location.
++func (images *Images) GetLatestUbuntuImage(releaseName string, location string) (image *OSImage, err error) {
++    // The Less method defined above can panic if one of the published dates cannot be parsed,
++    // this code recovers from that and transforms that into an error.
++    defer func() {
++        if recoveredErr := recover(); recoveredErr != nil {
++            image = nil
++            err = recoveredErr.(error)
++        }
++    }()
++    matcherRegexp := regexp.MustCompile(fmt.Sprintf(imageFamilyFormatRegexp, releaseName))
++    matchingImages := Images{}
++    for _, image := range images.Images {
++        if image.PublisherName == canonicalPublisherName &&
++            matcherRegexp.MatchString(image.ImageFamily) &&
++            image.hasLocation(location) {
++            matchingImages.Images = append(matchingImages.Images, image)
++        }
++    }
++    if matchingImages.Len() == 0 {
++        return nil, fmt.Errorf("No matching images found")
++    }
++    sort.Sort(&matchingImages)
++    return &matchingImages.Images[0], nil
++}
++
  //
  // OSImage bits
  //
@@ -96,7 +159,7 @@
      AffinityGroup     string  `xml:"AffinityGroup,omitempty"`
      Category          string  `xml:"Category"`
      Label             string  `xml:"Label"`
--    Location          string  `xml:"Location,omitempty"`
++    Location          string  `xml:"Location"`
      LogicalSizeInGB   float32 `xml:"LogicalSizeInGB"`
      MediaLink         string  `xml:"MediaLink"`
      Name              string  `xml:"Name"`
@@ -105,12 +168,22 @@
      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.
++    IsPremium         string  `xml:"IsPremium,omitempty"`
      PrivacyURI        string  `xml:"PrivacyUri,omitempty"`
      RecommendedVMSize string  `xml:"RecommendedVMSize,omitempty"`
      PublisherName     string  `xml:"PublisherName"`
+ }
++func (image *OSImage) hasLocation(location string) bool {
++    locations := strings.Split(image.Location, ";")
++    for _, loc := range locations {
++        if loc == location {
++            return true
++        }
++    }
++    return false
++}
++
  func (i *OSImage) Deserialize(data []byte) error {
      return xml.Unmarshal(data, i)
+ }
 === modified file 'xmlobjects_test.go'
 --- xmlobjects_test.go	2013-04-29 07:19:57 +0000
 +++ xmlobjects_test.go	2013-05-15 07:42:26 +0000
@@ -9,6 +9,7 @@
      "fmt"
      . "launchpad.net/gocheck"
      "launchpad.net/gwacl/dedent"
++    "sort"
+ )
  type xmlSuite struct{}
@@ -744,3 +745,131 @@
      c.Assert(err, IsNil)
      c.Check(string(xml), Equals, expectedXML)
+ }
++
++func (suite *xmlSuite) TestOSImageHasLocation(c *C) {
++    image := &OSImage{
++        Location: "East Asia;Southeast Asia;North Europe;West Europe;East US;West US",
++    }
++
++    var testValues = []struct {
++        location       string
++        expectedResult bool
++    }{
++        {"East Asia", true},
++        {"West US", true},
++        {"Unknown location", false},
++    }
++    for _, test := range testValues {
++        c.Check(image.hasLocation(test.location), Equals, test.expectedResult)
++    }
++}
++
++func (suite *xmlSuite) TestSortImages(c *C) {
++    input := `
++<?xml version="1.0"?>
++<Images xmlns="http://schemas.microsoft.com/windowsazure" xmlns:i="http://www.w3.org/2001/XMLSchema-instance">
++  <OSImage>
++    <Label>Label 1</Label>
++    <PublishedDate>2012-04-25T00:00:00Z</PublishedDate>
++  </OSImage>
++  <OSImage>
++    <Label>Label 2</Label>
++    <PublishedDate>2013-02-15T00:00:00Z</PublishedDate>
++  </OSImage>
++  <OSImage>
++    <Label>Label 3</Label>
++    <PublishedDate>2013-04-13T00:00:00Z</PublishedDate>
++  </OSImage>
++  <OSImage>
++    <Label>Label 4</Label>
++    <PublishedDate>2013-03-15T00:00:00Z</PublishedDate>
++  </OSImage>
++</Images>`
++
++    images := &Images{}
++    err := images.Deserialize([]byte(input))
++    c.Assert(err, IsNil)
++
++    sort.Sort(images)
++    labels := []string{
++        (*images).Images[0].Label,
++        (*images).Images[1].Label,
++        (*images).Images[2].Label,
++        (*images).Images[3].Label,
++    }
++    c.Check(labels, DeepEquals, []string{"Label 3", "Label 4", "Label 2", "Label 1"})
++}
++
++func (suite *xmlSuite) TestGetLatestUbuntuImage(c *C) {
++    // This is real-world XML input.
++    input := `
++<?xml version="1.0"?>
++<Images xmlns="http://schemas.microsoft.com/windowsazure" xmlns:i="http://www.w3.org/2001/XMLSchema-instance">
++  <OSImage>
++    <Category>Canonical</Category>
++    <Label>Ubuntu Server 12.04.2 LTS</Label>
++    <Location>East Asia;Southeast Asia;North Europe;West Europe;East US;West US</Location>
++    <LogicalSizeInGB>30</LogicalSizeInGB>
++    <Name>b39f27a8b8c64d52b05eac6a62ebad85__Ubuntu-12_04_2-LTS-amd64-server-20130415-en-us-30GB</Name>
++    <OS>Linux</OS>
++    <ImageFamily>Ubuntu Server 12.04 LTS</ImageFamily>
++    <PublishedDate>2013-04-15T00:00:00Z</PublishedDate>
++    <IsPremium>false</IsPremium>
++    <PublisherName>Canonical</PublisherName>
++  </OSImage>
++  <OSImage>
++    <Category>Canonical</Category>
++    <Label>Ubuntu Server 12.10</Label>
++    <Location>East Asia;Southeast Asia;North Europe;West Europe;East US;West US</Location>
++    <LogicalSizeInGB>30</LogicalSizeInGB>
++    <Name>b39f27a8b8c64d52b05eac6a62ebad85__Ubuntu-12_10-amd64-server-20130414-en-us-30GB</Name>
++    <OS>Linux</OS>
++    <ImageFamily>Ubuntu 12.10</ImageFamily>
++    <PublishedDate>2013-04-15T00:00:00Z</PublishedDate>
++    <PublisherName>Canonical</PublisherName>
++  </OSImage>
++  <OSImage>
++    <Category>Canonical</Category>
++    <Label>Ubuntu Server 13.04</Label>
++    <Location>East Asia;Southeast Asia;North Europe;West Europe;East US;West US</Location>
++    <LogicalSizeInGB>30</LogicalSizeInGB>
++    <Name>b39f27a8b8c64d52b05eac6a62ebad85__Ubuntu-13_04-amd64-server-20130423-en-us-30GB</Name>
++    <OS>Linux</OS>
++    <ImageFamily>Ubuntu Server 13.04</ImageFamily>
++    <PublishedDate>2013-04-25T00:00:00Z</PublishedDate>
++    <PublisherName>Canonical</PublisherName>
++  </OSImage>
++  <OSImage>
++    <Category>Canonical</Category>
++    <Label>Ubuntu Server 13.04 bogus publisher name</Label>
++    <Location>East Asia;Southeast Asia;North Europe;West Europe;East US;West US</Location>
++    <LogicalSizeInGB>30</LogicalSizeInGB>
++    <Name>b39f27a8b8c64d52b05eac6a62ebad85__Ubuntu-13_04-amd64-server-20130423-en-us-30GB</Name>
++    <OS>Linux</OS>
++    <PublishedDate>2013-05-25T00:00:00Z</PublishedDate>
++    <ImageFamily>Ubuntu Server 13.04</ImageFamily>
++    <PublisherName>Bogus publisher name</PublisherName>
++  </OSImage>
++</Images>`
++    images := &Images{}
++    err := images.Deserialize([]byte(input))
++    c.Assert(err, IsNil)
++
++    var testValues = []struct {
++        releaseName   string
++        location      string
++        expectedError error
++        expectedLabel string
++    }{
++        {"13.04", "West US", nil, "Ubuntu Server 13.04"},
++        {"12.04", "West US", nil, "Ubuntu Server 12.04.2 LTS"},
++        {"bogus-name", "Unknown location", fmt.Errorf("No matching images found"), ""},
++    }
++    for _, test := range testValues {
++        image, err := images.GetLatestUbuntuImage(test.releaseName, test.location)
++        c.Check(err, DeepEquals, test.expectedError)
++        if image != nil {
++            c.Check(image.Label, Equals, test.expectedLabel)
++        }
++    }
++}