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

Check Logging Handlers and Secret Backends for recursion #31385

Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented May 18, 2023

Logging Handlers and secret backends have the potential of triggering Import/Attribute error due to circular imports. This check will attempt to import all those classes from our providers via airflow_local_settings, which should detect the case where such circular import condition is generated.


^ 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.

@potiuk
Copy link
Member Author

potiuk commented May 18, 2023

This should prevent any future errors where any of the logging handlers or secrets backend might trigger circular imports via airflows settings:

This is how it will look like:

Screenshot 2023-05-18 at 14 45 18

Are there any other ways we can trigger the circular import besides logging and secret backends ?

@ashb
Copy link
Member

ashb commented May 18, 2023

We can't merge this until #31379 or something like it is fixed right?

@potiuk
Copy link
Member Author

potiuk commented May 18, 2023

We can't merge this until #31379 or something like it is fixed right?

Right. It should fail now.

@potiuk potiuk force-pushed the add-detection-of-potential-recursive-error branch from 82f0432 to bea0dcd Compare May 18, 2023 12:58
Logging Handlers and secret backends have the potential of triggering
Import/Attribute error due to circular imports. This check will
attempt to import all those classes from our providers via
airflow_local_settings, which should detect the case where such
circular import condition is generated.
@potiuk potiuk force-pushed the add-detection-of-potential-recursive-error branch from bea0dcd to f912da4 Compare May 18, 2023 16:38
@potiuk
Copy link
Member Author

potiuk commented May 18, 2023

Rebased after #31379 was merged.

@potiuk potiuk merged commit 33709f0 into apache:main May 18, 2023
@potiuk potiuk deleted the add-detection-of-potential-recursive-error branch May 18, 2023 17:21
@potiuk
Copy link
Member Author

potiuk commented May 18, 2023

Ok. It seems to work !

@eladkal eladkal added this to the Airflow 2.6.2 milestone Jun 9, 2023
@eladkal eladkal added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jun 9, 2023
potiuk added a commit that referenced this pull request Jun 9, 2023
Logging Handlers and secret backends have the potential of triggering
Import/Attribute error due to circular imports. This check will
attempt to import all those classes from our providers via
airflow_local_settings, which should detect the case where such
circular import condition is generated.

(cherry picked from commit 33709f0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants