You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/09/29 21:24:19 UTC
[GitHub] [airflow] alex-astronomer opened a new pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
alex-astronomer opened a new pull request #18615:
URL: https://github.com/apache/airflow/pull/18615
closes: #18592
Moves the `convert_env_vars` portion of the `__init__(...)` into the `execute(...)` fn. This allows for `env_vars` to be templated before we try to perform any checks or operations on them. Also added (`pytest`) tests for this new function which confirms old behavior still works and tests the new behavior when `render_template_as_native_obj` is set to `True` at the DAG level.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] raphaelauv commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
raphaelauv commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-1030712969
@alex-astronomer are you still working on it ? thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RickRen7575 commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
RickRen7575 commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-936168221
> My main concern here is that `env_vars` intentionally takes `List[V1EnvVar]` - taking a `dict` is currently for backward compatibility only at this point. It's not that we couldn't support this, but there was a conscious move to using the models from the k8s client.
Passing in a `List[V1EnvVar]` also doesn't work for the same reason. If we just set it up so that `List[V1EnvVar]` could work with this, that would be great as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-974729819
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RickRen7575 commented on a change in pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
RickRen7575 commented on a change in pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#discussion_r719638211
##########
File path: airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py
##########
@@ -325,6 +324,16 @@ def create_labels_for_pod(context) -> dict:
def create_pod_launcher(self) -> Type[pod_launcher.PodLauncher]:
return pod_launcher.PodLauncher(kube_client=self.client, extract_xcom=self.do_xcom_push)
+ def _build_env_vars(self):
+ """
+ Convert env vars into expected object (should happen after templating) and add the
+ pod_runtime_info_envs to the env also.
+
+ """
+ self.env_vars = convert_env_vars(self.env_vars) if self.env_vars else []
+ if self.pod_runtime_info_envs:
+ self.env_vars.extend([convert_pod_runtime_info_env(p) for p in self.pod_runtime_info_envs])
+
Review comment:
Does this take a long time? The only thing I am thinking of is: would this slow down execution significantly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] alex-astronomer commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
alex-astronomer commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-1039493135
Yes I'm still working on this, I'll remove the conflict and clean everything up, check through it again, and then re-request by pinging Jed. This should be done by Wednesday. Sorry for the delay on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RickRen7575 commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
RickRen7575 commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-975548372
Any work needed to be done here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-997235009
I think you should rebase this (there is a conflict) and likely ping @jedcunningham to make his call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RickRen7575 commented on a change in pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
RickRen7575 commented on a change in pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#discussion_r719666235
##########
File path: airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py
##########
@@ -325,6 +324,16 @@ def create_labels_for_pod(context) -> dict:
def create_pod_launcher(self) -> Type[pod_launcher.PodLauncher]:
return pod_launcher.PodLauncher(kube_client=self.client, extract_xcom=self.do_xcom_push)
+ def _build_env_vars(self):
+ """
+ Convert env vars into expected object (should happen after templating) and add the
+ pod_runtime_info_envs to the env also.
+
+ """
+ self.env_vars = convert_env_vars(self.env_vars) if self.env_vars else []
+ if self.pod_runtime_info_envs:
+ self.env_vars.extend([convert_pod_runtime_info_env(p) for p in self.pod_runtime_info_envs])
+
Review comment:
Yeah that's what I figured. Otherwise, I don't see any issues with your changes!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] alex-astronomer commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
alex-astronomer commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-936668630
Passing a `List[k8s.V1EnvVar]` through XCOM will not work for the same reason, I can confirm @RickRen7575's point there. Here's some more color around that:
Even when passing an XCOM with the `render_template_as_native_obj` flag set to `True`, with the original code before these changes we would still be operating on that `List` before it gets rendered as the native object. Not a heavy operation, of course, just checking that the type of the value passed is actually a `List`, but if we were to do this check from the initializer, it would still be a string at that point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] wyattshapiro commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
wyattshapiro commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-994979582
It looks like a review is required from an Airflow contributor with write access to the repo!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] wyattshapiro commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
wyattshapiro commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-974901967
Commenting to remove stale label
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-974729819
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RickRen7575 edited a comment on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
RickRen7575 edited a comment on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-936168221
> My main concern here is that `env_vars` intentionally takes `List[V1EnvVar]` - taking a `dict` is currently for backward compatibility only at this point. It's not that we couldn't support this, but there was a conscious move to using the models from the k8s client.
@jedcunningham Passing in a `List[V1EnvVar]` also doesn't work for the same reason. If we just set it up so that `List[V1EnvVar]` could work with this, that would be great as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] RickRen7575 commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
RickRen7575 commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-1062050291
@alex-astronomer just following up on this! Lemme know if you need any assistance
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-934825317
My main concern here is that `env_vars` intentionally takes `List[V1EnvVar]` - taking a `dict` is currently for backward compatibility only at this point. It's not that we couldn't support this, but there was a conscious move to using the models from the k8s client.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] alex-astronomer commented on a change in pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
alex-astronomer commented on a change in pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#discussion_r719658930
##########
File path: airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py
##########
@@ -325,6 +324,16 @@ def create_labels_for_pod(context) -> dict:
def create_pod_launcher(self) -> Type[pod_launcher.PodLauncher]:
return pod_launcher.PodLauncher(kube_client=self.client, extract_xcom=self.do_xcom_push)
+ def _build_env_vars(self):
+ """
+ Convert env vars into expected object (should happen after templating) and add the
+ pod_runtime_info_envs to the env also.
+
+ """
+ self.env_vars = convert_env_vars(self.env_vars) if self.env_vars else []
+ if self.pod_runtime_info_envs:
+ self.env_vars.extend([convert_pod_runtime_info_env(p) for p in self.pod_runtime_info_envs])
+
Review comment:
Shouldn't take too long! Check out the source for `convert_env_vars`:
```
def convert_env_vars(env_vars) -> List[k8s.V1EnvVar]:
"""
Converts a dictionary into a list of env_vars
:param env_vars:
:return:
"""
if isinstance(env_vars, dict):
res = []
for k, v in env_vars.items():
res.append(k8s.V1EnvVar(name=k, value=v))
return res
elif isinstance(env_vars, list):
return env_vars
else:
raise AirflowException(f"Expected dict or list, got {type(env_vars)}")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] alex-astronomer commented on pull request #18615: fix template string passed to env_vars when using render_template_as_native_obj for Kubernetes Pod Operator and add Tests
Posted by GitBox <gi...@apache.org>.
alex-astronomer commented on pull request #18615:
URL: https://github.com/apache/airflow/pull/18615#issuecomment-1039493135
Yes I'm still working on this, I'll remove the conflict and clean everything up, check through it again, and then re-request by pinging Jed. This should be done by Wednesday. Sorry for the delay on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org