Open Stack

HI Security peeps,

I know some of you are looking at how to add something to hacking, this thread has some useful relevant information.

-Rob

From: Joe Gordon <joe.gordon0 at gmail.com<mailto:joe.gordon0 at gmail.com>>
Reply-To: OpenStack List <openstack-dev at lists.openstack.org<mailto:openstack-dev at lists.openstack.org>>
Date: Tuesday, 24 June 2014 03:55
To: OpenStack List <openstack-dev at lists.openstack.org<mailto:openstack-dev at lists.openstack.org>>
Subject: [openstack-dev] [hacking] community consensus and removing rules


Hi All,

After Friday's thread on removing several hacking rules. H402 and H803 are lines up to removed in the next few days, while Ben has volunteered to work on H305. In addition to helping clarify if we can remove a few specific rules, the thread touched on the bigger issue of how do make sure the rules in hacking reflect the communities lazy consensus of what should be enforced.

The hacking repo has consists primarily of two distinct things, HACKING.rst the OpenStack Style Guide that is published at [0], and hacking the tool. Hacking the style guide's goal is as Sean puts it [1]:

"OpenStack has a set of style guidelines for clarity. OpenStack is a very large code base (over 1 Million lines of python), spanning dozens of git trees, with over a thousand developers contributing every 12 months. As such common style helps developers understand code in reviews, move between projects smoothly, and overall make the code more maintainable."

While hacking the tool is there to move the burden of enforcing the style guide off of human reviewers, as human reviewers are our most constrained resource today.

In the past when evaluating if we should add a new rule to hacking the tool, we followed the  guidelines in [2]. Where consensus was met if the rule was already in HACKING.rst or there was lazy consensus on this mailing list. In retrospect this was a mistake, as this policy makes the assumption that the folks read HACKING.rst and have done a review to decide if any sections should be changed. For example we have a few unenforced sections that we may not want to keep [3][4]. Going forward I propose:

  *   Any addition or removal of a rule requires a ML thread and/or an oslo-specs blueprint (I am not sure which one makes the most sense here, but I am leaning towards the ML).
  *   Only accept new rules that are already being used as a local rule in at least one repository.
  *   Add a new directory, contrib, for local rules that multiple projects use but are not generally considered acceptable to be enabled by default. This way we can reduce the amount of cut and pasted code (thank you to Ben Nemec for this idea).

While turning off rules at the repository level has always been recommended as hacking is a tool to help projects and not a mechanism to dictate style, having rules enabled in hacking does have implications. So we need a mechanism to track which rules folks don't find useful so we can make sure they are not enabled by default (either move them to contrib or remove them entirely). We can track individual repositories hacking ignore lists and periodically reevaluate the rules with the most ignores. This means projects vote on which rules to remove by maintaining there ignore list in tox.ini. I put together a tiny script to do this and here are my findings so far.

rule: number of ignores


H803: 19
H302: 14
H904: 14
H305: 13
H405: 12
H307: 11
H404: 9
H402: 6
H: 4
H233: 3
H202: 3
H306: 2
H301: 2
H303: 1
H703: 1
H304: 1
H237: 1
H4: 1
H201: 1
H701: 1
H102: 1
H702: 1


Interestingly, of the two rules we just agreed to remove, H402 is not even close to the top of the most skipped list. Of the top 3 most skipped rules

  *   H803: The first line of the commit message must not end with a period and must be followed by a single blank line.
     *   Removing, as previously discussed
  *   H302: Do not import objects, only modules (*)
     *   This has been around for a long time, should we move this to contrib?
  *   H904: Wrap long lines in parentheses and not a backslash for line continuation.
     *   Although this has been in HACKING.rst for a while, the rule was added in hacking 0.9. So its still unclear if this is skipped because folks don't like it or because they haven't gotten around to fixing it up yet. Thoughts?


best,
Joe Gordon


[0] http://docs.openstack.org/developer/hacking/
[1] https://review.openstack.org/#/c/102014/2/HACKING.rst
[2] http://docs.openstack.org/developer/hacking/readme.html#adding-additional-checks
[3] http://docs.openstack.org/developer/hacking/#dictionaries-lists
[4] http://docs.openstack.org/developer/hacking/#calling-methods
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ATT00001.txt
URL: <http://lists.openstack.org/pipermail/openstack-security/attachments/20140624/63d9e8dc/attachment.txt>