Update cncf's import conf path to use common compat SDK#64143
Update cncf's import conf path to use common compat SDK#64143wjddn279 wants to merge 3 commits intoapache:mainfrom
Conversation
b27844e to
9346cd5
Compare
|
@wjddn279 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. |
f899ff3 to
832f278
Compare
|
Hi Jason. As intended by this PR, I removed the fallback from the existing logic. Currently, the compat tests for the previous version are failing — is that expected? I believe it's because the older version doesn't have the functionality to read from Am I misunderstanding something? |
da1e18a to
62f7102
Compare
...iders/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/executors/kubernetes_executor.py
Outdated
Show resolved
Hide resolved
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/kube_config.py
Outdated
Show resolved
Hide resolved
9eeab38 to
7cee6d1
Compare
jscheffl
left a comment
There was a problem hiding this comment.
OK for me, in other opinion please let me know, else propose to merge the next days.
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/template_rendering.py
Show resolved
Hide resolved
|
I think the review has resolved and there is no issue! Can you review once more? |
There was a problem hiding this comment.
Yes, I checked the three key where the fallback was removed (multi_namespace_mode_namespace_list, pod_template_file, namespace) and the defaults are explicitly specified in providers.yaml and the values also exist in provider_config_fallback_defaults.cfg.
I just double checked the options you mentioned as well, yes, they're specified in the provider.yaml.
Thanks for helping out, LGTM.
EDIT: we can't remove the exiting fallback as metioned in the latest review comment.
jason810496
left a comment
There was a problem hiding this comment.
Wait, I realized that we still need those fallback to keep it compatible or we might cause breaking change for users who using older Airflow in the last minute.
Let me explain why: users could still use latest providers in "older" Airflow Core (e.g. Airflow 3.1 or 2.11), for the conf in older Airflow version, they don't respect the provider.yaml.
That is the reason why we have the "common.compat" provider - to abstract the Airflow backend version away. So as the change is made and depednency in modelled in provider.yaml this should be fine. That is alo the reason why we have back-compat tests in CI |
7cee6d1 to
045e075
Compare
jason810496
left a comment
There was a problem hiding this comment.
Wait, I realized that we still need those fallback to keep it compatible or we might cause breaking change for users who using older Airflow in the last minute.
Let me explain why: users could still use latest providers in "older" Airflow Core (e.g. Airflow3.1or2.11), for theconfin older Airflow version, they don't respect theprovider.yaml.That is the reason why we have the "common.compat" provider - to abstract the Airflow backend version away. So as the change is made and depednency in modelled in provider.yaml this should be fine.
That is alo the reason why we have back-compat tests in CI
However, the conf will only respect the provider.yaml in the up coming 3.2.0 ( Make TaskSDK conf respect default config from provider metadata #62696 )
On top of this, if we replace the existing conf.get(..., fallback="default-value") with conf.get(...), which might cause breaking changes for users who using older Airflow backend version upgrade to next provider version.
|
I've done reverting the fallback! Maybe looks good and CI is green! |
| self.dags_folder = self._conf.get(self.core_section, "dags_folder") | ||
| self.parallelism = self._conf.getint(self.core_section, "parallelism") | ||
| self.pod_template_file = self._conf.get(self.kubernetes_section, "pod_template_file", fallback=None) | ||
| self.pod_template_file = self._conf.get(self.kubernetes_section, "pod_template_file", fallback="") |
There was a problem hiding this comment.
Should we fallback to None instead of "" here?
There was a problem hiding this comment.
After testing, I found that when the fallback is triggered and the value is None, it causes an error in the tests. It's this part.
This PR (#64209) likely eliminates the case where the fallback is triggered in test, but you can think of this change as a fix for the case where None is passed in and causes an error.
There was a problem hiding this comment.
#64209 was merged yesterday, could you please rebase when you have a moment? Thanks.
045e075 to
cfb6028
Compare

related: #60000
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.