Edward Haas has posted comments on this change. Change subject: net: api: refactoring of _handleBondings ......................................................................
Patch Set 10: Code-Review-1 (6 comments) I like this cleanup! Thank you! https://gerrit.ovirt.org/#/c/53695/10/lib/vdsm/network/api.py File lib/vdsm/network/api.py: Line 580: bridge.duid_source = bridge.port.name Line 581: break Line 582: Line 583: Line 584: def _objectivize_bond(bond, attrs, configurator, _netinfo, destroy): There needs to be a convincing argument on why this translation is needed. Usually the name of the function can do that (by describing the specific scenario), but here the name does not describe anything specific. Line 585: """Create Bond object based on query attributes""" Line 586: bond = Bond.objectivize(bond, configurator, attrs.get('options'), Line 587: attrs.get('nics'), mtu=None, _netinfo=_netinfo, Line 588: destroyOnMasterRemoval=destroy) Line 615: iteritems Use six Line 616: True Better specify what is True for Line 644: Line 645: return True Line 646: Line 647: Line 648: def _bond_setup(bondings, configurator, _netinfo): What about splitting this one as well? _bonds_edit (& merge with _edit_bonds) _bonds_add (& merge with _add_bonds) Line 649: """Gather to-be-edited/added bondings and apply changes""" Line 650: _netinfo = CachingNetInfo() Line 651: Line 652: edition = {} Line 650: _netinfo = CachingNetInfo() I think you meant _netinfo.updateDevices() Line 655: if 'remove' not in attrs It is strange to ask it again here, it was supposed to be handled in the 'removal' func. Any chance we can drop the question? I think this scenario can only occur with in_rollback=True. -- To view, visit https://gerrit.ovirt.org/53695 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I02e34470a39f84ada0246cf35e12c52990ed7134 Gerrit-PatchSet: 10 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: Ondřej Svoboda <osvob...@redhat.com> 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/mailman/listinfo/vdsm-patches
