-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Kubernetes decorator #21077
Kubernetes decorator #21077
Conversation
…cutors" This reverts commit a1c532e.
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.
Also need to add image
to the argument list of _KubernetesDecoratedOperator
Co-authored-by: Tzu-ping Chung <[email protected]>
@@ -0,0 +1,44 @@ | |||
{# |
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.
This has to be in the kube provider folder, else it won't get released with the provider, and it won't work.
@ashb , currently the decorator uses the write_python_script function thats in the utils/python_virtualenv.py file and thats why the template(python_kubernetes_script.jinja2) was also placed in the same folder. If we want to remove the dependency on the utils folder, I guess we can make a copy of the write_python_script in the cncf.providers.kubernetes folder. Question is if there a benefit of having a central place for the function and all templates, Docker decorator and python operator also call this function.
|
Bump? :) |
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.
Please also rebase your PR?
@@ -0,0 +1,44 @@ | |||
{# |
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.
Can you move this @subkanthi to the cncf kube providers please?
pod_runtime_info_envs: Optional[List[PodRuntimeInfoEnv]] = None, | ||
termination_grace_period: Optional[int] = None, | ||
configmaps: Optional[List[str]] = None, | ||
**kwargs, |
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 can go into kwargs
here? It seems unnecessary.
# Set defaults for name and namespace. | ||
if 'name' not in kwargs: | ||
kwargs['name'] = f'k8s_airflow_pod_{uuid.uuid4().hex}' | ||
|
||
if 'namespace' not in kwargs: | ||
kwargs['namespace'] = 'default' |
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.
Nit, it’s easier to use setdefault
for these
return res | ||
|
||
|
||
T = TypeVar("T", bound=Callable) |
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.
Unused?
Closing, as this has been already implemented and merged in #25663 |
Decorator to execute functions in k8s using KubernetesPodOperator
closes: #19135
If no parameters passed, defaults are set in kubernetes.py
This version sets the function as an environment variable - PYTHON_SCRIPT and in the command , the logic is to retrieve the environment variable and write to '/tmp/script.py' to execute the function(very similar to Docker)
^ 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.