kaxil commented on code in PR #71212:
URL: https://github.com/apache/airflow/pull/71212#discussion_r3737580622
##########
scripts/ci/prek/check_sdk_imports_in_core.py:
##########
@@ -42,23 +89,122 @@ def check_file_for_sdk_imports(file_path: Path) ->
list[tuple[int, str]]:
)
-def main():
- parser = argparse.ArgumentParser(description="Check for SDK imports in
airflow-core files")
- parser.add_argument("files", nargs="*", help="Files to check")
- args = parser.parse_args()
+class SdkImportsAllowlistManager(AllowlistManager):
+ def __init__(self, allowlist_file: Path) -> None:
+ super().__init__(allowlist_file, repo_root=REPO_ROOT)
- if not args.files:
- return
+ def is_excluded(self, path: Path) -> bool:
+ return not EXCLUDED_DIR_NAMES.isdisjoint(path.parts)
- report_import_violations(
- args.files,
- check_func=check_file_for_sdk_imports,
- violation_label="SDK import(s) in core files",
- nocheck_code=NOCHECK_CODE,
- only_python_files=True,
+ def iter_files(self) -> Iterable[Path]:
+ return (path for path in CORE_SRC_ROOT.rglob("*.py") if not
self.is_excluded(path))
+
+ def check(self, files: list[Path], allowlist: dict[str, int]) -> int:
+ return super().check([path for path in files if not
self.is_excluded(path)], allowlist)
+
+ def count_occurrences(self, path: Path) -> int:
+ return len(check_file_for_sdk_imports(path))
Review Comment:
`find_import_violations` returns `[]` when the file fails to parse (it
swallows `SyntaxError`, `OSError` and `UnicodeDecodeError`), so an unparseable
file counts as 0. That was harmless before, but now 0 < allowed means `check()`
reads it as a reduction and rewrites `generated/known_sdk_imports_in_core.txt`.
I put a syntax error into `models/taskinstance.py` and ran the hook: it printed
`taskinstance.py 1 -> 0`, deleted the entry from the allowlist on disk, and
told me to stage the updated file. Could `count_occurrences` skip files that
don't parse instead of counting them as zero?
##########
generated/known_sdk_imports_in_core.txt:
##########
@@ -0,0 +1,44 @@
+airflow-core/src/airflow/__init__.py::1
+airflow-core/src/airflow/api_fastapi/execution_api/versions/v2026_04_06.py::1
+airflow-core/src/airflow/cli/commands/task_command.py::7
+airflow-core/src/airflow/cli/commands/triggerer_command.py::1
+airflow-core/src/airflow/configuration.py::1
+airflow-core/src/airflow/dag_processing/dagbag.py::1
+airflow-core/src/airflow/dag_processing/importers/base.py::1
+airflow-core/src/airflow/dag_processing/importers/python_importer.py::7
+airflow-core/src/airflow/dag_processing/manager.py::4
+airflow-core/src/airflow/dag_processing/processor.py::15
+airflow-core/src/airflow/exceptions.py::1
+airflow-core/src/airflow/executors/base_executor.py::3
+airflow-core/src/airflow/jobs/triggerer_job_runner.py::18
+airflow-core/src/airflow/models/__init__.py::5
+airflow-core/src/airflow/models/asset.py::2
+airflow-core/src/airflow/models/connection.py::5
+airflow-core/src/airflow/models/dag.py::1
+airflow-core/src/airflow/models/dagrun.py::2
+airflow-core/src/airflow/models/taskinstance.py::1
Review Comment:
Since the entry is only a count, a file sitting at its ceiling can still
pick up a brand new `airflow.sdk` dependency as long as it drops another one. I
swapped this file's `airflow.sdk.definitions._internal.abstractoperator` import
for `airflow.sdk.execution_time.secrets_masker` and the hook passed with the
count still at 1. Given that #70370 landing a new sdk import in this exact file
is the motivating case, is recording the imported module per entry worth it
over a bare count? I realise this mirrors
`check_new_airflow_exception_usage.py`, so happy if the answer is "later".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
