)]}'
{"ironic/drivers/modules/ssh.py":[{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"119402eadf3282b575ebc6ef94e3dfa2e17e4993","unresolved":false,"context_lines":[{"line_number":344,"context_line":"    node_name \u003d _get_hosts_name_for_node(ssh_obj, driver_info)"},{"line_number":345,"context_line":"    if node_name:"},{"line_number":346,"context_line":"        # Command should return a list of running vms. If the current node is"},{"line_number":347,"context_line":"        # not listed then we can assume it is not powered on."},{"line_number":348,"context_line":"        cmd_to_exec \u003d \"%s %s\" % (driver_info[\u0027cmd_set\u0027][\u0027base_cmd\u0027],"},{"line_number":349,"context_line":"                                 driver_info[\u0027cmd_set\u0027][\u0027list_running\u0027])"},{"line_number":350,"context_line":"        # Replace for vmware, which has {_NodeName_} in list_running"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_76e564a7","line":347,"updated":"2014-12-08 16:46:52.000000000","message":"This might be a nit but I tend to think that comments w/i the if, reflect what is known to be true. So in the if, I think it makes more sense to be something like \u0027The node name is available so it is powered on\u0027.\n\nWell, maybe the comment makes more sense before the if, where it was before.","commit_id":"ed0754fdb51c39cc764ef0f735cfd20493e870dd"},{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"9130915c9dac089846f6c108f964f9d5ce9fd583","unresolved":false,"context_lines":[{"line_number":347,"context_line":"        # not listed then we can assume it is not powered on."},{"line_number":348,"context_line":"        cmd_to_exec \u003d \"%s %s\" % (driver_info[\u0027cmd_set\u0027][\u0027base_cmd\u0027],"},{"line_number":349,"context_line":"                                 driver_info[\u0027cmd_set\u0027][\u0027list_running\u0027])"},{"line_number":350,"context_line":"        # Replace for vmware, which has {_NodeName_} in list_running"},{"line_number":351,"context_line":"        cmd_to_exec \u003d cmd_to_exec.replace(\u0027{_NodeName_}\u0027, node_name)"},{"line_number":352,"context_line":"        running_list \u003d _ssh_execute(ssh_obj, cmd_to_exec)"},{"line_number":353,"context_line":"        for node in running_list:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_dd69ec26","line":350,"updated":"2014-12-08 03:40:55.000000000","message":"Do we need something like if driver_info[\u0027virt_type\u0027]\u003d\u003d \u0027vmware\u0027:","commit_id":"ed0754fdb51c39cc764ef0f735cfd20493e870dd"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"119402eadf3282b575ebc6ef94e3dfa2e17e4993","unresolved":false,"context_lines":[{"line_number":347,"context_line":"        # not listed then we can assume it is not powered on."},{"line_number":348,"context_line":"        cmd_to_exec \u003d \"%s %s\" % (driver_info[\u0027cmd_set\u0027][\u0027base_cmd\u0027],"},{"line_number":349,"context_line":"                                 driver_info[\u0027cmd_set\u0027][\u0027list_running\u0027])"},{"line_number":350,"context_line":"        # Replace for vmware, which has {_NodeName_} in list_running"},{"line_number":351,"context_line":"        cmd_to_exec \u003d cmd_to_exec.replace(\u0027{_NodeName_}\u0027, node_name)"},{"line_number":352,"context_line":"        running_list \u003d _ssh_execute(ssh_obj, cmd_to_exec)"},{"line_number":353,"context_line":"        for node in running_list:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_762f24a2","line":350,"in_reply_to":"3a961159_1ef28b9a","updated":"2014-12-08 16:46:52.000000000","message":"Tan makes a good point, but given the way the code is structured in this file (with _get_command_sets()), I don\u0027t think it makes sense to put an \u0027if vmware\u0027 thing here. The abstraction is via _get_command_sets(). If we put an \u0027if vmware\u0027 here, and another type is added that also has {_NodeName_}, it won\u0027t work unless the \u0027if vmware\u0027 is removed from here, if anyone remembers to look. So it seems best not to put it in at all.","commit_id":"ed0754fdb51c39cc764ef0f735cfd20493e870dd"},{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"18e37a9fd5079476789c5a7a94d830f2c9be4382","unresolved":false,"context_lines":[{"line_number":347,"context_line":"        # not listed then we can assume it is not powered on."},{"line_number":348,"context_line":"        cmd_to_exec \u003d \"%s %s\" % (driver_info[\u0027cmd_set\u0027][\u0027base_cmd\u0027],"},{"line_number":349,"context_line":"                                 driver_info[\u0027cmd_set\u0027][\u0027list_running\u0027])"},{"line_number":350,"context_line":"        # Replace for vmware, which has {_NodeName_} in list_running"},{"line_number":351,"context_line":"        cmd_to_exec \u003d cmd_to_exec.replace(\u0027{_NodeName_}\u0027, node_name)"},{"line_number":352,"context_line":"        running_list \u003d _ssh_execute(ssh_obj, cmd_to_exec)"},{"line_number":353,"context_line":"        for node in running_list:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_1ef28b9a","line":350,"in_reply_to":"3a961159_9edd9b45","updated":"2014-12-08 06:02:00.000000000","message":"Of course, there is no exception for doing this with other virt_types, but I get confuse about this.\nSo if only vmware has \u0027{__NodeName_}\u0027, we should only work on vmware.","commit_id":"ed0754fdb51c39cc764ef0f735cfd20493e870dd"},{"author":{"_account_id":3068,"name":"Arata Notsu","email":"notsu@virtualtech.jp","username":"arata776"},"change_message_id":"5b105b82c6fbc5a666b21d0096218d410182fe10","unresolved":false,"context_lines":[{"line_number":347,"context_line":"        # not listed then we can assume it is not powered on."},{"line_number":348,"context_line":"        cmd_to_exec \u003d \"%s %s\" % (driver_info[\u0027cmd_set\u0027][\u0027base_cmd\u0027],"},{"line_number":349,"context_line":"                                 driver_info[\u0027cmd_set\u0027][\u0027list_running\u0027])"},{"line_number":350,"context_line":"        # Replace for vmware, which has {_NodeName_} in list_running"},{"line_number":351,"context_line":"        cmd_to_exec \u003d cmd_to_exec.replace(\u0027{_NodeName_}\u0027, node_name)"},{"line_number":352,"context_line":"        running_list \u003d _ssh_execute(ssh_obj, cmd_to_exec)"},{"line_number":353,"context_line":"        for node in running_list:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_9edd9b45","line":350,"in_reply_to":"3a961159_dd69ec26","updated":"2014-12-08 05:49:40.000000000","message":"Although only vmware needs it and I can add the if, I don\u0027t know I should do it. I feel replacing occurs always is less confusing.","commit_id":"ed0754fdb51c39cc764ef0f735cfd20493e870dd"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"91b444dfa06c4c5c6b9c01e776acd248bfc150ed","unresolved":false,"context_lines":[{"line_number":344,"context_line":"    node_name \u003d _get_hosts_name_for_node(ssh_obj, driver_info)"},{"line_number":345,"context_line":"    if node_name:"},{"line_number":346,"context_line":"        # To check the found vm is running, we are going to get a list"},{"line_number":347,"context_line":"        # of running vms on the host."},{"line_number":348,"context_line":"        cmd_to_exec \u003d \"%s %s\" % (driver_info[\u0027cmd_set\u0027][\u0027base_cmd\u0027],"},{"line_number":349,"context_line":"                                 driver_info[\u0027cmd_set\u0027][\u0027list_running\u0027])"},{"line_number":350,"context_line":"        # As our only interest here is the target vm is running or not,"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a961159_ab590200","line":347,"updated":"2014-12-09 14:54:54.000000000","message":"What do you think of something like this, replacing both sets of comments:\n\n\"Get a list of vms running on the host. If the command supports it, explicitly specify the desired node.\"\n\nAs an aside, I see that vmware\u0027s \u0027list_running\u0027 command isn\u0027t a command to list the running vms, which could cause problems if we really needed a \u0027list_running\u0027 command, but we\u0027ll deal with that if we need to ;)","commit_id":"6502a77e2b437f63c7bf652260b7d6aa84036a66"}],"ironic/tests/drivers/test_ssh.py":[{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"119402eadf3282b575ebc6ef94e3dfa2e17e4993","unresolved":false,"context_lines":[{"line_number":280,"context_line":"                          self.sshclient,"},{"line_number":281,"context_line":"                          info)"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"        get_hosts_name_mock.assert_called_once_with(self.sshclient, info)"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    @mock.patch.object(processutils, \u0027ssh_execute\u0027)"},{"line_number":286,"context_line":"    def test__get_power_status_exception_early(self, exec_ssh_mock):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_d93c8b96","line":283,"updated":"2014-12-08 16:46:52.000000000","message":"you could check that exec_ssh_mock wasn\u0027t called.","commit_id":"ed0754fdb51c39cc764ef0f735cfd20493e870dd"},{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"0b3d8f8984d3f6b695520bf106136bc2de37536d","unresolved":false,"context_lines":[{"line_number":281,"context_line":"                          info)"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"        get_hosts_name_mock.assert_called_once_with(self.sshclient, info)"},{"line_number":284,"context_line":"        exec_ssh_mock.assert_not_called()"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    @mock.patch.object(processutils, \u0027ssh_execute\u0027)"},{"line_number":287,"context_line":"    def test__get_power_status_exception(self, exec_ssh_mock):"}],"source_content_type":"text/x-python","patch_set":4,"id":"3a961159_8856e8d0","line":284,"updated":"2014-12-10 12:05:45.000000000","message":"If exec_ssh_mock is not called, why do we need to mock it.\nproperly remove @mock.patch.object(processutils, \u0027ssh_execute\u0027)\nI search all OpenStack test cases, only used 4 times, kind of coding style, I assume.","commit_id":"af21ffd9867a175469ac1f11a394486b43abd519"},{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"0581f9d7554bf056c527bb17dcda19dca75e2355","unresolved":false,"context_lines":[{"line_number":281,"context_line":"                          info)"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"        get_hosts_name_mock.assert_called_once_with(self.sshclient, info)"},{"line_number":284,"context_line":"        exec_ssh_mock.assert_not_called()"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    @mock.patch.object(processutils, \u0027ssh_execute\u0027)"},{"line_number":287,"context_line":"    def test__get_power_status_exception(self, exec_ssh_mock):"}],"source_content_type":"text/x-python","patch_set":4,"id":"3a961159_e6c3ffea","line":284,"in_reply_to":"3a961159_3d78cdc3","updated":"2014-12-11 03:03:26.000000000","message":"OK, I get it. Thanks Ruby.","commit_id":"af21ffd9867a175469ac1f11a394486b43abd519"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"d1a4bc3e84080ebf42a46d7c99e85a5875c64fd9","unresolved":false,"context_lines":[{"line_number":281,"context_line":"                          info)"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"        get_hosts_name_mock.assert_called_once_with(self.sshclient, info)"},{"line_number":284,"context_line":"        exec_ssh_mock.assert_not_called()"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    @mock.patch.object(processutils, \u0027ssh_execute\u0027)"},{"line_number":287,"context_line":"    def test__get_power_status_exception(self, exec_ssh_mock):"}],"source_content_type":"text/x-python","patch_set":4,"id":"3a961159_3d78cdc3","line":284,"in_reply_to":"3a961159_8856e8d0","updated":"2014-12-10 14:59:22.000000000","message":"@Tan. It is useful to test that something isn\u0027t called. Without Arata\u0027s change, this would have been called. So eg if someone changes the code and it gets called when we\u0027re not expecting it to be called, this will fail.","commit_id":"af21ffd9867a175469ac1f11a394486b43abd519"},{"author":{"_account_id":13362,"name":"Tan Lin","email":"lin.tan@intel.com","username":"zero"},"change_message_id":"04b54c23d150c8877ed988c064420a46a446b3a3","unresolved":false,"context_lines":[{"line_number":281,"context_line":"                          info)"},{"line_number":282,"context_line":""},{"line_number":283,"context_line":"        get_hosts_name_mock.assert_called_once_with(self.sshclient, info)"},{"line_number":284,"context_line":"        exec_ssh_mock.assert_not_called()"},{"line_number":285,"context_line":""},{"line_number":286,"context_line":"    @mock.patch.object(processutils, \u0027ssh_execute\u0027)"},{"line_number":287,"context_line":"    def test__get_power_status_exception(self, exec_ssh_mock):"}],"source_content_type":"text/x-python","patch_set":4,"id":"3a961159_f9f1e001","line":284,"in_reply_to":"3a961159_e6c3ffea","updated":"2014-12-11 03:33:09.000000000","message":"My idea is if we don\u0027t mock it, and was called, there should be a SSHCommandFailed exception. But this is more explicit.","commit_id":"af21ffd9867a175469ac1f11a394486b43abd519"}]}