Petr Horáček has posted comments on this change. Change subject: net: introduce acquire module ......................................................................
Patch Set 6: (18 comments) https://gerrit.ovirt.org/#/c/60974/5//COMMIT_MSG Commit Message: PS5, Line 9: faces, both pers > 'external' seems redundant and I would use ifaces, seems more focused to wh Done 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? Done PS5, Line 40: > can be removed Done PS5, Line 41: > can be removed Done PS5, Line 49: > can be removed Done PS5, Line 54: > Did you mean non_ifcfg? There can be other types of persistence. Done PS5, Line 57: > can be removed Done Line 61 Line 62 Line 63 Line 64 Line 65 > Add TODOL For NM, the file needs to be reloaded explicitly. Done PS5, Line 74: > can be removed Done https://gerrit.ovirt.org/#/c/60974/5/tests/network/acquire_ifaces_test.py File tests/network/acquire_ifaces_test.py: PS5, Line 26: > not needed Done Line 52 Line 53 Line 54 Line 55 Line 56 > @mock.patch.object(acquire, 'address') is nicer in this case. Done PS5, Line 60: > Can be moved to the class level as it is common to all. Each of them use different return_value. I think it is better to have each test with all needed mocking near it. PS5, Line 61: > owned Done Line 62 Line 63 Line 64 Line 65 Line 66 > There is no need to check this, it is an internal intermediate usage and no Done Line 73 Line 74 Line 75 Line 76 Line 77 > If it is not costing a line, try to add the return_value in the patch defin Done Line 87 Line 88 Line 89 Line 90 Line 91 > If it is not costing a line, try to add the return_value in the patch defin Done Line 90 Line 91 Line 92 Line 93 Line 94 > Will this work? I'm not sure, i tried that but got this: [call('/etc/sysconfig/network-scripts/ifcfg-eth3', 'r+'), call().__enter__(), call().readlines(), call().readlines().__iter__(), call().seek(0), call().writelines(<MagicMock name='open().readlines()' id='139722185447312'>), call().write(u'\n# This device is now owned by VDSM\nONBOOT=no\nNM_CONTROLLED=no\n# end of VDSM suffix.'), call().__exit__(None, None, None)] I cannot assert writelines with original string/list. Line 93 Line 94 Line 95 Line 96 Line 97 > Why new line? 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: 6 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
