From 1649f02129822116c7bfda313894482a56f002a9 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Tue, 28 Jun 2022 18:00:16 +0200 Subject: [PATCH] Protect against using "hook_params" in "get_hook" until we move to 2.3+ (#24706) The "hook_params" method was added in Airflow 2.3 and we should not use it in providers (yet). --- .pre-commit-config.yaml | 4 +- STATIC_CODE_CHECKS.rst | 2 +- .../src/airflow_breeze/pre_commit_ids.py | 2 +- images/breeze/output-commands-hash.txt | 2 +- images/breeze/output-static-checks.svg | 224 +++++++++--------- .../pre_commit_check_2_2_compatibility.py | 10 + 6 files changed, 127 insertions(+), 117 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index ee5c4f6e20b68..bcb89ea708eba 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -334,8 +334,8 @@ repos: entry: ./scripts/ci/pre_commit/pre_commit_check_setup_extra_packages_ref.py additional_dependencies: ['rich>=12.4.4'] # This check might be removed when min-airflow-version in providers is 2.2 - - id: check-airflow-2-1-compatibility - name: Check that providers are 2.1 compatible. + - id: check-airflow-2-2-compatibility + name: Check that providers are 2.2 compatible. entry: ./scripts/ci/pre_commit/pre_commit_check_2_2_compatibility.py language: python pass_filenames: true diff --git a/STATIC_CODE_CHECKS.rst b/STATIC_CODE_CHECKS.rst index 27338c3f9d367..8190695b07f20 100644 --- a/STATIC_CODE_CHECKS.rst +++ b/STATIC_CODE_CHECKS.rst @@ -136,7 +136,7 @@ require Breeze Docker image to be build locally. +--------------------------------------------------------+------------------------------------------------------------------+---------+ | blacken-docs | Run black on python code blocks in documentation files | | +--------------------------------------------------------+------------------------------------------------------------------+---------+ -| check-airflow-2-1-compatibility | Check that providers are 2.1 compatible. | | +| check-airflow-2-2-compatibility | Check that providers are 2.2 compatible. | | +--------------------------------------------------------+------------------------------------------------------------------+---------+ | check-airflow-config-yaml-consistent | Checks for consistency between config.yml and default_config.cfg | | +--------------------------------------------------------+------------------------------------------------------------------+---------+ diff --git a/dev/breeze/src/airflow_breeze/pre_commit_ids.py b/dev/breeze/src/airflow_breeze/pre_commit_ids.py index 29427fea48cfb..db839f0712685 100644 --- a/dev/breeze/src/airflow_breeze/pre_commit_ids.py +++ b/dev/breeze/src/airflow_breeze/pre_commit_ids.py @@ -25,7 +25,7 @@ 'all', 'black', 'blacken-docs', - 'check-airflow-2-1-compatibility', + 'check-airflow-2-2-compatibility', 'check-airflow-config-yaml-consistent', 'check-airflow-providers-have-extras', 'check-apache-license-rat', diff --git a/images/breeze/output-commands-hash.txt b/images/breeze/output-commands-hash.txt index 43538474e18ec..bdcbef10e4649 100644 --- a/images/breeze/output-commands-hash.txt +++ b/images/breeze/output-commands-hash.txt @@ -2,4 +2,4 @@ # This file is automatically generated by pre-commit. If you have a conflict with this file # Please do not solve it but run `breeze regenerate-command-images`. # This command should fix the conflict and regenerate help images that you have conflict with. -906b523aaaaed54525b239a97f3303c1 +81bb48610ea6c4b13c1fa5d7d7cbd061 diff --git a/images/breeze/output-static-checks.svg b/images/breeze/output-static-checks.svg index 49cf642d99ad6..ae64d0be23df3 100644 --- a/images/breeze/output-static-checks.svg +++ b/images/breeze/output-static-checks.svg @@ -19,241 +19,241 @@ font-weight: 700; } - .terminal-749841752-matrix { + .terminal-1143647577-matrix { font-family: Fira Code, monospace; font-size: 20px; line-height: 24.4px; font-variant-east-asian: full-width; } - .terminal-749841752-title { + .terminal-1143647577-title { font-size: 18px; font-weight: bold; font-family: arial; } - .terminal-749841752-r1 { fill: #c5c8c6;font-weight: bold } -.terminal-749841752-r2 { fill: #c5c8c6 } -.terminal-749841752-r3 { fill: #d0b344;font-weight: bold } -.terminal-749841752-r4 { fill: #868887 } -.terminal-749841752-r5 { fill: #68a0b3;font-weight: bold } -.terminal-749841752-r6 { fill: #98a84b;font-weight: bold } -.terminal-749841752-r7 { fill: #8d7b39 } + .terminal-1143647577-r1 { fill: #c5c8c6;font-weight: bold } +.terminal-1143647577-r2 { fill: #c5c8c6 } +.terminal-1143647577-r3 { fill: #d0b344;font-weight: bold } +.terminal-1143647577-r4 { fill: #868887 } +.terminal-1143647577-r5 { fill: #68a0b3;font-weight: bold } +.terminal-1143647577-r6 { fill: #98a84b;font-weight: bold } +.terminal-1143647577-r7 { fill: #8d7b39 } - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - Command: static-checks + Command: static-checks - + - - -Usage: breeze static-checks [OPTIONS] [PRECOMMIT_ARGS]... - -Run static checks. - -╭─ Pre-commit flags ───────────────────────────────────────────────────────────────────────────────────────────────────╮ -│--type-tType(s) of the static checks to run (multiple can be added).                            │ -│(all | black | blacken-docs | check-airflow-2-1-compatibility |                         │ -│check-airflow-config-yaml-consistent | check-airflow-providers-have-extras |            │ -│check-apache-license-rat | check-base-operator-partial-arguments |                      │ -│check-base-operator-usage | check-boring-cyborg-configuration |                         │ -│check-breeze-top-dependencies-limited | check-builtin-literals |                        │ -│check-changelog-has-no-duplicates | check-daysago-import-from-utils |                   │ -│check-docstring-param-types | check-example-dags-urls | check-executables-have-shebangs │ -│| check-extra-packages-references | check-extras-order | check-for-inclusive-language | │ -│check-hooks-apply | check-incorrect-use-of-LoggingMixin |                               │ -│check-integrations-are-consistent | check-merge-conflict | check-newsfragments-are-valid│ -│| check-no-providers-in-core-examples | check-no-relative-imports |                     │ -│check-persist-credentials-disabled-in-github-workflows |                                │ -│check-pre-commit-information-consistent | check-provide-create-sessions-imports |       │ -│check-provider-yaml-valid | check-providers-init-file-missing |                         │ -│check-providers-subpackages-init-file-exist | check-pydevd-left-in-code |               │ -│check-revision-heads-map | check-safe-filter-usage-in-html | check-setup-order |        │ -│check-start-date-not-used-in-defaults | check-system-tests-present |                    │ -│check-system-tests-tocs | check-xml | codespell | create-missing-init-py-files-tests |  │ -│debug-statements | detect-private-key | doctoc | end-of-file-fixer | fix-encoding-pragma│ -│| flynt | forbid-tabs | identity | insert-license | isort | lint-chart-schema | lint-css│ -│| lint-dockerfile | lint-helm-chart | lint-javascript | lint-json-schema | lint-markdown│ -│| lint-openapi | mixed-line-ending | pretty-format-json | pydocstyle |                  │ -│python-no-log-warn | pyupgrade | rst-backticks | run-flake8 | run-mypy | run-shellcheck │ -│| static-check-autoflake | trailing-whitespace | update-breeze-cmd-output |             │ -│update-breeze-readme-config-hash | update-extras | update-in-the-wild-to-be-sorted |    │ -│update-inlined-dockerfile-scripts | update-local-yml-file | update-migration-references │ -│| update-providers-dependencies | update-setup-cfg-file |                               │ -│update-spelling-wordlist-to-be-sorted | update-supported-versions |                     │ -│update-vendored-in-k8s-json-schema | update-version | yamllint | yesqa)                 │ -│--file-fList of files to run the checks on.(PATH)│ -│--all-files-aRun checks on all files.│ -│--show-diff-on-failure-sShow diff for files modified by the checks.│ -│--last-commit-cRun checks for all files in last commit. Mutually exclusive with --commit-ref.│ -╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ -╭─ Options ────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ -│--commit-ref-rRun checks for this commit reference only (can be any git commit-ish reference). Mutually  │ -│exclusive with --last-commit.                                                              │ -│(TEXT)                                                                                     │ -│--verbose-vPrint verbose information about performed steps.│ -│--dry-run-DIf dry-run is set, commands are only printed, not executed.│ -│--github-repository-gGitHub repository used to pull, push run images.(TEXT)[default: apache/airflow]│ -│--help-hShow this message and exit.│ -╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ + + +Usage: breeze static-checks [OPTIONS] [PRECOMMIT_ARGS]... + +Run static checks. + +╭─ Pre-commit flags ───────────────────────────────────────────────────────────────────────────────────────────────────╮ +│--type-tType(s) of the static checks to run (multiple can be added).                            │ +│(all | black | blacken-docs | check-airflow-2-2-compatibility |                         │ +│check-airflow-config-yaml-consistent | check-airflow-providers-have-extras |            │ +│check-apache-license-rat | check-base-operator-partial-arguments |                      │ +│check-base-operator-usage | check-boring-cyborg-configuration |                         │ +│check-breeze-top-dependencies-limited | check-builtin-literals |                        │ +│check-changelog-has-no-duplicates | check-daysago-import-from-utils |                   │ +│check-docstring-param-types | check-example-dags-urls | check-executables-have-shebangs │ +│| check-extra-packages-references | check-extras-order | check-for-inclusive-language | │ +│check-hooks-apply | check-incorrect-use-of-LoggingMixin |                               │ +│check-integrations-are-consistent | check-merge-conflict | check-newsfragments-are-valid│ +│| check-no-providers-in-core-examples | check-no-relative-imports |                     │ +│check-persist-credentials-disabled-in-github-workflows |                                │ +│check-pre-commit-information-consistent | check-provide-create-sessions-imports |       │ +│check-provider-yaml-valid | check-providers-init-file-missing |                         │ +│check-providers-subpackages-init-file-exist | check-pydevd-left-in-code |               │ +│check-revision-heads-map | check-safe-filter-usage-in-html | check-setup-order |        │ +│check-start-date-not-used-in-defaults | check-system-tests-present |                    │ +│check-system-tests-tocs | check-xml | codespell | create-missing-init-py-files-tests |  │ +│debug-statements | detect-private-key | doctoc | end-of-file-fixer | fix-encoding-pragma│ +│| flynt | forbid-tabs | identity | insert-license | isort | lint-chart-schema | lint-css│ +│| lint-dockerfile | lint-helm-chart | lint-javascript | lint-json-schema | lint-markdown│ +│| lint-openapi | mixed-line-ending | pretty-format-json | pydocstyle |                  │ +│python-no-log-warn | pyupgrade | rst-backticks | run-flake8 | run-mypy | run-shellcheck │ +│| static-check-autoflake | trailing-whitespace | update-breeze-cmd-output |             │ +│update-breeze-readme-config-hash | update-extras | update-in-the-wild-to-be-sorted |    │ +│update-inlined-dockerfile-scripts | update-local-yml-file | update-migration-references │ +│| update-providers-dependencies | update-setup-cfg-file |                               │ +│update-spelling-wordlist-to-be-sorted | update-supported-versions |                     │ +│update-vendored-in-k8s-json-schema | update-version | yamllint | yesqa)                 │ +│--file-fList of files to run the checks on.(PATH)│ +│--all-files-aRun checks on all files.│ +│--show-diff-on-failure-sShow diff for files modified by the checks.│ +│--last-commit-cRun checks for all files in last commit. Mutually exclusive with --commit-ref.│ +╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ +╭─ Options ────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ +│--commit-ref-rRun checks for this commit reference only (can be any git commit-ish reference). Mutually  │ +│exclusive with --last-commit.                                                              │ +│(TEXT)                                                                                     │ +│--verbose-vPrint verbose information about performed steps.│ +│--dry-run-DIf dry-run is set, commands are only printed, not executed.│ +│--github-repository-gGitHub repository used to pull, push run images.(TEXT)[default: apache/airflow]│ +│--help-hShow this message and exit.│ +╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ diff --git a/scripts/ci/pre_commit/pre_commit_check_2_2_compatibility.py b/scripts/ci/pre_commit/pre_commit_check_2_2_compatibility.py index d9562029078f6..5c3c6dd89be47 100755 --- a/scripts/ci/pre_commit/pre_commit_check_2_2_compatibility.py +++ b/scripts/ci/pre_commit/pre_commit_check_2_2_compatibility.py @@ -36,6 +36,7 @@ TRY_NUM_MATCHER = re.compile(r".*context.*\[[\"']try_number[\"']].*") GET_MANDATORY_MATCHER = re.compile(r".*conf\.get_mandatory_value") GET_AIRFLOW_APP_MATCHER = re.compile(r".*get_airflow_app\(\)") +HOOK_PARAMS_MATCHER = re.compile(r".*get_hook\(hook_params") def _check_file(_file: Path): @@ -81,6 +82,15 @@ def _check_file(_file: Path): "as it is only available in Airflow 2.3+[/]" ) + if HOOK_PARAMS_MATCHER.match(line): + errors.append( + f"[red]In {_file}:{index} there is a forbidden construct " + "(Airflow 2.3+ only):[/]\n\n" + f"{lines[index]}\n\n" + "[yellow]You should not use 'hook_params' in get_hook as it has been added in providers " + "as it is not available in Airflow 2.3+. Use get_hook() instead.[/]" + ) + if GET_AIRFLOW_APP_MATCHER.match(line): errors.append( f"[red]In {_file}:{index} there is a forbidden construct "