Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pre-commit To Prevent Usage of session.query In Core Airflow #45714

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Prab-27
Copy link
Contributor

@Prab-27 Prab-27 commented Jan 16, 2025

closes : #45461
Introduce a pre-commit hook to prevent the use of session.query in the core Airflow code.
This is limited to the source code and excludes the tests/ package.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@Prab-27
Copy link
Contributor Author

Prab-27 commented Jan 16, 2025

a conflict occurred with the images, and I'm not sure how to resolve it. Would you please help me?

@potiuk
Copy link
Member

potiuk commented Jan 16, 2025

as explained in slack - you can replace the confilcting images / .txt with either versions, rebase and re-run pre-commit. The images wil get regenerated to be "latest" automatically (in fact it should happen automatically when you rebase your PR and resolve the conflicts if you did pre-commit install before).

Copy link
Contributor

@ephraimbuddy ephraimbuddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prove that this works would be that it detects all the session.query usages in Airflow core and we have to fix it.

Comment on lines 46 to 47
if file_path.name.startswith("test_"):
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check for tests file is not necessary because you have the files to look for at the pre-commit-config.yaml entry

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

and isinstance(node.func.value, ast.Name)
and node.func.value.id == "session"
):
console.print(f"Remove session.query from line {node.lineno}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are removing the session.query because it is deprecated in SQLAlchemy 2. The message should reflect this and suggest that the user use the new-style query constructs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done !
I tried to write msg from session.query
IS it okay ?

@Prab-27 Prab-27 force-pushed the add-pre-commit-to-prevent-usage-of-session.query branch from 433f780 to f5d0a67 Compare January 17, 2025 17:00
@Prab-27
Copy link
Contributor Author

Prab-27 commented Jan 18, 2025

Could you please clarify it for me?
1 Is it okay to include providers in this pre-commit hook ?
2 Do I need to remove session.query from the codebases to pass this pre-commit check ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pre-commit to prevent usage of session.query in Airflow Core
3 participants