)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"1a55155f4bb3d5916b04c7c655d094aafebbb136","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Giridhar Jayavelu \u003cgjayavelu@vmware.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2016-07-05 22:20:19 -0700"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"tests: fix false positive in test_set_host_enabled*"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"- disable_reason can be None when _set_host_enabled()"},{"line_number":10,"context_line":"is called and throw TypeError"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"7a8ec9b2_2b02d06f","line":7,"range":{"start_line":7,"start_character":0,"end_line":7,"end_character":51},"updated":"2016-09-15 00:18:09.000000000","message":"This isn\u0027t really the main point of this patch, IMO. There\u0027s a real bug in the implementation here when _set_host_enabled is called to toggle the disabled state of a host without providing a disable_reason. For example, switching a host from enabled to disabled, without giving a disable_reason. In this scenario, the service status will fail to be updated.\n\nSo, I think some of this commit message needs to be rewritten to clearly explain what is being fixed. Same for the launchpad bug, explain there what the bug is. Also explain that you have enhanced the tests to properly cover this code by validating the save call.","commit_id":"7ffca560ed596d2516320340db66918c30ea0a2b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"401991235051eb503be380305bf8364ea5dc1f35","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Giridhar Jayavelu \u003cgjayavelu@vmware.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2016-07-05 22:20:19 -0700"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"tests: fix false positive in test_set_host_enabled*"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"- disable_reason can be None when _set_host_enabled()"},{"line_number":10,"context_line":"is called and throw TypeError"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"7a8ec9b2_cb4a4c83","line":7,"range":{"start_line":7,"start_character":0,"end_line":7,"end_character":51},"in_reply_to":"7a8ec9b2_2b02d06f","updated":"2016-09-15 00:24:31.000000000","message":"Correction: The broken scenario is toggling an enabled host to disabled without providing a disable_reason. The other way around doesn\u0027t reach the bug.","commit_id":"7ffca560ed596d2516320340db66918c30ea0a2b"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"9fe7109a431f70c0f6d2b05853da93b0e6840125","unresolved":false,"context_lines":[{"line_number":696,"context_line":"    def _create_service(self, **kwargs):"},{"line_number":697,"context_line":"        service_ref \u003d {\u0027host\u0027: kwargs.get(\u0027host\u0027, \u0027dummy\u0027),"},{"line_number":698,"context_line":"                       \u0027disabled\u0027: kwargs.get(\u0027disabled\u0027, False),"},{"line_number":699,"context_line":"                       \u0027disabled_reason\u0027: kwargs.get(\u0027disabled_reason\u0027, None),"},{"line_number":700,"context_line":"                       \u0027binary\u0027: \u0027nova-compute\u0027,"},{"line_number":701,"context_line":"                       \u0027topic\u0027: \u0027compute\u0027,"},{"line_number":702,"context_line":"                       \u0027report_count\u0027: 0}"}],"source_content_type":"text/x-python","patch_set":3,"id":"bab6814e_692fbba1","line":699,"updated":"2016-05-25 07:58:17.000000000","message":"by default the get returns None. You can drop that","commit_id":"9b04236bb8eeed0609bd27a81c5f5e4dc2408cc0"},{"author":{"_account_id":16929,"name":"Giridhar Jayavelu","email":"gjayavelu@vmware.com","username":"gjayavelu"},"change_message_id":"663d4d911d4e47cb63df36eccf0c8ffde1cf401c","unresolved":false,"context_lines":[{"line_number":696,"context_line":"    def _create_service(self, **kwargs):"},{"line_number":697,"context_line":"        service_ref \u003d {\u0027host\u0027: kwargs.get(\u0027host\u0027, \u0027dummy\u0027),"},{"line_number":698,"context_line":"                       \u0027disabled\u0027: kwargs.get(\u0027disabled\u0027, False),"},{"line_number":699,"context_line":"                       \u0027disabled_reason\u0027: kwargs.get(\u0027disabled_reason\u0027, None),"},{"line_number":700,"context_line":"                       \u0027binary\u0027: \u0027nova-compute\u0027,"},{"line_number":701,"context_line":"                       \u0027topic\u0027: \u0027compute\u0027,"},{"line_number":702,"context_line":"                       \u0027report_count\u0027: 0}"}],"source_content_type":"text/x-python","patch_set":3,"id":"bab6814e_d86ab3ec","line":699,"in_reply_to":"bab6814e_692fbba1","updated":"2016-05-26 06:35:08.000000000","message":"Done","commit_id":"9b04236bb8eeed0609bd27a81c5f5e4dc2408cc0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"1a55155f4bb3d5916b04c7c655d094aafebbb136","unresolved":false,"context_lines":[{"line_number":1490,"context_line":""},{"line_number":1491,"context_line":"    @mock.patch.object(objects.Service, \u0027save\u0027)"},{"line_number":1492,"context_line":"    @mock.patch.object(objects.Service, \u0027get_by_compute_host\u0027)"},{"line_number":1493,"context_line":"    def test_set_host_enabled_with_disable(self, mock_svc, mock_save):"},{"line_number":1494,"context_line":"        # Tests disabling an enabled host."},{"line_number":1495,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)"},{"line_number":1496,"context_line":"        svc \u003d self._create_service(host\u003d\u0027fake-mini\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a8ec9b2_4b963c4d","line":1493,"updated":"2016-09-15 00:18:09.000000000","message":"This is the test that exposes the bug. If you back out the driver.py change, it fails because the save call was not reached because of the \"TypeError: cannot concatenate \u0027str\u0027 and \u0027NoneType\u0027 objects\" exception caught in the \u0027except Exception\u0027 block.","commit_id":"7ffca560ed596d2516320340db66918c30ea0a2b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"1a55155f4bb3d5916b04c7c655d094aafebbb136","unresolved":false,"context_lines":[{"line_number":1497,"context_line":"        mock_svc.return_value \u003d svc"},{"line_number":1498,"context_line":"        drvr._set_host_enabled(False)"},{"line_number":1499,"context_line":"        self.assertTrue(svc.disabled)"},{"line_number":1500,"context_line":"        self.assertTrue(mock_save.call_count)"},{"line_number":1501,"context_line":""},{"line_number":1502,"context_line":"    @mock.patch.object(objects.Service, \u0027save\u0027)"},{"line_number":1503,"context_line":"    @mock.patch.object(objects.Service, \u0027get_by_compute_host\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a8ec9b2_0bf434f0","line":1500,"range":{"start_line":1500,"start_character":8,"end_line":1500,"end_character":45},"updated":"2016-09-15 00:18:09.000000000","message":"This isn\u0027t ideal. It would be more specific and correct to use:\n\n mock_save.assert_called_once_with()\n\nwhich means it was called once.","commit_id":"7ffca560ed596d2516320340db66918c30ea0a2b"},{"author":{"_account_id":16929,"name":"Giridhar Jayavelu","email":"gjayavelu@vmware.com","username":"gjayavelu"},"change_message_id":"b3e5c3e0e6a8cb926ec7a74905cd9ddad7871af8","unresolved":false,"context_lines":[{"line_number":1497,"context_line":"        mock_svc.return_value \u003d svc"},{"line_number":1498,"context_line":"        drvr._set_host_enabled(False)"},{"line_number":1499,"context_line":"        self.assertTrue(svc.disabled)"},{"line_number":1500,"context_line":"        self.assertTrue(mock_save.call_count)"},{"line_number":1501,"context_line":""},{"line_number":1502,"context_line":"    @mock.patch.object(objects.Service, \u0027save\u0027)"},{"line_number":1503,"context_line":"    @mock.patch.object(objects.Service, \u0027get_by_compute_host\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a8ec9b2_611cf713","line":1500,"range":{"start_line":1500,"start_character":8,"end_line":1500,"end_character":45},"in_reply_to":"7a8ec9b2_0bf434f0","updated":"2016-09-15 01:22:03.000000000","message":"sure will fix it. since there was no parameter to validate, I just validated that it was called.","commit_id":"7ffca560ed596d2516320340db66918c30ea0a2b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"1a55155f4bb3d5916b04c7c655d094aafebbb136","unresolved":false,"context_lines":[{"line_number":1509,"context_line":"        drvr._set_host_enabled(True)"},{"line_number":1510,"context_line":"        # since disabled_reason is not set and not prefixed with \"AUTO:\","},{"line_number":1511,"context_line":"        # service should not be enabled."},{"line_number":1512,"context_line":"        self.assertFalse(mock_save.call_count)"},{"line_number":1513,"context_line":"        self.assertTrue(svc.disabled)"},{"line_number":1514,"context_line":""},{"line_number":1515,"context_line":"    @mock.patch.object(objects.Service, \u0027save\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a8ec9b2_2bce50b4","line":1512,"range":{"start_line":1512,"start_character":8,"end_line":1512,"end_character":46},"updated":"2016-09-15 00:18:09.000000000","message":"call_count is an integer, I\u0027d rather you be direct and use one of:\n\n mock_save.assert_not_called()\n\nor\n\n self.assertFalse(mock_save.called)\n\nor\n\n self.assertEqual(0, mock_save.call_count)","commit_id":"7ffca560ed596d2516320340db66918c30ea0a2b"},{"author":{"_account_id":16929,"name":"Giridhar Jayavelu","email":"gjayavelu@vmware.com","username":"gjayavelu"},"change_message_id":"b3e5c3e0e6a8cb926ec7a74905cd9ddad7871af8","unresolved":false,"context_lines":[{"line_number":1509,"context_line":"        drvr._set_host_enabled(True)"},{"line_number":1510,"context_line":"        # since disabled_reason is not set and not prefixed with \"AUTO:\","},{"line_number":1511,"context_line":"        # service should not be enabled."},{"line_number":1512,"context_line":"        self.assertFalse(mock_save.call_count)"},{"line_number":1513,"context_line":"        self.assertTrue(svc.disabled)"},{"line_number":1514,"context_line":""},{"line_number":1515,"context_line":"    @mock.patch.object(objects.Service, \u0027save\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a8ec9b2_21b82fe5","line":1512,"range":{"start_line":1512,"start_character":8,"end_line":1512,"end_character":46},"in_reply_to":"7a8ec9b2_2bce50b4","updated":"2016-09-15 01:22:03.000000000","message":"Done","commit_id":"7ffca560ed596d2516320340db66918c30ea0a2b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"1a55155f4bb3d5916b04c7c655d094aafebbb136","unresolved":false,"context_lines":[{"line_number":1522,"context_line":"        mock_svc.return_value \u003d svc"},{"line_number":1523,"context_line":"        drvr._set_host_enabled(True)"},{"line_number":1524,"context_line":"        self.assertFalse(svc.disabled)"},{"line_number":1525,"context_line":"        self.assertFalse(mock_save.call_count)"},{"line_number":1526,"context_line":""},{"line_number":1527,"context_line":"    @mock.patch.object(objects.Service, \u0027save\u0027)"},{"line_number":1528,"context_line":"    @mock.patch.object(objects.Service, \u0027get_by_compute_host\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a8ec9b2_6bc0587d","line":1525,"range":{"start_line":1525,"start_character":8,"end_line":1525,"end_character":46},"updated":"2016-09-15 00:18:09.000000000","message":"Same comment","commit_id":"7ffca560ed596d2516320340db66918c30ea0a2b"},{"author":{"_account_id":16929,"name":"Giridhar Jayavelu","email":"gjayavelu@vmware.com","username":"gjayavelu"},"change_message_id":"b3e5c3e0e6a8cb926ec7a74905cd9ddad7871af8","unresolved":false,"context_lines":[{"line_number":1522,"context_line":"        mock_svc.return_value \u003d svc"},{"line_number":1523,"context_line":"        drvr._set_host_enabled(True)"},{"line_number":1524,"context_line":"        self.assertFalse(svc.disabled)"},{"line_number":1525,"context_line":"        self.assertFalse(mock_save.call_count)"},{"line_number":1526,"context_line":""},{"line_number":1527,"context_line":"    @mock.patch.object(objects.Service, \u0027save\u0027)"},{"line_number":1528,"context_line":"    @mock.patch.object(objects.Service, \u0027get_by_compute_host\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a8ec9b2_41cffb7e","line":1525,"range":{"start_line":1525,"start_character":8,"end_line":1525,"end_character":46},"in_reply_to":"7a8ec9b2_6bc0587d","updated":"2016-09-15 01:22:03.000000000","message":"Done","commit_id":"7ffca560ed596d2516320340db66918c30ea0a2b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"1a55155f4bb3d5916b04c7c655d094aafebbb136","unresolved":false,"context_lines":[{"line_number":1533,"context_line":"        svc \u003d self._create_service(disabled\u003dTrue, host\u003d\u0027fake-mini\u0027)"},{"line_number":1534,"context_line":"        mock_svc.return_value \u003d svc"},{"line_number":1535,"context_line":"        drvr._set_host_enabled(False)"},{"line_number":1536,"context_line":"        self.assertFalse(mock_save.call_count)"},{"line_number":1537,"context_line":"        self.assertTrue(svc.disabled)"},{"line_number":1538,"context_line":""},{"line_number":1539,"context_line":"    def test_set_host_enabled_swallows_exceptions(self):"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a8ec9b2_8bbf64fa","line":1536,"range":{"start_line":1536,"start_character":8,"end_line":1536,"end_character":46},"updated":"2016-09-15 00:18:09.000000000","message":"Same comment","commit_id":"7ffca560ed596d2516320340db66918c30ea0a2b"},{"author":{"_account_id":16929,"name":"Giridhar Jayavelu","email":"gjayavelu@vmware.com","username":"gjayavelu"},"change_message_id":"b3e5c3e0e6a8cb926ec7a74905cd9ddad7871af8","unresolved":false,"context_lines":[{"line_number":1533,"context_line":"        svc \u003d self._create_service(disabled\u003dTrue, host\u003d\u0027fake-mini\u0027)"},{"line_number":1534,"context_line":"        mock_svc.return_value \u003d svc"},{"line_number":1535,"context_line":"        drvr._set_host_enabled(False)"},{"line_number":1536,"context_line":"        self.assertFalse(mock_save.call_count)"},{"line_number":1537,"context_line":"        self.assertTrue(svc.disabled)"},{"line_number":1538,"context_line":""},{"line_number":1539,"context_line":"    def test_set_host_enabled_swallows_exceptions(self):"}],"source_content_type":"text/x-python","patch_set":5,"id":"7a8ec9b2_61ca3770","line":1536,"range":{"start_line":1536,"start_character":8,"end_line":1536,"end_character":46},"in_reply_to":"7a8ec9b2_8bbf64fa","updated":"2016-09-15 01:22:03.000000000","message":"Done","commit_id":"7ffca560ed596d2516320340db66918c30ea0a2b"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"460bb3fd56a07575b73c0399f24ea552cf11c643","unresolved":false,"context_lines":[{"line_number":3227,"context_line":""},{"line_number":3228,"context_line":"        disable_service \u003d not enabled"},{"line_number":3229,"context_line":""},{"line_number":3230,"context_line":"        if disable_service and disable_reason is None:"},{"line_number":3231,"context_line":"            msg \u003d _(\"Disable service requested, but no reason provided.\")"},{"line_number":3232,"context_line":"            raise exception.Invalid(msg)"},{"line_number":3233,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1aa78d24_67ed1625","line":3230,"range":{"start_line":3230,"start_character":49,"end_line":3230,"end_character":53},"updated":"2016-07-05 12:23:42.000000000","message":"Note that DISABLE_REASON_UNDEFINED \u003d None, so this seems a little odd here. Why have an optional arg\u0027s default value cause an exception.","commit_id":"a1190801940371df6d5c23421a8d3bd082fb13ec"},{"author":{"_account_id":16929,"name":"Giridhar Jayavelu","email":"gjayavelu@vmware.com","username":"gjayavelu"},"change_message_id":"198059334f955ebd51bdbb4b4adb16401244a6b6","unresolved":false,"context_lines":[{"line_number":3227,"context_line":""},{"line_number":3228,"context_line":"        disable_service \u003d not enabled"},{"line_number":3229,"context_line":""},{"line_number":3230,"context_line":"        if disable_service and disable_reason is None:"},{"line_number":3231,"context_line":"            msg \u003d _(\"Disable service requested, but no reason provided.\")"},{"line_number":3232,"context_line":"            raise exception.Invalid(msg)"},{"line_number":3233,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"1aa78d24_a7cfd527","line":3230,"range":{"start_line":3230,"start_character":49,"end_line":3230,"end_character":53},"in_reply_to":"1aa78d24_67ed1625","updated":"2016-07-06 05:23:36.000000000","message":"when the input enabled\u003dTrue, disable_reason need not be specified. So, it is not required to check the value of the optional parameter disable_reason.","commit_id":"a1190801940371df6d5c23421a8d3bd082fb13ec"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"460bb3fd56a07575b73c0399f24ea552cf11c643","unresolved":false,"context_lines":[{"line_number":3229,"context_line":""},{"line_number":3230,"context_line":"        if disable_service and disable_reason is None:"},{"line_number":3231,"context_line":"            msg \u003d _(\"Disable service requested, but no reason provided.\")"},{"line_number":3232,"context_line":"            raise exception.Invalid(msg)"},{"line_number":3233,"context_line":""},{"line_number":3234,"context_line":"        ctx \u003d nova_context.get_admin_context()"},{"line_number":3235,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"1aa78d24_a7a9bee1","line":3232,"updated":"2016-07-05 12:23:42.000000000","message":"Its totally valid to disable a service without specifying a reason.\n\nWhy are we raising that exception here?","commit_id":"a1190801940371df6d5c23421a8d3bd082fb13ec"},{"author":{"_account_id":16929,"name":"Giridhar Jayavelu","email":"gjayavelu@vmware.com","username":"gjayavelu"},"change_message_id":"198059334f955ebd51bdbb4b4adb16401244a6b6","unresolved":false,"context_lines":[{"line_number":3229,"context_line":""},{"line_number":3230,"context_line":"        if disable_service and disable_reason is None:"},{"line_number":3231,"context_line":"            msg \u003d _(\"Disable service requested, but no reason provided.\")"},{"line_number":3232,"context_line":"            raise exception.Invalid(msg)"},{"line_number":3233,"context_line":""},{"line_number":3234,"context_line":"        ctx \u003d nova_context.get_admin_context()"},{"line_number":3235,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"1aa78d24_87473976","line":3232,"in_reply_to":"1aa78d24_a7a9bee1","updated":"2016-07-06 05:23:36.000000000","message":"sure. I had a wrong assumption that it is mandatory to give a reason when disabling service based on L3250.\n\nIf disable_reason is undefined, we would hit this error.\nTypeError: \"cannot concatenate \u0027str\u0027 and \u0027NoneType\u0027 objects\"\n\nThis error was not caught in the unit test because we did not validate if service.save() was called. \n\nI can add a check if disable_reason is defined in L3250. It won\u0027t be a behavior change.","commit_id":"a1190801940371df6d5c23421a8d3bd082fb13ec"}]}