ferruzzi commented on code in PR #71212:
URL: https://github.com/apache/airflow/pull/71212#discussion_r3732298805
##########
scripts/ci/prek/check_sdk_imports_in_core.py:
##########
@@ -42,23 +78,75 @@ 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 iter_files(self) -> Iterable[Path]:
+ return CORE_SRC_ROOT.rglob("*.py")
- 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 count_occurrences(self, path: Path) -> int:
+ return len(check_file_for_sdk_imports(path))
+
+ def violation_panel_text(self) -> str:
+ return (
+ "New [bold]airflow.sdk[/bold] import detected in airflow-core.\n"
+ "Core (scheduler/API server) should not gain new runtime
dependencies "
+ "on the Task SDK.\n"
+ "If this import is a genuine one-off, append `# noqa: SDK001` to
the "
+ "import line.\n"
+ "If it's an intentional, broader exception, run:\n\n"
+ " [cyan]uv run ./scripts/ci/prek/check_sdk_imports_in_core.py
--generate[/cyan]\n\n"
+ "to regenerate the allowlist, then commit the updated\n"
+ "[cyan]generated/known_sdk_imports_in_core.txt[/cyan]."
+ )
+
+
+def main(argv: list[str] | None = None) -> int:
+ parser = argparse.ArgumentParser(
+ description="Prevent new airflow.sdk imports in airflow-core.",
+ formatter_class=argparse.RawDescriptionHelpFormatter,
+ epilog=__doc__,
+ )
+ parser.add_argument("files", nargs="*", metavar="FILE", help="Files to
check (provided by prek)")
+ parser.add_argument(
+ "--all-files",
+ action="store_true",
+ help="Check every Python file under airflow-core/src/airflow",
+ )
+ parser.add_argument(
+ "--cleanup",
+ action="store_true",
+ help="Remove stale entries from the allowlist and exit",
)
+ parser.add_argument(
+ "--generate",
+ action="store_true",
+ help="Regenerate the allowlist from the current codebase and exit",
+ )
+ args = parser.parse_args(argv)
+
+ manager = SdkImportsAllowlistManager(REPO_ROOT / "generated" /
"known_sdk_imports_in_core.txt")
+
+ if args.generate:
+ return manager.generate()
Review Comment:
Non-blocking nit: `generate()` will rebuild the whole thing, maybe an
optional filename to update would be worth adding at some point? This has a
chance of accidentally approving a change you didn't intend to whereas
requiring the filename limits that... but it also adds friction, so I'm not
sure.
##########
scripts/ci/prek/check_sdk_imports_in_core.py:
##########
@@ -42,23 +78,75 @@ 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 iter_files(self) -> Iterable[Path]:
+ return CORE_SRC_ROOT.rglob("*.py")
- 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 count_occurrences(self, path: Path) -> int:
+ return len(check_file_for_sdk_imports(path))
+
+ def violation_panel_text(self) -> str:
+ return (
+ "New [bold]airflow.sdk[/bold] import detected in airflow-core.\n"
+ "Core (scheduler/API server) should not gain new runtime
dependencies "
+ "on the Task SDK.\n"
+ "If this import is a genuine one-off, append `# noqa: SDK001` to
the "
+ "import line.\n"
+ "If it's an intentional, broader exception, run:\n\n"
+ " [cyan]uv run ./scripts/ci/prek/check_sdk_imports_in_core.py
--generate[/cyan]\n\n"
+ "to regenerate the allowlist, then commit the updated\n"
+ "[cyan]generated/known_sdk_imports_in_core.txt[/cyan]."
+ )
Review Comment:
Non-blocking nit: Should this message propose the NOQA option from the
doctring above as an alternative to rebuilding the allowlist? I'm not entirely
sure on the intended difference between the two routes.
--
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]
