)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"e383936719f7d8f2109f21a2df018ae5d3ecc938","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     whoami-rajat \u003crajatdhasmana@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2021-05-20 09:31:53 -0400"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Dont log error trace when nvme is not used"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When nvme is not configured, the connector still logs error"},{"line_number":10,"context_line":"traceback and creates /etc/nvme directory."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"e5c62969_5eb976bd","line":7,"range":{"start_line":7,"start_character":0,"end_line":7,"end_character":4},"updated":"2021-05-20 13:47:46.000000000","message":"nit: Don\u0027t","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"217f3cc7d47c40a6528d75565806c7f7c8aaecac","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     whoami-rajat \u003crajatdhasmana@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2021-05-20 09:31:53 -0400"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Dont log error trace when nvme is not used"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When nvme is not configured, the connector still logs error"},{"line_number":10,"context_line":"traceback and creates /etc/nvme directory."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"4eea4c7a_d05a9a84","line":7,"range":{"start_line":7,"start_character":0,"end_line":7,"end_character":4},"in_reply_to":"e5c62969_5eb976bd","updated":"2021-05-21 13:11:32.000000000","message":"Done","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"e383936719f7d8f2109f21a2df018ae5d3ecc938","unresolved":true,"context_lines":[{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Closes-Bug: #1929074"},{"line_number":15,"context_line":"Closes-Bug: #1929075"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Change-Id: I2473f50671b9c3f1840efd0d12d2a2572a21d850"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"0316676f_ca235e49","line":16,"updated":"2021-05-20 13:47:46.000000000","message":"nit: unnecessary blank line","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"217f3cc7d47c40a6528d75565806c7f7c8aaecac","unresolved":false,"context_lines":[{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Closes-Bug: #1929074"},{"line_number":15,"context_line":"Closes-Bug: #1929075"},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Change-Id: I2473f50671b9c3f1840efd0d12d2a2572a21d850"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"9305e2d6_cd32874d","line":16,"in_reply_to":"0316676f_ca235e49","updated":"2021-05-21 13:11:32.000000000","message":"Done","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"}],"os_brick/initiator/connectors/nvmeof.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"595e13ff19c6613fbb1ef90da242b68d5696f7e1","unresolved":true,"context_lines":[{"line_number":130,"context_line":"                    root_helper\u003dself._root_helper, run_as_root\u003dTrue)"},{"line_number":131,"context_line":"            except Exception as e:"},{"line_number":132,"context_line":"                LOG.warning(\"Could not generate host nqn: %s\" % str(e),"},{"line_number":133,"context_line":"                            exc_info\u003dFalse)"},{"line_number":134,"context_line":"        return host_nqn"},{"line_number":135,"context_line":""},{"line_number":136,"context_line":"    def _get_system_uuid(self):"}],"source_content_type":"text/x-python","patch_set":1,"id":"9736423f_644ad2ee","line":133,"updated":"2021-05-19 15:58:51.000000000","message":"-1: I\u0027ve looked into this, and I think the approach should be different, because we\u0027ll want to log this information when there\u0027s a valid error.\n\nIn my opinion we should change the get_connector_properties method and return empty dict when the nvme cli tool is not installed:\n\n    @classmethod\n    def nvme_present(cls, execute):\n        try:\n            execute(\u0027nvme\u0027, \u0027version\u0027)\n        except putils.ProcessExecutionError:\n            LOG.debug(\u0027nvme not present on system\u0027)\n            return False\n        return True\n\n    @classmethod\n    def get_connector_properties(cls, root_helper, *args, **kwargs):\n        \"\"\"The NVMe-oF connector properties (initiator uuid and nqn.)\"\"\"\n        execute \u003d kwargs.get(\u0027execute\u0027) or priv_rootwrap.execute\n        nvmf \u003d NVMeOFConnector(root_helper\u003droot_helper, execute\u003dexecute)\n        ret \u003d {}\n\n        if cls.nvme_present(execute):\n            uuid \u003d nvmf._get_host_uuid()\n\nThis has an added benefit, of not creating the /etc/nvme directory on every system.\n\nIn my opinion we should have 2 bugs for those:\n- This error showing in the logs\n- Creating /etc/nvme on deployments that don\u0027t have nvme","commit_id":"b92c2f2df3b62d6de6d7316f39571e84d110c34c"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"12cec4927227a7ef5f7af7ec48e6f1002400fd7f","unresolved":false,"context_lines":[{"line_number":130,"context_line":"                    root_helper\u003dself._root_helper, run_as_root\u003dTrue)"},{"line_number":131,"context_line":"            except Exception as e:"},{"line_number":132,"context_line":"                LOG.warning(\"Could not generate host nqn: %s\" % str(e),"},{"line_number":133,"context_line":"                            exc_info\u003dFalse)"},{"line_number":134,"context_line":"        return host_nqn"},{"line_number":135,"context_line":""},{"line_number":136,"context_line":"    def _get_system_uuid(self):"}],"source_content_type":"text/x-python","patch_set":1,"id":"9124d80c_6ce74958","line":133,"in_reply_to":"5e706147_f140b0d0","updated":"2021-05-20 13:35:41.000000000","message":"Done","commit_id":"b92c2f2df3b62d6de6d7316f39571e84d110c34c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"716d9c2224190acac8d425e6b11cf3d201b0bf8a","unresolved":true,"context_lines":[{"line_number":130,"context_line":"                    root_helper\u003dself._root_helper, run_as_root\u003dTrue)"},{"line_number":131,"context_line":"            except Exception as e:"},{"line_number":132,"context_line":"                LOG.warning(\"Could not generate host nqn: %s\" % str(e),"},{"line_number":133,"context_line":"                            exc_info\u003dFalse)"},{"line_number":134,"context_line":"        return host_nqn"},{"line_number":135,"context_line":""},{"line_number":136,"context_line":"    def _get_system_uuid(self):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5e706147_f140b0d0","line":133,"in_reply_to":"9736423f_644ad2ee","updated":"2021-05-19 18:49:24.000000000","message":"By the way, this area\u0027s existing code in master is pretty broken, so I\u0027m redoing it [1], but I\u0027ll leave fixing those 2 bugs for this patch.\n\n[1]: https://review.opendev.org/c/openstack/os-brick/+/792237","commit_id":"b92c2f2df3b62d6de6d7316f39571e84d110c34c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"e383936719f7d8f2109f21a2df018ae5d3ecc938","unresolved":true,"context_lines":[{"line_number":82,"context_line":"    def nvme_present(cls, execute):"},{"line_number":83,"context_line":"        try:"},{"line_number":84,"context_line":"            execute(\u0027nvme\u0027, \u0027version\u0027)"},{"line_number":85,"context_line":"        except putils.ProcessExecutionError:"},{"line_number":86,"context_line":"            LOG.debug(\u0027nvme not present on system\u0027)"},{"line_number":87,"context_line":"            return False"},{"line_number":88,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":2,"id":"d5a8f578_09100dd7","line":85,"range":{"start_line":85,"start_character":15,"end_line":85,"end_character":44},"updated":"2021-05-20 13:47:46.000000000","message":"-1: My suggestion was wrong, sorry. You won\u0027t get ProcessExecutionError but OSError here instead when nvme is missing, so we should do something like:\n\n    @classmethod\n    def nvme_present(cls, execute):\n        try:\n            execute(\u0027nvme\u0027, \u0027version\u0027)\n            return True\n        except Exception as exc:\n            if isinstance(exc, OSError) and exc.errno \u003d\u003d errno.ENOENT:\n                LOG.debug(\u0027nvme not present on system\u0027)\n            else:\n                LOG.warning(\u0027Unknown error when checking presence of nvme: %s\u0027,\n                            exc)\n        return False","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9ca9ebfb1fe5c9143663b812202bd7feb49bbf78","unresolved":false,"context_lines":[{"line_number":82,"context_line":"    def nvme_present(cls, execute):"},{"line_number":83,"context_line":"        try:"},{"line_number":84,"context_line":"            execute(\u0027nvme\u0027, \u0027version\u0027)"},{"line_number":85,"context_line":"        except putils.ProcessExecutionError:"},{"line_number":86,"context_line":"            LOG.debug(\u0027nvme not present on system\u0027)"},{"line_number":87,"context_line":"            return False"},{"line_number":88,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":2,"id":"98218a80_577c2282","line":85,"range":{"start_line":85,"start_character":15,"end_line":85,"end_character":44},"in_reply_to":"528cfde2_2c80ffc9","updated":"2021-05-31 10:04:46.000000000","message":"OK.\n\nIn a future patch we may need to change execute method to create the `ProcessExecutionError` instance with the `exit_code` parameter set to OSError\u0027s `errno` attribute.","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"dd14940117aef8cbb7b0c1f9092cd6c1df7e1720","unresolved":false,"context_lines":[{"line_number":82,"context_line":"    def nvme_present(cls, execute):"},{"line_number":83,"context_line":"        try:"},{"line_number":84,"context_line":"            execute(\u0027nvme\u0027, \u0027version\u0027)"},{"line_number":85,"context_line":"        except putils.ProcessExecutionError:"},{"line_number":86,"context_line":"            LOG.debug(\u0027nvme not present on system\u0027)"},{"line_number":87,"context_line":"            return False"},{"line_number":88,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":2,"id":"737113f9_dd4a8a3a","line":85,"range":{"start_line":85,"start_character":15,"end_line":85,"end_character":44},"in_reply_to":"98218a80_577c2282","updated":"2021-05-31 15:40:49.000000000","message":"Makes sense, since this seems out of scope of what this patch is addressing, i will do that in a followup patch.","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"217f3cc7d47c40a6528d75565806c7f7c8aaecac","unresolved":false,"context_lines":[{"line_number":82,"context_line":"    def nvme_present(cls, execute):"},{"line_number":83,"context_line":"        try:"},{"line_number":84,"context_line":"            execute(\u0027nvme\u0027, \u0027version\u0027)"},{"line_number":85,"context_line":"        except putils.ProcessExecutionError:"},{"line_number":86,"context_line":"            LOG.debug(\u0027nvme not present on system\u0027)"},{"line_number":87,"context_line":"            return False"},{"line_number":88,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":2,"id":"528cfde2_2c80ffc9","line":85,"range":{"start_line":85,"start_character":15,"end_line":85,"end_character":44},"in_reply_to":"d5a8f578_09100dd7","updated":"2021-05-21 13:11:32.000000000","message":"It was actually raising ProcessExecutionError since execute converts OSError to ProcessExecutionError[1] but it doesn\u0027t contain errno property so used custom_execute instead.\n\n[1] https://github.com/openstack/os-brick/blob/master/os_brick/privileged/rootwrap.py#L175-L190","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"217f3cc7d47c40a6528d75565806c7f7c8aaecac","unresolved":false,"context_lines":[{"line_number":94,"context_line":"        nvmf \u003d NVMeOFConnector(root_helper\u003droot_helper, execute\u003dexecute)"},{"line_number":95,"context_line":"        ret \u003d {}"},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"        uuid \u003d None"},{"line_number":98,"context_line":"        if cls.nvme_present(execute):"},{"line_number":99,"context_line":"            uuid \u003d nvmf._get_host_uuid()"},{"line_number":100,"context_line":"        suuid \u003d nvmf._get_system_uuid()"},{"line_number":101,"context_line":"        nqn \u003d nvmf._get_host_nqn()"}],"source_content_type":"text/x-python","patch_set":2,"id":"97155c13_8ce64346","line":98,"range":{"start_line":97,"start_character":8,"end_line":98,"end_character":37},"updated":"2021-05-21 13:11:32.000000000","message":"This should be\nnqn \u003d None\nif cls.nvme_present(execute):\n    nqn \u003d nvmf._get_host_nqn()","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"5f7bc633b098b31e6c794c8f6f46d6312ee90ce5","unresolved":true,"context_lines":[{"line_number":139,"context_line":"                    \u0027mkdir\u0027, \u0027-m\u0027, \u0027755\u0027, \u0027-p\u0027, \u0027/etc/nvme\u0027,"},{"line_number":140,"context_line":"                    root_helper\u003dself._root_helper, run_as_root\u003dTrue)"},{"line_number":141,"context_line":"                out, err \u003d self._execute("},{"line_number":142,"context_line":"                    \u0027nvme\u0027, \u0027gen-hostnqn\u0027, \u0027|\u0027, \u0027tee\u0027, \u0027/etc/nvme/hostnqn\u0027,"},{"line_number":143,"context_line":"                    root_helper\u003dself._root_helper, run_as_root\u003dTrue)"},{"line_number":144,"context_line":"                if out.strip():"},{"line_number":145,"context_line":"                    host_nqn \u003d out.strip()"}],"source_content_type":"text/x-python","patch_set":6,"id":"37b39ca2_343be732","line":142,"range":{"start_line":142,"start_character":43,"end_line":142,"end_character":46},"updated":"2021-06-02 14:17:52.000000000","message":"Unrelated to the current change: does this work w/o passing shell\u003dTrue?","commit_id":"f2bc082b079cd5c7534c959c0fc339c9832e081b"}],"os_brick/tests/initiator/connectors/test_nvmeof.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9ca9ebfb1fe5c9143663b812202bd7feb49bbf78","unresolved":true,"context_lines":[{"line_number":65,"context_line":"    @mock.patch(\u0027os_brick.initiator.connectors.nvmeof.LOG\u0027)"},{"line_number":66,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027custom_execute\u0027, autospec\u003dTrue)"},{"line_number":67,"context_line":"    def test_nvme_present(self, exc, mock_execute, mock_log):"},{"line_number":68,"context_line":"        if isinstance(exc, bool):"},{"line_number":69,"context_line":"            nvme_present \u003d self.connector.nvme_present()"},{"line_number":70,"context_line":"            self.assertTrue(nvme_present)"},{"line_number":71,"context_line":"        else:"},{"line_number":72,"context_line":"            if exc \u003d\u003d OSError:"},{"line_number":73,"context_line":"                mock_execute.side_effect \u003d exc(2, \u0027FileNotFoundError\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"d94241d0_19e42bdf","line":70,"range":{"start_line":68,"start_character":0,"end_line":70,"end_character":41},"updated":"2021-05-31 10:04:46.000000000","message":"If there is no common code between the True case and the exceptions we should have 2 different tests.","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"dd14940117aef8cbb7b0c1f9092cd6c1df7e1720","unresolved":false,"context_lines":[{"line_number":65,"context_line":"    @mock.patch(\u0027os_brick.initiator.connectors.nvmeof.LOG\u0027)"},{"line_number":66,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027custom_execute\u0027, autospec\u003dTrue)"},{"line_number":67,"context_line":"    def test_nvme_present(self, exc, mock_execute, mock_log):"},{"line_number":68,"context_line":"        if isinstance(exc, bool):"},{"line_number":69,"context_line":"            nvme_present \u003d self.connector.nvme_present()"},{"line_number":70,"context_line":"            self.assertTrue(nvme_present)"},{"line_number":71,"context_line":"        else:"},{"line_number":72,"context_line":"            if exc \u003d\u003d OSError:"},{"line_number":73,"context_line":"                mock_execute.side_effect \u003d exc(2, \u0027FileNotFoundError\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"080b9c56_f24945fd","line":70,"range":{"start_line":68,"start_character":0,"end_line":70,"end_character":41},"in_reply_to":"d94241d0_19e42bdf","updated":"2021-05-31 15:40:49.000000000","message":"Done","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9ca9ebfb1fe5c9143663b812202bd7feb49bbf78","unresolved":true,"context_lines":[{"line_number":69,"context_line":"            nvme_present \u003d self.connector.nvme_present()"},{"line_number":70,"context_line":"            self.assertTrue(nvme_present)"},{"line_number":71,"context_line":"        else:"},{"line_number":72,"context_line":"            if exc \u003d\u003d OSError:"},{"line_number":73,"context_line":"                mock_execute.side_effect \u003d exc(2, \u0027FileNotFoundError\u0027)"},{"line_number":74,"context_line":"            else:"},{"line_number":75,"context_line":"                mock_execute.side_effect \u003d exc"},{"line_number":76,"context_line":"            nvme_present \u003d self.connector.nvme_present()"},{"line_number":77,"context_line":"            if exc \u003d\u003d OSError:"},{"line_number":78,"context_line":"                mock_log.debug.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":5,"id":"6fb4e7eb_df5a45b3","line":75,"range":{"start_line":72,"start_character":0,"end_line":75,"end_character":46},"updated":"2021-05-31 10:04:46.000000000","message":"It\u0027s clearer to pass instances to the ddt.data instead of doing it here:\n\n    @ddt.data(OSError(2, \u0027FileNotFoundError\u0027), Exception())\n\n    [ . . . ]\n\n    mock_execute.side_effect \u003d exc","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"dd14940117aef8cbb7b0c1f9092cd6c1df7e1720","unresolved":false,"context_lines":[{"line_number":69,"context_line":"            nvme_present \u003d self.connector.nvme_present()"},{"line_number":70,"context_line":"            self.assertTrue(nvme_present)"},{"line_number":71,"context_line":"        else:"},{"line_number":72,"context_line":"            if exc \u003d\u003d OSError:"},{"line_number":73,"context_line":"                mock_execute.side_effect \u003d exc(2, \u0027FileNotFoundError\u0027)"},{"line_number":74,"context_line":"            else:"},{"line_number":75,"context_line":"                mock_execute.side_effect \u003d exc"},{"line_number":76,"context_line":"            nvme_present \u003d self.connector.nvme_present()"},{"line_number":77,"context_line":"            if exc \u003d\u003d OSError:"},{"line_number":78,"context_line":"                mock_log.debug.assert_called_once_with("}],"source_content_type":"text/x-python","patch_set":5,"id":"d769d019_640bebe8","line":75,"range":{"start_line":72,"start_character":0,"end_line":75,"end_character":46},"in_reply_to":"6fb4e7eb_df5a45b3","updated":"2021-05-31 15:40:49.000000000","message":"Done","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9ca9ebfb1fe5c9143663b812202bd7feb49bbf78","unresolved":true,"context_lines":[{"line_number":74,"context_line":"            else:"},{"line_number":75,"context_line":"                mock_execute.side_effect \u003d exc"},{"line_number":76,"context_line":"            nvme_present \u003d self.connector.nvme_present()"},{"line_number":77,"context_line":"            if exc \u003d\u003d OSError:"},{"line_number":78,"context_line":"                mock_log.debug.assert_called_once_with("},{"line_number":79,"context_line":"                    \u0027nvme not present on system\u0027)"},{"line_number":80,"context_line":"            else:"},{"line_number":81,"context_line":"                expected \u003d \u0027Unknown error when checking presence of nvme: %s\u0027"},{"line_number":82,"context_line":"                if sys.version_info.minor \u003c 8:"},{"line_number":83,"context_line":"                    self.assertEqual(expected,"},{"line_number":84,"context_line":"                                     mock_log.warning.call_args[0][0])"},{"line_number":85,"context_line":"                else:"},{"line_number":86,"context_line":"                    self.assertEqual(expected,"},{"line_number":87,"context_line":"                                     mock_log.warning.call_args.args[0])"},{"line_number":88,"context_line":"            self.assertFalse(nvme_present)"},{"line_number":89,"context_line":""},{"line_number":90,"context_line":"    @mock.patch.object(nvmeof.NVMeOFConnector, \u0027_execute\u0027, autospec\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":5,"id":"05f74ecf_a7605611","line":87,"range":{"start_line":77,"start_character":0,"end_line":87,"end_character":72},"updated":"2021-05-31 10:04:46.000000000","message":"nit: Maybe we are being too specific with the testing and we could just test that we called the right log level method:\n\n    log \u003d mock_log.debug if isinstance(exc, OSError) else mock_log.warning\n    log.assert_called_once()\n\nOr use assert_called_once_with instead of doing the minor version dance.  After all we have the exception that was passed to the log in mock_execute.side_effect in your code (with my change is just exc)","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"dd14940117aef8cbb7b0c1f9092cd6c1df7e1720","unresolved":false,"context_lines":[{"line_number":74,"context_line":"            else:"},{"line_number":75,"context_line":"                mock_execute.side_effect \u003d exc"},{"line_number":76,"context_line":"            nvme_present \u003d self.connector.nvme_present()"},{"line_number":77,"context_line":"            if exc \u003d\u003d OSError:"},{"line_number":78,"context_line":"                mock_log.debug.assert_called_once_with("},{"line_number":79,"context_line":"                    \u0027nvme not present on system\u0027)"},{"line_number":80,"context_line":"            else:"},{"line_number":81,"context_line":"                expected \u003d \u0027Unknown error when checking presence of nvme: %s\u0027"},{"line_number":82,"context_line":"                if sys.version_info.minor \u003c 8:"},{"line_number":83,"context_line":"                    self.assertEqual(expected,"},{"line_number":84,"context_line":"                                     mock_log.warning.call_args[0][0])"},{"line_number":85,"context_line":"                else:"},{"line_number":86,"context_line":"                    self.assertEqual(expected,"},{"line_number":87,"context_line":"                                     mock_log.warning.call_args.args[0])"},{"line_number":88,"context_line":"            self.assertFalse(nvme_present)"},{"line_number":89,"context_line":""},{"line_number":90,"context_line":"    @mock.patch.object(nvmeof.NVMeOFConnector, \u0027_execute\u0027, autospec\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba657694_9746b11d","line":87,"range":{"start_line":77,"start_character":0,"end_line":87,"end_character":72},"in_reply_to":"05f74ecf_a7605611","updated":"2021-05-31 15:40:49.000000000","message":"Done","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9ca9ebfb1fe5c9143663b812202bd7feb49bbf78","unresolved":true,"context_lines":[{"line_number":127,"context_line":"        mock_host_uuid.return_value \u003d HOST_UUID"},{"line_number":128,"context_line":"        mock_sysuuid.return_value \u003d SYS_UUID"},{"line_number":129,"context_line":"        mock_nqn.return_value \u003d HOST_NQN"},{"line_number":130,"context_line":"        mock_nvme_present.return_value \u003d True"},{"line_number":131,"context_line":"        props \u003d self.connector.get_connector_properties(\u0027sudo\u0027)"},{"line_number":132,"context_line":"        expected_props \u003d {\"system uuid\": SYS_UUID, \"nqn\": HOST_NQN,"},{"line_number":133,"context_line":"                          \"uuid\": HOST_UUID}"}],"source_content_type":"text/x-python","patch_set":5,"id":"f3fe6b2a_b9333bda","line":130,"updated":"2021-05-31 10:04:46.000000000","message":"nit: On L105 we pass it to the mock.path.object method","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"174ffa57acd092d9504bca1e3becfe44d8626cb1","unresolved":false,"context_lines":[{"line_number":127,"context_line":"        mock_host_uuid.return_value \u003d HOST_UUID"},{"line_number":128,"context_line":"        mock_sysuuid.return_value \u003d SYS_UUID"},{"line_number":129,"context_line":"        mock_nqn.return_value \u003d HOST_NQN"},{"line_number":130,"context_line":"        mock_nvme_present.return_value \u003d True"},{"line_number":131,"context_line":"        props \u003d self.connector.get_connector_properties(\u0027sudo\u0027)"},{"line_number":132,"context_line":"        expected_props \u003d {\"system uuid\": SYS_UUID, \"nqn\": HOST_NQN,"},{"line_number":133,"context_line":"                          \"uuid\": HOST_UUID}"}],"source_content_type":"text/x-python","patch_set":5,"id":"104f9442_7c690888","line":130,"in_reply_to":"d5e3bd43_8f92a223","updated":"2021-06-01 13:23:54.000000000","message":"Ack","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"dd14940117aef8cbb7b0c1f9092cd6c1df7e1720","unresolved":true,"context_lines":[{"line_number":127,"context_line":"        mock_host_uuid.return_value \u003d HOST_UUID"},{"line_number":128,"context_line":"        mock_sysuuid.return_value \u003d SYS_UUID"},{"line_number":129,"context_line":"        mock_nqn.return_value \u003d HOST_NQN"},{"line_number":130,"context_line":"        mock_nvme_present.return_value \u003d True"},{"line_number":131,"context_line":"        props \u003d self.connector.get_connector_properties(\u0027sudo\u0027)"},{"line_number":132,"context_line":"        expected_props \u003d {\"system uuid\": SYS_UUID, \"nqn\": HOST_NQN,"},{"line_number":133,"context_line":"                          \"uuid\": HOST_UUID}"}],"source_content_type":"text/x-python","patch_set":5,"id":"d5e3bd43_8f92a223","line":130,"in_reply_to":"f3fe6b2a_b9333bda","updated":"2021-05-31 15:40:49.000000000","message":"The reason is all of the return value in that test are passed in the decorator whereas in this test all return values are assigned here. I\u0027m not aware of the reason since all the values assigned here are global vars but i wrote it here for consistency.","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"}],"os_brick/tests/initiator/test_connector.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"e383936719f7d8f2109f21a2df018ae5d3ecc938","unresolved":true,"context_lines":[{"line_number":124,"context_line":"                 mock.call(\u0027nvme\u0027, \u0027version\u0027)]"},{"line_number":125,"context_line":"        self.assertEqual(2, mock_execute.call_count)"},{"line_number":126,"context_line":"        mock_execute.assert_has_calls(calls)"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027execute\u0027,"},{"line_number":129,"context_line":"                       side_effect\u003dputils.ProcessExecutionError)"},{"line_number":130,"context_line":"    def test_brick_get_connector_properties_raise(self, mock_execute):"}],"source_content_type":"text/x-python","patch_set":2,"id":"d1eecfbf_4c4d4276","line":127,"updated":"2021-05-20 13:47:46.000000000","message":"-1: We should have unit tests for the new method.","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"217f3cc7d47c40a6528d75565806c7f7c8aaecac","unresolved":false,"context_lines":[{"line_number":124,"context_line":"                 mock.call(\u0027nvme\u0027, \u0027version\u0027)]"},{"line_number":125,"context_line":"        self.assertEqual(2, mock_execute.call_count)"},{"line_number":126,"context_line":"        mock_execute.assert_has_calls(calls)"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027execute\u0027,"},{"line_number":129,"context_line":"                       side_effect\u003dputils.ProcessExecutionError)"},{"line_number":130,"context_line":"    def test_brick_get_connector_properties_raise(self, mock_execute):"}],"source_content_type":"text/x-python","patch_set":2,"id":"af6b8cec_a3e15878","line":127,"in_reply_to":"d1eecfbf_4c4d4276","updated":"2021-05-21 13:11:32.000000000","message":"Done","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9ca9ebfb1fe5c9143663b812202bd7feb49bbf78","unresolved":true,"context_lines":[{"line_number":107,"context_line":"        self._test_brick_get_connector_properties(False, False, False)"},{"line_number":108,"context_line":""},{"line_number":109,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027custom_execute\u0027,"},{"line_number":110,"context_line":"                       side_effect\u003dIOError)"},{"line_number":111,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027execute\u0027, return_value\u003d(\u0027\u0027, \u0027\u0027))"},{"line_number":112,"context_line":"    def test_brick_get_connector_properties_multipath(self, mock_execute,"},{"line_number":113,"context_line":"                                                      mock_custom_execute):"}],"source_content_type":"text/x-python","patch_set":5,"id":"d51d6969_a643bd35","line":110,"updated":"2021-05-31 10:04:46.000000000","message":"Why did we chose IOError instead of OSError(2)?","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"dd14940117aef8cbb7b0c1f9092cd6c1df7e1720","unresolved":false,"context_lines":[{"line_number":107,"context_line":"        self._test_brick_get_connector_properties(False, False, False)"},{"line_number":108,"context_line":""},{"line_number":109,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027custom_execute\u0027,"},{"line_number":110,"context_line":"                       side_effect\u003dIOError)"},{"line_number":111,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027execute\u0027, return_value\u003d(\u0027\u0027, \u0027\u0027))"},{"line_number":112,"context_line":"    def test_brick_get_connector_properties_multipath(self, mock_execute,"},{"line_number":113,"context_line":"                                                      mock_custom_execute):"}],"source_content_type":"text/x-python","patch_set":5,"id":"711fed28_79c4ffb0","line":110,"in_reply_to":"d51d6969_a643bd35","updated":"2021-05-31 15:40:49.000000000","message":"Done","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9ca9ebfb1fe5c9143663b812202bd7feb49bbf78","unresolved":true,"context_lines":[{"line_number":118,"context_line":"        mock_custom_execute.assert_called_once_with(\u0027nvme\u0027, \u0027version\u0027)"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027custom_execute\u0027,"},{"line_number":121,"context_line":"                       side_effect\u003dIOError)"},{"line_number":122,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027execute\u0027,"},{"line_number":123,"context_line":"                       side_effect\u003dputils.ProcessExecutionError)"},{"line_number":124,"context_line":"    def test_brick_get_connector_properties_fallback(self, mock_execute,"}],"source_content_type":"text/x-python","patch_set":5,"id":"a5391a3d_d5b0819d","line":121,"updated":"2021-05-31 10:04:46.000000000","message":"ditto","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"dd14940117aef8cbb7b0c1f9092cd6c1df7e1720","unresolved":false,"context_lines":[{"line_number":118,"context_line":"        mock_custom_execute.assert_called_once_with(\u0027nvme\u0027, \u0027version\u0027)"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027custom_execute\u0027,"},{"line_number":121,"context_line":"                       side_effect\u003dIOError)"},{"line_number":122,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027execute\u0027,"},{"line_number":123,"context_line":"                       side_effect\u003dputils.ProcessExecutionError)"},{"line_number":124,"context_line":"    def test_brick_get_connector_properties_fallback(self, mock_execute,"}],"source_content_type":"text/x-python","patch_set":5,"id":"15771e8d_24268685","line":121,"in_reply_to":"a5391a3d_d5b0819d","updated":"2021-05-31 15:40:49.000000000","message":"Done","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"}],"releasenotes/notes/fix-nvme-issues-8dfc15cb691389fe.yaml":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"e383936719f7d8f2109f21a2df018ae5d3ecc938","unresolved":true,"context_lines":[{"line_number":4,"context_line":"    Fixed issue with nvme logging error trace when not configured."},{"line_number":5,"context_line":"  - |"},{"line_number":6,"context_line":"    Fixed issue when nvme connector is creating /etc/nvme directory"},{"line_number":7,"context_line":"    even when not configured."}],"source_content_type":"text/x-yaml","patch_set":2,"id":"6554bf45_b657fd42","line":7,"updated":"2021-05-20 13:47:46.000000000","message":"-1: We should follow similar format to the one in Cinder:\n\n\nfixes:\n  - |\n    NVMe-oF connector `bug #1929074\n    \u003chttps://bugs.launchpad.net/os-brick/+bug/1929074\u003e`_: Fixed issue with\n    nvme logging error trace when not configured.","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"217f3cc7d47c40a6528d75565806c7f7c8aaecac","unresolved":false,"context_lines":[{"line_number":4,"context_line":"    Fixed issue with nvme logging error trace when not configured."},{"line_number":5,"context_line":"  - |"},{"line_number":6,"context_line":"    Fixed issue when nvme connector is creating /etc/nvme directory"},{"line_number":7,"context_line":"    even when not configured."}],"source_content_type":"text/x-yaml","patch_set":2,"id":"88e9e71d_3045b459","line":7,"in_reply_to":"6554bf45_b657fd42","updated":"2021-05-21 13:11:32.000000000","message":"Done","commit_id":"310857d3e3d35122e16251928df65405a66be1fa"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9ca9ebfb1fe5c9143663b812202bd7feb49bbf78","unresolved":true,"context_lines":[{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    NVMe-oF connector `bug #1929074"},{"line_number":5,"context_line":"    \u003chttps://bugs.launchpad.net/os-brick/+bug/1929074\u003e`_: Fixed issue with"},{"line_number":6,"context_line":"    nvme logging error trace when not configured."},{"line_number":7,"context_line":"  - |"},{"line_number":8,"context_line":"    NVMe-oF connector `bug #1929075"},{"line_number":9,"context_line":"    \u003chttps://bugs.launchpad.net/os-brick/+bug/1929075\u003e`_: Fixed issue with"}],"source_content_type":"text/x-yaml","patch_set":5,"id":"5dc5bafd_f1a02e88","line":6,"range":{"start_line":6,"start_character":34,"end_line":6,"end_character":48},"updated":"2021-05-31 10:04:46.000000000","message":"Should we change this to \"not present\"","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"dd14940117aef8cbb7b0c1f9092cd6c1df7e1720","unresolved":false,"context_lines":[{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    NVMe-oF connector `bug #1929074"},{"line_number":5,"context_line":"    \u003chttps://bugs.launchpad.net/os-brick/+bug/1929074\u003e`_: Fixed issue with"},{"line_number":6,"context_line":"    nvme logging error trace when not configured."},{"line_number":7,"context_line":"  - |"},{"line_number":8,"context_line":"    NVMe-oF connector `bug #1929075"},{"line_number":9,"context_line":"    \u003chttps://bugs.launchpad.net/os-brick/+bug/1929075\u003e`_: Fixed issue with"}],"source_content_type":"text/x-yaml","patch_set":5,"id":"e50944bb_b954400c","line":6,"range":{"start_line":6,"start_character":34,"end_line":6,"end_character":48},"in_reply_to":"5dc5bafd_f1a02e88","updated":"2021-05-31 15:40:49.000000000","message":"Done","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9ca9ebfb1fe5c9143663b812202bd7feb49bbf78","unresolved":true,"context_lines":[{"line_number":7,"context_line":"  - |"},{"line_number":8,"context_line":"    NVMe-oF connector `bug #1929075"},{"line_number":9,"context_line":"    \u003chttps://bugs.launchpad.net/os-brick/+bug/1929075\u003e`_: Fixed issue with"},{"line_number":10,"context_line":"    nvme connector creating /etc/nvme directory when not configured."}],"source_content_type":"text/x-yaml","patch_set":5,"id":"29d1be59_0434a20a","line":10,"range":{"start_line":10,"start_character":53,"end_line":10,"end_character":68},"updated":"2021-05-31 10:04:46.000000000","message":"ditto","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"dd14940117aef8cbb7b0c1f9092cd6c1df7e1720","unresolved":false,"context_lines":[{"line_number":7,"context_line":"  - |"},{"line_number":8,"context_line":"    NVMe-oF connector `bug #1929075"},{"line_number":9,"context_line":"    \u003chttps://bugs.launchpad.net/os-brick/+bug/1929075\u003e`_: Fixed issue with"},{"line_number":10,"context_line":"    nvme connector creating /etc/nvme directory when not configured."}],"source_content_type":"text/x-yaml","patch_set":5,"id":"b09a1fee_7cd60ad7","line":10,"range":{"start_line":10,"start_character":53,"end_line":10,"end_character":68},"in_reply_to":"29d1be59_0434a20a","updated":"2021-05-31 15:40:49.000000000","message":"Done","commit_id":"f95d9cb514e294ceafe5c0de7fcf91e79dd43490"}]}