Repository: aurora Updated Branches: refs/heads/master a0e128d2f -> cd4f7ad6d
Replace Twitter checkstyle with pants checkstyle. This installs the ~newly split off pants python checks contrib plugin. Release notes for that were here: https://pypi.python.org/pypi/pantsbuild.pants/0.0.58 The plugin provides both python checkstyle (`compile.pythonstyle`), and a python eval task (`compile.python-eval`). The `python-eval` is turned off since at least one of the Aurora python targets has files that have side-effects upon import (a repl is started). Now style checks run before compile (and thus before tests) and they benefit from fingerprinting; ie: if you test your changes, those tests will run style checks and when you go to commit, those checks will not be re-run by the commit hook (although files you did not test will still need to be checked). A few production files were fixed up according to style failures coming from: + no space after comment opening '#' + unused variables + mis-aligned hanging closing parens. Bugs closed: AURORA-1532 Reviewed at https://reviews.apache.org/r/40310/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/cd4f7ad6 Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/cd4f7ad6 Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/cd4f7ad6 Branch: refs/heads/master Commit: cd4f7ad6d52bde51e3adbd27139c88ac4bfcf73e Parents: a0e128d Author: John Sirois <[email protected]> Authored: Mon Nov 23 16:49:48 2015 -0800 Committer: Bill Farner <[email protected]> Committed: Mon Nov 23 16:49:48 2015 -0800 ---------------------------------------------------------------------- build-support/hooks/pre-commit | 3 +- build-support/jenkins/build.sh | 8 ++-- build-support/python/checkstyle | 34 ------------- build-support/python/checkstyle-check | 6 +-- pants.ini | 50 ++++++++++++++++++++ .../python/apache/aurora/admin/maintenance.py | 2 +- .../python/apache/aurora/client/api/__init__.py | 7 ++- .../python/apache/aurora/client/cli/client.py | 2 +- .../python/apache/aurora/client/cli/cron.py | 2 +- src/main/python/apache/thermos/core/process.py | 6 +-- .../monitoring/process_collector_psutil.py | 1 - .../python/apache/aurora/admin/test_admin.py | 12 ++--- src/test/python/apache/aurora/admin/util.py | 2 +- .../apache/aurora/client/cli/test_task.py | 3 +- 14 files changed, 77 insertions(+), 61 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/build-support/hooks/pre-commit ---------------------------------------------------------------------- diff --git a/build-support/hooks/pre-commit b/build-support/hooks/pre-commit index 619fa9e..51d97d3 100755 --- a/build-support/hooks/pre-commit +++ b/build-support/hooks/pre-commit @@ -20,7 +20,6 @@ if [ $SKIP_AURORA_HOOKS ]; then fi HERE=$(cd `dirname "${BASH_SOURCE[0]}"` && pwd) -TARGET=$HERE/../../src echo "Performing Python import order check." if ! $HERE/../../build-support/python/isort-check >&/dev/null; then @@ -43,7 +42,7 @@ if ! $HERE/../../build-support/python/checkstyle-check $TARGET >&/dev/null; then echo "" echo "** PYTHON CHECKSTYLE FAILED" echo "*" - echo "* For more information please run: build-support/python/checkstyle-check $TARGET" + echo "* For more information please run: build-support/python/checkstyle-check" echo "*" echo "**" echo "" http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/build-support/jenkins/build.sh ---------------------------------------------------------------------- diff --git a/build-support/jenkins/build.sh b/build-support/jenkins/build.sh index 41a3921..1de1446 100755 --- a/build-support/jenkins/build.sh +++ b/build-support/jenkins/build.sh @@ -20,12 +20,12 @@ date # Run all Java tests ./gradlew -Pq clean build -# Run Python style checks +# Run Python import ordering check ./build-support/python/isort-check -./build-support/python/checkstyle-check src -# Run all Python tests -./pants test.pytest --junit-xml-dir="$PWD/dist/test-results" src/test/python:: -- -v +# Run remaining Python style checks and all tests +./pants test.pytest --junit-xml-dir="$PWD/dist/test-results" \ + src/{main,test}/python:: -- -v # Ensure we can build python sdists (AURORA-1174) ./build-support/release/make-python-sdists http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/build-support/python/checkstyle ---------------------------------------------------------------------- diff --git a/build-support/python/checkstyle b/build-support/python/checkstyle deleted file mode 100755 index 61acc22..0000000 --- a/build-support/python/checkstyle +++ /dev/null @@ -1,34 +0,0 @@ -#!/usr/bin/env bash -# -# Copyright 2014 Apache Software Foundation -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -# -# Wrapper script for running isort -set -e - -HERE=$(cd `dirname "${BASH_SOURCE[0]}"` && pwd) -CHECKSTYLE_VERSION=0.1.0 - -if ! [ -f "$HERE/checkstyle.venv/BOOTSTRAPPED" ] || \ - [ x`cat "$HERE/checkstyle.venv/BOOTSTRAPPED"` != x$CHECKSTYLE_VERSION ]; then - echo Bootstrapping checkstyle @ $CHECKSTYLE_VERSION - rm -fr "$HERE/checkstyle.venv" - "$HERE/../virtualenv" "$HERE/checkstyle.venv" - source "$HERE/checkstyle.venv/bin/activate" - python -m pip install "twitter.checkstyle==$CHECKSTYLE_VERSION" - echo $CHECKSTYLE_VERSION > "$HERE/checkstyle.venv/BOOTSTRAPPED" -fi - -source "$HERE/checkstyle.venv/bin/activate" -exec twitterstyle "$@" http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/build-support/python/checkstyle-check ---------------------------------------------------------------------- diff --git a/build-support/python/checkstyle-check b/build-support/python/checkstyle-check index b2bfc5d..0c88002 100755 --- a/build-support/python/checkstyle-check +++ b/build-support/python/checkstyle-check @@ -18,6 +18,6 @@ set -e HERE=$(cd `dirname "${BASH_SOURCE[0]}"` && pwd) -# Run checkstyle but skip the ImportOrder check as that is enforced by -# isort. -$HERE/checkstyle -n ImportOrder "$@" +# TODO(John Sirois): Consider using `./pants changed` as a feed for this check. +# We need not be checking the whole repo on evey commit. +$HERE/../../pants compile.pythonstyle src/{main,test}/python:: http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/pants.ini ---------------------------------------------------------------------- diff --git a/pants.ini b/pants.ini index d58908c..1e19d9a 100644 --- a/pants.ini +++ b/pants.ini @@ -14,6 +14,10 @@ [DEFAULT] pants_version: 0.0.59 +plugins: [ + 'pantsbuild.pants.contrib.python.checks==%(pants_version)s', + ] + [thrift-binary] # Pants 0.0.57 defaults to 0.9.2, we want to stay pinned down to 0.9.1 for now. @@ -29,3 +33,49 @@ interpreter_requirement: CPython>=2.7,<3 # isolates one pytest session in one chroot per test target. More info here: # http://pantsbuild.github.io/options_reference.html#group_testpytest fast: False + + +# We have some modules that have side-effects upon import, including starting a repl, so we can't +# use python-eval to validate our BUILD deps currently. +[compile.python-eval] +skip: True + + +# We use isort for this. +[pycheck-import-order] +skip: True + + +[pycheck-pep8] +# Code reference is here: http://pep8.readthedocs.org/en/latest/intro.html#error-codes +ignore: [ + # Aurora custom ignores: + 'E114', # indentation is not a multiple of four (comment) + 'E116', # unexpected indentation (comment) + 'E122', # continuation line missing indentation or outdented + 'E126', # continuation line over-indented for hanging indent + 'E129', # visually indented line with same indent as next logical line + 'E131', # continuation line unaligned for hanging indent + 'E731', # do not assign a lambda expression, use a def + 'W503', # line break before binary operator + + # These are a subset of the standard ignores pre-packaged for pycheck-pep8/pep8, but we need to + # repeat here since we add our own above: + 'E111', # indentation is not a multiple of four + 'E121', # continuation line under-indented for hanging indent + 'E125', # continuation line with same indent as next logical line + 'E127', # continuation line over-indented for visual indent + 'E128', # continuation line under-indented for visual indent + 'E301', # expected 1 blank line, found 0 # We allow consecutive exception declarations. + 'E401', # multiple imports on one line + 'E701', # multiple statements on one line (colon) # We allow: `class Exc(Exception): pass`. + ] + + +# We disable the class factoring check since it flags calls to superclass constructors from nested +# classes. We do this commonly enough in nested exception classes. +# The error looks like so: +# T800 Instead of Context.CommandError use self.CommandError or cls.CommandError with +# instancemethods and classmethods respectively. +[pycheck-class-factoring] +skip: True http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/src/main/python/apache/aurora/admin/maintenance.py ---------------------------------------------------------------------- diff --git a/src/main/python/apache/aurora/admin/maintenance.py b/src/main/python/apache/aurora/admin/maintenance.py index 6d94c92..bf44651 100644 --- a/src/main/python/apache/aurora/admin/maintenance.py +++ b/src/main/python/apache/aurora/admin/maintenance.py @@ -32,7 +32,7 @@ from .admin_util import ( from .host_maintenance import HostMaintenance -#TODO(maxim): merge with admin.py commands. +# TODO(maxim): merge with admin.py commands. @app.command @app.command_option(FILENAME_OPTION) @app.command_option(HOSTS_OPTION) http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/src/main/python/apache/aurora/client/api/__init__.py ---------------------------------------------------------------------- diff --git a/src/main/python/apache/aurora/client/api/__init__.py b/src/main/python/apache/aurora/client/api/__init__.py index 6f07a30..1514394 100644 --- a/src/main/python/apache/aurora/client/api/__init__.py +++ b/src/main/python/apache/aurora/client/api/__init__.py @@ -280,8 +280,11 @@ class AuroraClientAPI(object): """ self._assert_valid_job_key(job_key) - return Restarter(job_key, updater_config, health_check_interval_seconds, self._scheduler_proxy - ).restart(instances) + return Restarter( + job_key, + updater_config, + health_check_interval_seconds, + self._scheduler_proxy).restart(instances) def start_maintenance(self, hosts): log.info("Starting maintenance for: %s" % hosts.hostNames) http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/src/main/python/apache/aurora/client/cli/client.py ---------------------------------------------------------------------- diff --git a/src/main/python/apache/aurora/client/cli/client.py b/src/main/python/apache/aurora/client/cli/client.py index 297fb58..fa0c264 100644 --- a/src/main/python/apache/aurora/client/cli/client.py +++ b/src/main/python/apache/aurora/client/cli/client.py @@ -36,7 +36,7 @@ class AuroraLogConfigurationPlugin(ConfigurationPlugin): ] def before_dispatch(self, raw_args): - #TODO(zmanji): Consider raising the default log level to WARN. + # TODO(zmanji): Consider raising the default log level to WARN. loglevel = logging.INFO for arg in raw_args: if arg == "--verbose" or arg == "-v": http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/src/main/python/apache/aurora/client/cli/cron.py ---------------------------------------------------------------------- diff --git a/src/main/python/apache/aurora/client/cli/cron.py b/src/main/python/apache/aurora/client/cli/cron.py index 6376fd0..33087f1 100644 --- a/src/main/python/apache/aurora/client/cli/cron.py +++ b/src/main/python/apache/aurora/client/cli/cron.py @@ -130,7 +130,7 @@ class Show(Verb): return [JOBSPEC_ARGUMENT] def execute(self, context): - #TODO(mchucarroll): do we want to support wildcards here? + # TODO(mchucarroll): do we want to support wildcards here? jobkey = context.options.jobspec api = context.get_api(jobkey.cluster) resp = api.get_jobs(jobkey.role) http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/src/main/python/apache/thermos/core/process.py ---------------------------------------------------------------------- diff --git a/src/main/python/apache/thermos/core/process.py b/src/main/python/apache/thermos/core/process.py index fe95cb3..f214bcc 100644 --- a/src/main/python/apache/thermos/core/process.py +++ b/src/main/python/apache/thermos/core/process.py @@ -205,13 +205,13 @@ class ProcessBase(object): if self._user: if user != current_user and os.geteuid() != 0: raise self.PermissionError('Must be root to run processes as other users!') - uid, gid = user.pw_uid, user.pw_gid self._fork_time = self._platform.clock().time() self._setup_ckpt() self._stdout = safe_open(self._pathspec.with_filename('stdout').getpath('process_logdir'), "a") self._stderr = safe_open(self._pathspec.with_filename('stderr').getpath('process_logdir'), "a") - os.chown(self._stdout.name, user.pw_uid, user.pw_gid) - os.chown(self._stderr.name, user.pw_uid, user.pw_gid) + uid, gid = user.pw_uid, user.pw_gid + os.chown(self._stdout.name, uid, gid) + os.chown(self._stderr.name, uid, gid) def _finalize_fork(self): self._write_initial_update() http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/src/main/python/apache/thermos/monitoring/process_collector_psutil.py ---------------------------------------------------------------------- diff --git a/src/main/python/apache/thermos/monitoring/process_collector_psutil.py b/src/main/python/apache/thermos/monitoring/process_collector_psutil.py index f1ec5a9..bb7c902 100644 --- a/src/main/python/apache/thermos/monitoring/process_collector_psutil.py +++ b/src/main/python/apache/thermos/monitoring/process_collector_psutil.py @@ -61,7 +61,6 @@ class ProcessTreeCollector(object): Returns None: result is stored in self.value """ try: - last_sample, last_stamp = self._sample, self._stamp if self._process is None: self._process = Process(self._pid) parent = self._process http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/src/test/python/apache/aurora/admin/test_admin.py ---------------------------------------------------------------------- diff --git a/src/test/python/apache/aurora/admin/test_admin.py b/src/test/python/apache/aurora/admin/test_admin.py index 8e204ab..e8da335 100644 --- a/src/test/python/apache/aurora/admin/test_admin.py +++ b/src/test/python/apache/aurora/admin/test_admin.py @@ -155,9 +155,9 @@ class TestIncreaseQuotaCommand(AuroraClientCommandTest): increase_quota([self.TEST_CLUSTER, role, '4.0', '1MB', '1MB']) api.set_quota.assert_called_with(role, 24.0, 4001, 6001) - assert type(api.set_quota.call_args[0][1]) == type(float()) - assert type(api.set_quota.call_args[0][2]) == type(int()) - assert type(api.set_quota.call_args[0][3]) == type(int()) + assert isinstance(api.set_quota.call_args[0][1], float) + assert isinstance(api.set_quota.call_args[0][2], int) + assert isinstance(api.set_quota.call_args[0][3], int) class TestSetQuotaCommand(AuroraClientCommandTest): @@ -187,9 +187,9 @@ class TestSetQuotaCommand(AuroraClientCommandTest): set_quota([self.TEST_CLUSTER, role, '4.0', '10MB', '10MB']) api.set_quota.assert_called_with(role, 4.0, 10, 10) - assert type(api.set_quota.call_args[0][1]) == type(float()) - assert type(api.set_quota.call_args[0][2]) == type(int()) - assert type(api.set_quota.call_args[0][3]) == type(int()) + assert isinstance(api.set_quota.call_args[0][1], float) + assert isinstance(api.set_quota.call_args[0][2], int) + assert isinstance(api.set_quota.call_args[0][3], int) class TestGetLocksCommand(AuroraClientCommandTest): http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/src/test/python/apache/aurora/admin/util.py ---------------------------------------------------------------------- diff --git a/src/test/python/apache/aurora/admin/util.py b/src/test/python/apache/aurora/admin/util.py index 3570407..d0a915c 100644 --- a/src/test/python/apache/aurora/admin/util.py +++ b/src/test/python/apache/aurora/admin/util.py @@ -76,7 +76,7 @@ class AuroraClientCommandTest(unittest.TestCase): hosts[hostname].append(JobUpTimeDetails(job, predicted, safe, safe_in)) return [hosts] - #TODO(wfarner): Remove this, force tests to call out their flags. + # TODO(wfarner): Remove this, force tests to call out their flags. @classmethod def setup_mock_options(cls): mock_options = create_autospec(spec=['verbosity'], instance=True) http://git-wip-us.apache.org/repos/asf/aurora/blob/cd4f7ad6/src/test/python/apache/aurora/client/cli/test_task.py ---------------------------------------------------------------------- diff --git a/src/test/python/apache/aurora/client/cli/test_task.py b/src/test/python/apache/aurora/client/cli/test_task.py index 5432a3d..d2233b6 100644 --- a/src/test/python/apache/aurora/client/cli/test_task.py +++ b/src/test/python/apache/aurora/client/cli/test_task.py @@ -147,8 +147,7 @@ class TestSshCommand(AuroraClientCommandTest): jobKeys=[JobKey(role='bozo', environment='test', name='hello')], instanceIds=set([1]), statuses=set([ScheduleStatus.RUNNING, ScheduleStatus.KILLING, ScheduleStatus.RESTARTING, - ScheduleStatus.PREEMPTING, ScheduleStatus.DRAINING - ]))) + ScheduleStatus.PREEMPTING, ScheduleStatus.DRAINING]))) mock_subprocess.assert_called_with(['ssh', '-t', '-v', 'bozo@slavehost', 'cd /slaveroot/slaves/*/frameworks/*/executors/thermos-1287391823/runs/' 'slaverun/sandbox;ls'])
