Change in vdsm[master]: hooks: Add fcoe hook
nsoffer at redhat.com
nsoffer at redhat.com
Thu Apr 7 15:02:35 UTC 2016
Nir Soffer has posted comments on this change.
Change subject: hooks: Add fcoe hook
......................................................................
Patch Set 18:
(5 comments)
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.
+1
Line 32: and hooking.tobool(net_attr['custom'].get('fcoe'))
Line 33:
Line 34:
Line 35: def _configure(interface):
Line 34:
Line 35: def _configure(interface):
Line 36: """
Line 37: Enable FCoE on specified interface by coping default configuration
Line 38: """
> Why should I do it? Any code convention? I can see hundreds of docstrings i
Peter, this docstring is fine, no change is needed.
We use either:
" title ...last word "
Note the space before and after the title. pep8 allow this format without spaces but I think it is ugly.
Or:
"""
title ...
"""
The second format is better because it is easier to add more text later, not need to change the quotes.
Line 39: # IOError can be raised here is FCOE_DEFAULT_CONFIG doesn't exist
Line 40: # or we're not able to copy file
Line 41: # Don't catch exceptions here. Propagate them to main
Line 42: # and show traceback in supervdsm log
Line 38: """
Line 39: # IOError can be raised here is FCOE_DEFAULT_CONFIG doesn't exist
Line 40: # or we're not able to copy file
Line 41: # Don't catch exceptions here. Propagate them to main
Line 42: # and show traceback in supervdsm log
> I don't think so.
We like to see nicely wrapped text in comments/docstrings.
(for example in vim, ctrl+shift+v, select the lines, press gq).
And please do not call pep8 stupid, you may upset the pep8 gods :-)
Line 43: if interface is None:
Line 44: return
Line 45: filename = os.path.join(FCOE_CONFIG_DIR, 'cfg-%s' % interface)
Line 46: if not os.path.exists(filename):
Line 50:
Line 51:
Line 52: def _unconfigure(interface):
Line 53: """
Line 54: Remove config file for specified interface
> the same here.
This docstring is fine, no change is needed.
Line 55: """
Line 56: if interface is None:
Line 57: return
Line 58: filename = os.path.join(FCOE_CONFIG_DIR, 'cfg-%s' % (interface))
Line 75: for net, net_attr in six.iteritems(config.networks):
Line 76: if _has_fcoe_param(net_attr):
Line 77: if configured[net] is None:
Line 78: hooking.exit_hook("""Failed to configure fcoe
Line 79: on %s with no physical nic""" %
This should be:
hooking.exit_hook("first line ...last word "
"second line ...last word."
% (args, ...))
Note the space at the end of the first line to keep the words separated when in the final message.
Line 80: (net))
Line 81: else:
Line 82: configured[net] = net_attr.get('nic')
Line 83:
--
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: Nir Soffer <nsoffer at redhat.com>
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