Merge ~jslarraz/review-tools:add-pre-commit-hook into review-tools:master

[Jorge Sancho Larraz]

Add pre-commit hook to run black, flake8, pylint and unittests

[Alex Murray (alexmurray)]

Thanks for this @jslarraz - I wonder if perhaps we should adapt the existing run-black etc scripts to be able to operate on a list of files, then we can make sure we run it with the same arguments etc as what is done in lpci?

Or if instead the hooks should just call `run-black` etc and run against all the files, and not worry about limiting it to the list of changed files...

Thoughts?

[Jorge Sancho Larraz (jslarraz)]

From my point of view, ideally we would like the git-hook to run black and linters only on modified files (as a `quick` check), and lpci to run them on the whole project anyway.

Independently on how those tools are run (i.e. git-hooks or lpci), they should be called with the same arguments. I tried to use the same arguments in the git-hook as used in current scripts (with the exception of black, as we agreed to start enforcing the format instead of just complain) but I agree that them should be handled in just one place for maintainability. Thus, scripts should be updated to be able to operate on a list of files.

I'm now wondering whether we should define those black/flake8/pylint runners directly in the Makefile defining the relevant targets and get rid of `run-black`, `run-flake8`, ` run-pylint` and `run-helper` to clean up the repository top level directory. Does it make sense?

[Alex Murray (alexmurray)]

Sure, I think putting them in the Makefile sounds like a good idea.

[Emilia Torino (emitorino)]

I only wonder if we should fix the older issues, and have the project clean? this way the check does not need to be run only on changed files.... thoughts?

[Jorge Sancho Larraz (jslarraz)]

@emitorino running checks only on changed files is mainly done for efficiency as you can only introduce format issues on files you are effectively modifying (hopefully). If by "older issues" you mean the code that didn't follow the black convention, it was reformatted on https://code.launchpad.net/~jslarraz/review-tools/+git/review-tools/+merge/465415, if it is anything else, please let me know to take a look :)

[Jorge Sancho Larraz (jslarraz)]

Added a couple of commits on top

https://git.launchpad.net/~jslarraz/review-tools/commit/?id=dd72f359ce84c75e6502e829da1a31f65ac1c9af run black on some files that were not included or explicitly excluded in run-helper. After inspecting the output of black I don't see a good reason to don't format them properly. It will be great if you can take a look to double check I'm not missing anything there.

https://git.launchpad.net/~jslarraz/review-tools/commit/?id=52a3f0fbb8e2db4cc4926a416e18a2850dcf4942 moves test runners to makefile and update pre-commit hook accordingly. **Some special considerations as inline comments**

[Emilia Torino (emitorino)]

> @emitorino running checks only on changed files is mainly done for efficiency
> as you can only introduce format issues on files you are effectively modifying
> (hopefully).

ACK!

If by "older issues" you mean the code that didn't follow the
> black convention, it was reformatted on
> https://code.launchpad.net/~jslarraz/review-tools/+git/review-
> tools/+merge/465415, if it is anything else, please let me know to take a look
> :)

Ah nice! I havent run this recently, sorry!

[Alex Murray (alexmurray)]

LGTM! Thanks @jslarraz

review: Approve

[Alex Murray (alexmurray)]

It would seem that the snap build is now failing - can you take a look @jslarraz? https://launchpad.net/~myapps-reviewers/+snap/review-tools

diff --git a/Makefile b/Makefile
index df2bc11..7acb156 100644
--- a/Makefile
+++ b/Makefile
@@ -25,6 +25,22 @@ DEB_DEPENDENCIES := \
 SNAP_DEPENDENCIES := \
 	black
 
+ifndef FILES
+	FILES := \
+		./reviewtools/*py \
+		./reviewtools/tests/*py \
+		./bin/snap-review \
+		./bin/create-snap-declaration \
+		./bin/dump-tool \
+		./bin/get-base-declaration \
+		./bin/store-query \
+		./bin/rock-check-notices \
+		./bin/rock-updates-available \
+		./bin/snap-updates-available \
+		./bin/snap-check-* \
+		./bin/snap-verify-*
+endif
+
 check-deb-deps:
 	@for dep in $(DEB_DEPENDENCIES); do if ! dpkg -s $$dep 1>/dev/null 2>&1; then echo "Please apt install $$dep"; exit 1; fi; done
 
@@ -33,8 +49,40 @@ check-snap-deps:
 
 check-deps: check-deb-deps check-snap-deps
 
-test:
-	./run-tests
+black: clean
+	black --check $(FILES)
+
+check-style: black
+
+pylint: clean
+	PYTHONPATH=. pylint -E --disable=no-member $(FILES)
+
+flake8: clean
+	flake8 $(FILES)
+
+check-syntax: pylint flake8
+
+.PHONY: check-names.list
+check-names.list:
+	(python3 collect-check-names-from-tests 2>&1 \
+	| grep 'CHECK|' | cut -d '|' -f 2- | egrep -v '(skeleton|some-check)' | LC_ALL=C sort \
+	| awk -F '|' '(l && $$1 != l1) {print l} {l1=$$1; l=$$0} END {print l}') > check-names.list
+
+check-names: check-names.list
+	# make sure check-names.list is up to date
+	cp -f check-names.list check-names.list.orig
+	$(MAKE) check-names.list
+	diff -Naur check-names.list.orig check-names.list || exit 1
+	rm -f check-names.list.orig
+
+unittest:
+	python3 -m unittest discover -v -s ./reviewtools/tests -p 'test_*'
+
+coverage:
+	python3 -m coverage run -m unittest discover -s ./reviewtools/tests -p 'test_*'
+
+coverage-report:
+	python3 -m coverage report --show-missing --omit="*skeleton*,*/dist-packages/*"
 
 functest:
 	./tests/test.sh
@@ -51,27 +99,7 @@ functest-updates:
 functest-dump-tool:
 	./tests/test-dump-tool.sh
 
-coverage:
-	python3 -m coverage run ./run-tests
-
-coverage-report:
-	python3 -m coverage report --show-missing --omit="*skeleton*,*/dist-packages/*"
-
-syntax-check: clean
-	./run-flake8
-	./run-pylint
-
-style-check: clean
-	./run-black
-
-check-names:
-	# make sure check-names.list is up to date
-	cp -f check-names.list check-names.list.orig
-	./collect-check-names
-	diff -Naur check-names.list.orig check-names.list || exit 1
-	rm -f check-names.list.orig
-
-check: check-deps test functest-updates functest-dump-tool functest syntax-check style-check check-names
+check: check-deps check-style check-syntax check-names unittest functest-updates functest-dump-tool functest
 
 clean:
 	rm -rf ./reviewtools/__pycache__ ./reviewtools/tests/__pycache__
@@ -80,6 +108,14 @@ clean:
 	rm -rf ./review-tools-*
 	rm -rf ./build ./review_tools.egg-info
 
-.PHONY: check-names.list
-check-names.list:
-	./collect-check-names
+install-hooks:
+	@if ! snap list | grep black -c >>/dev/null; then \
+		echo '*** black snap is not installed. Please install it and run `make install-hook` again ***'; \
+	elif ! dpkg -l | grep pylint -c >>/dev/null; then \
+		echo '*** pylint package is not installed. Please install it and run `make install-hook` again ***'; \
+	elif ! dpkg -l | grep flake8 -c >>/dev/null; then \
+		echo '*** flake8 package is not installed. Please install it and run `make install-hook` again ***'; \
+	else \
+		echo install -m 755 -b -S .backup git-hooks/pre-commit .git/hooks/pre-commit ; \
+		install -m 755 -b -S .backup git-hooks/pre-commit .git/hooks/pre-commit ; \
+	fi
diff --git a/bin/find-unmatched-bin-versions b/bin/find-unmatched-bin-versions
index 1b85a20..4316f35 100755
--- a/bin/find-unmatched-bin-versions
+++ b/bin/find-unmatched-bin-versions
@@ -22,14 +22,16 @@ for line in lines:
         pkg = line[9:]
         db[rel][pkg] = {}
     elif pkg is not None and line.startswith("Version: "):
-        db[rel][pkg]['version'] = line[9:]
-    elif pkg is not None and line.startswith("Source: ") and ' (' in line:
-        db[rel][pkg]['source'] = line[8:].split(' (')[0]
-        db[rel][pkg]['source_version'] = line.split('(')[1].split(')')[0]
-    elif line == '' and pkg in db[rel]:
-        if 'version' not in db[rel][pkg] or \
-                'source_version' not in db[rel][pkg] or \
-                db[rel][pkg]['version'] == db[rel][pkg]['source_version']:
+        db[rel][pkg]["version"] = line[9:]
+    elif pkg is not None and line.startswith("Source: ") and " (" in line:
+        db[rel][pkg]["source"] = line[8:].split(" (")[0]
+        db[rel][pkg]["source_version"] = line.split("(")[1].split(")")[0]
+    elif line == "" and pkg in db[rel]:
+        if (
+            "version" not in db[rel][pkg]
+            or "source_version" not in db[rel][pkg]
+            or db[rel][pkg]["version"] == db[rel][pkg]["source_version"]
+        ):
             del db[rel][pkg]
 
 print(json.dumps(db, sort_keys=True, indent=2))
diff --git a/bin/rock-check-notices b/bin/rock-check-notices
index 73406f7..e2e0271 100755
--- a/bin/rock-check-notices
+++ b/bin/rock-check-notices
@@ -18,7 +18,6 @@ import argparse
 import json
 import os
 import sys
-import tarfile
 
 import reviewtools.common as common
 from reviewtools.common import (
@@ -49,8 +48,10 @@ def main():
         "--no-fetch", help="use existing security db", action="store_true"
     )
     parser.add_argument("--with-cves", help="show referenced cves", action="store_true")
-    parser.add_argument('--ignore-pockets',
-                        help='ignore updates published to the comma-separated list of pockets')
+    parser.add_argument(
+        "--ignore-pockets",
+        help="ignore updates published to the comma-separated list of pockets",
+    )
     (args, argv) = parser.parse_known_args()
 
     common.REPORT_OUTPUT = "console"
@@ -95,9 +96,11 @@ def main():
         if "USNDB" in os.environ:
             usndb_fn = os.environ["USNDB"]
 
-        cmd_args = ["--usn-db=%s" % usndb_fn,
-                    "--rock=%s" % os.path.abspath(pkg),
-                    "--ignore-pockets=%s" % args.ignore_pockets]
+        cmd_args = [
+            "--usn-db=%s" % usndb_fn,
+            "--rock=%s" % os.path.abspath(pkg),
+            "--ignore-pockets=%s" % args.ignore_pockets,
+        ]
         if args.with_cves:
             cmd_args.append("--with-cves")
 
diff --git a/bin/snap-check-notices b/bin/snap-check-notices
index 76ef609..e1b16c2 100755
--- a/bin/snap-check-notices
+++ b/bin/snap-check-notices
@@ -31,22 +31,26 @@ from reviewtools.common import (
 
 
 def help():
-    print('''Usage:
+    print(
+        """Usage:
 $ %s SNAP1 SNAP2 ...
-''' % os.path.basename(sys.argv[0]))
+"""
+        % os.path.basename(sys.argv[0])
+    )
 
 
 def main():
     parser = argparse.ArgumentParser(
-        prog='check-notices',
-        description='Check a snap for needed security notices'
+        prog="check-notices", description="Check a snap for needed security notices"
+    )
+    parser.add_argument(
+        "--no-fetch", help="use existing security db", action="store_true"
+    )
+    parser.add_argument("--with-cves", help="show referenced cves", action="store_true")
+    parser.add_argument(
+        "--ignore-pockets",
+        help="ignore updates published to the comma-separated list of pockets",
     )
-    parser.add_argument('--no-fetch', help='use existing security db',
-                        action='store_true')
-    parser.add_argument('--with-cves', help='show referenced cves',
-                        action='store_true')
-    parser.add_argument('--ignore-pockets',
-                        help='ignore updates published to the comma-separated list of pockets')
     (args, argv) = parser.parse_known_args()
 
     common.REPORT_OUTPUT = "console"
@@ -75,34 +79,36 @@ def main():
             warn("Skipping '%s', not a regular file" % pkg)
             continue
 
-        snap = os.path.basename(pkg).split('_')[0]
+        snap = os.path.basename(pkg).split("_")[0]
         rev = "unknown"
-        if '_' in pkg:
-            rev = os.path.splitext(os.path.basename(pkg))[0].split('_')[1]
+        if "_" in pkg:
+            rev = os.path.splitext(os.path.basename(pkg))[0].split("_")[1]
 
         if snap in reports and rev in reports[snap]:
             debug("Skipping %s with revision %s" % (snap, rev))
             continue
 
-        usndb_fn = os.path.join(os.environ['SNAP_USER_COMMON'], usndb)
-        if 'USNDB' in os.environ:
-            usndb_fn = os.environ['USNDB']
+        usndb_fn = os.path.join(os.environ["SNAP_USER_COMMON"], usndb)
+        if "USNDB" in os.environ:
+            usndb_fn = os.environ["USNDB"]
 
-        cmd_args = ['--usn-db=%s' % usndb_fn,
-                    '--snap=%s' % os.path.abspath(pkg),
-                    '--ignore-pockets=%s' % args.ignore_pockets]
+        cmd_args = [
+            "--usn-db=%s" % usndb_fn,
+            "--snap=%s" % os.path.abspath(pkg),
+            "--ignore-pockets=%s" % args.ignore_pockets,
+        ]
         if args.with_cves:
-            cmd_args.append('--with-cves')
+            cmd_args.append("--with-cves")
 
         # this carries through to available
         if had_debug is not None:
-            os.unsetenv('SNAP_DEBUG')
+            os.unsetenv("SNAP_DEBUG")
         # don't include stderr in the output since we try and parse this
         # output as JSON below and hence including errors from stderr in
         # the output will make the JSON malformed
         rc, out = cmd([available] + cmd_args, stderr=None)
         if had_debug is not None:
-            os.environ['SNAP_DEBUG'] = had_debug
+            os.environ["SNAP_DEBUG"] = had_debug
 
         if rc != 0:
             warn(out)
@@ -110,7 +116,7 @@ def main():
 
         if snap not in reports:
             reports[snap] = dict()
-        if out == '':
+        if out == "":
             reports[snap][rev] = dict()
         else:
             reports[snap][rev] = json.loads(out)
@@ -118,7 +124,7 @@ def main():
     print(json.dumps(reports, indent=2, sort_keys=True))
 
 
-if __name__ == '__main__':
+if __name__ == "__main__":
     try:
         main()
     except KeyboardInterrupt:
diff --git a/bin/unpack-package b/bin/unpack-package
index e85469c..4b08dbf 100755
--- a/bin/unpack-package
+++ b/bin/unpack-package
@@ -5,7 +5,7 @@ import sys
 
 from reviewtools import common
 
-if __name__ == '__main__':
+if __name__ == "__main__":
     if len(sys.argv) != 3:
         common.error("%s <pkg> <dir>" % os.path.basename(sys.argv[0]))
 
diff --git a/collect-check-names b/collect-check-names
deleted file mode 100755
index a562f8e..0000000
--- a/collect-check-names
+++ /dev/null
@@ -1,5 +0,0 @@
-#!/bin/sh
-
-(./collect-check-names-from-tests 2>&1 | grep 'CHECK|' | cut -d '|' -f 2- \
-    | egrep -v '(skeleton|some-check)' | LC_ALL=C sort \
-    | awk -F '|' '(l && $1 != l1) {print l} {l1=$1; l=$0} END {print l}') > check-names.list
diff --git a/git-hooks/pre-commit b/git-hooks/pre-commit
new file mode 100644
index 0000000..986b90d
--- /dev/null
+++ b/git-hooks/pre-commit
@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+
+# If any command fails, exit immediately with that command's exit status
+set -eo pipefail
+
+# Find all changed files for this commit
+# Compute the diff only once to save a small amount of time.
+CHANGED_FILES=$(git diff --name-only --cached --diff-filter=ACMR)
+# Get only changed files that match our file suffix pattern
+get_pattern_files() {
+    pattern=$(echo "$*" | sed "s/ /\$\\\|/g")
+    echo "$CHANGED_FILES" | { grep "$pattern$" || true; }
+}
+# Get all changed python files
+PY_FILES=$(get_pattern_files .py)
+
+if [[ -n "$PY_FILES" ]]
+then
+    # Run black against changed python files for this commit
+    make black FILES=$PY_FILES
+    echo "black passes all altered python sources."
+    # Run flake8 against changed python files for this commit
+    make flake8 FILES=$PY_FILES
+    echo "flake8 passed!"
+    # Run pylint against changed python files for this commit
+    make pylint FILES=$PY_FILES
+    echo "pylint passed!"
+fi
+
+make unittest
+echo "unit tests passed!"
diff --git a/run-black b/run-black
deleted file mode 100755
index 975b35e..0000000
--- a/run-black
+++ /dev/null
@@ -1,8 +0,0 @@
-#!/bin/sh
-set -e
-
-if command -v black > /dev/null ; then
-   ./run-helper black
-else
-    echo "Could not find black"
-fi
diff --git a/run-flake8 b/run-flake8
deleted file mode 100755
index 4233dd1..0000000
--- a/run-flake8
+++ /dev/null
@@ -1,5 +0,0 @@
-#!/bin/sh
-set -e
-
-flake8 --version
-./run-helper flake8
diff --git a/run-helper b/run-helper
deleted file mode 100755
index e19dadd..0000000
--- a/run-helper
+++ /dev/null
@@ -1,49 +0,0 @@
-#!/bin/sh
-set -e
-
-exe=
-args=
-env=
-case "$1" in
-    flake8)
-        exe="$1"
-        ;;
-    black)
-        exe="$1"
-        # for now, only check
-        args="--diff --quiet"
-        ;;
-    pylint3|pylint)
-        exe="$1"
-        env="PYTHONPATH=."
-        # for now, only errors
-        args="-E --disable=no-member"
-        ;;
-    *)
-        echo "run-helper black|flake8|pylint3|pylint"
-        exit 1
-        ;;
-esac
-
-echo "= $exe ="
-if [ -n "$env" ]; then
-  export "$env"
-fi
-for i in ./reviewtools/*py \
-         ./reviewtools/tests/*py \
-         ./bin/snap-review \
-         ./bin/create-snap-declaration \
-         ./bin/dump-tool \
-         ./bin/get-base-declaration \
-         ./bin/store-query \
-         ./bin/rock-check-notices \
-         ./bin/rock-updates-available \
-         ./bin/snap-updates-available \
-         ./bin/snap-check-* \
-         ./bin/snap-verify-* ; do
-    if echo "$i" | grep -q 'snap-check-notices\|rock-check-notices' ; then
-        continue  # shell
-    fi
-    echo "Checking $i"
-    "$exe" $args "$i"
-done
diff --git a/run-pylint b/run-pylint
deleted file mode 100755
index d00614b..0000000
--- a/run-pylint
+++ /dev/null
@@ -1,10 +0,0 @@
-#!/bin/sh
-set -e
-
-if command -v pylint3 > /dev/null ; then
-    ./run-helper pylint3
-elif command -v pylint > /dev/null ; then
-    ./run-helper pylint
-else
-    echo "Could not find pylint"
-fi
diff --git a/run-tests b/run-tests
deleted file mode 100755
index 4c7d5a0..0000000
--- a/run-tests
+++ /dev/null
@@ -1,38 +0,0 @@
-#!/usr/bin/python3
-'''run-tests: run the test suite'''
-#
-# Copyright (C) 2013 Canonical Ltd.
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation; version 3 of the License.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-import os
-import sys
-import unittest
-
-# NOTE: changes to this file may also need to be made to
-# 'collect-check-names-from-tests'
-
-test_directory = 'reviewtools/tests/'
-if 'RT_EXTRAS_PATH' in os.environ and \
-        os.path.isdir(os.environ['RT_EXTRAS_PATH'] + '/tests'):
-    test_directory = os.environ['RT_EXTRAS_PATH'] + '/tests'
-
-test_filename = 'test_*'
-if len(sys.argv) > 1:
-    test_filename = sys.argv[1]
-
-suite = unittest.TestLoader().discover(test_directory, pattern=test_filename)
-test_result = unittest.TextTestRunner(verbosity=2).run(suite)
-if len(test_result.errors) > 0 or len(test_result.failures) > 0 or \
-        len(test_result.unexpectedSuccesses) > 0:
-    sys.exit(1)
diff --git a/setup.py b/setup.py
index 5d16646..eb4aeb8 100755
--- a/setup.py
+++ b/setup.py
@@ -10,7 +10,7 @@ import re
 changelog = "debian/changelog"
 if os.path.exists(changelog):
     head = codecs.open(changelog, encoding="utf-8").readline()
-    match = re.compile(".*\((.*)\).*").match(head)
+    match = re.compile(".*\((.*)\).*").match(head)  # noqa: W605
     if match:
         version = match.group(1)