Edward Haas has posted comments on this change. Change subject: net: introduce acquire module ......................................................................
Patch Set 19: Code-Review-1 (11 comments) https://gerrit.ovirt.org/#/c/60974/19/lib/vdsm/network/ifacquire.py File lib/vdsm/network/ifacquire.py: Line 51: Line 52: def __enter__(self): Line 53: return self Line 54: Line 55: def __exit__(self, type, value, traceback): We better log the exception and the transaction failure. Line 56: if type is None: Line 57: try: Line 58: self._disable_onboot() Line 59: except: Line 65: for iface, ifcfg_lines in six.iteritems(self._ifaces): Line 66: if ifcfg_lines: Line 67: _rollback_ifcfg_iface(iface, ifcfg_lines) Line 68: Line 69: def acquire(self, ifaces): Move after __init__ please or right after __exit__. Line 70: self._backup(ifaces) Line 71: self._turn_down() Line 72: Line 73: def _backup(self, ifaces): PS19, Line 80: _turn_down How about _release_iface ? PS19, Line 83: turn_down And here 'release' as well... (if you agree with the previous one) For non_ifcfg as well https://gerrit.ovirt.org/#/c/60974/19/tests/network/ifacquire_test.py File tests/network/ifacquire_test.py: PS19, Line 82: mock_isfile not needed, no need to generate a MagicMock, just replace 'return_value' with a lambda. @mock.patch.object(ifacquire.os.path, 'isfile', lambda x: False) PS19, Line 109: mock_isfile not needed. PS19, Line 113: handle How about 'file' instead of 'handle'? True for all 'handle' usages... Line 116: atomic_write_handle.readlines.return_value = original_ifcfg Line 117: Line 118: with ifacquire.Transaction(netinfo_nets={}) as a: Line 119: a.acquire(ifaces=[NIC_NAME]) Line 120: atomic_write_handle.writelines.assert_called_with( New line before this please. Please separate between the action and the assertion parts.. just for readability. Line 121: ifcfg_after_turn_down) Line 122: atomic_write_handle.readlines.return_value = ifcfg_after_turn_down Line 123: mock_ifdown.assert_called_with(NIC_NAME) Line 124: atomic_write_handle.writelines.assert_called_with( Line 120: atomic_write_handle.writelines.assert_called_with( Line 121: ifcfg_after_turn_down) Line 122: atomic_write_handle.readlines.return_value = ifcfg_after_turn_down Line 123: mock_ifdown.assert_called_with(NIC_NAME) Line 124: atomic_write_handle.writelines.assert_called_with( New line before this please Line 125: ifcfg_after_disable_onboot) Line 126: Line 127: @mock.patch.object(ifacquire.ifcfg, 'ifdown', return_value=None) Line 128: @mock.patch.object(ifacquire.os.path, 'isfile', return_value=True) PS19, Line 134: mock_isfile, : mock_ifdown No need for these two. @mock.patch.object(ifacquire.ifcfg, 'ifdown', lambda x: None) @mock.patch.object(ifacquire.os.path, 'isfile', lambda x: True) Line 140: Line 141: with self.assertRaises(TestException): Line 142: with ifacquire.Transaction(netinfo_nets={}) as a: Line 143: a.acquire(ifaces=[NIC_NAME]) Line 144: raise TestException() new line before this one. -- 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: 19 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <phora...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Petr Horáček <phora...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
