)]}'
{"nova/virt/xenapi/vmops.py":[{"author":{"_account_id":6735,"name":"Bob Ball","email":"bob.ball@citrix.com","username":"bob-ball"},"change_message_id":"33580f17fad5c2037babef1e763f2174a3978020","unresolved":false,"context_lines":[{"line_number":133,"context_line":"    return step_decorator"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":""},{"line_number":136,"context_line":"class VMOpsSpawnHelper(object):"},{"line_number":137,"context_line":"    \"\"\"def init(**kwargs):"},{"line_number":138,"context_line":"        for key, value in kwargs.items():"},{"line_number":139,"context_line":"            setattr(VMOpsSpawnHelper, key, value)"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_713fdcb3","line":136,"updated":"2017-03-06 11:07:54.000000000","message":"I really don\u0027t like this approach; it seems like an automated approach to moving methods to a new class - which isn\u0027t giving the best solution.  See below.","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":18602,"name":"Gábor Antal","email":"antal@inf.u-szeged.hu","username":"antal"},"change_message_id":"f275a53353f2c32eb0123c2bc50152a4c90f506c","unresolved":false,"context_lines":[{"line_number":134,"context_line":""},{"line_number":135,"context_line":""},{"line_number":136,"context_line":"class VMOpsSpawnHelper(object):"},{"line_number":137,"context_line":"    \"\"\"def init(**kwargs):"},{"line_number":138,"context_line":"        for key, value in kwargs.items():"},{"line_number":139,"context_line":"            setattr(VMOpsSpawnHelper, key, value)"},{"line_number":140,"context_line":"    \"\"\""},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"    def __init__(self, selfdata, context, instance, image_meta, step,"},{"line_number":143,"context_line":"                 create_disks_step, first_boot, session, injected_files\u003dNone,"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_018047bf","line":140,"range":{"start_line":137,"start_character":0,"end_line":140,"end_character":7},"updated":"2017-03-03 18:13:53.000000000","message":"I hope it will be removed, as in my opinion, setattr() on a class (NOT on an instance) is bad. Your new version is a way cleaner.","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":21239,"name":"Alex Szarka","email":"szarka@inf.u-szeged.hu","username":"xavvior"},"change_message_id":"5def7dfcf9d070f8076df5268ded9b96efc4672d","unresolved":false,"context_lines":[{"line_number":134,"context_line":""},{"line_number":135,"context_line":""},{"line_number":136,"context_line":"class VMOpsSpawnHelper(object):"},{"line_number":137,"context_line":"    \"\"\"def init(**kwargs):"},{"line_number":138,"context_line":"        for key, value in kwargs.items():"},{"line_number":139,"context_line":"            setattr(VMOpsSpawnHelper, key, value)"},{"line_number":140,"context_line":"    \"\"\""},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"    def __init__(self, selfdata, context, instance, image_meta, step,"},{"line_number":143,"context_line":"                 create_disks_step, first_boot, session, injected_files\u003dNone,"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_fcebb510","line":140,"range":{"start_line":137,"start_character":0,"end_line":140,"end_character":7},"in_reply_to":"ba2be162_018047bf","updated":"2017-03-05 21:27:23.000000000","message":"I will remove this, at next patchset. This just temporary [workflow-1] :)","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":6735,"name":"Bob Ball","email":"bob.ball@citrix.com","username":"bob-ball"},"change_message_id":"33580f17fad5c2037babef1e763f2174a3978020","unresolved":false,"context_lines":[{"line_number":154,"context_line":"        self.injected_files \u003d injected_files"},{"line_number":155,"context_line":"        self.admin_password \u003d admin_password"},{"line_number":156,"context_line":"        self.network_info \u003d network_info"},{"line_number":157,"context_line":"        self.block_device_info \u003d block_device_info"},{"line_number":158,"context_line":"        self.name_label \u003d name_label"},{"line_number":159,"context_line":"        self.rescue \u003d rescue"},{"line_number":160,"context_line":"        self.power_on \u003d power_on"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_f163ccae","line":157,"updated":"2017-03-06 11:07:54.000000000","message":"I know the aim is to reduce \"code complexity\" - but this change is substantially increasing understandability IMO.  There are far too many values being passed through to VMOpsSpawnHelper, and the class name itself almost implies that we\u0027re trying to move *everything* here and make this class just as complicated as the original spawn method.\n\nIf we\u0027re going to refactor spawn then can we consider each method one-by-one and do the right thing for that method rather than pushing them all to a \u0027helper\u0027 class?","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":18602,"name":"Gábor Antal","email":"antal@inf.u-szeged.hu","username":"antal"},"change_message_id":"f275a53353f2c32eb0123c2bc50152a4c90f506c","unresolved":false,"context_lines":[{"line_number":160,"context_line":"        self.power_on \u003d power_on"},{"line_number":161,"context_line":"        self.resize \u003d resize"},{"line_number":162,"context_line":"        self.completed_callback \u003d completed_callback"},{"line_number":163,"context_line":"        self._session \u003d session"},{"line_number":164,"context_line":"        self.vm_ref \u003d None"},{"line_number":165,"context_line":"        self.vdis \u003d None"},{"line_number":166,"context_line":"        self.disk_image_type \u003d None"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_c172af10","line":163,"range":{"start_line":163,"start_character":13,"end_line":163,"end_character":14},"updated":"2017-03-03 18:13:53.000000000","message":"Why it is underscored and others are not? In a newer version it would be good to make this consistent (all attributes should get an underscore I think).","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":21239,"name":"Alex Szarka","email":"szarka@inf.u-szeged.hu","username":"xavvior"},"change_message_id":"5def7dfcf9d070f8076df5268ded9b96efc4672d","unresolved":false,"context_lines":[{"line_number":160,"context_line":"        self.power_on \u003d power_on"},{"line_number":161,"context_line":"        self.resize \u003d resize"},{"line_number":162,"context_line":"        self.completed_callback \u003d completed_callback"},{"line_number":163,"context_line":"        self._session \u003d session"},{"line_number":164,"context_line":"        self.vm_ref \u003d None"},{"line_number":165,"context_line":"        self.vdis \u003d None"},{"line_number":166,"context_line":"        self.disk_image_type \u003d None"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_fc39f5a4","line":163,"range":{"start_line":163,"start_character":13,"end_line":163,"end_character":14},"in_reply_to":"ba2be162_c172af10","updated":"2017-03-05 21:27:23.000000000","message":"In caller class call the session as _session. I just copy this name, but if better without underscore, i remove this.","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":18599,"name":"Péter Hegedűs","email":"hpeter@inf.u-szeged.hu","username":"hpeter"},"change_message_id":"d68c50ecc054a37de87c5d96f46cbb4b3c1e993e","unresolved":false,"context_lines":[{"line_number":520,"context_line":"        self._ensure_enough_free_mem(instance)"},{"line_number":521,"context_line":""},{"line_number":522,"context_line":"        helper \u003d VMOpsSpawnHelper(self, context, instance, image_meta, step,"},{"line_number":523,"context_line":"            create_disks_step, first_boot, self._session, injected_files,"},{"line_number":524,"context_line":"            admin_password,network_info, block_device_info,"},{"line_number":525,"context_line":"            name_label, rescue, power_on, resize,completed_callback)"},{"line_number":526,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_1ea5e045","line":523,"updated":"2017-03-05 20:56:01.000000000","message":"If you pass \u0027self\u0027 entirely, it is pointless to pass self._session as well.","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":21239,"name":"Alex Szarka","email":"szarka@inf.u-szeged.hu","username":"xavvior"},"change_message_id":"5def7dfcf9d070f8076df5268ded9b96efc4672d","unresolved":false,"context_lines":[{"line_number":520,"context_line":"        self._ensure_enough_free_mem(instance)"},{"line_number":521,"context_line":""},{"line_number":522,"context_line":"        helper \u003d VMOpsSpawnHelper(self, context, instance, image_meta, step,"},{"line_number":523,"context_line":"            create_disks_step, first_boot, self._session, injected_files,"},{"line_number":524,"context_line":"            admin_password,network_info, block_device_info,"},{"line_number":525,"context_line":"            name_label, rescue, power_on, resize,completed_callback)"},{"line_number":526,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_5c5a01f3","line":523,"in_reply_to":"ba2be162_1ea5e045","updated":"2017-03-05 21:27:23.000000000","message":"I see it. You maybe right.","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":18602,"name":"Gábor Antal","email":"antal@inf.u-szeged.hu","username":"antal"},"change_message_id":"f275a53353f2c32eb0123c2bc50152a4c90f506c","unresolved":false,"context_lines":[{"line_number":519,"context_line":"        self._ensure_instance_name_unique(name_label)"},{"line_number":520,"context_line":"        self._ensure_enough_free_mem(instance)"},{"line_number":521,"context_line":""},{"line_number":522,"context_line":"        helper \u003d VMOpsSpawnHelper(self, context, instance, image_meta, step,"},{"line_number":523,"context_line":"            create_disks_step, first_boot, self._session, injected_files,"},{"line_number":524,"context_line":"            admin_password,network_info, block_device_info,"},{"line_number":525,"context_line":"            name_label, rescue, power_on, resize,completed_callback)"},{"line_number":526,"context_line":""},{"line_number":527,"context_line":"        @step"},{"line_number":528,"context_line":"        def determine_disk_image_type_step(undo_mgr):"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_bc5de016","line":525,"range":{"start_line":522,"start_character":7,"end_line":525,"end_character":68},"updated":"2017-03-03 18:13:53.000000000","message":"I think you should use this instead of locals() as locals() can pass useless parameters in the future.","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":21239,"name":"Alex Szarka","email":"szarka@inf.u-szeged.hu","username":"xavvior"},"change_message_id":"5def7dfcf9d070f8076df5268ded9b96efc4672d","unresolved":false,"context_lines":[{"line_number":519,"context_line":"        self._ensure_instance_name_unique(name_label)"},{"line_number":520,"context_line":"        self._ensure_enough_free_mem(instance)"},{"line_number":521,"context_line":""},{"line_number":522,"context_line":"        helper \u003d VMOpsSpawnHelper(self, context, instance, image_meta, step,"},{"line_number":523,"context_line":"            create_disks_step, first_boot, self._session, injected_files,"},{"line_number":524,"context_line":"            admin_password,network_info, block_device_info,"},{"line_number":525,"context_line":"            name_label, rescue, power_on, resize,completed_callback)"},{"line_number":526,"context_line":""},{"line_number":527,"context_line":"        @step"},{"line_number":528,"context_line":"        def determine_disk_image_type_step(undo_mgr):"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_bc54ddc9","line":525,"range":{"start_line":522,"start_character":7,"end_line":525,"end_character":68},"in_reply_to":"ba2be162_bc5de016","updated":"2017-03-05 21:27:23.000000000","message":"Okay, i remove the items of locals() solution from this patch at next petchset.","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":18599,"name":"Péter Hegedűs","email":"hpeter@inf.u-szeged.hu","username":"hpeter"},"change_message_id":"d68c50ecc054a37de87c5d96f46cbb4b3c1e993e","unresolved":false,"context_lines":[{"line_number":524,"context_line":"            admin_password,network_info, block_device_info,"},{"line_number":525,"context_line":"            name_label, rescue, power_on, resize,completed_callback)"},{"line_number":526,"context_line":""},{"line_number":527,"context_line":"        @step"},{"line_number":528,"context_line":"        def determine_disk_image_type_step(undo_mgr):"},{"line_number":529,"context_line":"            return vm_utils.determine_disk_image_type(image_meta)"},{"line_number":530,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_9e6f10fb","line":527,"updated":"2017-03-05 20:56:01.000000000","message":"If you move the helper methods into a separate class, why not move the step methods as well?","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":21239,"name":"Alex Szarka","email":"szarka@inf.u-szeged.hu","username":"xavvior"},"change_message_id":"74dbaa386b1a585ab27e8c8bac5a519d0024d973","unresolved":false,"context_lines":[{"line_number":524,"context_line":"            admin_password,network_info, block_device_info,"},{"line_number":525,"context_line":"            name_label, rescue, power_on, resize,completed_callback)"},{"line_number":526,"context_line":""},{"line_number":527,"context_line":"        @step"},{"line_number":528,"context_line":"        def determine_disk_image_type_step(undo_mgr):"},{"line_number":529,"context_line":"            return vm_utils.determine_disk_image_type(image_meta)"},{"line_number":530,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_a6f12397","line":527,"in_reply_to":"ba2be162_9e6f10fb","updated":"2017-03-06 08:02:38.000000000","message":"I looking for possible solutions, that i can move this methods to the helper class.","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":6735,"name":"Bob Ball","email":"bob.ball@citrix.com","username":"bob-ball"},"change_message_id":"33580f17fad5c2037babef1e763f2174a3978020","unresolved":false,"context_lines":[{"line_number":556,"context_line":"        @step"},{"line_number":557,"context_line":"        def attach_devices_step(undo_mgr, vm_ref, vdis, disk_image_type):"},{"line_number":558,"context_line":"            helper.attach_disks(undo_mgr, vm_ref, vdis, disk_image_type)"},{"line_number":559,"context_line":"            helper.attach_pci_devices(undo_mgr, vm_ref)"},{"line_number":560,"context_line":""},{"line_number":561,"context_line":"        if rescue:"},{"line_number":562,"context_line":"            # NOTE(johannes): Attach disks from original VM to rescue VM now,"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba2be162_f1874c5e","line":559,"updated":"2017-03-06 11:07:54.000000000","message":"Since we already pass undo_mgr and vm_ref, we only need instance and a session.  Adding these to parametesr to the attach_pci_devices method would enable it to be a standalone method without a helper class.","commit_id":"46cee52bbc684cd96860d76d6bbc79b666bd2d3a"},{"author":{"_account_id":24711,"name":"Jackie Truong","email":"jacklyn.truong@jhuapl.edu","username":"jackie-truong"},"change_message_id":"371df0dd0f781161f6a7091e3905d687f83ee4e6","unresolved":false,"context_lines":[{"line_number":746,"context_line":"        flavor \u003d instance.get_flavor()"},{"line_number":747,"context_line":""},{"line_number":748,"context_line":"        # Attach (required) root disk"},{"line_number":749,"context_line":"        if disk_image_type \u003d\u003d vm_utils.ImageType.DISK_ISO:"},{"line_number":750,"context_line":"            # DISK_ISO needs two VBDs: the ISO disk and a blank RW disk"},{"line_number":751,"context_line":"            root_disk_size \u003d flavor.root_gb"},{"line_number":752,"context_line":"            if root_disk_size \u003e 0:"}],"source_content_type":"text/x-python","patch_set":6,"id":"dfeb2761_22818790","line":749,"range":{"start_line":749,"start_character":11,"end_line":749,"end_character":26},"updated":"2017-04-03 02:39:04.000000000","message":"In an effort to reduce code complexity, could we just use vm_utils\u0027s determine_disk_image_type to grab the disk image type, thus eliminating the need to pass disk_image_type in as an argument?\n\n  vm_utils.determine_disk_image_type(image_meta)\n\nThis would mean that the disk_image_type argument in attach_disks could be removed.\n\nJust looking for ways to shrink the number of arguments being passed around.","commit_id":"72ba59e985222da200d4b7545f67c69581112fa1"},{"author":{"_account_id":6735,"name":"Bob Ball","email":"bob.ball@citrix.com","username":"bob-ball"},"change_message_id":"39b0549ab4285ff16c4d22bc23629f574f56999c","unresolved":false,"context_lines":[{"line_number":495,"context_line":"        self._ensure_enough_free_mem(instance)"},{"line_number":496,"context_line":""},{"line_number":497,"context_line":"        def attach_disks(undo_mgr, vm_ref, vdis, disk_image_type):"},{"line_number":498,"context_line":"            self._attach_disks_helper(context, undo_mgr, vm_ref, vdis,"},{"line_number":499,"context_line":"                disk_image_type, instance, image_meta, network_info, resize,"},{"line_number":500,"context_line":"                name_label, rescue, admin_password, injected_files,"},{"line_number":501,"context_line":"                first_boot, block_device_info)"}],"source_content_type":"text/x-python","patch_set":7,"id":"dfeb2761_4709342a","line":498,"updated":"2017-04-03 08:58:52.000000000","message":"I don\u0027t think this makes sense unfortunately - I hadn\u0027t realised quite so many parameters would need to be passed through.  I believe this change isn\u0027t worth it for the sake of \"code complexity\" reasons.\nPotentially you could move the ipxe iso / resize code out and keep the _attach_disks and first_boot calls here, but I don\u0027t think that would provide much benefit really.","commit_id":"6b38c882d90f51e8e1422def65cef2361d7fdbac"},{"author":{"_account_id":6735,"name":"Bob Ball","email":"bob.ball@citrix.com","username":"bob-ball"},"change_message_id":"39b0549ab4285ff16c4d22bc23629f574f56999c","unresolved":false,"context_lines":[{"line_number":500,"context_line":"                name_label, rescue, admin_password, injected_files,"},{"line_number":501,"context_line":"                first_boot, block_device_info)"},{"line_number":502,"context_line":""},{"line_number":503,"context_line":"        def attach_pci_devices(undo_mgr, vm_ref):"},{"line_number":504,"context_line":"            self._attach_pci_devices_helper(instance, vm_ref)"},{"line_number":505,"context_line":""},{"line_number":506,"context_line":"        @step"}],"source_content_type":"text/x-python","patch_set":7,"id":"dfeb2761_c798a51c","line":503,"updated":"2017-04-03 08:58:52.000000000","message":"I think this suggests that actually we can go one step further.\nIf undo_mgr is not used, and only instance is passed, why does this need a separate call?\ni.e. can\u0027t we just move attach_pci_devices and attach_disks outside the spawn function entirely?\n\nI may be missing something obvious though...","commit_id":"6b38c882d90f51e8e1422def65cef2361d7fdbac"}]}