Code review comment for lp:~markwash/nova/lp727225

Hi!

Good stuff :)

As mentioned on IRC, I'd suggest a global search and replace:

s/onset_files/injected_files/g

104 + expl = 'Bad personality format: missing %s' % key
107 + raise exc.HTTPBadRequest(explanation='Bad personality format')
111 + msg = 'Personality content for %s cannot be decoded' % path
121 + expl = "Personality file limit exceeded"
124 + expl = "Personality file path too long"
127 + expl = "Personality file content too long"

Please i18n all those! If you're unclear on how to do that, let us know.

309 + return int(FLAGS.quota_max_onset_files)
314 + return int(FLAGS.quota_max_onset_file_content_bytes)
319 + return int(FLAGS.quota_max_onset_file_path_bytes)

You used flags.DEFINE_integer, so the int()'s above are redundant. Not sure any of those functions are really necessary?

Throughout the test cases in TestServerCreateRequestXMLDeserializer, you put backslashes on the end of text lines, when the text is encloses in """'s. This is incorrect and these need to be removed. As an example:

364 + def test_minimal_request(self):
365 + serial_request = """
366 +<server xmlns="http://docs.rackspacecloud.com/servers/api/v1.0"\
367 + name="new-server-test" imageId="1" flavorId="1"/>"""
368 + request = self.deserializer.deserialize(serial_request)
369 + expected = {"server": {
370 + "name": "new-server-test",
371 + "imageId": "1",
372 + "flavorId": "1",
373 + }}
374 + self.assertEquals(request, expected)

Should be:

def test_minimal_request(self):
     serial_request = """
<server xmlns="http://docs.rackspacecloud.com/servers/api/v1.0"
name="new-server-test" imageId="1" flavorId="1"/>"""
    request = self.deserializer.deserialize(serial_request)
    expected = {"server": {
        "name": "new-server-test",
        "imageId": "1",
        "flavorId": "1"}}
    self.assertEquals(request, expected)

This is important, because currently, you are testing an invalid XML string (having a \ between attributes and in other places, having a \ between element tags) and the serializer tests are apparently passing.

507 + def test_request_with_metadata_missing_key(self):
508 + serial_request = """
509 +<server xmlns="http://docs.rackspacecloud.com/servers/api/v1.0"\
510 + name="new-server-test" imageId="1" flavorId="1">\
511 +<metadata><meta>beta</meta></metadata></server>"""
512 + request = self.deserializer.deserialize(serial_request)
513 + expected = {"": "beta"}
514 + self.assertEquals(request["server"]["metadata"], expected)

The test case above technically passes. However, try adding in an additional <meta>gamma</meta> and see if the test passes ;)

Generally, when testing the parsing/deserialization of lists/dicts, you will want to add at least one test that has >1 element in the list being tested, and check that tests with one element work the same as tests with >1 element. Please feel free to add a test case for each of these cases:

1) >1 metadata element with no key
2) >1 injected file in the personality
3) >1 personality in the server

I think you may find some interesting results :)

The tests in TestServerInstanceCreation look excellent. Nice work on those!

Cheers!
jay