Code review comment for lp:~termie/nova/libvirt_snapshot

Revision history for this message
Jay Pipes (jaypipes) wrote :

I agree that this is great, Andy. Very nice work.

7 import time
8 +import tempfile

i before e except when listing alphabetically. ;)

52 + 'image_location': 'snapshot',

Since there is a 'location' key already returned from Glance, perhaps you could come up with a slightly different key name than 'image_location'? However, I don't see this key used anywhere else in the code? Could you explain what code uses the 'image_location' property?

53 + 'image_state': 'available',

The image services have a 'status' column, containing ('active', 'queued', 'saving', 'killed'). Is it possible to remove the above custom property in favour of the status column? Could you explain what code uses this custom property, if any?

Just want to make sure we don't add seemingly redundant columns that could potentially introduce confusion into the image service API. We've had some issues in the last months with columns in the various image services meaning different things and have tried to get the problem under control.

Thanks in advance for the answers, Andy.

-jay

review: Needs Fixing

« Back to merge proposal