Skip to content

Commit

Permalink
Removes pylint from our toolchain (apache#16682)
Browse files Browse the repository at this point in the history
We've agreed during the voting process that Pylint support
should be disabled: https://lists.apache.org/thread.html/r9e2cc385db8737ec0874ad09872081bd083593ee29e8303e58d21efb%40%3Cdev.airflow.apache.org%3E

This PR:

* removes all # pylint comments
* removes pylint pre-commits and related scripts/files
* removes CI jobs running pylint checks
* removes documentation about pylint
* removes unnecessary #noga (adds pre-commit for that)
* fixes some remaining pydocstyle errors after removing #noqa's

(cherry picked from commit 866a601)
  • Loading branch information
potiuk authored and jhtimmins committed Jul 9, 2021
1 parent e245360 commit 064dd1c
Show file tree
Hide file tree
Showing 884 changed files with 1,895 additions and 4,466 deletions.
2 changes: 0 additions & 2 deletions .dockerignore
Expand Up @@ -51,8 +51,6 @@
!.rat-excludes
!.flake8
!.dockerignore
!pylintrc
!pylintrc-tests
!pytest.ini
!CHANGELOG.txt
!LICENSE
Expand Down
2 changes: 1 addition & 1 deletion .github/boring-cyborg.yml
Expand Up @@ -203,7 +203,7 @@ firstPRWelcomeComment: >
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits](
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits](
https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks)
will help you with that.
Expand Down
73 changes: 9 additions & 64 deletions .github/workflows/ci.yml
Expand Up @@ -308,7 +308,7 @@ jobs:
needs: [build-info, ci-images]
env:
RUNS_ON: ${{ fromJson(needs.build-info.outputs.runsOn) }}
SKIP: "pylint,identity"
SKIP: "identity"
MOUNT_SELECTED_LOCAL_SOURCES: "true"
PYTHON_MAJOR_MINOR_VERSION: ${{needs.build-info.outputs.defaultPythonVersion}}
GITHUB_REGISTRY: ${{ needs.ci-images.outputs.githubRegistry }}
Expand Down Expand Up @@ -338,20 +338,20 @@ jobs:
${{ hashFiles('setup.py', 'setup.cfg') }}"
restore-keys: "\
pre-commit-local-installation-${{steps.host-python-version.outputs.host-python-version}}-"
- name: "Cache pre-commit envs: no-pylint"
- name: "Cache pre-commit envs"
uses: actions/cache@v2
with:
path: ~/.cache/pre-commit
key: "pre-commit-no-pylint-${{steps.host-python-version.outputs.host-python-version}}-\
key: "pre-commit-${{steps.host-python-version.outputs.host-python-version}}-\
${{ hashFiles('.pre-commit-config.yaml') }}"
restore-keys: pre-commit-no-pylint-${{steps.host-python-version.outputs.host-python-version}}
restore-keys: pre-commit-${{steps.host-python-version.outputs.host-python-version}}

- name: "Cache eslint"
uses: actions/cache@v2
with:
path: 'airflow/ui/node_modules'
key: ${{ runner.os }}-ui-node-modules-${{ hashFiles('airflow/ui/**/yarn.lock') }}
- name: "Static checks: except pylint"
- name: "Static checks"
run: ./scripts/ci/static_checks/run_static_checks.sh
env:
VERBOSE: false
Expand All @@ -366,7 +366,7 @@ ${{ hashFiles('.pre-commit-config.yaml') }}"
needs: [build-info]
env:
RUNS_ON: ${{ fromJson(needs.build-info.outputs.runsOn) }}
SKIP: "build,mypy,flake8,pylint,bats-in-container-tests,identity"
SKIP: "build,mypy,flake8,bats-in-container-tests,identity"
MOUNT_SELECTED_LOCAL_SOURCES: "true"
PYTHON_MAJOR_MINOR_VERSION: ${{needs.build-info.outputs.defaultPythonVersion}}
if: needs.build-info.outputs.basic-checks-only == 'true'
Expand Down Expand Up @@ -398,68 +398,17 @@ ${{ hashFiles('.pre-commit-config.yaml') }}"
${{ hashFiles('setup.py', 'setup.cfg') }}"
restore-keys: "\
pre-commit-local-installation-${{steps.host-python-version.outputs.host-python-version}}-"
- name: "Cache pre-commit envs: no-pylint"
- name: "Cache pre-commit envs"
uses: actions/cache@v2
with:
path: ~/.cache/pre-commit
key: "pre-commit-no-pylint-${{steps.host-python-version.outputs.host-python-version}}-\
key: "pre-commit-basic-${{steps.host-python-version.outputs.host-python-version}}-\
${{ hashFiles('.pre-commit-config.yaml') }}"
restore-keys: pre-commit-no-pylint-${{steps.host-python-version.outputs.host-python-version}}
restore-keys: pre-commit-basic-${{steps.host-python-version.outputs.host-python-version}}
- name: "Static checks: basic checks only"
run: ./scripts/ci/static_checks/run_basic_static_checks.sh "${{ github.sha }}"
env:
VERBOSE: false

static-checks-pylint:
timeout-minutes: 60
name: "Pylint"
runs-on: ${{ fromJson(needs.build-info.outputs.runsOn) }}
needs: [build-info, ci-images]
if: needs.build-info.outputs.basic-checks-only == 'false'
env:
RUNS_ON: ${{ fromJson(needs.build-info.outputs.runsOn) }}
# We want to make sure we have latest sources as only in_container scripts are added
# to the image but we want to static-check all of them
MOUNT_SELECTED_LOCAL_SOURCES: "true"
PYTHON_MAJOR_MINOR_VERSION: ${{needs.build-info.outputs.defaultPythonVersion}}
GITHUB_REGISTRY: ${{ needs.ci-images.outputs.githubRegistry }}
steps:
- name: "Checkout ${{ github.ref }} ( ${{ github.sha }} )"
uses: actions/checkout@v2
with:
persist-credentials: false
- name: "Setup python"
uses: actions/setup-python@v2
with:
python-version: ${{needs.build-info.outputs.defaultPythonVersion}}
- name: "Free space"
run: ./scripts/ci/tools/ci_free_space_on_ci.sh
- name: "Prepare CI image ${{env.PYTHON_MAJOR_MINOR_VERSION}}:${{ env.GITHUB_REGISTRY_PULL_IMAGE_TAG }}"
run: ./scripts/ci/images/ci_prepare_ci_image_on_ci.sh
- name: "Get Python version"
run: "echo \"::set-output name=host-python-version::$(python -c
'import platform; print(platform.python_version())')\""
id: host-python-version
- name: "Cache pre-commit local-installation"
uses: actions/cache@v2
with:
path: ~/.local
key: "pre-commit-local-installation-${{steps.host-python-version.outputs.host-python-version}}-\
${{ hashFiles('setup.py', 'setup.cfg') }}"
restore-keys: "\
pre-commit-local-installation-${{steps.host-python-version.outputs.host-python-version}}-"
- name: "Cache pre-commit envs - pylint"
uses: actions/cache@v2
with:
path: ~/.cache/pre-commit
key: "pre-commit-pylint-${{steps.host-python-version.outputs.host-python-version}}-\
${{ hashFiles('.pre-commit-config.yaml') }}"
restore-keys: pre-commit-pylint-${{steps.host-python-version.outputs.host-python-version}}
- name: "Static checks: pylint"
run: ./scripts/ci/static_checks/run_static_checks.sh pylint
env:
VERBOSE: false

docs:
timeout-minutes: 45
name: "Build docs"
Expand Down Expand Up @@ -1042,7 +991,6 @@ ${{ hashFiles('.pre-commit-config.yaml') }}"
needs:
- build-info
- static-checks
- static-checks-pylint
- tests-sqlite
- tests-postgres
- tests-mysql
Expand Down Expand Up @@ -1105,7 +1053,6 @@ ${{ hashFiles('.pre-commit-config.yaml') }}"
needs:
- build-info
- static-checks
- static-checks-pylint
- tests-sqlite
- tests-postgres
- tests-mysql
Expand Down Expand Up @@ -1152,7 +1099,6 @@ ${{ hashFiles('.pre-commit-config.yaml') }}"
- ci-images
- prod-images
- static-checks
- static-checks-pylint
- tests-sqlite
- tests-mysql
- tests-postgres
Expand Down Expand Up @@ -1221,7 +1167,6 @@ ${{ hashFiles('.pre-commit-config.yaml') }}"
- docs
- build-info
- static-checks
- static-checks-pylint
- tests-sqlite
- tests-postgres
- tests-mysql
Expand Down
56 changes: 21 additions & 35 deletions .pre-commit-config.yaml
Expand Up @@ -218,7 +218,7 @@ repos:
name: Run pydocstyle
args:
- --convention=pep257
- --add-ignore=D100,D102,D104,D105,D107,D205,D400,D401
- --add-ignore=D100,D102,D103,D104,D105,D107,D205,D400,D401
exclude: |
(?x)
^tests/.*\.py$|
Expand All @@ -229,6 +229,14 @@ repos:
.*example_dags/.*|
^chart/.*\.py$|
^airflow/_vendor/
additional_dependencies: ['toml']
- repo: https://github.com/asottile/yesqa
rev: v1.2.3
hooks:
- id: yesqa
exclude: |
(?x)
^airflow/_vendor/
- repo: local
hooks:
- id: lint-openapi
Expand Down Expand Up @@ -339,14 +347,7 @@ repos:
^docs/apache-airflow-providers-apache-cassandra/connections/cassandra\.rst$|
^docs/apache-airflow-providers-apache-hive/commits\.rst$|
git|
^pylintrc |
^CHANGELOG\.txt$
- id: consistent-pylint
language: pygrep
name: Check for inconsistent pylint disable/enable without space
entry: "pylint:disable|pylint:enable"
pass_filenames: true
files: \.py$
- id: base-operator
language: pygrep
name: Check BaseOperator[Link] core imports
Expand Down Expand Up @@ -609,11 +610,12 @@ repos:
additional_dependencies: ['flynt==0.63']
files: \.py$
exclude: ^airflow/_vendor/
- id: bats-in-container-tests
name: Run in container bats tests
language: system
entry: ./scripts/ci/pre_commit/pre_commit_in_container_bats_test.sh
files: ^tests/bats/in_container/.*\.bats$|^scripts/in_container/.*sh
- id: ui-lint
name: ESLint against airflow/ui
language: node
'types_or': [javascript, tsx, ts]
files: ^airflow/ui/
entry: ./scripts/ci/static_checks/eslint.sh
pass_filenames: false
## ADD MOST PRE-COMMITS ABOVE THAT LINE
# The below pre-commits are those requiring CI image to be built
Expand All @@ -623,6 +625,12 @@ repos:
language: system
always_run: true
pass_filenames: false
- id: bats-in-container-tests
name: Run in container bats tests
language: system
entry: ./scripts/ci/pre_commit/pre_commit_in_container_bats_test.sh
files: ^tests/bats/in_container/.*\.bats$|^scripts/in_container/.*sh
pass_filenames: false
- id: mypy
name: Run mypy
language: system
Expand All @@ -642,28 +650,6 @@ repos:
files: ^docs/.*\.py$
exclude: rtd-deprecation
require_serial: false
- id: pylint
name: Run pylint for main code
language: system
entry: ./scripts/ci/pre_commit/pre_commit_pylint.sh
files: \.py$
exclude: ^scripts/.*\.py$|^dev|^provider_packages|^chart|^tests|^kubernetes_tests|^airflow/_vendor/
pass_filenames: true
require_serial: true
- id: pylint
name: Run pylint for tests
language: system
entry: env PYLINTRC=pylintrc-tests ./scripts/ci/pre_commit/pre_commit_pylint.sh
files: ^tests/.*\.py$
pass_filenames: true
require_serial: true
- id: pylint
name: Run pylint for helm chart tests
language: system
entry: env PYLINTRC=pylintrc-tests ./scripts/ci/pre_commit/pre_commit_pylint.sh
files: ^chart/.*\.py$
pass_filenames: true
require_serial: true
- id: flake8
name: Run flake8
language: system
Expand Down
3 changes: 0 additions & 3 deletions .rat-excludes
Expand Up @@ -83,9 +83,6 @@ PROVIDER_CHANGES*.md
manifests/*
redirects.txt

# Temporary list of files to make compatible with Pylint
pylint_todo.txt

# Locally mounted files
.*egg-info/*
.bash_history
Expand Down
30 changes: 15 additions & 15 deletions BREEZE.rst
Expand Up @@ -2239,22 +2239,22 @@ This is the current syntax for `./breeze <./breeze>`_:
Run selected static checks for currently changed files. You should specify static check that
you would like to run or 'all' to run all checks. One of:
all all-but-pylint airflow-config-yaml airflow-providers-available
airflow-provider-yaml-files-ok base-operator bats-tests bats-in-container-tests
black build build-providers-dependencies check-apache-license check-builtin-literals
all airflow-config-yaml airflow-providers-available airflow-provider-yaml-files-ok
base-operator bats-tests bats-in-container-tests black build
build-providers-dependencies check-apache-license check-builtin-literals
check-executables-have-shebangs check-hooks-apply check-integrations
check-merge-conflict check-xml consistent-pylint daysago-import-check
debug-statements detect-private-key doctoc dont-use-safe-filter end-of-file-fixer
fix-encoding-pragma flake8 flynt forbid-tabs helm-lint identity
incorrect-use-of-LoggingMixin insert-license isort json-schema language-matters
lint-dockerfile lint-openapi markdownlint mermaid mixed-line-ending mypy mypy-helm
no-providers-in-core-examples no-relative-imports pre-commit-descriptions
pre-commit-hook-names provide-create-sessions providers-changelogs
providers-init-file provider-yamls pydevd pydocstyle pylint pylint-tests
python-no-log-warn pyupgrade restrict-start_date rst-backticks setup-order
setup-extra-packages shellcheck sort-in-the-wild sort-spelling-wordlist stylelint
trailing-whitespace ui-lint update-breeze-file update-extras update-local-yml-file
update-setup-cfg-file verify-db-migrations-documented version-sync yamllint
check-merge-conflict check-xml daysago-import-check debug-statements
detect-private-key doctoc dont-use-safe-filter end-of-file-fixer fix-encoding-pragma
flake8 flynt forbid-tabs helm-lint identity incorrect-use-of-LoggingMixin
insert-license isort json-schema language-matters lint-dockerfile lint-openapi
markdownlint mermaid mixed-line-ending mypy mypy-helm no-providers-in-core-examples
no-relative-imports pre-commit-descriptions pre-commit-hook-names
provide-create-sessions providers-changelogs providers-init-file provider-yamls
pydevd pydocstyle python-no-log-warn pyupgrade restrict-start_date rst-backticks
setup-order setup-extra-packages shellcheck sort-in-the-wild sort-spelling-wordlist
stylelint trailing-whitespace ui-lint update-breeze-file update-extras
update-local-yml-file update-setup-cfg-file verify-db-migrations-documented
version-sync yamllint yesqa
You can pass extra arguments including options to the pre-commit framework as
<EXTRA_ARGS> passed after --. For example:
Expand Down
4 changes: 1 addition & 3 deletions CI.rst
Expand Up @@ -669,9 +669,7 @@ This workflow is a regular workflow that performs all checks of Airflow code.
+---------------------------+----------------------------------------------+-------+-------+------+
| CI Images | Waits for CI Images (3) | Yes | Yes | Yes |
+---------------------------+----------------------------------------------+-------+-------+------+
| Static checks | Performs static checks without pylint | Yes | Yes | Yes |
+---------------------------+----------------------------------------------+-------+-------+------+
| Static checks: pylint | Performs pylint static checks | Yes | Yes | Yes |
| Static checks | Performs static checks | Yes | Yes | Yes |
+---------------------------+----------------------------------------------+-------+-------+------+
| Build docs | Builds documentation | Yes | Yes | Yes |
+---------------------------+----------------------------------------------+-------+-------+------+
Expand Down
2 changes: 1 addition & 1 deletion CONTRIBUTING.rst
Expand Up @@ -925,7 +925,7 @@ as described in the static code checks documentation.
Coding style and best practices
===============================

Most of our coding style rules are enforced programmatically by flake8 and pylint (which are run automatically
Most of our coding style rules are enforced programmatically by flake8 and mypy (which are run automatically
on every pull request), but there are some rules that are not yet automated and are more Airflow specific or
semantic than style

Expand Down
14 changes: 2 additions & 12 deletions CONTRIBUTORS_QUICK_START.rst
Expand Up @@ -614,8 +614,7 @@ All Tests are inside ./tests directory.
entrypoint_exec.sh* run_install_and_test_provider_packages.sh*
_in_container_script_init.sh* run_mypy.sh*
prod/ run_prepare_provider_packages.sh*
refresh_pylint_todo.sh* run_prepare_provider_documentation.sh*
run_ci_tests.sh* run_pylint.sh*
run_ci_tests.sh* run_prepare_provider_documentation.sh*
run_clear_tmp.sh* run_system_tests.sh*
run_docs_build.sh* run_tmux_welcome.sh*
run_extract_tests.sh* stop_tmux_airflow.sh*
Expand Down Expand Up @@ -812,8 +811,7 @@ To avoid burden on CI infrastructure and to save time, Pre-commit hooks can be r
entrypoint_exec.sh* run_install_and_test_provider_packages.sh*
_in_container_script_init.sh* run_mypy.sh*
prod/ run_prepare_provider_packages.sh*
refresh_pylint_todo.sh* run_prepare_provider_documentation.sh*
run_ci_tests.sh* run_pylint.sh*
run_ci_tests.sh* run_prepare_provider_documentation.sh*
run_clear_tmp.sh* run_system_tests.sh*
run_docs_build.sh* run_tmux_welcome.sh*
run_extract_tests.sh* stop_tmux_airflow.sh*
Expand Down Expand Up @@ -857,14 +855,6 @@ To avoid burden on CI infrastructure and to save time, Pre-commit hooks can be r
<a href="https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#pre-commit-hooks" target="_blank">
Pre-commit Hooks</a>

- |Pylint Static Code Checks|

.. |Pylint Static Code Checks| raw:: html

<a href="https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#pylint-static-code-checks"
target="_blank">Pylint Static Code Checks</a>


- |Running Static Code Checks via Breeze|

.. |Running Static Code Checks via Breeze| raw:: html
Expand Down
6 changes: 3 additions & 3 deletions PULL_REQUEST_WORKFLOW.rst
Expand Up @@ -109,7 +109,7 @@ We have the following test types (separated by packages in which they are):

We also have several special kinds of tests that are not separated by packages but they are marked with
pytest markers. They can be found in any of those packages and they can be selected by the appropriate
pylint custom command line options. See `TESTING.rst <TESTING.rst>`_ for details but those are:
pytest custom command line options. See `TESTING.rst <TESTING.rst>`_ for details but those are:

* Integration - tests that require external integration images running in docker-compose
* Quarantined - tests that are flaky and need to be fixed
Expand Down Expand Up @@ -175,11 +175,11 @@ The logic implemented for the changes works as follows:
Quarantined tests are described in `TESTING.rst <TESTING.rst>`_

11) There is a special case of static checks. In case the above logic determines that the CI image
needs to be build, we run long and more comprehensive version of static checks - including Pylint,
needs to be build, we run long and more comprehensive version of static checks - including
Mypy, Flake8. And those tests are run on all files, no matter how many files changed.
In case the image is not built, we run only simpler set of changes - the longer static checks
that require CI image are skipped, and we only run the tests on the files that changed in the incoming
commit - unlike pylint/flake8/mypy, those static checks are per-file based and they should not miss any
commit - unlike flake8/mypy, those static checks are per-file based and they should not miss any
important change.

Similarly to selective tests we also run selective security scans. In Pull requests,
Expand Down

0 comments on commit 064dd1c

Please sign in to comment.