Deprecate use of core kube_client in PodManager #26848
Conversation
We are no longer using this in KPO (or anywhere else in the codebase for that matter) now that KPO uses K8s hook to generate the client. However, it seems we neglected to deprecate this call to get_kube_client as a fallback. Since this deprecation warning won't go out until provider version 5.0, we have to wait until next major to actually remove (and i add a test to remind us to do so).
boring-cyborg
bot
added
provider:cncf-kubernetes
| from airflow.providers_manager import ProvidersManager | ||
|
|
||
| info = ProvidersManager().providers['apache-airflow-providers-cncf-kubernetes'] | ||
| if semver.VersionInfo.parse(info.version) >= (6, 0): |
There was a problem hiding this comment.
Love it. This is cool for deprecating certain features in major version. I guess it will look a bit differently when we want to remove a code that implements a workaround of "min airflow version?
I thin we should have it as a parameterized tests (and for example pass a lambda ?) to avoid some boierplate ?
There was a problem hiding this comment.
what kind of lambda are you think of?
one thing we could do is add version_info to ProviderInfo. that would reduce a little logic here.
Beyond that i'm not seeing a lot of boilerplate to reduce.
i suppose you could add a method like ProvidersManager.get_provider_version_info(name) ... and perhaps ProvidersManager.get_provider_min_airflow_version(name) for that case.
also occurred to me that it might be nice to promote min_airflow_version to top level on ProviderInfo, though, i am not that familiar with all of the workings so unsure whether that would be problematic.
There was a problem hiding this comment.
I was thinking about extracting the "from .. import" as lambda in the form of:
lambda: __import__('airflow.providers.cncf.kubernetes.utils.pod_manager.get_kube_client')
Passed by parameterized. Then there could be one test covering multiple similar cases rather than having to copy the try/except/raise provider in multiple tests.
There was a problem hiding this comment.
I see. that makes sense for the use case where the deprecation is removing a function (or, i suppose, anything in globals()) but it might not be that simple if removing a method or a param.
i don't think you'd need a lambda... you could just parametrize with the import string, and supply it to import_string.
i tried to go with the spirit of your comment in the latest commit. test is marginally more readable. i reckon should be good enough for now!
could imagine parametrizing but... really the devil is in the details
dstandish
added
the
type:misc/internal
Note: if this goes out in 4.x have to update the test to check for 5.0
We are no longer using this in KPO (or anywhere else in the codebase for that matter) now that KPO uses K8s hook to generate the client. However, it seems we neglected to deprecate this call to get_kube_client as a fallback. Since this deprecation warning won't go out until provider version 5.0, we have to wait until next major to actually remove (and i add a test to remind us to do so).