Edward Haas has posted comments on this change. Change subject: net: introduce acquire module ......................................................................
Patch Set 5: Code-Review-1 (18 comments) https://gerrit.ovirt.org/#/c/60974/5//COMMIT_MSG Commit Message: PS5, Line 9: external devices 'external' seems redundant and I would use ifaces, seems more focused to what we do. https://gerrit.ovirt.org/#/c/60974/5/lib/vdsm/network/acquire.py File lib/vdsm/network/acquire.py: The name is a bit too generic... acquire what? Here some options for you too choose from: netacquire ifacquire acquire_iface Line 1: # Copyright 2016 Red Hat, Inc. Line 2: # Line 3: # This program is free software; you can redistribute it and/or modify Line 4: # it under the terms of the GNU General Public License as published by PS5, Line 40: external_ can be removed PS5, Line 41: external can be removed PS5, Line 49: external can be removed PS5, Line 54: non_persistent Did you mean non_ifcfg? There can be other types of persistence. PS5, Line 57: external can be removed Line 61: _comment_out_parameters(('ONBOOT', 'NM_CONTROLLED'), lines) Line 62: f.seek(0) Line 63: f.writelines(lines) Line 64: f.write(ACQUIRED_IFCFG_SUFFIX) Line 65: ifcfg.ifdown(iface) Add TODOL For NM, the file needs to be reloaded explicitly. Line 66: Line 67: Line 68: def _comment_out_parameters(parameters, lines): Line 69: for i, line in enumerate(lines): PS5, Line 74: external can be removed https://gerrit.ovirt.org/#/c/60974/5/tests/network/acquire_ifaces_test.py File tests/network/acquire_ifaces_test.py: PS5, Line 26: as TestCaseBase not needed Line 52: Line 53: @attr(type='unit') Line 54: class AcquireNicTest(TestCaseBase): Line 55: Line 56: @mock.patch('vdsm.network.acquire.address') @mock.patch.object(acquire, 'address') is nicer in this case. Same for the rest. Line 57: @mock.patch('vdsm.network.acquire.dhclient') Line 58: @mock.patch('vdsm.network.acquire.ifcfg') Line 59: @mock.patch('vdsm.network.acquire.open', create=True) Line 60: @mock.patch('vdsm.network.acquire.os') PS5, Line 60: @mock.patch('vdsm.network.acquire.os') Can be moved to the class level as it is common to all. PS5, Line 61: acquired owned Line 62: mock_dhclient, mock_address): Line 63: acquire.acquire_unowned_ifaces( Line 64: netinfo_nets=NETINFO_NETS, external_ifaces=[NIC_NAME]) Line 65: Line 66: mock_os.path.isfile.assert_not_called() There is no need to check this, it is an internal intermediate usage and not a result. A test cares about a result only. You use a mock if the result is not returned but passed to a 3rd party (like creating a file). Line 67: mock_open.assert_not_called() Line 68: mock_ifcfg.ifdown.assert_not_called() Line 69: mock_dhclient.kill.assert_not_called() Line 70: mock_address.flush.assert_not_called() Line 73: @mock.patch('vdsm.network.acquire.dhclient') Line 74: @mock.patch('vdsm.network.acquire.os') Line 75: def test_acquire_non_persisted_nic(self, mock_os, mock_dhclient, Line 76: mock_address): Line 77: mock_os.path.isfile.return_value = False If it is not costing a line, try to add the return_value in the patch definition. Line 78: mock_dhclient.kill.return_value = None Line 79: mock_address.flush.return_value = None Line 80: Line 81: acquire.acquire_unowned_ifaces( Line 87: Line 88: @mock.patch('vdsm.network.acquire.ifcfg') Line 89: @mock.patch('vdsm.network.acquire.os') Line 90: def test_acquire_ifcfg_persisted_nic(self, mock_os, mock_ifcfg): Line 91: mock_os.path.isfile.return_value = True If it is not costing a line, try to add the return_value in the patch definition. Line 92: mock_ifcfg.ifdown.return_value = None Line 93: Line 94: mock_file = PersistingStringIO(NIC_IFCFG) Line 95: with mock.patch('vdsm.network.acquire.open', return_value=mock_file, Line 90: def test_acquire_ifcfg_persisted_nic(self, mock_os, mock_ifcfg): Line 91: mock_os.path.isfile.return_value = True Line 92: mock_ifcfg.ifdown.return_value = None Line 93: Line 94: mock_file = PersistingStringIO(NIC_IFCFG) Will this work? http://www.voidspace.org.uk/python/mock/helpers.html#mock-open Do we have the correct version of mock for it to work? (For EL7) Line 95: with mock.patch('vdsm.network.acquire.open', return_value=mock_file, Line 96: create=True): Line 97: Line 98: acquire.acquire_unowned_ifaces( Line 93: Line 94: mock_file = PersistingStringIO(NIC_IFCFG) Line 95: with mock.patch('vdsm.network.acquire.open', return_value=mock_file, Line 96: create=True): Line 97: Why new line? Line 98: acquire.acquire_unowned_ifaces( Line 99: netinfo_nets={}, external_ifaces=[NIC_NAME]) Line 100: Line 101: mock_ifcfg.ifdown.assert_called_with(NIC_NAME) -- To view, visit https://gerrit.ovirt.org/60974 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I180cfd7d69c0ae0a24188bc3d909b9d3d7c12145 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Edward Haas <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
