)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7040,"name":"GuoHui LIu","email":"liuguohui@gmail.com","username":"guohliu"},"change_message_id":"bb5d71febee8fcb091621cabe7ea91f4e9b6997d","unresolved":false,"context_lines":[{"line_number":12,"context_line":"file when detach it from a VM instance."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"The fix was remove the fileOperation field when detach vmdk volume"},{"line_number":15,"context_line":"so as to make sure the volume will not be destriyed; but for iscsi"},{"line_number":16,"context_line":"we still need to destroy the volume."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Also I changed the function name from delete_virtual_disk_spec to"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"AAAATH%2F%2FQSY%3D","line":15,"updated":"2013-10-19 02:23:29.000000000","message":"s/destriyed/destroyed","commit_id":"573078c1c006946650e7b582799d8d1a1f7407a7"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"2d8a08d04870f8ef889c7f0a217b56ea94f4e71c","unresolved":false,"context_lines":[{"line_number":12,"context_line":"file when detach it from a VM instance."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"The fix was remove the fileOperation field when detach vmdk volume"},{"line_number":15,"context_line":"so as to make sure the volume will not be destriyed; but for iscsi"},{"line_number":16,"context_line":"we still need to destroy the volume."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Also I changed the function name from delete_virtual_disk_spec to"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"AAAATH%2F%2FQR4%3D","line":15,"in_reply_to":"AAAATH%2F%2FQSY%3D","updated":"2013-10-19 02:27:18.000000000","message":"Done","commit_id":"573078c1c006946650e7b582799d8d1a1f7407a7"},{"author":{"_account_id":7629,"name":"Shawn Hartsock","email":"hartsock@acm.org","username":"hartsocks"},"change_message_id":"ef9585f68f141714160a37e65d3d51bb633f89d9","unresolved":false,"context_lines":[{"line_number":19,"context_line":"detach_virtual_disk_spec which is more accurate."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change-Id: Ibd218d6a8cfeede4f5ca74f28bc0c3d0c185bb14"},{"line_number":22,"context_line":"Closes-Bug: #1241350"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"AAAATX%2F%2F9Os%3D","line":22,"updated":"2013-10-23 17:34:26.000000000","message":"Something is wrong with this link. Should this comment be different or should someone fix the comment parser?","commit_id":"17be4060683d219a80bfb0ed8bd5b5c425a7a893"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"c80320d961b714c66cc1c649ca4b715a962277e9","unresolved":false,"context_lines":[{"line_number":19,"context_line":"detach_virtual_disk_spec which is more accurate."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change-Id: Ibd218d6a8cfeede4f5ca74f28bc0c3d0c185bb14"},{"line_number":22,"context_line":"Closes-Bug: #1241350"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"AAAATn%2F%2F%2FII%3D","line":22,"in_reply_to":"AAAATX%2F%2F8zU%3D","updated":"2013-10-24 08:20:56.000000000","message":"This was addressed yesterday by sdague - he fixed the bug on infra","commit_id":"17be4060683d219a80bfb0ed8bd5b5c425a7a893"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6fcc1cddc7a77e61aa50bc91cfbbca0d92537f5f","unresolved":false,"context_lines":[{"line_number":19,"context_line":"detach_virtual_disk_spec which is more accurate."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Change-Id: Ibd218d6a8cfeede4f5ca74f28bc0c3d0c185bb14"},{"line_number":22,"context_line":"Closes-Bug: #1241350"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"AAAATX%2F%2F8zU%3D","line":22,"in_reply_to":"AAAATX%2F%2F9Os%3D","updated":"2013-10-23 19:20:33.000000000","message":"Shawn, this used to work, I\u0027ve been seeing it today on other patches too, must be something wrong with the comment parser but I have no idea where that code is.","commit_id":"17be4060683d219a80bfb0ed8bd5b5c425a7a893"}],"nova/tests/virt/vmwareapi/test_vmwareapi.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"60f2dce13a4650ae64f5721dff503da9383a728b","unresolved":false,"context_lines":[{"line_number":848,"context_line":"        self.mox.StubOutWithMock(vm_util, \u0027get_vmdk_backed_disk_device\u0027)"},{"line_number":849,"context_line":"        vm_util.get_vmdk_backed_disk_device(mox.IgnoreArg(),"},{"line_number":850,"context_line":"                \u0027fake_disk_uuid\u0027).AndReturn(\u0027fake_device\u0027)"},{"line_number":851,"context_line":"        self.mox.StubOutWithMock(volumeops.VMwareVolumeOps,"},{"line_number":852,"context_line":"                                 \u0027_consolidate_vmdk_volume\u0027)"},{"line_number":853,"context_line":"        volumeops.VMwareVolumeOps._consolidate_vmdk_volume(self.instance,"},{"line_number":854,"context_line":"                 mox.IgnoreArg(), \u0027fake_device\u0027, mox.IgnoreArg())"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATX%2F%2F%2BQM%3D","line":851,"updated":"2013-10-23 13:35:18.000000000","message":"It\u0027s fine to mock _consolidate_vmdk_volume here since it\u0027s a method that\u0027s calling _consolidate_vmdk_volume which is being tested.\n\nYou need to have a new test case for covering _consolidate_vmdk_volume and the changes you\u0027re making to it.","commit_id":"17be4060683d219a80bfb0ed8bd5b5c425a7a893"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"970cf3441d28fbfc13c1a62a3a451517e05dcf8e","unresolved":false,"context_lines":[{"line_number":848,"context_line":"        self.mox.StubOutWithMock(vm_util, \u0027get_vmdk_backed_disk_device\u0027)"},{"line_number":849,"context_line":"        vm_util.get_vmdk_backed_disk_device(mox.IgnoreArg(),"},{"line_number":850,"context_line":"                \u0027fake_disk_uuid\u0027).AndReturn(\u0027fake_device\u0027)"},{"line_number":851,"context_line":"        self.mox.StubOutWithMock(volumeops.VMwareVolumeOps,"},{"line_number":852,"context_line":"                                 \u0027_consolidate_vmdk_volume\u0027)"},{"line_number":853,"context_line":"        volumeops.VMwareVolumeOps._consolidate_vmdk_volume(self.instance,"},{"line_number":854,"context_line":"                 mox.IgnoreArg(), \u0027fake_device\u0027, mox.IgnoreArg())"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2F7Sc%3D","line":851,"in_reply_to":"AAAATX%2F%2F%2BQM%3D","updated":"2013-10-25 05:41:47.000000000","message":"Thanks Matt, but I\u0027m not sure if it is a must to add unit test for _consolidate_vmdk_volume. As my code change actually did not touch the logic and it is a private method. Also I have added unit test for detach_virtual_disk_spec which can also cover the code change here, hope it is OK.","commit_id":"17be4060683d219a80bfb0ed8bd5b5c425a7a893"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"98edd29d0aebf3ed3bb87ae12bb72c62bf1e6fa3","unresolved":false,"context_lines":[{"line_number":848,"context_line":"        self.mox.StubOutWithMock(vm_util, \u0027get_vmdk_backed_disk_device\u0027)"},{"line_number":849,"context_line":"        vm_util.get_vmdk_backed_disk_device(mox.IgnoreArg(),"},{"line_number":850,"context_line":"                \u0027fake_disk_uuid\u0027).AndReturn(\u0027fake_device\u0027)"},{"line_number":851,"context_line":"        self.mox.StubOutWithMock(volumeops.VMwareVolumeOps,"},{"line_number":852,"context_line":"                                 \u0027_consolidate_vmdk_volume\u0027)"},{"line_number":853,"context_line":"        volumeops.VMwareVolumeOps._consolidate_vmdk_volume(self.instance,"},{"line_number":854,"context_line":"                 mox.IgnoreArg(), \u0027fake_device\u0027, mox.IgnoreArg())"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2F6Lw%3D","line":851,"in_reply_to":"AAAATn%2F%2F7Sc%3D","updated":"2013-10-25 12:06:26.000000000","message":"Ummm, no, private methods still get unit tested.  You should be able to call them from a unit test to make sure they get covered against regression.  The existing tests completely mock the method out right now which is not acceptable.","commit_id":"17be4060683d219a80bfb0ed8bd5b5c425a7a893"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dcc8cd31ff68a8b049ecfdf3b08ef05a888da41a","unresolved":false,"context_lines":[{"line_number":863,"context_line":"                                mount_point)"},{"line_number":864,"context_line":""},{"line_number":865,"context_line":"    def test_detach_vmdk_disk_from_vm(self):"},{"line_number":866,"context_line":"        def fake_attach_disk_to_vm(*args, **kwargs):"},{"line_number":867,"context_line":"            pass"},{"line_number":868,"context_line":""},{"line_number":869,"context_line":"        def fake_detach_disk_from_vm(volume_ref, instance, original_device,"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2F2Mk%3D","line":866,"updated":"2013-10-27 21:10:15.000000000","message":"I don\u0027t think this is really what I had in mind when I asked that _consolidate_vmdk_volume get test coverage.  With this, rather than ignoring the args using mox, you\u0027re just ignoring the args using stubs, I don\u0027t see anything different about this.  I would have expected the actual args to be verified.  If there is a reason this can\u0027t be done and it must all be stubbed out, it should be explained.","commit_id":"b6154ac09673e401dd2f92e08cb3921bcad15b5c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"035994ad670279a384a8ac1f9fa74e6978baa178","unresolved":false,"context_lines":[{"line_number":863,"context_line":"                                mount_point)"},{"line_number":864,"context_line":""},{"line_number":865,"context_line":"    def test_detach_vmdk_disk_from_vm(self):"},{"line_number":866,"context_line":"        def fake_attach_disk_to_vm(*args, **kwargs):"},{"line_number":867,"context_line":"            pass"},{"line_number":868,"context_line":""},{"line_number":869,"context_line":"        def fake_detach_disk_from_vm(volume_ref, instance, original_device,"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2F0fk%3D","line":866,"in_reply_to":"AAAATn%2F%2F0gU%3D","updated":"2013-10-28 14:04:39.000000000","message":"Furthermore, with mox it will at least verify that the stubbed methods are called.  By changing to these stubs here and not using mox, you lose the verification that the stubs are even called which is a regression in test coverage in my opinion.","commit_id":"b6154ac09673e401dd2f92e08cb3921bcad15b5c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"74cc240be83d4168b0d25e2be807fb9601e63a78","unresolved":false,"context_lines":[{"line_number":863,"context_line":"                                mount_point)"},{"line_number":864,"context_line":""},{"line_number":865,"context_line":"    def test_detach_vmdk_disk_from_vm(self):"},{"line_number":866,"context_line":"        def fake_attach_disk_to_vm(*args, **kwargs):"},{"line_number":867,"context_line":"            pass"},{"line_number":868,"context_line":""},{"line_number":869,"context_line":"        def fake_detach_disk_from_vm(volume_ref, instance, original_device,"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2F0gU%3D","line":866,"in_reply_to":"AAAATn%2F%2F2HA%3D","updated":"2013-10-28 14:01:29.000000000","message":"At least with mox it\u0027s verifying that the instance you\u0027re testing with is the one passed to the method (see line 883 in the original code).  So it\u0027s one thing to use stubs, but you should be asserting/verifying what you can and only ignore the args that you have to, like how the code uses reflection to call the vim_util.get_dynamic_property method.  I agree that can be stubbed/mocked out.\n\nBut in the fake_detach_disk_from_vm stub here, it\u0027s not even asserting the instance you\u0027re passing is what you expect or the destroy_disk value is what you expect.  Seems if you\u0027re going to just stub and ignore stuff, you could have left the mox stubs that were already in place and just change line 883 to assert that destroy_disk\u003dTrue when it\u0027s called.","commit_id":"b6154ac09673e401dd2f92e08cb3921bcad15b5c"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"2649ad954bac48a09fd1cf3c8d8cefdba6ae4d17","unresolved":false,"context_lines":[{"line_number":863,"context_line":"                                mount_point)"},{"line_number":864,"context_line":""},{"line_number":865,"context_line":"    def test_detach_vmdk_disk_from_vm(self):"},{"line_number":866,"context_line":"        def fake_attach_disk_to_vm(*args, **kwargs):"},{"line_number":867,"context_line":"            pass"},{"line_number":868,"context_line":""},{"line_number":869,"context_line":"        def fake_detach_disk_from_vm(volume_ref, instance, original_device,"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAATn%2F%2F2HA%3D","line":866,"in_reply_to":"AAAATn%2F%2F2Mk%3D","updated":"2013-10-27 23:10:27.000000000","message":"Thanks Matt. Most of the parameters used in the function of _consolidate_vmdk_volume are local variables, and the actual values cannot be verified, so I used stub methods instead.\n\nOne example is:\nhw_devices \u003d self._session._call_method(vim_util,\n                                                \u0027get_dynamic_property\u0027,\n                                                volume_ref, \u0027VirtualMachine\u0027,\n                                                \u0027config.hardware.device\u0027)\n        (vmdk_file_path, controller_key, adapter_type, disk_type,\n         unit_number) \u003d vm_util.get_vmdk_path_and_adapter_type(hw_devices) \n\nFor above, if the hw_devices is not right, then vm_util.get_vmdk_path_and_adapter_type will report error.\n\nThere are also other case:\nvolume_device \u003d self.attach_disk_to_vm(volume_ref, instance,\n                                               adapter_type, disk_type,\n                                               vmdk_path\u003dcurrent_device_path,\n                                               controller_key\u003dcontroller_key,\n                                               unit_number\u003dunit_number)\n\nSo my patch was use fake_detach_disk_from_vm() to make sure the parameter change can be handled, I think that this is the key point for the unit test.\n\nFor the real logic of detach_disk_from_vm(), it is actually covered in delete_virtual_disk_spec(), hope it is OK.","commit_id":"b6154ac09673e401dd2f92e08cb3921bcad15b5c"}],"nova/tests/virt/vmwareapi/test_vmwareapi_vm_util.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"db920f0603971a9db5ec532758b8f25ad56687a3","unresolved":false,"context_lines":[{"line_number":368,"context_line":"        virtual_device_config \u003d vm_util.remove_virtual_disk_spec("},{"line_number":369,"context_line":"                                                     fake.FakeFactory(),"},{"line_number":370,"context_line":"                                                     \u0027fake_device\u0027)"},{"line_number":371,"context_line":"        self.assertEqual(virtual_device_config.operation, \u0027remove\u0027)"},{"line_number":372,"context_line":"        self.assertEqual(virtual_device_config.device, \u0027fake_device\u0027)"},{"line_number":373,"context_line":"        self.assertEqual(virtual_device_config.obj_name,"},{"line_number":374,"context_line":"                         \u0027ns0:VirtualDeviceConfigSpec\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATH%2F%2FRq0%3D","line":371,"updated":"2013-10-18 15:08:49.000000000","message":"Arguments should go (expected, actual).","commit_id":"cf36e007ad4ea523abd0d9cb41f4a76e4efd096e"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"158dcec0de8618d1b057e43ae5cab3d6fb067bc2","unresolved":false,"context_lines":[{"line_number":368,"context_line":"        virtual_device_config \u003d vm_util.remove_virtual_disk_spec("},{"line_number":369,"context_line":"                                                     fake.FakeFactory(),"},{"line_number":370,"context_line":"                                                     \u0027fake_device\u0027)"},{"line_number":371,"context_line":"        self.assertEqual(virtual_device_config.operation, \u0027remove\u0027)"},{"line_number":372,"context_line":"        self.assertEqual(virtual_device_config.device, \u0027fake_device\u0027)"},{"line_number":373,"context_line":"        self.assertEqual(virtual_device_config.obj_name,"},{"line_number":374,"context_line":"                         \u0027ns0:VirtualDeviceConfigSpec\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATH%2F%2FRlk%3D","line":371,"in_reply_to":"AAAATH%2F%2FRq0%3D","updated":"2013-10-18 15:25:59.000000000","message":"Done","commit_id":"cf36e007ad4ea523abd0d9cb41f4a76e4efd096e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"db920f0603971a9db5ec532758b8f25ad56687a3","unresolved":false,"context_lines":[{"line_number":371,"context_line":"        self.assertEqual(virtual_device_config.operation, \u0027remove\u0027)"},{"line_number":372,"context_line":"        self.assertEqual(virtual_device_config.device, \u0027fake_device\u0027)"},{"line_number":373,"context_line":"        self.assertEqual(virtual_device_config.obj_name,"},{"line_number":374,"context_line":"                         \u0027ns0:VirtualDeviceConfigSpec\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATH%2F%2FRqo%3D","line":374,"updated":"2013-10-18 15:08:49.000000000","message":"You should assert that fileOperation is not set (or is not \u0027destroy\u0027).","commit_id":"cf36e007ad4ea523abd0d9cb41f4a76e4efd096e"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"158dcec0de8618d1b057e43ae5cab3d6fb067bc2","unresolved":false,"context_lines":[{"line_number":371,"context_line":"        self.assertEqual(virtual_device_config.operation, \u0027remove\u0027)"},{"line_number":372,"context_line":"        self.assertEqual(virtual_device_config.device, \u0027fake_device\u0027)"},{"line_number":373,"context_line":"        self.assertEqual(virtual_device_config.obj_name,"},{"line_number":374,"context_line":"                         \u0027ns0:VirtualDeviceConfigSpec\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATH%2F%2FRlg%3D","line":374,"in_reply_to":"AAAATH%2F%2FRqo%3D","updated":"2013-10-18 15:25:59.000000000","message":"Done","commit_id":"cf36e007ad4ea523abd0d9cb41f4a76e4efd096e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"38e3b0f18e5807635329a6cfab916534ed2a0913","unresolved":false,"context_lines":[{"line_number":373,"context_line":"        self.assertEqual(\u0027fake_device\u0027, virtual_device_config.device)"},{"line_number":374,"context_line":"        self.assertEqual(\u0027ns0:VirtualDeviceConfigSpec\u0027,"},{"line_number":375,"context_line":"                         virtual_device_config.obj_name)"},{"line_number":376,"context_line":""},{"line_number":377,"context_line":"    def test_detach_virtual_disk_destroy_spec(self):"},{"line_number":378,"context_line":"        virtual_device_config \u003d vm_util.detach_virtual_disk_spec("},{"line_number":379,"context_line":"                                                     fake.FakeFactory(),"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATH%2F%2FJDs%3D","line":376,"updated":"2013-10-22 15:45:36.000000000","message":"What happened to checking if fileOperation is set in virtual_device_config?  I asked for that in patch set 1 and you added it in patch set 2, removed in patch set 3.  It probably blew up because the attribute didn\u0027t exist, but you could use the hasattr built-in method for that check.","commit_id":"d354b6f869d111d3c1aeb4e0fca8b9a98a631e80"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"abc9095455204514e9a0dfe72f490bb0af2d6033","unresolved":false,"context_lines":[{"line_number":373,"context_line":"        self.assertEqual(\u0027fake_device\u0027, virtual_device_config.device)"},{"line_number":374,"context_line":"        self.assertEqual(\u0027ns0:VirtualDeviceConfigSpec\u0027,"},{"line_number":375,"context_line":"                         virtual_device_config.obj_name)"},{"line_number":376,"context_line":""},{"line_number":377,"context_line":"    def test_detach_virtual_disk_destroy_spec(self):"},{"line_number":378,"context_line":"        virtual_device_config \u003d vm_util.detach_virtual_disk_spec("},{"line_number":379,"context_line":"                                                     fake.FakeFactory(),"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATX%2F%2F%2FoA%3D","line":376,"in_reply_to":"AAAATH%2F%2FJDs%3D","updated":"2013-10-23 07:04:23.000000000","message":"Done","commit_id":"d354b6f869d111d3c1aeb4e0fca8b9a98a631e80"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"dd99ec3222a58445eedf26d325ec27b90229df77","unresolved":false,"context_lines":[{"line_number":364,"context_line":"        self.assertEqual(\"else\", propdict[\u0027some.thing\u0027].value)"},{"line_number":365,"context_line":"        self.assertEqual(\"value\", propdict[\u0027another.thing\u0027])"},{"line_number":366,"context_line":""},{"line_number":367,"context_line":"    def test_detach_virtual_disk_spec(self):"},{"line_number":368,"context_line":"        virtual_device_config \u003d vm_util.detach_virtual_disk_spec("},{"line_number":369,"context_line":"                                                     fake.FakeFactory(),"},{"line_number":370,"context_line":"                                                     \u0027fake_device\u0027,"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATX%2F%2F%2BLg%3D","line":367,"updated":"2013-10-23 13:52:37.000000000","message":"quite a lot of code is duplicated here. how about having a common function that received the destroy_disk as a flag. If this is true then you check the file opertaion","commit_id":"17be4060683d219a80bfb0ed8bd5b5c425a7a893"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"34dd91309126856ec0f3ae4c749ce69f8783af30","unresolved":false,"context_lines":[{"line_number":364,"context_line":"        self.assertEqual(\"else\", propdict[\u0027some.thing\u0027].value)"},{"line_number":365,"context_line":"        self.assertEqual(\"value\", propdict[\u0027another.thing\u0027])"},{"line_number":366,"context_line":""},{"line_number":367,"context_line":"    def test_detach_virtual_disk_spec(self):"},{"line_number":368,"context_line":"        virtual_device_config \u003d vm_util.detach_virtual_disk_spec("},{"line_number":369,"context_line":"                                                     fake.FakeFactory(),"},{"line_number":370,"context_line":"                                                     \u0027fake_device\u0027,"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATX%2F%2F97o%3D","line":367,"in_reply_to":"AAAATX%2F%2F%2BLg%3D","updated":"2013-10-23 14:47:53.000000000","message":"+1","commit_id":"17be4060683d219a80bfb0ed8bd5b5c425a7a893"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"970cf3441d28fbfc13c1a62a3a451517e05dcf8e","unresolved":false,"context_lines":[{"line_number":364,"context_line":"        self.assertEqual(\"else\", propdict[\u0027some.thing\u0027].value)"},{"line_number":365,"context_line":"        self.assertEqual(\"value\", propdict[\u0027another.thing\u0027])"},{"line_number":366,"context_line":""},{"line_number":367,"context_line":"    def test_detach_virtual_disk_spec(self):"},{"line_number":368,"context_line":"        virtual_device_config \u003d vm_util.detach_virtual_disk_spec("},{"line_number":369,"context_line":"                                                     fake.FakeFactory(),"},{"line_number":370,"context_line":"                                                     \u0027fake_device\u0027,"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2F7Rw%3D","line":367,"in_reply_to":"AAAATX%2F%2F97o%3D","updated":"2013-10-25 05:41:47.000000000","message":"Done","commit_id":"17be4060683d219a80bfb0ed8bd5b5c425a7a893"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"60f2dce13a4650ae64f5721dff503da9383a728b","unresolved":false,"context_lines":[{"line_number":373,"context_line":"        self.assertEqual(\u0027fake_device\u0027, virtual_device_config.device)"},{"line_number":374,"context_line":"        self.assertEqual(\u0027ns0:VirtualDeviceConfigSpec\u0027,"},{"line_number":375,"context_line":"                         virtual_device_config.obj_name)"},{"line_number":376,"context_line":"        self.assertFalse(hasattr(virtual_device_config, \u0027obj\u0027))"},{"line_number":377,"context_line":""},{"line_number":378,"context_line":"    def test_detach_virtual_disk_destroy_spec(self):"},{"line_number":379,"context_line":"        virtual_device_config \u003d vm_util.detach_virtual_disk_spec("}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATX%2F%2F%2BQo%3D","line":376,"updated":"2013-10-23 13:35:18.000000000","message":"obj?  This should be \u0027fileOperation\u0027.  See line 384.","commit_id":"17be4060683d219a80bfb0ed8bd5b5c425a7a893"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"970cf3441d28fbfc13c1a62a3a451517e05dcf8e","unresolved":false,"context_lines":[{"line_number":373,"context_line":"        self.assertEqual(\u0027fake_device\u0027, virtual_device_config.device)"},{"line_number":374,"context_line":"        self.assertEqual(\u0027ns0:VirtualDeviceConfigSpec\u0027,"},{"line_number":375,"context_line":"                         virtual_device_config.obj_name)"},{"line_number":376,"context_line":"        self.assertFalse(hasattr(virtual_device_config, \u0027obj\u0027))"},{"line_number":377,"context_line":""},{"line_number":378,"context_line":"    def test_detach_virtual_disk_destroy_spec(self):"},{"line_number":379,"context_line":"        virtual_device_config \u003d vm_util.detach_virtual_disk_spec("}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAATn%2F%2F7Rs%3D","line":376,"in_reply_to":"AAAATX%2F%2F%2BQo%3D","updated":"2013-10-25 05:41:47.000000000","message":"Done","commit_id":"17be4060683d219a80bfb0ed8bd5b5c425a7a893"}],"nova/tests/virt/vmwareapi/test_vmwareapi_volumeops.py":[{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"01aba8a4e5cbfcd46baf6a79b58a6bf42f89deaa","unresolved":false,"context_lines":[{"line_number":13,"context_line":"#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the"},{"line_number":14,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":15,"context_line":"#    under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from nova import test"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from nova.virt.vmwareapi import driver"}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2Ft1o%3D","line":16,"updated":"2013-10-30 01:49:08.000000000","message":"@Matt, I created this new file to test all cases in volumeops.py for vmware, we can add more unit test cases in future. Thanks.","commit_id":"998b6a4a20bbd47e89da4c44be6656d6ab8b0d72"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"015bf36b89aad5128d4184fd3438b966380884a6","unresolved":false,"context_lines":[{"line_number":15,"context_line":"#    under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from nova import test"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from nova.virt.vmwareapi import driver"},{"line_number":20,"context_line":"from nova.virt.vmwareapi import fake as vmwareapi_fake"},{"line_number":21,"context_line":"from nova.virt.vmwareapi import volumeops"}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2FobI%3D","line":18,"updated":"2013-10-31 06:39:11.000000000","message":"new line is not needed here","commit_id":"998b6a4a20bbd47e89da4c44be6656d6ab8b0d72"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"17c11f3b89a7e4ed5a5d2bc5937d89eaa0c5b614","unresolved":false,"context_lines":[{"line_number":15,"context_line":"#    under the License."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"from nova import test"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from nova.virt.vmwareapi import driver"},{"line_number":20,"context_line":"from nova.virt.vmwareapi import fake as vmwareapi_fake"},{"line_number":21,"context_line":"from nova.virt.vmwareapi import volumeops"}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2FoWs%3D","line":18,"in_reply_to":"AAAATn%2F%2FobI%3D","updated":"2013-10-31 07:01:26.000000000","message":"Done","commit_id":"998b6a4a20bbd47e89da4c44be6656d6ab8b0d72"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"015bf36b89aad5128d4184fd3438b966380884a6","unresolved":false,"context_lines":[{"line_number":19,"context_line":"from nova.virt.vmwareapi import driver"},{"line_number":20,"context_line":"from nova.virt.vmwareapi import fake as vmwareapi_fake"},{"line_number":21,"context_line":"from nova.virt.vmwareapi import volumeops"},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"from nova.tests.virt.vmwareapi import stubs"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2FobA%3D","line":22,"updated":"2013-10-31 06:39:11.000000000","message":"ditto","commit_id":"998b6a4a20bbd47e89da4c44be6656d6ab8b0d72"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"17c11f3b89a7e4ed5a5d2bc5937d89eaa0c5b614","unresolved":false,"context_lines":[{"line_number":19,"context_line":"from nova.virt.vmwareapi import driver"},{"line_number":20,"context_line":"from nova.virt.vmwareapi import fake as vmwareapi_fake"},{"line_number":21,"context_line":"from nova.virt.vmwareapi import volumeops"},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"from nova.tests.virt.vmwareapi import stubs"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAATn%2F%2FoWo%3D","line":22,"in_reply_to":"AAAATn%2F%2FobA%3D","updated":"2013-10-31 07:01:26.000000000","message":"Done","commit_id":"998b6a4a20bbd47e89da4c44be6656d6ab8b0d72"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f13519ba4006922257745a22acc63c7d5992cf28","unresolved":false,"context_lines":[{"line_number":32,"context_line":"        self._volumeops \u003d volumeops.VMwareVolumeOps(self._session)"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    def _test_detach_disk_from_vm(self, destroy_disk\u003dFalse):"},{"line_number":35,"context_line":"        def fake_wait_for_task(instance_uuid, task_ref):"},{"line_number":36,"context_line":"            pass"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        self.stubs.Set(self._session, \u0027_wait_for_task\u0027, fake_wait_for_task)"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAATn%2F%2FaCY%3D","line":35,"updated":"2013-11-07 02:50:51.000000000","message":"Prefer to use mock here.","commit_id":"bf8f156d7530f4f9bbc3bd490cc73c90a079ede9"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"1d977f8f2f68aeb4fa0909724062add00cbcc58c","unresolved":false,"context_lines":[{"line_number":32,"context_line":"        self._volumeops \u003d volumeops.VMwareVolumeOps(self._session)"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    def _test_detach_disk_from_vm(self, destroy_disk\u003dFalse):"},{"line_number":35,"context_line":"        def fake_wait_for_task(instance_uuid, task_ref):"},{"line_number":36,"context_line":"            pass"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"        self.stubs.Set(self._session, \u0027_wait_for_task\u0027, fake_wait_for_task)"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAATn%2F%2FVHI%3D","line":35,"in_reply_to":"AAAATn%2F%2FaCY%3D","updated":"2013-11-10 09:28:04.000000000","message":"Done","commit_id":"bf8f156d7530f4f9bbc3bd490cc73c90a079ede9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f13519ba4006922257745a22acc63c7d5992cf28","unresolved":false,"context_lines":[{"line_number":46,"context_line":"                                            fake_device, destroy_disk)"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"    def test_detach_with_destroy_disk_from_vm(self):"},{"line_number":49,"context_line":"        def fake_call_method(module, method, *args, **kwargs):"},{"line_number":50,"context_line":"            vmdk_detach_config_spec \u003d kwargs.get(\u0027spec\u0027)"},{"line_number":51,"context_line":"            virtual_device_config \u003d vmdk_detach_config_spec.deviceChange[0]"},{"line_number":52,"context_line":"            self.assertEqual(\u0027remove\u0027, virtual_device_config.operation)"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAATn%2F%2FaCk%3D","line":49,"updated":"2013-11-07 02:50:51.000000000","message":"Is it possible to use mock here instead of stubs so you can confirm the method is actually called?","commit_id":"bf8f156d7530f4f9bbc3bd490cc73c90a079ede9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8ab1ad761101ec123f165b728e2b0911276bd8d7","unresolved":false,"context_lines":[{"line_number":46,"context_line":"                                            fake_device, destroy_disk)"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"    def test_detach_with_destroy_disk_from_vm(self):"},{"line_number":49,"context_line":"        def fake_call_method(module, method, *args, **kwargs):"},{"line_number":50,"context_line":"            vmdk_detach_config_spec \u003d kwargs.get(\u0027spec\u0027)"},{"line_number":51,"context_line":"            virtual_device_config \u003d vmdk_detach_config_spec.deviceChange[0]"},{"line_number":52,"context_line":"            self.assertEqual(\u0027remove\u0027, virtual_device_config.operation)"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAATn%2F%2FR18%3D","line":49,"in_reply_to":"AAAATn%2F%2FVHE%3D","updated":"2013-11-11 20:03:07.000000000","message":"You still can, see how Chris does it by stubbing out the _service_update method here and uses it with the mock, and does assertions within the stubbed method:\n\nhttps://review.openstack.org/#/c/54597/4/nova/tests/test_nova_manage.py","commit_id":"bf8f156d7530f4f9bbc3bd490cc73c90a079ede9"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"1d977f8f2f68aeb4fa0909724062add00cbcc58c","unresolved":false,"context_lines":[{"line_number":46,"context_line":"                                            fake_device, destroy_disk)"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"    def test_detach_with_destroy_disk_from_vm(self):"},{"line_number":49,"context_line":"        def fake_call_method(module, method, *args, **kwargs):"},{"line_number":50,"context_line":"            vmdk_detach_config_spec \u003d kwargs.get(\u0027spec\u0027)"},{"line_number":51,"context_line":"            virtual_device_config \u003d vmdk_detach_config_spec.deviceChange[0]"},{"line_number":52,"context_line":"            self.assertEqual(\u0027remove\u0027, virtual_device_config.operation)"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAATn%2F%2FVHE%3D","line":49,"in_reply_to":"AAAATn%2F%2FaCk%3D","updated":"2013-11-10 09:28:04.000000000","message":"I would suggest that we still use stub here as I need to check some variables in the fake method.","commit_id":"bf8f156d7530f4f9bbc3bd490cc73c90a079ede9"},{"author":{"_account_id":5638,"name":"Davanum Srinivas","email":"davanum@gmail.com","username":"dims-v"},"change_message_id":"335ff83b08dc982a54c49c13526f78d778caaa6f","unresolved":false,"context_lines":[{"line_number":46,"context_line":"                                            fake_device, destroy_disk)"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"    def test_detach_with_destroy_disk_from_vm(self):"},{"line_number":49,"context_line":"        def fake_call_method(module, method, *args, **kwargs):"},{"line_number":50,"context_line":"            vmdk_detach_config_spec \u003d kwargs.get(\u0027spec\u0027)"},{"line_number":51,"context_line":"            virtual_device_config \u003d vmdk_detach_config_spec.deviceChange[0]"},{"line_number":52,"context_line":"            self.assertEqual(\u0027remove\u0027, virtual_device_config.operation)"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAATn%2F%2FXuI%3D","line":49,"in_reply_to":"AAAATn%2F%2FaCk%3D","updated":"2013-11-08 02:48:34.000000000","message":"quick tip - If you grep the tests for assert_has_calls, you can see some examples with mock that ensures that specific calls are made","commit_id":"bf8f156d7530f4f9bbc3bd490cc73c90a079ede9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f13519ba4006922257745a22acc63c7d5992cf28","unresolved":false,"context_lines":[{"line_number":58,"context_line":"        self._test_detach_disk_from_vm(destroy_disk\u003dTrue)"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    def test_detach_without_destroy_disk_from_vm(self):"},{"line_number":61,"context_line":"        def fake_call_method(module, method, *args, **kwargs):"},{"line_number":62,"context_line":"            vmdk_detach_config_spec \u003d kwargs.get(\u0027spec\u0027)"},{"line_number":63,"context_line":"            virtual_device_config \u003d vmdk_detach_config_spec.deviceChange[0]"},{"line_number":64,"context_line":"            self.assertEqual(\u0027remove\u0027, virtual_device_config.operation)"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAATn%2F%2FaCc%3D","line":61,"updated":"2013-11-07 02:50:51.000000000","message":"Same here, would prefer that you use mock so we can assert the mocked method is called.","commit_id":"bf8f156d7530f4f9bbc3bd490cc73c90a079ede9"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"1d977f8f2f68aeb4fa0909724062add00cbcc58c","unresolved":false,"context_lines":[{"line_number":58,"context_line":"        self._test_detach_disk_from_vm(destroy_disk\u003dTrue)"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    def test_detach_without_destroy_disk_from_vm(self):"},{"line_number":61,"context_line":"        def fake_call_method(module, method, *args, **kwargs):"},{"line_number":62,"context_line":"            vmdk_detach_config_spec \u003d kwargs.get(\u0027spec\u0027)"},{"line_number":63,"context_line":"            virtual_device_config \u003d vmdk_detach_config_spec.deviceChange[0]"},{"line_number":64,"context_line":"            self.assertEqual(\u0027remove\u0027, virtual_device_config.operation)"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAATn%2F%2FVHA%3D","line":61,"in_reply_to":"AAAATn%2F%2FaCc%3D","updated":"2013-11-10 09:28:04.000000000","message":"ditto","commit_id":"bf8f156d7530f4f9bbc3bd490cc73c90a079ede9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"92330626187eac900e218cc8c5a5cbd586d882a8","unresolved":false,"context_lines":[{"line_number":36,"context_line":"        self.instance \u003d {\u0027name\u0027: \u0027fake_name\u0027, \u0027uuid\u0027: \u0027fake_uuid\u0027}"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def _test_detach_disk_from_vm(self, destroy_disk\u003dFalse):"},{"line_number":39,"context_line":"        with contextlib.nested("},{"line_number":40,"context_line":"            mock.patch.object(self._session, \u0027_wait_for_task\u0027)"},{"line_number":41,"context_line":"        ) as (_wait_for_task):"},{"line_number":42,"context_line":"            fake_device \u003d vmwareapi_fake.DataObject()"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAATn%2F%2FR14%3D","line":39,"updated":"2013-11-11 20:03:24.000000000","message":"Shouldn\u0027t need a nested context for the one mock.","commit_id":"b78820cefae424962e3e2e61a33fc135528f8582"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"d4bc421930b8068298404f372d8254faed1bdbcf","unresolved":false,"context_lines":[{"line_number":36,"context_line":"        self.instance \u003d {\u0027name\u0027: \u0027fake_name\u0027, \u0027uuid\u0027: \u0027fake_uuid\u0027}"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def _test_detach_disk_from_vm(self, destroy_disk\u003dFalse):"},{"line_number":39,"context_line":"        with contextlib.nested("},{"line_number":40,"context_line":"            mock.patch.object(self._session, \u0027_wait_for_task\u0027)"},{"line_number":41,"context_line":"        ) as (_wait_for_task):"},{"line_number":42,"context_line":"            fake_device \u003d vmwareapi_fake.DataObject()"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAATn%2F%2FPj8%3D","line":39,"in_reply_to":"AAAATn%2F%2FR14%3D","updated":"2013-11-12 11:22:12.000000000","message":"Done","commit_id":"b78820cefae424962e3e2e61a33fc135528f8582"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"92330626187eac900e218cc8c5a5cbd586d882a8","unresolved":false,"context_lines":[{"line_number":49,"context_line":"                mock.call(self.instance[\u0027uuid\u0027], \u0027fake_configure_task\u0027)])"},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"    def test_detach_with_destroy_disk_from_vm(self):"},{"line_number":52,"context_line":"        def fake_call_method(module, method, *args, **kwargs):"},{"line_number":53,"context_line":"            vmdk_detach_config_spec \u003d kwargs.get(\u0027spec\u0027)"},{"line_number":54,"context_line":"            virtual_device_config \u003d vmdk_detach_config_spec.deviceChange[0]"},{"line_number":55,"context_line":"            self.assertEqual(\u0027remove\u0027, virtual_device_config.operation)"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAATn%2F%2FR1w%3D","line":52,"updated":"2013-11-11 20:03:24.000000000","message":"See my comments in the previous patch set about asserting within stubs but integrating that with mock.","commit_id":"b78820cefae424962e3e2e61a33fc135528f8582"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"d4bc421930b8068298404f372d8254faed1bdbcf","unresolved":false,"context_lines":[{"line_number":49,"context_line":"                mock.call(self.instance[\u0027uuid\u0027], \u0027fake_configure_task\u0027)])"},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"    def test_detach_with_destroy_disk_from_vm(self):"},{"line_number":52,"context_line":"        def fake_call_method(module, method, *args, **kwargs):"},{"line_number":53,"context_line":"            vmdk_detach_config_spec \u003d kwargs.get(\u0027spec\u0027)"},{"line_number":54,"context_line":"            virtual_device_config \u003d vmdk_detach_config_spec.deviceChange[0]"},{"line_number":55,"context_line":"            self.assertEqual(\u0027remove\u0027, virtual_device_config.operation)"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAATn%2F%2FPj4%3D","line":52,"in_reply_to":"AAAATn%2F%2FR1w%3D","updated":"2013-11-12 11:22:12.000000000","message":"Done","commit_id":"b78820cefae424962e3e2e61a33fc135528f8582"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"80d8a165bbe21757498e8abb23b647a751b50418","unresolved":false,"context_lines":[{"line_number":44,"context_line":"            self._volumeops.detach_disk_from_vm(\u0027fake_vm_ref\u0027, self.instance,"},{"line_number":45,"context_line":"                                                fake_device, destroy_disk)"},{"line_number":46,"context_line":"            _wait_for_task.assert_has_calls(["},{"line_number":47,"context_line":"                   mock.call(self.instance[\u0027uuid\u0027], \u0027fake_configure_task\u0027)])"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"    def test_detach_with_destroy_disk_from_vm(self):"},{"line_number":50,"context_line":"        def fake_call_method(module, method, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":13,"id":"AAAATn%2F%2FOuU%3D","line":47,"updated":"2013-11-12 15:06:07.000000000","message":"Hmm, the only problem I have with this is it relies on the test cases calling it to always be returning \u0027fake_configure_task\u0027, so you have a circular dependency in the test logic.\n\nLooking at the two tests that are calling the common method, the only difference in the fake_call_method stubs is how virtual_device_config.fileOperation is handled, and since the common method takes the destroy_disk parameter, I think you should mock the fake_call_method in this common method and handle the destroy_disk logic in the stub here, then your two public tests simply call the common method with the destroy_disk flag toggled for the different cases.","commit_id":"3a7ea11a2058cacc5217541d2cbea31212c6df05"}],"nova/virt/vmwareapi/vm_util.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"db920f0603971a9db5ec532758b8f25ad56687a3","unresolved":false,"context_lines":[{"line_number":468,"context_line":"    virtual_device_config \u003d client_factory.create("},{"line_number":469,"context_line":"                            \u0027ns0:VirtualDeviceConfigSpec\u0027)"},{"line_number":470,"context_line":"    virtual_device_config.operation \u003d \"remove\""},{"line_number":471,"context_line":"    virtual_device_config.fileOperation \u003d \"destroy\""},{"line_number":472,"context_line":"    virtual_device_config.device \u003d device"},{"line_number":473,"context_line":""},{"line_number":474,"context_line":"    return virtual_device_config"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATH%2F%2FRrI%3D","side":"PARENT","line":471,"updated":"2013-10-18 15:08:49.000000000","message":"Looks like this was added with change I25e0c79ffb0b762726fb931233f7beeb53092a34 by Sean Chen, we should probably get him to review.","commit_id":"531adcd3d9d098bd784f56ce5e9e0d423e7f8a3d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"db920f0603971a9db5ec532758b8f25ad56687a3","unresolved":false,"context_lines":[{"line_number":463,"context_line":""},{"line_number":464,"context_line":"def remove_virtual_disk_spec(client_factory, device):"},{"line_number":465,"context_line":"    \"\"\""},{"line_number":466,"context_line":"    Builds spec for the remove of an already existing Virtual Disk from VM."},{"line_number":467,"context_line":"    \"\"\""},{"line_number":468,"context_line":"    virtual_device_config \u003d client_factory.create("},{"line_number":469,"context_line":"                            \u0027ns0:VirtualDeviceConfigSpec\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATH%2F%2FRsE%3D","line":466,"updated":"2013-10-18 15:08:49.000000000","message":"s/remove/removal/","commit_id":"cf36e007ad4ea523abd0d9cb41f4a76e4efd096e"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"158dcec0de8618d1b057e43ae5cab3d6fb067bc2","unresolved":false,"context_lines":[{"line_number":463,"context_line":""},{"line_number":464,"context_line":"def remove_virtual_disk_spec(client_factory, device):"},{"line_number":465,"context_line":"    \"\"\""},{"line_number":466,"context_line":"    Builds spec for the remove of an already existing Virtual Disk from VM."},{"line_number":467,"context_line":"    \"\"\""},{"line_number":468,"context_line":"    virtual_device_config \u003d client_factory.create("},{"line_number":469,"context_line":"                            \u0027ns0:VirtualDeviceConfigSpec\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAATH%2F%2FRlY%3D","line":466,"in_reply_to":"AAAATH%2F%2FRsE%3D","updated":"2013-10-18 15:25:59.000000000","message":"Done","commit_id":"cf36e007ad4ea523abd0d9cb41f4a76e4efd096e"},{"author":{"_account_id":5149,"name":"Sean Chen","email":"xuchenx@gmail.com","username":"opencompute"},"change_message_id":"9fcbef67cc7aebe8d66d8fc828823b1e58df9085","unresolved":false,"context_lines":[{"line_number":260,"context_line":"    config_spec \u003d client_factory.create(\u0027ns0:VirtualMachineConfigSpec\u0027)"},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"    device_config_spec \u003d []"},{"line_number":263,"context_line":"    virtual_device_config_spec \u003d delete_virtual_disk_spec(client_factory,"},{"line_number":264,"context_line":"                                                          device)"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    device_config_spec.append(virtual_device_config_spec)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAATH%2F%2FRWM%3D","side":"PARENT","line":263,"updated":"2013-10-18 16:36:03.000000000","message":"There are two types of attached disks, VMDK (see _attach_volume_vmdk) or iSCSI (see _attach_volume_iscsi).\n\nI agree for VMDK disks, we should not detroy the VMDK file.\n\nBut for iSCSI volumes attached as RDM disk, an RDM vmdk file was created to attached to the VM, the file contains only iSCSI volume information that describes the mapping to the LUN on the ESXi host. See create_virtual_disk_spec and attach_disk_to_vm. So we would like to detroy the VMDK we created.","commit_id":"531adcd3d9d098bd784f56ce5e9e0d423e7f8a3d"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"448f894848c744674f13fc495ed9c7eaa85afbb4","unresolved":false,"context_lines":[{"line_number":255,"context_line":"    return config_spec"},{"line_number":256,"context_line":""},{"line_number":257,"context_line":""},{"line_number":258,"context_line":"def get_vmdk_detach_config_spec(client_factory, device):"},{"line_number":259,"context_line":"    \"\"\"Builds the vmdk detach config spec.\"\"\""},{"line_number":260,"context_line":"    config_spec \u003d client_factory.create(\u0027ns0:VirtualMachineConfigSpec\u0027)"},{"line_number":261,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATH%2F%2FRVQ%3D","line":258,"updated":"2013-10-18 16:41:07.000000000","message":"I agree with Sean. You should have a flag here which indicates if this should be removed.","commit_id":"af5ae5d97d298f4688b40551d52d3cd05c853c85"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"634c53c2699dabcdf04bd42ccdd18295e81024a8","unresolved":false,"context_lines":[{"line_number":255,"context_line":"    return config_spec"},{"line_number":256,"context_line":""},{"line_number":257,"context_line":""},{"line_number":258,"context_line":"def get_vmdk_detach_config_spec(client_factory, device):"},{"line_number":259,"context_line":"    \"\"\"Builds the vmdk detach config spec.\"\"\""},{"line_number":260,"context_line":"    config_spec \u003d client_factory.create(\u0027ns0:VirtualMachineConfigSpec\u0027)"},{"line_number":261,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATH%2F%2FQYw%3D","line":258,"in_reply_to":"AAAATH%2F%2FRVQ%3D","updated":"2013-10-18 23:54:54.000000000","message":"Done","commit_id":"af5ae5d97d298f4688b40551d52d3cd05c853c85"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"f526376b2ee93a936490c87117d5d6e73aa219c6","unresolved":false,"context_lines":[{"line_number":260,"context_line":"    config_spec \u003d client_factory.create(\u0027ns0:VirtualMachineConfigSpec\u0027)"},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"    device_config_spec \u003d []"},{"line_number":263,"context_line":"    virtual_device_config_spec \u003d remove_virtual_disk_spec(client_factory,"},{"line_number":264,"context_line":"                                                          device)"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    device_config_spec.append(virtual_device_config_spec)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATH%2F%2FRWI%3D","line":263,"updated":"2013-10-18 16:38:11.000000000","message":"consider renaming to detach_virtual_disk_spec","commit_id":"af5ae5d97d298f4688b40551d52d3cd05c853c85"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"634c53c2699dabcdf04bd42ccdd18295e81024a8","unresolved":false,"context_lines":[{"line_number":260,"context_line":"    config_spec \u003d client_factory.create(\u0027ns0:VirtualMachineConfigSpec\u0027)"},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"    device_config_spec \u003d []"},{"line_number":263,"context_line":"    virtual_device_config_spec \u003d remove_virtual_disk_spec(client_factory,"},{"line_number":264,"context_line":"                                                          device)"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    device_config_spec.append(virtual_device_config_spec)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAATH%2F%2FQYs%3D","line":263,"in_reply_to":"AAAATH%2F%2FRWI%3D","updated":"2013-10-18 23:54:54.000000000","message":"Done","commit_id":"af5ae5d97d298f4688b40551d52d3cd05c853c85"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"38e3b0f18e5807635329a6cfab916534ed2a0913","unresolved":false,"context_lines":[{"line_number":470,"context_line":"    virtual_device_config \u003d client_factory.create("},{"line_number":471,"context_line":"                            \u0027ns0:VirtualDeviceConfigSpec\u0027)"},{"line_number":472,"context_line":"    virtual_device_config.operation \u003d \"remove\""},{"line_number":473,"context_line":"    if destroy_disk:"},{"line_number":474,"context_line":"        virtual_device_config.fileOperation \u003d \"destroy\""},{"line_number":475,"context_line":"    virtual_device_config.device \u003d device"},{"line_number":476,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATH%2F%2FJBw%3D","line":473,"updated":"2013-10-22 15:45:36.000000000","message":"Rather than add a new arg, isn\u0027t there any way to tell if we need to do this from the device itself?","commit_id":"d354b6f869d111d3c1aeb4e0fca8b9a98a631e80"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"cb6e5c79c139754779250c69bca7c3710e3625df","unresolved":false,"context_lines":[{"line_number":470,"context_line":"    virtual_device_config \u003d client_factory.create("},{"line_number":471,"context_line":"                            \u0027ns0:VirtualDeviceConfigSpec\u0027)"},{"line_number":472,"context_line":"    virtual_device_config.operation \u003d \"remove\""},{"line_number":473,"context_line":"    if destroy_disk:"},{"line_number":474,"context_line":"        virtual_device_config.fileOperation \u003d \"destroy\""},{"line_number":475,"context_line":"    virtual_device_config.device \u003d device"},{"line_number":476,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATX%2F%2F%2Fp4%3D","line":473,"in_reply_to":"AAAATH%2F%2FJBw%3D","updated":"2013-10-23 06:24:52.000000000","message":"I do not think that the device will help here. An example is the vmdk. When a cinder volume is detached we do not want to destory the disk as it is owned by Cinder. When a consolidation is done we do want to destroy the disk.","commit_id":"d354b6f869d111d3c1aeb4e0fca8b9a98a631e80"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"abc9095455204514e9a0dfe72f490bb0af2d6033","unresolved":false,"context_lines":[{"line_number":470,"context_line":"    virtual_device_config \u003d client_factory.create("},{"line_number":471,"context_line":"                            \u0027ns0:VirtualDeviceConfigSpec\u0027)"},{"line_number":472,"context_line":"    virtual_device_config.operation \u003d \"remove\""},{"line_number":473,"context_line":"    if destroy_disk:"},{"line_number":474,"context_line":"        virtual_device_config.fileOperation \u003d \"destroy\""},{"line_number":475,"context_line":"    virtual_device_config.device \u003d device"},{"line_number":476,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATX%2F%2F%2Fj4%3D","line":473,"in_reply_to":"AAAATX%2F%2F%2Fp4%3D","updated":"2013-10-23 07:04:23.000000000","message":"Thanks Gary and Matt. Yes, agree with Gary, also seems using destroy_disk is more easy, as we can just set this flag in the function of _detach_volume_xxxx.","commit_id":"d354b6f869d111d3c1aeb4e0fca8b9a98a631e80"}],"nova/virt/vmwareapi/volumeops.py":[{"author":{"_account_id":5149,"name":"Sean Chen","email":"xuchenx@gmail.com","username":"opencompute"},"change_message_id":"391f6c8fcaa337180447a80e914016827a08972b","unresolved":false,"context_lines":[{"line_number":111,"context_line":"                if option.key \u003d\u003d volume_option:"},{"line_number":112,"context_line":"                    return option.value"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    def detach_disk_from_vm(self, vm_ref, instance, device):"},{"line_number":115,"context_line":"        \"\"\""},{"line_number":116,"context_line":"        Detach disk from VM by reconfiguration."},{"line_number":117,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAATH%2F%2FQW4%3D","side":"PARENT","line":114,"updated":"2013-10-19 00:25:15.000000000","message":"I suggest naming the option destroy_disk, and defaults to False\n\ndestroy_disk\u003dFalse","commit_id":"2b9c147626ff82c86a400c420b928bf2e321c531"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"f24de951f6708f9e7549cca9326879e4197690df","unresolved":false,"context_lines":[{"line_number":111,"context_line":"                if option.key \u003d\u003d volume_option:"},{"line_number":112,"context_line":"                    return option.value"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    def detach_disk_from_vm(self, vm_ref, instance, device):"},{"line_number":115,"context_line":"        \"\"\""},{"line_number":116,"context_line":"        Detach disk from VM by reconfiguration."},{"line_number":117,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAATH%2F%2FQWM%3D","side":"PARENT","line":114,"in_reply_to":"AAAATH%2F%2FQW4%3D","updated":"2013-10-19 00:56:50.000000000","message":"Done","commit_id":"2b9c147626ff82c86a400c420b928bf2e321c531"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"38e3b0f18e5807635329a6cfab916534ed2a0913","unresolved":false,"context_lines":[{"line_number":112,"context_line":"                    return option.value"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    def detach_disk_from_vm(self, vm_ref, instance, device,"},{"line_number":115,"context_line":"                            destroy_disk\u003dFalse):"},{"line_number":116,"context_line":"        \"\"\""},{"line_number":117,"context_line":"        Detach disk from VM by reconfiguration."},{"line_number":118,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATH%2F%2FJCY%3D","line":115,"updated":"2013-10-22 15:45:36.000000000","message":"Isn\u0027t there some metadata you could check so we don\u0027t need an arg here and know in which cases to set it to True?","commit_id":"d354b6f869d111d3c1aeb4e0fca8b9a98a631e80"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"38e3b0f18e5807635329a6cfab916534ed2a0913","unresolved":false,"context_lines":[{"line_number":361,"context_line":""},{"line_number":362,"context_line":"        # Delete the original disk from the volume_ref"},{"line_number":363,"context_line":"        self.detach_disk_from_vm(volume_ref, instance, original_device,"},{"line_number":364,"context_line":"                                 destroy_disk\u003dTrue)"},{"line_number":365,"context_line":"        # Attach the current disk to the volume_ref"},{"line_number":366,"context_line":"        # Get details required for adding disk device such as"},{"line_number":367,"context_line":"        # adapter_type, unit_number, controller_key"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATH%2F%2FJCA%3D","line":364,"updated":"2013-10-22 15:45:36.000000000","message":"I don\u0027t see a backing unit test for this, I only see a change in the test module for the iscsi change.","commit_id":"d354b6f869d111d3c1aeb4e0fca8b9a98a631e80"},{"author":{"_account_id":7494,"name":"Guangya Liu","email":"gyliu513@gmail.com","username":"JayLau"},"change_message_id":"abc9095455204514e9a0dfe72f490bb0af2d6033","unresolved":false,"context_lines":[{"line_number":361,"context_line":""},{"line_number":362,"context_line":"        # Delete the original disk from the volume_ref"},{"line_number":363,"context_line":"        self.detach_disk_from_vm(volume_ref, instance, original_device,"},{"line_number":364,"context_line":"                                 destroy_disk\u003dTrue)"},{"line_number":365,"context_line":"        # Attach the current disk to the volume_ref"},{"line_number":366,"context_line":"        # Get details required for adding disk device such as"},{"line_number":367,"context_line":"        # adapter_type, unit_number, controller_key"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATX%2F%2F%2FkY%3D","line":364,"in_reply_to":"AAAATH%2F%2FJCA%3D","updated":"2013-10-23 07:04:23.000000000","message":"Thanks Matt, because the unit test is mocking _consolidate_vmdk_volume, so I did not add a unit test for this, hope it is OK.","commit_id":"d354b6f869d111d3c1aeb4e0fca8b9a98a631e80"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1a9314b52793b08009f6f5956e0af820695ecf18","unresolved":false,"context_lines":[{"line_number":361,"context_line":""},{"line_number":362,"context_line":"        # Delete the original disk from the volume_ref"},{"line_number":363,"context_line":"        self.detach_disk_from_vm(volume_ref, instance, original_device,"},{"line_number":364,"context_line":"                                 destroy_disk\u003dTrue)"},{"line_number":365,"context_line":"        # Attach the current disk to the volume_ref"},{"line_number":366,"context_line":"        # Get details required for adding disk device such as"},{"line_number":367,"context_line":"        # adapter_type, unit_number, controller_key"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAATX%2F%2F%2BRI%3D","line":364,"in_reply_to":"AAAATX%2F%2F%2FkY%3D","updated":"2013-10-23 13:47:59.000000000","message":"If the existing modules are mocking out this method, that means it\u0027s not tested, so it\u0027s not OK.  You should be able to write a test case that unsets the stub so the method can be tested.  If there is something within the method that needs to be mocked, that\u0027s fine, but the inputs/outputs/flow should be validated so it\u0027s not regressed later.","commit_id":"d354b6f869d111d3c1aeb4e0fca8b9a98a631e80"}]}