diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 00000000..55976622 --- /dev/null +++ b/.dockerignore @@ -0,0 +1,14 @@ +*.pyc +build/ +dist/ +*.egg-info +tags +/env +/settings.py +/workspace +/ca.crt +/node_modules +/vendor +/bundle +/.bundle +output.log diff --git a/.travis.yml b/.travis.yml index c0971cb1..d81012c1 100644 --- a/.travis.yml +++ b/.travis.yml @@ -20,6 +20,7 @@ before_install: install: - pip install -r requirements-dev.txt + - pip install -r requirements-linters.txt - pip install . - pip install codecov - pip install coverage diff --git a/Dockerfile b/Dockerfile index 880ce5d7..95c29392 100644 --- a/Dockerfile +++ b/Dockerfile @@ -36,8 +36,8 @@ ENV BUNDLE_SILENCE_ROOT_WARNING 1 ADD Gemfile Gemfile.lock /code/ RUN bundler install --system -ADD requirements.txt /code/ -RUN pip install -r requirements.txt +ADD requirements.txt requirements-linters.txt /code/ +RUN pip install -r requirements.txt -r requirements-linters.txt ADD . /code RUN pip install . RUN cp /code/settings.sample.py /code/settings.py diff --git a/README.mdown b/README.mdown index fb78de23..5e9d96ed 100644 --- a/README.mdown +++ b/README.mdown @@ -178,6 +178,23 @@ are used to find and exclude files when doing a review. Ignore patterns use glob to find files. The patterns start at the reviewed repository root. If you need to ignore mulitple patterns separate them with new lines. +### Multiple projects in one repo + +You can list multiple `tool_` sections as follows (**note the _# suffixes**): + + [tool_eslint_#client] + config = client/.eslintrc.json + working_dir = client + install = true + + [tool_eslint_#server] + config = server/.eslintrc.json + working_dir = server + install = true + +Each section must have a unique name, but anything after and including `_#` is ignored when determining the linter for which it applies. This allows you to run the same linter multiple times with different settings for each run. This can be useful if, as in the example above, you have two different projects (e.g. client and server) in the same repository. + +To avoid duplicate or conflicting comments, make sure that the each project ignores the files of the other. Using the example above, there should be a .eslintignore file in the `client` directory that ignores files in `server`, and vice versa. Depending on the tool, you might also be able to do this using an ignore pattern in the .lintrc. The documentation for each linter outlines which settings are available. ## Running Lint Review @@ -254,6 +271,15 @@ to do style checks on PHP, Javascript and or CSS files. * `exclude` A comma separated list of sniffs to not apply. * `tab_width` The number of spaces to convert tabs into, this is useful for projects using tabs for indentation. +* `working_dir` Change the working directory before linting. +* `install` Run composer install before linting. Runs in `working_dir`, if specified. Otherwise, runs in the root directory of this linter. Any non-empty value for this option will trigger an install. + +`working_dir` and `install` can be used together to avoid having to pre-install linting packages inside this linter. Instead, set `working_dir` to the location of your project's composer.json and set `install = true`. + +**NOTE** If `install` isn't specified, you'll need to install the PHPCS globall: + + cd path/to/lintreview + composer require "squizlabs/php_codesniffer=*" ### Javascript: @@ -284,15 +310,39 @@ you can use this linter you'll need to install nodejs and the jshint npm package #### ESLint -Uses the [eslint](http://eslint.org/) npm module to check javascript files. Before -you can use this linter you'll need to install the eslint npm package: +Uses the [eslint](http://eslint.org/) npm module to check javascript files. + +*Options* + +* `config` Provide a path to the json config file for eslint. +* `extensions` The extensions to check. By default only `.js` and `.jsx` files will be checked. +* `working_dir` Change the working directory before linting. +* `install` Run npm install before linting. Runs in `working_dir`, if specified. Otherwise, runs in the root directory of this linter. Any non-empty value for this option will trigger an install. + +`working_dir` and `install` can be used together to avoid having to pre-install linting packages inside this linter. For example, if one of your projects requires `eslint-plugin-vue` while the other needs `eslint-plugin-react`, you don't need to install them both inside the linter. Instead, set `working_dir` to the location of your project's package.json and set `install = true`. This will also help if you're having problems with the linter being unable to resolve required packages. + +**NOTE** If `install` isn't specified, you'll need to install the eslint npm package: cd path/to/lintreview npm install eslint -*Options* +If you have multiple eslint configurations listed, and if your projects are laid out such that one project's `working_dir` is *inside* the other's, then you should do one of two things: +1. Add `root: true` to the .eslintrc of the project in the child directory +2. Run the project with the top-most `working_dir` first. -* `config` Provide a path to the json config file for eslint. +For example, consider the following setup: + + [tool_eslint_#server] + config = app/.eslintrc.json + working_dir = app + install = true + + [tool_eslint_#client] + config = app/public/.eslintrc.json + working_dir = app/public + install = true + +The client is in `app/public`, which is inside `app`. If you are intentionally extending the server eslint config in the client directory, then the server should be run first by renaming the sections `tool_eslint_#1_server` and `tool_eslint_#2_client`. Otherwiser, if the client's eslint config should be completely independent of the server's, then `root: true` should be added to `app/public/.eslintrc.json`. #### StandardJs diff --git a/alpine.Dockerfile b/alpine.Dockerfile new file mode 100644 index 00000000..99b6b003 --- /dev/null +++ b/alpine.Dockerfile @@ -0,0 +1,21 @@ +FROM alpine:3.7 +RUN apk add --no-cache \ + curl=7.58.0-r1 \ + gcc=6.4.0-r5 \ + git=2.15.0-r1 \ + libffi-dev=3.2.1-r4 \ + libxml2=2.9.7-r0 \ + musl-dev=1.1.18-r3 \ + openssl-dev=1.0.2n-r0 \ + openssl=1.0.2n-r0 \ + python2=2.7.14-r2 \ + python2-dev=2.7.14-r2 \ + py2-pip=9.0.1-r1 \ + zlib-dev=1.2.11-r1 + +WORKDIR /code +ADD requirements.txt ./ +RUN pip install -r requirements.txt +ADD . ./ +RUN pip install . \ + && cp settings.sample.py settings.py diff --git a/docker_healthcheck.sh b/docker_healthcheck.sh index b45bee7b..447a0b20 100755 --- a/docker_healthcheck.sh +++ b/docker_healthcheck.sh @@ -1,10 +1,10 @@ -#!/bin/bash +#!/bin/sh set -eo pipefail -if [ "$1" == "web" ]; then +if [ "$1" = "web" ]; then curl -f http://localhost:5000/ping fi -if [ "$1" == "worker" ]; then +if [ "$1" = "worker" ]; then celery inspect ping -A lintreview.tasks -d "celery@$HOSTNAME" fi diff --git a/lintreview/tasks.py b/lintreview/tasks.py index 3a9fece2..62d442a0 100644 --- a/lintreview/tasks.py +++ b/lintreview/tasks.py @@ -7,13 +7,12 @@ from lintreview.repo import GithubRepository from lintreview.processor import Processor -config = load_config() celery = Celery('lintreview.tasks') -celery.config_from_object(config) +celery.config_from_object('settings') +config = load_config() log = logging.getLogger(__name__) - @celery.task(ignore_result=True) def process_pull_request(user, repo_name, number, lintrc): """ diff --git a/lintreview/tools/__init__.py b/lintreview/tools/__init__.py index 15f13b91..748c3c91 100644 --- a/lintreview/tools/__init__.py +++ b/lintreview/tools/__init__.py @@ -23,6 +23,12 @@ def __init__(self, problems, options=None, base_path=None): if isinstance(options, dict): self.options = options + def get_working_dir(self): + working_dir = self.options.get('working_dir') + if working_dir: + return os.path.join(self.base_path, working_dir) + return os.getcwd() + def check_dependencies(self): """ Used to check for a tools commandline @@ -196,6 +202,7 @@ def factory(config, problems, base_path): for linter in config.linters(): linter_config = config.linter_config(linter) try: + linter = linter.split('_#')[0] classname = linter.capitalize() log.debug("Attempting to import 'lintreview.tools.%s'", linter) mod = __import__('lintreview.tools.' + linter, fromlist='*') @@ -263,6 +270,8 @@ def process_checkstyle(problems, xml, filename_converter): message = err.get('message') if ',' in line: lines = [int(x) for x in line.split(',')] + elif not line.isdigit(): + continue else: lines = [int(line)] list(map(lambda x: problems.add(filename, x, message), lines)) diff --git a/lintreview/tools/eslint.py b/lintreview/tools/eslint.py index 0a0d2745..10e18a3a 100644 --- a/lintreview/tools/eslint.py +++ b/lintreview/tools/eslint.py @@ -6,6 +6,7 @@ from lintreview.review import IssueComment from lintreview.tools import Tool, run_command, process_checkstyle from lintreview.utils import in_path, npm_exists +from lintreview.config import comma_value log = logging.getLogger(__name__) @@ -17,14 +18,17 @@ class Eslint(Tool): def check_dependencies(self): """See if ESLint is on the system path. """ - return in_path('eslint') or npm_exists('eslint') + working_dir = self.get_working_dir() + return in_path('eslint') or npm_exists('eslint', cwd=working_dir) def match_file(self, filename): """Check if a file should be linted using ESLint. """ base = os.path.basename(filename) name, ext = os.path.splitext(base) - return ext == '.js' or ext == '.jsx' + extensions = comma_value(self.options.get('extensions', '.js,.jsx')) + log.debug('Using extensions %s', extensions) + return ext in extensions def has_fixer(self): """Eslint has a fixer that can be enabled @@ -36,12 +40,25 @@ def process_files(self, files): """Run code checks with ESLint. """ log.debug('Processing %s files with %s', files, self.name) + + working_dir = self.get_working_dir() + log.debug('Working in dir %s', working_dir) + + if self.options.get('install'): + output = run_command( + ['npm', 'install'], + ignore_error=True, + cwd=working_dir) + log.debug('Install output: %s', output) + command = self._create_command() command += files output = run_command( command, - ignore_error=True) + ignore_error=True, + cwd=working_dir) + self._process_output(output, files) def process_fixer(self, files): @@ -51,7 +68,8 @@ def process_fixer(self, files): output = run_command( command, ignore_error=True, - include_errors=False) + include_errors=False, + cwd=self.get_working_dir()) log.debug(output) def create_fixer_command(self, files): @@ -62,8 +80,9 @@ def create_fixer_command(self, files): def _create_command(self): cmd = 'eslint' - if npm_exists('eslint'): - cmd = os.path.join(os.getcwd(), 'node_modules', '.bin', 'eslint') + working_dir = self.get_working_dir() + if npm_exists('eslint', cwd=working_dir): + cmd = os.path.join(working_dir, 'node_modules', '.bin', 'eslint') command = [cmd, '--format', 'checkstyle'] # Add config file or default to recommended linters @@ -72,6 +91,7 @@ def _create_command(self): return command def _process_output(self, output, files): + log.debug("Linting output:\n%s", output) if '=1.7.0,<2.0.0 +flake8>=3.3.0,<3.4.0 +yamllint>=1.6.1,<1.7.0 +demjson>=2.2.3,<2.3.0 +ansible-lint>=3.4.11,<3.5.0 +pylint>=1.7.4,<2.0.0 +autopep8>=1.3.0,<2.0.0 diff --git a/requirements.txt b/requirements.txt index 679705da..596837cf 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,15 +1,5 @@ -Flask==0.10.1 -github3.py==1.0.0a4 -celery==3.1.23 -gunicorn==0.17.2 argparse>=1.2.0,<=1.3.0 - -# linters -jinja2<2.9 -pep8>=1.7.0,<2.0.0 -flake8>=3.3.0,<3.4.0 -yamllint>=1.6.1,<1.7.0 -demjson>=2.2.3,<2.3.0 -ansible-lint>=3.4.11,<3.5.0 -pylint>=1.7.4,<2.0.0 -autopep8>=1.3.0,<2.0.0 +celery==4.1.0 +Flask==0.12.2 +github3.py==1.0.0a4 +gunicorn==19.7.1 diff --git a/settings.sample.py b/settings.sample.py index db2de88a..daa4662f 100644 --- a/settings.sample.py +++ b/settings.sample.py @@ -36,7 +36,7 @@ def env(key, default=None, cast=str): # AMQP or other celery broker URL. # amqp paths should be in the form of user:pass@host:port//virtualhost -BROKER_URL = 'amqp://'+''.join([ +broker_url = 'amqp://'+''.join([ env('LINTREVIEW_MQ_USER', 'guest'), ':', env('LINTREVIEW_MQ_PASS', 'guest'), '@', env('LINTREVIEW_MQ_HOST', 'broker'), ':', @@ -45,11 +45,11 @@ def env(key, default=None, cast=str): ]) # Use json for serializing messages. -CELERY_TASK_SERIALIZER = 'json' -CELERY_ACCEPT_CONTENT = ['json'] +task_serializer = 'json' +accept_content = ['json'] # Show dates and times in UTC -CELERY_ENABLE_UTC = True +enable_utc = True # General project configuration @@ -87,13 +87,13 @@ def env(key, default=None, cast=str): SUMMARY_THRESHOLD = env('LINTREVIEW_SUMMARY_THRESHOLD', 50, int) # Used as the author information when making commits -GITHUB_AUTHOR_NAME = 'lintreview' -GITHUB_AUTHOR_EMAIL = 'lintreview@example.com' +GITHUB_AUTHOR_NAME = env('GITHUB_AUTHOR_NAME', 'lintreview') +GITHUB_AUTHOR_EMAIL = env('GITHUB_AUTHOR_EMAIL', 'lintreview@example.com') # Status Configuration ###################### # Customize the build status integration name. Defaults to lintreview. -# APP_NAME = 'lintreview' +APP_NAME = env('LINTREVIEW_APP_NAME', 'lintreview') # Publish result to a pull requests status PULLREQUEST_STATUS = env('LINTREVIEW_PULLREQUEST_STATUS', True, bool)