testservice: version/network/details support

testservice: version/network/details support

These changes are for juju-core project testing. They include new API endpoints:

- /version/
  return capabilities, currently only networks-management
- /network/<name>/?op=list_connected_macs
  returns list of MAC addresses connected to a network
- /nodes/<system_id>/?op=details
  returns lshw XML as application/bson, the XML data should be added before with AddNodeDetails(systemId, xml) function

Now for API endpoint:
  /networks?node=system_id
TestService returns empty list if node is not found or no networks are connected to the node.
It panicked before.

https://code.launchpad.net/~klyachin/gomaasapi/101-testserver-extensions/+merge/214961

(do not edit description out of merge proposal)

[vladislav.klyachin, 2014-04-09 15:02:31]

Please take a look.

[rvb, 2014-04-09 15:19:48]

LGTM with a couple of remarks.

https://codereview.appspot.com/86070043/diff/1/testservice.go
File testservice.go (right):

https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode482
testservice.go:482: // AddNodeDetails srores node details, expected in XML
format.
typo: stores

https://codereview.appspot.com/86070043/diff/1/testservice_test.go
File testservice_test.go (right):

https://codereview.appspot.com/86070043/diff/1/testservice_test.go#newcode647
testservice_test.go:647: expectedResourceURI =
fmt.Sprintf("/api/%s/nodes/mysystemid/macs/aa%%3Abb%%3Acc%%3Add%%3Aee%%3Aff/",
apiVersion)
Probably better to express this as the url-escaped version of
'aa:bb:cc:dd:ee:ff'.  Here and elsewhere.

https://codereview.appspot.com/86070043/diff/1/testservice_test.go#newcode669
testservice_test.go:669: c.Assert(err, IsNil)
This is a bit awkward, can we check the entire set instead of picking the first
node and testing that?

https://codereview.appspot.com/86070043/diff/1/testservice_test.go#newcode718
testservice_test.go:718: xmlText := `<?xml version="1.0" standalone="yes" ?>
It's a detail but don't we have a equivalent to Python textwrap.dedent to better
format this?

[dimitern, 2014-04-09 15:58:23]

Looks almost there, but I have a few suggestions below:

https://codereview.appspot.com/86070043/diff/1/testservice.go
File testservice.go (right):

https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode279
testservice.go:279: func (server *TestServer)
ConnectNodeToNetworkWithMACAddress(systemId, name, macAddress string) {
s/name/networkName/ ?

https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode282
testservice.go:282: panic("no node with given system id")
s/given/the given/ ?

https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode284
testservice.go:284: _, hasNetwork := server.networks[name]
or:

if _, hasNetwork := server.networks[name]; !hasNetwork {
...
}

https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode300
testservice.go:300: } else {
No need for that else block.

https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode332
testservice.go:332: // Register handler for '/api/version/'.
... for '/api/<version>/version/'

https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode494
testservice.go:494: attrs["lldp"] = []byte("<?xml version=\"1.0\"
encoding=\"UTF-8\"?>\n<lldp label=\"LLDP neighbors\"/>")
Can you define:

const lldpXML = `
<?xml version="1.0" encoding="UTF-8"?>
<lldp label="LLDP neighbors"/>
`

...and use it here? It will look better I think.

https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode664
testservice.go:664: // networkListingConnectedMACSHandler handles requests for
'/api/<version>/networks/<network>/?op=list_connected_macs'
s/Listing/List/ ?

https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode673
testservice.go:673: var convertedMacAddresses = []map[string]JSONObject{}
Or just convertedMacAddresses := []map[string]JSONObject{}

https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode701
testservice.go:701: panic("only list_connected_macs operation implemented")
"only list_connected_macs and default operations implemented" ?

(default: op=="")

https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode708
testservice.go:708: if !hasNetworks {
I think you can skip this case and just leave the else part (Marshal should work
with empty slices just the same as with nil slices I hope).

https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode723
testservice.go:723: // versionHandler handles requests for '/api/version/'.
'/api/<version>/version/'

https://codereview.appspot.com/86070043/diff/1/testservice_test.go
File testservice_test.go (right):

https://codereview.appspot.com/86070043/diff/1/testservice_test.go#newcode624
testservice_test.go:624: c.Check(err, IsNil)
Assert on errors, rather than Check, that way if it's not what you expect the
test case is halted immediately, rather than blowing up down the line (see
gocheck docs about Assert vs. Check).

https://codereview.appspot.com/86070043/diff/1/testservice_test.go#newcode627
testservice_test.go:627: c.Check(len(listNodes), Equals, 1)
c.Check(listNodes, HasLen, 1)

https://codereview.appspot.com/86070043/diff/1/testservice_test.go#newcode640
testservice_test.go:640: c.Check(len(macAddressSet), Equals, 1)
..HasLen, 1

https://codereview.appspot.com/86070043/diff/1/testservice_test.go#newcode651
testservice_test.go:651: func (suite *TestMAASObjectSuite)
TestListNetworkConnectedMACAddresses(c *C) {
Please, separate blocks of tests with blank lines (to group them logically and
for better readability - here and in other test cases).

https://codereview.appspot.com/86070043/diff/1/testservice_test.go#newcode664
testservice_test.go:664: c.Check(err, IsNil)
Assert instead of Check

[vladislav.klyachin, 2014-04-09 17:04:19]

Please take a look.

[vladislav.klyachin, 2014-04-09 17:09:19]

On 2014/04/09 15:19:48, rvb wrote:
> LGTM with a couple of remarks.
> 
> https://codereview.appspot.com/86070043/diff/1/testservice.go
> File testservice.go (right):
> 
> https://codereview.appspot.com/86070043/diff/1/testservice.go#newcode482
> testservice.go:482: // AddNodeDetails srores node details, expected in XML
> format.
> typo: stores
Done.

> 
> https://codereview.appspot.com/86070043/diff/1/testservice_test.go
> File testservice_test.go (right):
> 
> https://codereview.appspot.com/86070043/diff/1/testservice_test.go#newcode647
> testservice_test.go:647: expectedResourceURI =
> fmt.Sprintf("/api/%s/nodes/mysystemid/macs/aa%%3Abb%%3Acc%%3Add%%3Aee%%3Aff/",
> apiVersion)
> Probably better to express this as the url-escaped version of
> 'aa:bb:cc:dd:ee:ff'.  Here and elsewhere.
Done.

> 
> https://codereview.appspot.com/86070043/diff/1/testservice_test.go#newcode669
> testservice_test.go:669: c.Assert(err, IsNil)
> This is a bit awkward, can we check the entire set instead of picking the
first
> node and testing that?
The problem is that mac addresses can be in an arbitrary order.
Golang language spec explicitly defines maps as having undefined ordering of
keys.

> 
> https://codereview.appspot.com/86070043/diff/1/testservice_test.go#newcode718
> testservice_test.go:718: xmlText := `<?xml version="1.0" standalone="yes" ?>
> It's a detail but don't we have a equivalent to Python textwrap.dedent to
better
> format this?
Didn't find dedent analogue in Go.
Moved text to a string constant outside of the function.

[vladislav.klyachin, 2014-04-09 17:10:23]

On 2014/04/09 15:58:23, dimitern wrote:
> Looks almost there, but I have a few suggestions below:
> 
Thanks for the review, dimitern. All changes suggested were made.

[dimitern, 2014-04-10 08:13:45]

On 2014/04/09 17:10:23, vladislav.klyachin wrote:
> On 2014/04/09 15:58:23, dimitern wrote:
> > Looks almost there, but I have a few suggestions below:
> > 
> Thanks for the review, dimitern. All changes suggested were made.

Thanks, LGTM!