Code review comment for lp:~rackspace-titan/nova/metadata_v1.1

Revision history for this message
Dan Prince (dan-prince) wrote :

Hi Justin,

I made a couple of changes based on your comments:

--Regarding 'collections' within the openstack API. I didn't update the JSON/XML responses to nest things in 'values' collections yet. We'll handle this specific issue globally across the API. What I've done here should be consistent with what already exists within the API's.

--The PUT call now guards against requests with multiple keys.

--My update delete SQL command now filters by deleted=False. I've tested the PUT/DELETE/GET commands extensively and I we should be fine if someone deletes a key and then recreates the same key again. I did however modify the update command so that it sets 'deleted=0' on an update (thus reactivating a key). I think 'update_or_create_instance_metadata' should be good to go now.

--Regarding the XML serialization stuff. Nova already has issues here. A call to get metadata in XML format with return something that looks like this:

 <metadata> <key2> value2 </key2> <key1> value1 </key1> </metadata>

Again this isn't "too the spec" but we'll handle proper serialization as a separate issue across the board.

--Regarding the exception throwing. I throw an exception in the 'get_instance_metadata_item' if the key isn't found. In 'get_instance_metadata' I simply return an empty dict if there is not data sense no metadata implies that metadata is just empty. I don't see any calls with find in the name so I suppose we could start doing this as a convention but I'm not sure I need to change anything to follow it currently.

Let me know what you think with the latest updates. I think we should be good to go.

« Back to merge proposal