Lints all Python files #2944

conorsch · 2018-01-27T04:43:07Z

Status

Ready for review.

Description of Changes

Lints the entire repo with flake8, omitting only:

config.py (intending specifically securedrop/config.py)
.venv/ (the virtualenv for prod environments, containing pip-installed libs we don't control)

Trimmed out a few unused files, e.g. install_files/ansible-base/roles/restore/files/0.3_restore.py, but only if not used anywhere. Additional files can be removed, but would require QA, so opened #2943 to track one example.

Testing

Run make flake8 and confirm it passes.
Choose Python files at various locations throughout the repository, introduce formatting violations.
Repeat.

See if you can find any Python files that are not caught by this strategy.

Deployment

Minor: as long as the changes presented are genuinely lint-resolving, and not functional changes, then no concern for prod deployments. CI should catch any egregious mistakes.

Checklist

If you made changes to the app code:

Unit and functional tests pass on the development VM

If you made changes to the system configuration:

Configuration tests pass

If you made changes to documentation:

Doc linting passed locally

The Makefile "flake8" target is now more comprehensive, using a blacklist approach rather than a whitelist approach to decide which files to lint. This strategy should catch new Python files introduced to the repository, so that CI will lint them even if they're in new directories. Rather than using a find-based approach, as with the "shellcheck" target, we'll run flake8 on the entire repository and trust flake8 to figure out which files it wants to inspect.

The molecule test suite includes some unique tests, and some that were ported from the older `testinfra/` location. The location under molecule/ was not being linted, and that's now fixed.

The vast majority of changes were simply resolving the formatting of comments. One option was to remove the comments entirely, since they're certainly not frequently consulted, but they may yet be of use to maintainers in the future, so reformatted rather than excised.

Minimal usage on this script, but let's clean it up anyway. If it's in the repo and a Python file that we control, it should pass flake8 linting. Added a few "noqa" lines here to permit non-standard imports, which are required due to the use of "config.py" for SecureDrop configuration. Also tacking on changes to the "restore.py" script, which is closely related.

All required logic has been ported to "restore.py", which is still used by the "restore" role.

The Ansible module was pulled in from a separate repository (see #1468), so the poor formatting is a product of lack of linting over there. Since this is an Ansible module, and not a regular Python module, there are a few common exceptions we'll make, specifically: * F401 * E402 * F403 * F405 For reference, see: https://github.com/HewlettPackard/oneview-ansible/blob/3dc8596861b10d16afe57a4d24bf7bd3dc514f3e/TESTING.md

Straightforward. Removed some extraneous imports and placed a comment on its own line to reduce line length.

This file goes back a while. We've never linted it, so even more recent changes weren't flake8 compliant. Now we're up to snuff. Excused the E501 up top because breaking up the filepath file tuple-syntax would only hurt readability.

The "profile_tasks" Ansible callback plugin was added to the repository back in the Ansible 1.x days. It's no longer necessary, since as of Ansible v2 task profiling was merged into core, and can be enabled via a whitelist option for callbacks in ansible.cfg. Removing the plugin should be handled separately, since all the changes in the current push involve strictly flake8 linting.

Minor changes throughout, all of them were reducing line length.

codecov-io · 2018-01-27T05:25:10Z

Codecov Report

Merging #2944 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2944   +/-   ##
========================================
  Coverage    85.24%   85.24%           
========================================
  Files           32       32           
  Lines         1952     1952           
  Branches       218      218           
========================================
  Hits          1664     1664           
  Misses         237      237           
  Partials        51       51

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58ab66e...e36196e. Read the comment docs.

ghost

Nice zen cleanup :-) A few non-blocking nits in case you don't have anything better to do during the week-end. But good to merge.

ghost · 2018-01-27T08:17:32Z

install_files/ansible-base/callback_plugins/profile_tasks.py

@@ -65,9 +65,10 @@ def playbook_on_stats(self, stats):
            )

        total_seconds = sum([x[1] for x in self.stats.items()])
-        print("\nPlaybook finished: {0}, {1} total tasks.  {2} elapsed. \n".format(
+        print("\nPlaybook finished: {0}, {1} total tasks."
+              "{2} elapsed. \n".format(


Missing space before {2}.

ghost · 2018-01-27T08:19:51Z

install_files/ansible-base/roles/backup/files/0.3_collect.py

-import gnupg
-import subprocess
+import config # noqa: ignore=F403
+import gnupg # noqa: ignore=F403


Note to self: verified manually re, functools and subprocess do not show in the file because CI does not cover this file.

ghost · 2018-01-27T09:02:35Z

install_files/ansible-base/roles/build-ossec-deb-pkg/library/ossec_urls.py

@@ -96,23 +90,23 @@ def parse_checksums(self):


 def main():
-    module = AnsibleModule(
+    module = AnsibleModule( # noqa E405


s/E405/F405/

This one was doubly bad because it was ignoring all flake8 violations, not merely E405 as it appears. Instead of # noqa F405, it should be # noqa: F405 to permit just that one violation.

conorsch · 2018-01-29T23:37:28Z

Thank you for the careful, review @dachary. I agree with your recommendations for changes, will push up corresponding fixes shortly!

@dachary

@dachary pointed out a few touch-ups. In particular, the change to `ossec_urls.py` was a nice catch, in that the "noqa" line was ignoring *all* flake8 violations, not merely the specific one mentioned. @dachary further pointed out that it wasn't E405, but rather F405, that should have been permitted.

ghost

There still are a few noqa that miss the trailing : to ignore only the selected violations

Once that's done, it's all good to merge 👍

@dachary

Again, pointed out by @dachary, there were several instances of "# noqa" improperly disabling *all* flake8 checks, when only a specific check was intended. Resolved by adding colon, i.e. "# noqa:".

conorsch · 2018-01-30T00:03:14Z

Fixed, @dachary! Used this regex to validate:

$ grep -RPis 'noqa\s+\w+$'

The above showed zero results. Compare with:

$ grep -RPis 'noqa:?\s+\w+$'  
.venv/lib/python2.7/site-packages/wheel/util.py:    text_type = unicode  # noqa: F821
.venv/lib/python2.7/site-packages/wheel/tool/__init__.py:        import pkg_resources  # noqa: F401
install_files/ansible-base/roles/build-ossec-deb-pkg/library/ossec_urls.py:import re  # noqa: E402
install_files/ansible-base/roles/build-ossec-deb-pkg/library/ossec_urls.py:        return "https://github.com/ossec/ossec-hids/releases/download/{}/{}".format(  # noqa: E501
install_files/ansible-base/roles/build-ossec-deb-pkg/library/ossec_urls.py:    module = AnsibleModule(  # noqa: F405
install_files/ansible-base/roles/tails-config/files/securedrop_init.py:path_persistent_desktop = '/lib/live/mount/persistence/TailsData_unlocked/dotfiles/Desktop/'  # noqa: E501

Looks good to me at this point, but please point out anything you think I missed!

ghost

Thanks for the quick fix ! Good to merge once CI passes (and while I'm asleep presumably ;-)

Poor form on my part, was again omitting *all* flake8 checks, rather than only the ostensibly declared F403.

ghost

Restamping

Conor Schaefer added 10 commits January 26, 2018 20:36

Flake8: Lints all tests under molecule/

b6c58f0

The molecule test suite includes some unique tests, and some that were ported from the older `testinfra/` location. The location under molecule/ was not being linted, and that's now fixed.

Removes unused 0.3_restore.py script

0278796

All required logic has been ported to "restore.py", which is still used by the "restore" role.

Flake8: securedrop/tests/utils/env.py

37f6c0f

Straightforward. Removed some extraneous imports and placed a comment on its own line to reduce line length.

Flake8: securedrop_init.py

384fcc4

This file goes back a while. We've never linted it, so even more recent changes weren't flake8 compliant. Now we're up to snuff. Excused the E501 up top because breaking up the filepath file tuple-syntax would only hurt readability.

Flake8: Linting on ansible_version_check callback

454b867

Minor changes throughout, all of them were reducing line length.

conorsch requested review from a user and redshiftzero January 27, 2018 04:43

conorsch requested review from heartsucker and msheiny as code owners January 27, 2018 04:43

ghost added feature goals: improve developer workflow labels Jan 27, 2018

ghost previously approved these changes Jan 27, 2018

View reviewed changes

ghost mentioned this pull request Jan 28, 2018

LINT ALL THE THINGS #2947

Closed

conorsch dismissed ghost ’s stale review via 054c4bd January 29, 2018 23:43

conorsch force-pushed the lint-all-python-files branch from 054c4bd to 1a9ffe4 Compare January 29, 2018 23:44

ghost suggested changes Jan 29, 2018

View reviewed changes

Further flake8 changes regarding postfix noqa

8f81caa

Again, pointed out by @dachary, there were several instances of "# noqa" improperly disabling *all* flake8 checks, when only a specific check was intended. Resolved by adding colon, i.e. "# noqa:".

ghost previously approved these changes Jan 30, 2018

View reviewed changes

Yet another flake8 noqa postfix cleanup

e36196e

Poor form on my part, was again omitting *all* flake8 checks, rather than only the ostensibly declared F403.

conorsch dismissed ghost ’s stale review via e36196e January 30, 2018 00:41

ghost approved these changes Jan 30, 2018

View reviewed changes

ghost merged commit 4fd70c4 into develop Jan 30, 2018

msheiny deleted the lint-all-python-files branch April 10, 2018 15:05

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lints all Python files #2944

Lints all Python files #2944

conorsch commented Jan 27, 2018

codecov-io commented Jan 27, 2018 •

edited

Loading

ghost left a comment

ghost Jan 27, 2018

ghost Jan 27, 2018

ghost Jan 27, 2018

conorsch Jan 29, 2018

conorsch commented Jan 29, 2018

ghost left a comment

conorsch commented Jan 30, 2018

ghost left a comment

ghost left a comment

Lints all Python files #2944

Lints all Python files #2944

Conversation

conorsch commented Jan 27, 2018

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to the app code:

If you made changes to the system configuration:

If you made changes to documentation:

codecov-io commented Jan 27, 2018 • edited Loading

Codecov Report

ghost left a comment

Choose a reason for hiding this comment

ghost Jan 27, 2018

Choose a reason for hiding this comment

ghost Jan 27, 2018

Choose a reason for hiding this comment

ghost Jan 27, 2018

Choose a reason for hiding this comment

conorsch Jan 29, 2018

Choose a reason for hiding this comment

conorsch commented Jan 29, 2018

ghost left a comment

Choose a reason for hiding this comment

conorsch commented Jan 30, 2018

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 27, 2018 •

edited

Loading