From 1c6f334c41cc1bf7bc93fc9bf44193ccbc51fe16 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Thu, 22 Aug 2019 11:52:29 -0400 Subject: [PATCH 1/3] [AIRFLOW-5285] Pylint pre-commit filters out pylint_todo files --- .pre-commit-config.yaml | 21 +++++++++++---- .travis.yml | 8 +----- CONTRIBUTING.md | 7 +---- airflow/gcp/operators/vision.py | 3 +-- scripts/ci/_utils.sh | 13 +++++++++ scripts/ci/ci_before_install.sh | 3 +-- scripts/ci/ci_build.sh | 39 +++++++++++++++++++++++++++ scripts/ci/ci_pylint_main.sh | 12 ++++++++- scripts/ci/ci_pylint_tests.sh | 12 ++++++++- scripts/ci/ci_run_all_static_tests.sh | 4 ++- 10 files changed, 97 insertions(+), 25 deletions(-) create mode 100755 scripts/ci/ci_build.sh diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 8f93b232a0a0a1..01d14306ac7d2d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -20,6 +20,20 @@ default_language_version: # force all unspecified python hooks to run python3 python: python3 repos: + - repo: local + hooks: + - id: build + name: Check if image build is needed + entry: ./scripts/ci/ci_build.sh + language: system + always_run: true + pass_filenames: false + - id: check-apache-license + name: Check if licences are OK for Apache + entry: ./scripts/ci/ci_check_license.sh + language: system + always_run: true + pass_filenames: false - repo: https://github.com/Lucas-C/pre-commit-hooks rev: v1.1.6 hooks: @@ -142,23 +156,20 @@ repos: language: system entry: ./scripts/ci/ci_mypy.sh files: \.py$ - require_serial: true + exclude: ^airflow/_vendor/.*$ - id: pylint name: Run pylint for main sources language: system entry: ./scripts/ci/ci_pylint_main.sh files: \.py$ - exclude: ^tests/.*\.py$ - require_serial: true + exclude: ^tests/.*\.py$|^airflow/_vendor/.*$ - id: pylint name: Run pylint for tests language: system entry: ./scripts/ci/ci_pylint_tests.sh files: ^tests/.*\.py$ - require_serial: true - id: flake8 name: Run flake8 language: system entry: ./scripts/ci/ci_flake8.sh files: \.py$ - require_serial: true diff --git a/.travis.yml b/.travis.yml index b9ca7c9ae53beb..9cd9568130e959 100644 --- a/.travis.yml +++ b/.travis.yml @@ -50,18 +50,12 @@ jobs: env: BACKEND=postgres ENV=kubernetes KUBERNETES_VERSION=v1.13.0 KUBERNETES_MODE=git_mode python: "3.6" stage: test - - name: "Static checks except pylint and docs" + - name: "Static checks" stage: pre-test script: ./scripts/ci/ci_run_all_static_tests.sh - name: Check docs stage: pre-test script: ./scripts/ci/ci_docs.sh - - name: Pylint main - stage: pre-test - script: ./scripts/ci/ci_pylint_main.sh - - name: Pylint tests - stage: pre-test - script: ./scripts/ci/ci_pylint_tests.sh services: - docker before_install: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5fbf752635038f..a8705a26478fa6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -687,7 +687,7 @@ In Linux you typically install them with `sudo apt install` on MacOS with `brew The current list of prerequisites: -* xmllint: Linux - install via `sudo apt install xmllint`, MacOS - install via`brew install xmllint` +* xmllint: Linux - install via `sudo apt install xmllint`, MacOS - install via `brew install xmllint` * yamllint: install via `pip install yamllint` ## Pre-commit hooks installed @@ -729,11 +729,6 @@ run pre-commit hooks manually as needed. *You can run all checks manually on all files by running:* `SKIP=pylint pre-commit run --all-files` -Note this might be very slow for individual tests with pylint because of passing individual files. It is -recommended to run `/scripts/ci/ci_pylint_main.sh` (for the main application files) or -`/scripts/ci/ci_pylint_tests.sh` (for tests) for pylint check. -You can also adding SKIP=pylint variable (as in the example above) if you run pre-commit hooks with --all-files switch. - *You can skip one or more of the checks by specifying comma-separated list of checks to skip in SKIP variable:* `SKIP=pylint,mypy pre-commit run --all-files` diff --git a/airflow/gcp/operators/vision.py b/airflow/gcp/operators/vision.py index 410137046462b0..070c225187a8db 100644 --- a/airflow/gcp/operators/vision.py +++ b/airflow/gcp/operators/vision.py @@ -1,4 +1,4 @@ -# -*- coding: utf-8 -*- +# -*- coding: utf-8 -*- # pylint: disable=too-many-lines # # Licensed to the Apache Software Foundation (ASF) under one # or more contributor license agreements. See the NOTICE file @@ -20,7 +20,6 @@ This module contains a Google Cloud Vision operator. """ -# pylint: disable=too-many-lines from copy import deepcopy from typing import Union, List, Dict, Any, Sequence, Tuple, Optional diff --git a/scripts/ci/_utils.sh b/scripts/ci/_utils.sh index 14a257a6e6cddb..b96cc240c8cde3 100644 --- a/scripts/ci/_utils.sh +++ b/scripts/ci/_utils.sh @@ -660,3 +660,16 @@ function run_docker_lint() { echo fi } + +function filter_out_files_from_pylint_todo_list() { + FILTERED_FILES=() + set +e + for FILE in "$@" + do + if ! grep -x "./${FILE}" <"${AIRFLOW_SOURCES}/scripts/ci/pylint_todo.txt" >/dev/null; then + FILTERED_FILES+=("${FILE}") + fi + done + set -e + export FILTERED_FILES +} diff --git a/scripts/ci/ci_before_install.sh b/scripts/ci/ci_before_install.sh index e95416e4e73c44..e66e0da7451946 100755 --- a/scripts/ci/ci_before_install.sh +++ b/scripts/ci/ci_before_install.sh @@ -34,9 +34,8 @@ docker system prune --all --force if [[ ${TRAVIS_JOB_NAME} == "Tests"* ]]; then rebuild_image_if_needed_for_tests -elif [[ ${TRAVIS_JOB_NAME} == "Check license headers" ]]; then - rebuild_image_if_needed_for_checklicence else + rebuild_image_if_needed_for_checklicence rebuild_image_if_needed_for_static_checks fi diff --git a/scripts/ci/ci_build.sh b/scripts/ci/ci_build.sh new file mode 100755 index 00000000000000..f57a3d7fb68b5f --- /dev/null +++ b/scripts/ci/ci_build.sh @@ -0,0 +1,39 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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. + +set -euo pipefail + +MY_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" + +export AIRFLOW_CI_SILENT=${AIRFLOW_CI_SILENT:="true"} +export ASSUME_QUIT_TO_ALL_QUESTIONS=${ASSUME_QUIT_TO_ALL_QUESTIONS:="true"} + +# shellcheck source=scripts/ci/_utils.sh +. "${MY_DIR}/_utils.sh" + +basic_sanity_checks + +force_python_3_5 + +script_start + +rebuild_image_if_needed_for_static_checks + +rebuild_image_if_needed_for_checklicence + +script_end diff --git a/scripts/ci/ci_pylint_main.sh b/scripts/ci/ci_pylint_main.sh index baa3143e80197c..9e104a7b3b28e3 100755 --- a/scripts/ci/ci_pylint_main.sh +++ b/scripts/ci/ci_pylint_main.sh @@ -34,6 +34,16 @@ script_start rebuild_image_if_needed_for_static_checks -run_pylint_main "$@" +if [[ "${#@}" != "0" ]]; then + filter_out_files_from_pylint_todo_list "$@" + + if [[ "${#FILTERED_FILES[@]}" == "0" ]]; then + echo "Filtered out all files. Skipping pylint." + else + run_pylint_main "${FILTERED_FILES[@]}" + fi +else + run_pylint_main +fi script_end diff --git a/scripts/ci/ci_pylint_tests.sh b/scripts/ci/ci_pylint_tests.sh index b075e51fe0f39d..a906ae934453b1 100755 --- a/scripts/ci/ci_pylint_tests.sh +++ b/scripts/ci/ci_pylint_tests.sh @@ -34,6 +34,16 @@ script_start rebuild_image_if_needed_for_static_checks -run_pylint_tests "$@" +if [[ "${#@}" != "0" ]]; then + filter_out_files_from_pylint_todo_list "$@" + + if [[ "${#FILTERED_FILES[@]}" == "0" ]]; then + echo "Filtered out all files. Skipping pylint." + else + run_pylint_tests "${FILTERED_FILES[@]}" + fi +else + run_pylint_tests +fi script_end diff --git a/scripts/ci/ci_run_all_static_tests.sh b/scripts/ci/ci_run_all_static_tests.sh index 2fea4c6235176f..fabc8d5ff77f54 100755 --- a/scripts/ci/ci_run_all_static_tests.sh +++ b/scripts/ci/ci_run_all_static_tests.sh @@ -34,6 +34,8 @@ script_start rebuild_image_if_needed_for_static_checks -SKIP=pylint pre-commit run --all-files --show-diff-on-failure +rebuild_image_if_needed_for_checklicence + +pre-commit run --all-files --show-diff-on-failure script_end From 5b0af509a46f9ce207d3e26e7373989682cc1416 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Thu, 22 Aug 2019 13:21:02 -0400 Subject: [PATCH 2/3] fixup! [AIRFLOW-5285] Pylint pre-commit filters out pylint_todo files --- .travis.yml | 7 +++- .../ci_run_all_static_tests_except_pylint.sh | 41 +++++++++++++++++++ scripts/ci/ci_run_all_static_tests_pylint.sh | 41 +++++++++++++++++++ 3 files changed, 87 insertions(+), 2 deletions(-) create mode 100755 scripts/ci/ci_run_all_static_tests_except_pylint.sh create mode 100755 scripts/ci/ci_run_all_static_tests_pylint.sh diff --git a/.travis.yml b/.travis.yml index 9cd9568130e959..93243f2b9efff5 100644 --- a/.travis.yml +++ b/.travis.yml @@ -50,9 +50,12 @@ jobs: env: BACKEND=postgres ENV=kubernetes KUBERNETES_VERSION=v1.13.0 KUBERNETES_MODE=git_mode python: "3.6" stage: test - - name: "Static checks" + - name: "Static checks (no pylint)" stage: pre-test - script: ./scripts/ci/ci_run_all_static_tests.sh + script: ./scripts/ci/ci_run_all_static_tests_except_pylint.sh + - name: "Pylint checks" + stage: pre-test + script: ./scripts/ci/ci_run_all_static_tests_pylint.sh - name: Check docs stage: pre-test script: ./scripts/ci/ci_docs.sh diff --git a/scripts/ci/ci_run_all_static_tests_except_pylint.sh b/scripts/ci/ci_run_all_static_tests_except_pylint.sh new file mode 100755 index 00000000000000..276c945604e64c --- /dev/null +++ b/scripts/ci/ci_run_all_static_tests_except_pylint.sh @@ -0,0 +1,41 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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. + +set -euo pipefail + +MY_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" + +export AIRFLOW_CI_SILENT=${AIRFLOW_CI_SILENT:="true"} +export ASSUME_QUIT_TO_ALL_QUESTIONS=${ASSUME_QUIT_TO_ALL_QUESTIONS:="true"} + +# shellcheck source=scripts/ci/_utils.sh +. "${MY_DIR}/_utils.sh" + +basic_sanity_checks + +force_python_3_5 + +script_start + +rebuild_image_if_needed_for_static_checks + +rebuild_image_if_needed_for_checklicence + +SKIP=pylint pre-commit run --all-files --show-diff-on-failure + +script_end diff --git a/scripts/ci/ci_run_all_static_tests_pylint.sh b/scripts/ci/ci_run_all_static_tests_pylint.sh new file mode 100755 index 00000000000000..12e1db51c18a6b --- /dev/null +++ b/scripts/ci/ci_run_all_static_tests_pylint.sh @@ -0,0 +1,41 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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. + +set -euo pipefail + +MY_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" + +export AIRFLOW_CI_SILENT=${AIRFLOW_CI_SILENT:="true"} +export ASSUME_QUIT_TO_ALL_QUESTIONS=${ASSUME_QUIT_TO_ALL_QUESTIONS:="true"} + +# shellcheck source=scripts/ci/_utils.sh +. "${MY_DIR}/_utils.sh" + +basic_sanity_checks + +force_python_3_5 + +script_start + +rebuild_image_if_needed_for_static_checks + +rebuild_image_if_needed_for_checklicence + +pre-commit run pylint --all-files --show-diff-on-failure + +script_end From 26307d81e874c5343693626b2993c3d7304b2cee Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Thu, 22 Aug 2019 20:29:25 -0400 Subject: [PATCH 3/3] fixup! fixup! [AIRFLOW-5285] Pylint pre-commit filters out pylint_todo files --- .travis.yml | 9 +++-- scripts/ci/ci_before_install.sh | 3 +- scripts/ci/ci_build.sh | 2 - ..._all_static_tests_except_pylint_licence.sh | 39 +++++++++++++++++++ scripts/ci/ci_run_all_static_tests_pylint.sh | 2 - 5 files changed, 47 insertions(+), 8 deletions(-) create mode 100755 scripts/ci/ci_run_all_static_tests_except_pylint_licence.sh diff --git a/.travis.yml b/.travis.yml index 93243f2b9efff5..09241adeb989e9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -50,13 +50,16 @@ jobs: env: BACKEND=postgres ENV=kubernetes KUBERNETES_VERSION=v1.13.0 KUBERNETES_MODE=git_mode python: "3.6" stage: test - - name: "Static checks (no pylint)" + - name: "Static checks (no pylint, no licence check)" stage: pre-test - script: ./scripts/ci/ci_run_all_static_tests_except_pylint.sh + script: ./scripts/ci/ci_run_all_static_tests_except_pylint_licence.sh + - name: "Check licence compliance for Apache" + stage: pre-test + script: ./scripts/ci/ci_check_license.sh - name: "Pylint checks" stage: pre-test script: ./scripts/ci/ci_run_all_static_tests_pylint.sh - - name: Check docs + - name: "Build documentation" stage: pre-test script: ./scripts/ci/ci_docs.sh services: diff --git a/scripts/ci/ci_before_install.sh b/scripts/ci/ci_before_install.sh index e66e0da7451946..21ebf4dec4a030 100755 --- a/scripts/ci/ci_before_install.sh +++ b/scripts/ci/ci_before_install.sh @@ -34,8 +34,9 @@ docker system prune --all --force if [[ ${TRAVIS_JOB_NAME} == "Tests"* ]]; then rebuild_image_if_needed_for_tests -else +elif [[ ${TRAVIS_JOB_NAME} == "Check lic"* ]]; then rebuild_image_if_needed_for_checklicence +else rebuild_image_if_needed_for_static_checks fi diff --git a/scripts/ci/ci_build.sh b/scripts/ci/ci_build.sh index f57a3d7fb68b5f..8177695b00e761 100755 --- a/scripts/ci/ci_build.sh +++ b/scripts/ci/ci_build.sh @@ -34,6 +34,4 @@ script_start rebuild_image_if_needed_for_static_checks -rebuild_image_if_needed_for_checklicence - script_end diff --git a/scripts/ci/ci_run_all_static_tests_except_pylint_licence.sh b/scripts/ci/ci_run_all_static_tests_except_pylint_licence.sh new file mode 100755 index 00000000000000..81ea21c3514174 --- /dev/null +++ b/scripts/ci/ci_run_all_static_tests_except_pylint_licence.sh @@ -0,0 +1,39 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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. + +set -euo pipefail + +MY_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" + +export AIRFLOW_CI_SILENT=${AIRFLOW_CI_SILENT:="true"} +export ASSUME_QUIT_TO_ALL_QUESTIONS=${ASSUME_QUIT_TO_ALL_QUESTIONS:="true"} + +# shellcheck source=scripts/ci/_utils.sh +. "${MY_DIR}/_utils.sh" + +basic_sanity_checks + +force_python_3_5 + +script_start + +rebuild_image_if_needed_for_static_checks + +SKIP=pylint,check-apache-license pre-commit run --all-files --show-diff-on-failure + +script_end diff --git a/scripts/ci/ci_run_all_static_tests_pylint.sh b/scripts/ci/ci_run_all_static_tests_pylint.sh index 12e1db51c18a6b..b4980b34aa98b3 100755 --- a/scripts/ci/ci_run_all_static_tests_pylint.sh +++ b/scripts/ci/ci_run_all_static_tests_pylint.sh @@ -34,8 +34,6 @@ script_start rebuild_image_if_needed_for_static_checks -rebuild_image_if_needed_for_checklicence - pre-commit run pylint --all-files --show-diff-on-failure script_end