Change in vdsm[master]: hooks: Add fcoe hook
phoracek at redhat.com
phoracek at redhat.com
Thu Apr 7 12:27:48 UTC 2016
Petr Horáček has posted comments on this change.
Change subject: hooks: Add fcoe hook
......................................................................
Patch Set 18: Code-Review-1
(10 comments)
most of them are 'code typography', i won't fight for them
https://gerrit.ovirt.org/#/c/55029/18/vdsm_hooks/fcoe/README
File vdsm_hooks/fcoe/README:
PS18, Line 7: --cver=3.6
i don't really mind, but if you drop --cver opt, engine-config will ask user to select version and we won't have to update this file with 4.0
PS18, Line 10:
double space (i see a double space, i can't let it be)
PS18, Line 10:
nasty trailing whitespace
Line 7: sudo engine-config -s UserDefinedNetworkCustomProperties='fcoe=^(true|false)$' --cver=3.6
Line 8:
Line 9: * Verify that custom property was added:
Line 10: engine-config -g UserDefinedNetworkCustomProperties
Line 11:
engine needs to be restarted so changes take place
Line 12: Usage:
Line 13: * Define virtual network for FCoE
Line 14: * Under "setup host networks" attach (drag) it to interface.
Line 15: * Click small "Edit" button and add fcoe property with value "true"
Line 14: * Under "setup host networks" attach (drag) it to interface.
Line 15: * Click small "Edit" button and add fcoe property with value "true"
Line 16:
Line 17:
Line 18:
pending lines
https://gerrit.ovirt.org/#/c/55029/18/vdsm_hooks/fcoe/fcoe_before_network_setup.py
File vdsm_hooks/fcoe/fcoe_before_network_setup.py:
Line 27: def _has_fcoe_param(net_attr):
Line 28: """
Line 29: Check if fcoe parameter was specified for net
Line 30: """
Line 31: return net_attr.get('custom') \
you should use parantheses instead of \ if it is possible.
for example:
return (net_attr.get('custom') and
and hooking.tobool(net_attr['custom'].get('fcoe')))
Line 32: and hooking.tobool(net_attr['custom'].get('fcoe'))
Line 33:
Line 34:
Line 35: def _configure(interface):
PS18, Line 36: """
: Enable FCoE on specified interface by coping default configuration
: """
I think you should fit in on one line:
"""doc string"""
PS18, Line 39: # IOError can be raised here is FCOE_DEFAULT_CONFIG doesn't exist
: # or we're not able to copy file
: # Don't catch exceptions here. Propagate them to main
: # and show traceback in supervdsm log
i feel bad for nagging for code typography but i cannot not comment this.
please fit the full width of the line if possible and does not make sense to create a new line.
Line 50:
Line 51:
Line 52: def _unconfigure(interface):
Line 53: """
Line 54: Remove config file for specified interface
the same here.
Line 55: """
Line 56: if interface is None:
Line 57: return
Line 58: filename = os.path.join(FCOE_CONFIG_DIR, 'cfg-%s' % (interface))
PS18, Line 90: elif hooking.tobool(net_attr.get('remove')):
you don't have to use tobool, as we canonicalize values before we expose them to the hook.
--
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: 18
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: Petr Horáček <phoracek 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