-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Update conf import for edge3 #60093
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
base: main
Are you sure you want to change the base?
Update conf import for edge3 #60093
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
|
what if we also update pyproject.toml, since it also has dependencies such as airflow/providers/edge3/pyproject.toml Line 62 in 17fbc9d
as issue describes ? |
|
There are some errors in tests -> i guess adding defaults in tests is needed. It happened in one of the related PRs. |
|
Hi @potiuk , Tests fail because
What is the correct pattern in providers to safely read provider config defaults in unit tests? |
It seems we are currently missing this functionality in the new conf. We have been discussing it in the cncf.kubernetes conf PR #60074 (comment) -> and I think we will get it implemented there as a generic feature that you will be able to reuse here. So I guess good idea for you is to read what we proposed there and possibly review/participate in implementation (and then rebase when we merge the implementation and apply it here). |
bbfe3be to
ecbd5de
Compare
dheerajturaga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Tests are failing |
|
@Divyaselva14, overall, the changes look good, but the test seems to be failing. |
ecbd5de to
be988f5
Compare
|
@sunank200 , I have rebased but tried adding fallbacks. I am looking into this approach [60074#issuecomment] |
This PR migrates the edge3 provider to import conf from the common compat module instead of directly from airflow.configuration, ensuring compatibility across Airflow 2.11+ and 3.0+.
Changes
Updated import statements from from airflow.configuration import conf to from airflow.providers.common.compat.sdk import conf
Related: #60000
^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.