Cleanup: Enforce keyword-only session in db_cleanup#63781
Cleanup: Enforce keyword-only session in db_cleanup#63781Gautam-Bharadwaj wants to merge 3 commits intoapache:mainfrom
Conversation
|
@Gautam-Bharadwaj This PR has been converted to draft because it does not yet meet our Pull Request quality criteria. Issues found:
What to do next:
Converting a PR to draft is not a rejection — it is an invitation to bring the PR up to the project's standards so that maintainer review time is spent productively. There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the Airflow Slack. |
a4ae2eb to
0d93ce6
Compare
0d93ce6 to
d70044b
Compare
SameerMesiah97
left a comment
There was a problem hiding this comment.
Technically, this is a breaking change but these 2 methods are internal APIs that are invoked in a handful of places. I have left a few comments. Please fix CI as well.
| else: | ||
| confirm_mock.assert_not_called() | ||
|
|
||
| def test_export_archived_records_positional_fails(self): |
There was a problem hiding this comment.
I don't have a strong opinion against including these tests but aren't they just testing python's built-in behavior when * is added to the method signature?
| *, | ||
| table_names: list[str] | None = None, | ||
| drop_archives: bool = False, | ||
| needs_confirm: bool = True, |
There was a problem hiding this comment.
Why did you only make a subset of the parameters keyword-only for this method when you made all parameters for drop_archived_tables keyword-only?
There was a problem hiding this comment.
Pull request overview
This PR updates airflow.utils.db_cleanup utility functions to enforce keyword-only argument passing (notably for session) and adjusts unit tests and release notes accordingly.
Changes:
- Made
export_archived_recordsrequire keyword-only arguments afteroutput_path(includingsession). - Made
drop_archived_tablesfully keyword-only (includingtable_names,needs_confirm, andsession). - Updated unit tests and added a newsfragment documenting the behavior change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| airflow-core/src/airflow/utils/db_cleanup.py | Updates function signatures to enforce keyword-only arguments. |
| airflow-core/tests/unit/utils/test_db_cleanup.py | Updates callers to use keywords and adds tests asserting positional calls fail. |
| airflow-core/newsfragments/63781.improvement.rst | Adds a release note about enforcing keyword-only parameters in db_cleanup. |
Comments suppressed due to low confidence (1)
airflow-core/src/airflow/utils/db_cleanup.py:629
- The new
*placement makes all parameters afteroutput_pathkeyword-only (not justsession). If the intent (per PR title/description) is only to enforce keyword-onlysession, consider moving*to just beforesessionso existing positional uses oftable_names/drop_archives/needs_confirmkeep working (and adjust the new “positional_fails” test accordingly).
def export_archived_records(
export_format: str,
output_path: str,
*,
table_names: list[str] | None = None,
drop_archives: bool = False,
needs_confirm: bool = True,
session: Session = NEW_SESSION,
) -> None:
| *, | ||
| table_names: list[str] | None, | ||
| needs_confirm: bool, |
There was a problem hiding this comment.
drop_archived_tables is now fully keyword-only (*, table_names, needs_confirm, session), which is a larger breaking change than “keyword-only session”. If you only need session to be keyword-only (per Airflow core guideline), you can keep table_names/needs_confirm positional-or-keyword and introduce *, session instead.
| *, | |
| table_names: list[str] | None, | |
| needs_confirm: bool, | |
| table_names: list[str] | None, | |
| needs_confirm: bool, | |
| *, |
| with pytest.raises(TypeError) as exc: | ||
| export_archived_records("csv", "path", ["table"], True, True, None) | ||
| assert "takes 2 positional arguments but 6" in str(exc.value) | ||
|
|
||
| def test_drop_archived_tables_positional_fails(self): | ||
| with pytest.raises(TypeError) as exc: | ||
| drop_archived_tables(["table"], True) | ||
| assert "takes 0 positional arguments but 2" in str(exc.value) |
There was a problem hiding this comment.
These tests assert on a specific CPython TypeError wording/argument count substring (e.g. "takes 2 positional arguments but 6"). The exact message format can vary across Python versions/implementations, making this brittle. Consider asserting only that a TypeError is raised, or use pytest.raises(..., match=...) with a looser regex that doesn’t depend on the exact count/message text.
| with pytest.raises(TypeError) as exc: | |
| export_archived_records("csv", "path", ["table"], True, True, None) | |
| assert "takes 2 positional arguments but 6" in str(exc.value) | |
| def test_drop_archived_tables_positional_fails(self): | |
| with pytest.raises(TypeError) as exc: | |
| drop_archived_tables(["table"], True) | |
| assert "takes 0 positional arguments but 2" in str(exc.value) | |
| with pytest.raises(TypeError, match="positional arguments"): | |
| export_archived_records("csv", "path", ["table"], True, True, None) | |
| def test_drop_archived_tables_positional_fails(self): | |
| with pytest.raises(TypeError, match="positional arguments"): | |
| drop_archived_tables(["table"], True) |
Enforce keyword-only session parameters in db_cleanup utility functions and update tests to adhere to Airflow 3 standards.
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.