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 2022/04/06 01:05:17 UTC

[GitHub] [airflow] jedcunningham opened a new pull request, #22770: Fix `email_on_failure` with `render_template_as_native_obj`

jedcunningham opened a new pull request, #22770:
URL: https://github.com/apache/airflow/pull/22770

   Co-authored-by: andyhuang <an...@mirrormedia.mg>
   
   Closes: #22152
   Alternative of #22218, refactored to only change behavior when generating emails and including test coverage.
   
   cc @andyfcx


-- 
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] dstandish commented on pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #22770:
URL: https://github.com/apache/airflow/pull/22770#issuecomment-1161955109

   And here's an example of how we patched another PR in a dockerfile
   
   ```Dockerfile
   RUN apt update && apt install -y patch patchutils
   
   RUN set -ex; \
       curl -o /tmp/kpo.patch https://patch-diff.githubusercontent.com/raw/apache/airflow/pull/24117.patch; \
       cd /usr/local/lib/python3.9/site-packages/airflow; \
       filterdiff -p1 -i 'airflow*' /tmp/kpo.patch | patch -u -p 2; \
       rm /tmp/kpo.patch
   ```


-- 
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] uranusjr commented on a diff in pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22770:
URL: https://github.com/apache/airflow/pull/22770#discussion_r844585644


##########
airflow/models/abstractoperator.py:
##########
@@ -106,7 +106,7 @@ def dag_id(self) -> str:
     def node_id(self) -> str:
         return self.task_id
 
-    def get_template_env(self) -> "jinja2.Environment":
+    def get_template_env(self, *, force_sandboxed: bool = False) -> "jinja2.Environment":

Review Comment:
   +1 to removing it since it’d make more obvious why we call the DAG’s `get_template_env` instead (so we can pass this option). It’d be nice to have a comment there describing why we don’t call the task’s `get_template_env` there.



-- 
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] ashb commented on a diff in pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22770:
URL: https://github.com/apache/airflow/pull/22770#discussion_r844430580


##########
airflow/models/abstractoperator.py:
##########
@@ -106,7 +106,7 @@ def dag_id(self) -> str:
     def node_id(self) -> str:
         return self.task_id
 
-    def get_template_env(self) -> "jinja2.Environment":
+    def get_template_env(self, *, force_sandboxed: bool = False) -> "jinja2.Environment":

Review Comment:
   Doesn't do any harm since its an optional arg, but for the sake of a smaller diff, yes we could remove it.



-- 
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] vandanthaker commented on pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
vandanthaker commented on PR #22770:
URL: https://github.com/apache/airflow/pull/22770#issuecomment-1161221525

   Agree :)


-- 
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] vandanthaker commented on pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
vandanthaker commented on PR #22770:
URL: https://github.com/apache/airflow/pull/22770#issuecomment-1160229277

   Any alternative to fix this on v2.2.5 without upgrading to 2.3.X?
   Thanks in advance


-- 
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 a diff in pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #22770:
URL: https://github.com/apache/airflow/pull/22770#discussion_r844409500


##########
airflow/models/taskinstance.py:
##########
@@ -2196,7 +2196,7 @@ def get_email_subject_content(self, exception: BaseException) -> Tuple[str, str,
             html_content_err = jinja_env.from_string(default_html_content_err).render(**default_context)
 
         else:
-            jinja_env = self.task.get_template_env()
+            jinja_env = self.task.get_template_env(force_sandboxed=True)

Review Comment:
   Oh, good idea, it uses `dag` anyways! Let me try that.



-- 
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] dstandish commented on a diff in pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #22770:
URL: https://github.com/apache/airflow/pull/22770#discussion_r844294612


##########
airflow/models/taskinstance.py:
##########
@@ -2196,7 +2196,7 @@ def get_email_subject_content(self, exception: BaseException) -> Tuple[str, str,
             html_content_err = jinja_env.from_string(default_html_content_err).render(**default_context)
 
         else:
-            jinja_env = self.task.get_template_env()
+            jinja_env = self.task.get_template_env(force_sandboxed=True)

Review Comment:
   i think you may need to update this operator to handle kwargs
   https://github.com/apache/airflow/blob/be0a4e413117c5ceb0a245402cdfabe1a3d82ace/airflow/providers/microsoft/psrp/operators/psrp.py#L158
   
   also, this illustrates a backcompat issue if a user implemented this method (since it's a signature change).
   
   if we wanted to be real careful we could inspect the signature before calling....
   
   wdyt?



##########
tests/models/test_dag.py:
##########
@@ -473,6 +474,18 @@ def test_template_undefined(self):
         jinja_env = dag.get_template_env()
         assert jinja_env.undefined is jinja2.Undefined
 
+    @parameterized.expand(
+        [
+            (False, False, SandboxedEnvironment),

Review Comment:
   ```suggestion
               (False, True, SandboxedEnvironment),
               (False, False, SandboxedEnvironment),
   ```



-- 
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 a diff in pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #22770:
URL: https://github.com/apache/airflow/pull/22770#discussion_r844427729


##########
airflow/models/abstractoperator.py:
##########
@@ -106,7 +106,7 @@ def dag_id(self) -> str:
     def node_id(self) -> str:
         return self.task_id
 
-    def get_template_env(self) -> "jinja2.Environment":
+    def get_template_env(self, *, force_sandboxed: bool = False) -> "jinja2.Environment":

Review Comment:
   Think we should remove support for `force_sandboxed` here so we arent changing the signature of the method psrp is overriding?



-- 
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] uranusjr commented on pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #22770:
URL: https://github.com/apache/airflow/pull/22770#issuecomment-1161272622

   Note that you can always manually add the patch—this one is simple enough I believe a simple `patch` command should work.


-- 
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 a diff in pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #22770:
URL: https://github.com/apache/airflow/pull/22770#discussion_r844406827


##########
airflow/models/taskinstance.py:
##########
@@ -2196,7 +2196,7 @@ def get_email_subject_content(self, exception: BaseException) -> Tuple[str, str,
             html_content_err = jinja_env.from_string(default_html_content_err).render(**default_context)
 
         else:
-            jinja_env = self.task.get_template_env()
+            jinja_env = self.task.get_template_env(force_sandboxed=True)

Review Comment:
   I think I lean toward this being "internal" to the task, but I'm not sure how others view it.
   
   We could inspect, but emails would still fail even then. I wonder if we should instead release a new psrp provider that has a minimum core of 2.3.0. I guess the most 'flexible', ignoring the email issue, is to inspect in both so new providers still work on old core and vice versa.



-- 
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] uranusjr commented on a diff in pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22770:
URL: https://github.com/apache/airflow/pull/22770#discussion_r843492352


##########
airflow/models/dag.py:
##########
@@ -1260,7 +1260,7 @@ def resolve_template_files(self):
         for t in self.tasks:
             t.resolve_template_files()
 
-    def get_template_env(self) -> jinja2.Environment:
+    def get_template_env(self, force_sandboxed: bool = False) -> jinja2.Environment:

Review Comment:
   ```suggestion
       def get_template_env(self, *, force_sandboxed: bool = False) -> jinja2.Environment:
   ```



##########
airflow/models/abstractoperator.py:
##########
@@ -106,7 +106,7 @@ def dag_id(self) -> str:
     def node_id(self) -> str:
         return self.task_id
 
-    def get_template_env(self) -> "jinja2.Environment":
+    def get_template_env(self, force_sandboxed: bool = False) -> "jinja2.Environment":

Review Comment:
   ```suggestion
       def get_template_env(self, *, force_sandboxed: bool = False) -> "jinja2.Environment":
   ```
   
   Let’s make this keyword-only for explicitness.



-- 
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 #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22770:
URL: https://github.com/apache/airflow/pull/22770#issuecomment-1089851165

   The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.


-- 
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] ashb commented on a diff in pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22770:
URL: https://github.com/apache/airflow/pull/22770#discussion_r844408416


##########
airflow/models/taskinstance.py:
##########
@@ -2196,7 +2196,7 @@ def get_email_subject_content(self, exception: BaseException) -> Tuple[str, str,
             html_content_err = jinja_env.from_string(default_html_content_err).render(**default_context)
 
         else:
-            jinja_env = self.task.get_template_env()
+            jinja_env = self.task.get_template_env(force_sandboxed=True)

Review Comment:
   We could use `dag.get_template_env(force_sandbox)` instead of `self` in side the email rendering?



-- 
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 #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22770:
URL: https://github.com/apache/airflow/pull/22770#issuecomment-1160398708

   > @jedcunningham Any alternative to fix this on v2.2.5 without upgrading to 2.3.X? Thanks in advance
   
   As usual in open source - you can cherry-pick and apply it yourself. But we recommend to upgrade to 2.3 instead. Upgrading to latest released version is generally good thing to do.


-- 
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 #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22770:
URL: https://github.com/apache/airflow/pull/22770#issuecomment-1160400233

   This is really the trade-off - either you take the burden of migration to what community maintain, or you take the maintenance burden yourself :). Choose one that is best for you.


-- 
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 closed pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
jedcunningham closed pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`
URL: https://github.com/apache/airflow/pull/22770


-- 
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 a diff in pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #22770:
URL: https://github.com/apache/airflow/pull/22770#discussion_r844419838


##########
airflow/models/taskinstance.py:
##########
@@ -2196,7 +2196,7 @@ def get_email_subject_content(self, exception: BaseException) -> Tuple[str, str,
             html_content_err = jinja_env.from_string(default_html_content_err).render(**default_context)
 
         else:
-            jinja_env = self.task.get_template_env()
+            jinja_env = self.task.get_template_env(force_sandboxed=True)

Review Comment:
   Yeah, that works 🍺



-- 
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] ashb commented on a diff in pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #22770:
URL: https://github.com/apache/airflow/pull/22770#discussion_r844405858


##########
airflow/models/taskinstance.py:
##########
@@ -2196,7 +2196,7 @@ def get_email_subject_content(self, exception: BaseException) -> Tuple[str, str,
             html_content_err = jinja_env.from_string(default_html_content_err).render(**default_context)
 
         else:
-            jinja_env = self.task.get_template_env()
+            jinja_env = self.task.get_template_env(force_sandboxed=True)

Review Comment:
   Oh ugh that's going to make this approach hard/basically impossible. Nice catch @dstandish 



-- 
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] ashb merged pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
ashb merged PR #22770:
URL: https://github.com/apache/airflow/pull/22770


-- 
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] uranusjr commented on a diff in pull request #22770: Fix `email_on_failure` with `render_template_as_native_obj`

Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22770:
URL: https://github.com/apache/airflow/pull/22770#discussion_r844753126


##########
airflow/models/abstractoperator.py:
##########
@@ -106,7 +106,7 @@ def dag_id(self) -> str:
     def node_id(self) -> str:
         return self.task_id
 
-    def get_template_env(self) -> "jinja2.Environment":
+    def get_template_env(self, *, force_sandboxed: bool = False) -> "jinja2.Environment":

Review Comment:
   I pushed a commit for this (plus a small tweak for Mypy).



-- 
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