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