-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Make K8sPodOperator backwards compatible #12384
Make K8sPodOperator backwards compatible #12384
Conversation
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
Just discussed with Daniel: We're going to add (back) a from airflow.providers.cncf.kubernetes.backcompat.volume_mount import VolumeMount
warnings.warn(
"This module is deprecated. Please use `kubernetes.client.models.V1Volume`.",
DeprecationWarning,
stacklevel=2,
) We may need to wrap that import in a `warnings.catch_warnings, otherwise the warning will be reported from the wrong context (we ideally want the file:lineno of the warning to be the import line in the user's DAG): import warnings
with warnings.catch_warnings():
from airflow.providers.cncf.kubernetes.backcompat.volume_mount import VolumeMount
warnings.warn(
"This module is deprecated. Please use `kubernetes.client.models.V1Volume`.",
DeprecationWarning,
stacklevel=2,
) This means the old imports continue to work, but the bulk of the compat code lives in the provider. |
24673f6
to
9236703
Compare
The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*. |
8da4e53
to
4ef1815
Compare
Love it! |
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.
Great job @dimberman and @ashb . I think we are nicely handling all cases :). I think we might merge it just in time to release new backports :)
The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it! |
self.qry.select_module("airflow.providers.cncf.kubernetes.backcompat").filter( | ||
callback=is_not_k8spodop | ||
).rename("airflow.kubernetes") |
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.
Waaaaait -- just noticed that 1.10.x still has an airflow/kubernetes/pod.py
etc. What is this going to do?
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.
Yeah. They should be removed after rename. However Interesting thing is that the package installed correctly (prepare backport providers job in CI does it).
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.
Yeah, pip doesn't really care if two distributions install the same file -- it'll just happily overwrite the existing file.
Which is fine right until you come to uninstall things.
This brings up an interesting point. What is the behaviour when upgrading from 1.10.x with backport provider to 2.0.0 with normal provider. Yes pip will complain about version conflicts, but that still isn't an error (especially given many people will be on an older pip still)
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.
From this respect good @ashb ! the backport provider package only contains airflow.providers files. We do not add airflow/kubernetes.
Just double-checked and installed both airflow 1.10.12 and backport cncf,kubernetes from pcakges and seem to be all ok.
This is what you get in 'backwards_compat_converters.py in the backport package:
from typing import List
from kubernetes.client import models as k8s
from airflow.exceptions import AirflowException
from airflow.kubernetes.pod import Port, Resources
from airflow.kubernetes.pod_runtime_info_env import PodRuntimeInfoEnv
from airflow.kubernetes.volume import Volume
from airflow.kubernetes.volume_mount import VolumeMount
def _convert_kube_model_object(obj, old_class, new_class):
convert_op = getattr(obj, "to_k8s_client_obj", None)
if callable(convert_op):
return obj.to_k8s_client_obj()
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.
What is this rename()
achieving then? What dist does/should it appear in?
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.
Force-pushed back to the previous commit.
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.
But we should not I believe. This import should be changed in the backport provider as well.
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.
So the proper fix will be to remove the classes from backcompat and add rename for that one as well.
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.
the kubernetes_pod.py was excluded from the previous rename, but pod_runtime_info can be changed in all classes.
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.
@dimberman ^^ have you seen this discussion ?
d5d9e8b
to
4ef1815
Compare
airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py
Outdated
Show resolved
Hide resolved
@param volume_mount: | ||
@return: k8s.V1VolumeMount |
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.
same here
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.
Is there a setting on black we can create for this or some form of a static check that automatically fixes this?
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.
No, black doesn't look at doc comments.
pydocstyle might.
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
This were missed in apache/airflow#12384 GitOrigin-RevId: bc4bb30588607b10b069ab63ddf2ba7b7ee673ed
`from airflow.utils.state import State` is used in multiple functions. I suggested it in apache/airflow#12384 but was missed GitOrigin-RevId: ea865e4955e94326bbe9a12d2da653bd0f2c0993
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.