Change in vdsm[master]: hooks: Add fcoe hook

danken at redhat.com danken at redhat.com
Wed Mar 23 10:56:25 UTC 2016


Dan Kenigsberg has posted comments on this change.

Change subject: hooks: Add fcoe hook
......................................................................


Patch Set 11: Code-Review-1

(5 comments)

https://gerrit.ovirt.org/#/c/55029/11/vdsm_hooks/fcoe/fcoe_before_network_setup.py
File vdsm_hooks/fcoe/fcoe_before_network_setup.py:

Line 37:                   and config.networks[net]["custom"].get("fcoe") == "true"]
Line 38: 
Line 39:     setup_nets_config = hooking.read_json()
Line 40:     changed_networks = setup_nets_config["request"]["networks"]
Line 41: 
I love list comprehensions, but these are over complex, in my opinion.

The code would look simpler if you iterate over all provided networks, and add each one to its proper list.
Line 42:     changed_fcoe = [[net, changed_networks[net]["nic"]]
Line 43:                     for net in changed_networks
Line 44:                     if changed_networks[net].get("custom")
Line 45:                     and hooking.tobool(


PS11, Line 42: ["nic"]
not all networks have a 'nic'. (ps, use single quotes please)

some networks have a 'bond', and some have none. It is fine for this hook to handle only nic networks, but you should not explode on others.


PS11, Line 59: net
this is not a net, but a tuple of a net and its attributes.

I think you'd better make removed_networks and its friend as a dictionary, and then use six.iteritem to iterate over its content.


PS11, Line 74: net[1]
this should be a named variable e.g. net_attrs


PS11, Line 77: # TODO Remove
correct. and right now, you should collect (rc, out, err) and hooking.log() the error in case rc != 0.


-- 
To view, visit https://gerrit.ovirt.org/55029
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad2faed7205ca08801132df072b469dbe781318c
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Pavel Zhukov <pzhukov at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Pavel Zhukov <pzhukov at redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list