Code review comment for lp:~vishvananda/nova/kill-objectstore

Revision history for this message
Devin Carlen (devcamcar) wrote :

A few nits:

66 + def register_all(self, image, kernel, ramdisk, owner, name=None,

78 + def image_register(self, path, owner, name=None, is_public='T',

93 + def ramdisk_register(self, path, owner, name=None, is_public='T',

This feels a little inconsistent. Should be xxx_register or register_xxx instead of a mix.

66 + def register_all(self, image, kernel, ramdisk, owner, name=None,
67 + is_public='T', architecture='x86_64'):
68 + """Uploads an image, kernel, and ramdisk into the image_service
69 + arguments: image kernel ramdisk owner [name] [is_public='T']
70 + [architecture='x86_64']"""
71 + kernel_id = self._register('kernel', kernel, owner, None,
72 + is_public, architecture)
73 + ramdisk_id = self._register('ramdisk', ramdisk, owner, None,
74 + is_public, architecture)
75 + self._register('machine', image, owner, name, is_public,
76 + architecture, kernel_id, ramdisk_id)
77 +

Instead of the calls to self._register which results in some duplicated code, you could call the higher level method for better code reuse:

kernel_id = kernel_register(kernel, owner, None, is_public, architecture)
ramdisk_id = ramdisk_register(ramdisk, ramdisk, owner, None, is_public, architecture)
etc.

451 +<<<<<<< TREE
452 instance_id = ec2_id_to_id(instance_id)
453 +=======
454 + instance_id = ec2utils.ec2_id_to_id(ec2_id)
455 +>>>>>>> MERGE-SOURCE

Merge issue here.

review: Needs Fixing

« Back to merge proposal