Petr Horáček has posted comments on this change. Change subject: net: introduce acquire module ......................................................................
Patch Set 19: (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. IIUIC it will be raised up to rollback and rollback will log it, or am i wrong? 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__. Done 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 ? Done PS19, Line 83: turn_down > And here 'release' as well... (if you agree with the previous one) Done 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' wi Done PS19, Line 109: mock_isfile > not needed. Done PS19, Line 113: handle > How about 'file' instead of 'handle'? Done 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. Done 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 Done 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. Done 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. Done -- 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 <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Edward Haas <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Petr Horáček <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
