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 2020/01/24 02:22:51 UTC

[GitHub] [airflow] xinbinhuang opened a new pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

xinbinhuang opened a new pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249
 
 
   ---
   Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
   
   Make sure to mark the boxes below before creating PR: [x]
   
   - [x] Description above provides context of the change
   - [x] Commit message/PR title starts with `[AIRFLOW-NNNN]`. AIRFLOW-NNNN = JIRA ID<sup>*</sup>
   - [x] Unit tests coverage for changes (not needed for documentation changes)
   - [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
   - [x] Relevant documentation is updated including usage instructions.
   - [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
   
   <sup>*</sup> For document-only changes commit message can start with `[AIRFLOW-XXXX]`.
   
   ---
   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/master/UPDATING.md).
   Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r370552599
 
 

 ##########
 File path: airflow/models/baseoperator.py
 ##########
 @@ -299,8 +299,8 @@ def __init__(
         task_id: str,
         owner: str = conf.get('operators', 'DEFAULT_OWNER'),
         email: Optional[Union[str, Iterable[str]]] = None,
-        email_on_retry: bool = True,
-        email_on_failure: bool = True,
+        email_on_retry: bool = conf.getboolean('email', 'default_email_on_retry'),
 
 Review comment:
   Don't get me wrong, I definitely don't have a serious concern over this. Just want to think it through.
   
   So currently we can set this flag on task level, in default_args so that it applies to all tasks, dag level and with this change in the airflow config. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-577972968
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=h1) Report
   > Merging [#7249](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/21a46518d513f4b66ae1be748328b02903034f38?src=pr&el=desc) will **decrease** coverage by `0.23%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7249/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #7249      +/-   ##
   ==========================================
   - Coverage   85.34%   85.11%   -0.24%     
   ==========================================
     Files         791      791              
     Lines       40124    40137      +13     
   ==========================================
   - Hits        34243    34161      -82     
   - Misses       5881     5976      +95
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/models/baseoperator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvYmFzZW9wZXJhdG9yLnB5) | `96.28% <ø> (ø)` | :arrow_up: |
   | [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
   | [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
   | [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
   | [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
   | [...viders/cncf/kubernetes/operators/kubernetes\_pod.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvY25jZi9rdWJlcm5ldGVzL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZC5weQ==) | `76.47% <0%> (-21.18%)` | :arrow_down: |
   | [airflow/plugins\_manager.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wbHVnaW5zX21hbmFnZXIucHk=) | `87.57% <0%> (-3.11%)` | :arrow_down: |
   | [airflow/jobs/scheduler\_job.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9qb2JzL3NjaGVkdWxlcl9qb2IucHk=) | `89.34% <0%> (ø)` | :arrow_up: |
   | [airflow/providers/redis/hooks/redis.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvaG9va3MvcmVkaXMucHk=) | `100% <0%> (ø)` | :arrow_up: |
   | [airflow/bin/cli.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9iaW4vY2xpLnB5) | `94.73% <0%> (ø)` | :arrow_up: |
   | ... and [5 more](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=footer). Last update [21a4651...60394ec](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r373542146
 
 

 ##########
 File path: tests/models/test_baseoperator.py
 ##########
 @@ -248,6 +249,13 @@ def test_custom_resources(self):
         self.assertEqual(task.resources.cpus.qty, 1)
         self.assertEqual(task.resources.ram.qty, 1024)
 
+    @conf_vars({('email', 'default_email_on_retry'): 'True',
+                ('email', 'default_email_on_failure'): 'False'})
+    def test_default_email_on_actions(self):
+        test_task = DummyOperator(task_id='test_default_email_on_actions')
+        assert test_task.email_on_retry is True
+        assert test_task.email_on_failure is False
 
 Review comment:
   Oh I see! @mik-laj Thanks for the help. I will fix it and update the PR

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r373386436
 
 

 ##########
 File path: tests/models/test_baseoperator.py
 ##########
 @@ -248,6 +249,13 @@ def test_custom_resources(self):
         self.assertEqual(task.resources.cpus.qty, 1)
         self.assertEqual(task.resources.ram.qty, 1024)
 
+    @conf_vars({('email', 'default_email_on_retry'): 'True',
+                ('email', 'default_email_on_failure'): 'False'})
+    def test_default_email_on_actions(self):
+        test_task = DummyOperator(task_id='test_default_email_on_actions')
+        assert test_task.email_on_retry is True
+        assert test_task.email_on_failure is False
 
 Review comment:
   https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments 
   Default arguments are cached, so you have to reload the module
   Example:
   ```
       @conf_vars({("core", "executor"): "CeleryExecutor"})
       def setUpClass(cls):
           importlib.reload(cli)
           cls.parser = cli.CLIFactory.get_parser()
   ```

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-577972968
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=h1) Report
   > Merging [#7249](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/21a46518d513f4b66ae1be748328b02903034f38?src=pr&el=desc) will **decrease** coverage by `0.18%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7249/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #7249      +/-   ##
   ==========================================
   - Coverage   85.34%   85.15%   -0.19%     
   ==========================================
     Files         791      822      +31     
     Lines       40124    40261     +137     
   ==========================================
   + Hits        34243    34285      +42     
   - Misses       5881     5976      +95
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/models/baseoperator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvYmFzZW9wZXJhdG9yLnB5) | `96.28% <ø> (ø)` | :arrow_up: |
   | [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
   | [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
   | [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
   | [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
   | [...viders/cncf/kubernetes/operators/kubernetes\_pod.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvY25jZi9rdWJlcm5ldGVzL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZC5weQ==) | `76.47% <0%> (-21.18%)` | :arrow_down: |
   | [airflow/plugins\_manager.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wbHVnaW5zX21hbmFnZXIucHk=) | `87.57% <0%> (-3.11%)` | :arrow_down: |
   | [airflow/jobs/scheduler\_job.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9qb2JzL3NjaGVkdWxlcl9qb2IucHk=) | `89.34% <0%> (ø)` | :arrow_up: |
   | [airflow/contrib/hooks/datadog\_hook.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2hvb2tzL2RhdGFkb2dfaG9vay5weQ==) | `100% <0%> (ø)` | :arrow_up: |
   | [airflow/contrib/hooks/cloudant\_hook.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2hvb2tzL2Nsb3VkYW50X2hvb2sucHk=) | `100% <0%> (ø)` | :arrow_up: |
   | ... and [68 more](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=footer). Last update [21a4651...871c41d](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-581114611
 
 
   Thanks @xinbinhuang 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r370520397
 
 

 ##########
 File path: airflow/models/baseoperator.py
 ##########
 @@ -299,8 +299,8 @@ def __init__(
         task_id: str,
         owner: str = conf.get('operators', 'DEFAULT_OWNER'),
         email: Optional[Union[str, Iterable[str]]] = None,
-        email_on_retry: bool = True,
-        email_on_failure: bool = True,
+        email_on_retry: bool = conf.getboolean('email', 'default_email_on_retry'),
 
 Review comment:
   Do we want this globally? I think DAG level is fine enough, what is your use-case?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] xinbinhuang edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
xinbinhuang edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-578629524
 
 
   > Looks almost perfect but it would ideal to have test moved from pre-commit to "tests" folder.
   > 
   > The main reason is it would be ideal to have a test that checks that when a given config is passed, the values returned is boolean.
   > 
   > An example would be:
   > 
   > ```python
   > from tests.test_utils.config import conf_vars
   > 
   > # Some Base class ....
   >     @conf_vars({('email', 'default_email_on_retry'): 'True',
   >                 ('email', 'default_email_on_failure'): 'True'})
   >     def test_default_email_on_actions(self):
   >         # ... Some code to instantiate BaseOperator
   >         self.assertEqual(test_task.default_email_on_retry, True) # or assert test_task.default_email_on_retry == True
   >         self.assertEqual(test_task.default_email_on_failure, True)
   > ```
   > 
   > Something of that sorts
   
   Awesome! I was not aware that it is possible to do this type of testing. (I did start out thinking about testing it in the `tests` folder but could not find a way and came up with this hacky way; should've asked before head next time :) ). 
   
   I will update the PR tomorrow as now it's quite late for PST. Thanks for the advice!

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r370529894
 
 

 ##########
 File path: airflow/models/baseoperator.py
 ##########
 @@ -299,8 +299,8 @@ def __init__(
         task_id: str,
         owner: str = conf.get('operators', 'DEFAULT_OWNER'),
         email: Optional[Union[str, Iterable[str]]] = None,
-        email_on_retry: bool = True,
-        email_on_failure: bool = True,
+        email_on_retry: bool = conf.getboolean('email', 'default_email_on_retry'),
 
 Review comment:
   The main reason for proposing this change is that I believe not every team uses emails to alert on job failure (our team only use slack alert). So every time we create a dag, we need to remember to set these 2 options to `False`, which is error-prone sometimes. Once we does forget to set to False and somehow magically pass the code review, the error log would be messed with email related error when a task fail. I guess I can also have some some safe guard in CI/CD, but it will be nice if we can override the default setting globally. 
   
   I am not sure what problem would it cause if we set it globally. Can you explain a bit about your concern?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-577972968
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=h1) Report
   > Merging [#7249](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/21a46518d513f4b66ae1be748328b02903034f38?src=pr&el=desc) will **increase** coverage by `0.36%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7249/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##           master   #7249      +/-   ##
   =========================================
   + Coverage   85.34%   85.7%   +0.36%     
   =========================================
     Files         791     863      +72     
     Lines       40124   40520     +396     
   =========================================
   + Hits        34243   34729     +486     
   + Misses       5881    5791      -90
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/models/baseoperator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvYmFzZW9wZXJhdG9yLnB5) | `96.51% <ø> (+0.23%)` | :arrow_up: |
   | [airflow/operators/mysql\_to\_hive.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvbXlzcWxfdG9faGl2ZS5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
   | [airflow/providers/postgres/operators/postgres.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcG9zdGdyZXMvb3BlcmF0b3JzL3Bvc3RncmVzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
   | [airflow/operators/s3\_to\_hive\_operator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvczNfdG9faGl2ZV9vcGVyYXRvci5weQ==) | `0% <0%> (-93.97%)` | :arrow_down: |
   | [airflow/contrib/hooks/grpc\_hook.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2hvb2tzL2dycGNfaG9vay5weQ==) | `0% <0%> (-91.94%)` | :arrow_down: |
   | [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
   | [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
   | [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
   | [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
   | [...viders/cncf/kubernetes/operators/kubernetes\_pod.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvY25jZi9rdWJlcm5ldGVzL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZC5weQ==) | `76.47% <0%> (-21.18%)` | :arrow_down: |
   | ... and [466 more](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=footer). Last update [21a4651...e29bead](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-577972968
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=h1) Report
   > Merging [#7249](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/21a46518d513f4b66ae1be748328b02903034f38?src=pr&el=desc) will **decrease** coverage by `0.18%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7249/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #7249      +/-   ##
   ==========================================
   - Coverage   85.34%   85.15%   -0.19%     
   ==========================================
     Files         791      822      +31     
     Lines       40124    40261     +137     
   ==========================================
   + Hits        34243    34285      +42     
   - Misses       5881     5976      +95
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/models/baseoperator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvYmFzZW9wZXJhdG9yLnB5) | `96.28% <ø> (ø)` | :arrow_up: |
   | [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
   | [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
   | [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
   | [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
   | [...viders/cncf/kubernetes/operators/kubernetes\_pod.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvY25jZi9rdWJlcm5ldGVzL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZC5weQ==) | `76.47% <0%> (-21.18%)` | :arrow_down: |
   | [airflow/plugins\_manager.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wbHVnaW5zX21hbmFnZXIucHk=) | `87.57% <0%> (-3.11%)` | :arrow_down: |
   | [airflow/jobs/scheduler\_job.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9qb2JzL3NjaGVkdWxlcl9qb2IucHk=) | `89.34% <0%> (ø)` | :arrow_up: |
   | [airflow/contrib/hooks/datadog\_hook.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2hvb2tzL2RhdGFkb2dfaG9vay5weQ==) | `100% <0%> (ø)` | :arrow_up: |
   | [airflow/contrib/hooks/cloudant\_hook.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2hvb2tzL2Nsb3VkYW50X2hvb2sucHk=) | `100% <0%> (ø)` | :arrow_up: |
   | ... and [68 more](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=footer). Last update [21a4651...871c41d](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-577972968
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=h1) Report
   > Merging [#7249](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/21a46518d513f4b66ae1be748328b02903034f38?src=pr&el=desc) will **increase** coverage by `0.53%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7249/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #7249      +/-   ##
   ==========================================
   + Coverage   85.34%   85.88%   +0.53%     
   ==========================================
     Files         791      862      +71     
     Lines       40124    40484     +360     
   ==========================================
   + Hits        34243    34768     +525     
   + Misses       5881     5716     -165
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/models/baseoperator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvYmFzZW9wZXJhdG9yLnB5) | `96.28% <ø> (ø)` | :arrow_up: |
   | [airflow/operators/mysql\_to\_hive.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvbXlzcWxfdG9faGl2ZS5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
   | [airflow/operators/s3\_to\_hive\_operator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvczNfdG9faGl2ZV9vcGVyYXRvci5weQ==) | `0% <0%> (-93.97%)` | :arrow_down: |
   | [airflow/contrib/hooks/grpc\_hook.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2hvb2tzL2dycGNfaG9vay5weQ==) | `0% <0%> (-91.94%)` | :arrow_down: |
   | [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
   | [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
   | [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
   | [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
   | [...viders/cncf/kubernetes/operators/kubernetes\_pod.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvY25jZi9rdWJlcm5ldGVzL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZC5weQ==) | `76.47% <0%> (-21.18%)` | :arrow_down: |
   | [airflow/plugins\_manager.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wbHVnaW5zX21hbmFnZXIucHk=) | `87.57% <0%> (-3.11%)` | :arrow_down: |
   | ... and [459 more](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=footer). Last update [21a4651...5375db7](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r371029259
 
 

 ##########
 File path: airflow/models/baseoperator.py
 ##########
 @@ -299,8 +299,8 @@ def __init__(
         task_id: str,
         owner: str = conf.get('operators', 'DEFAULT_OWNER'),
         email: Optional[Union[str, Iterable[str]]] = None,
-        email_on_retry: bool = True,
-        email_on_failure: bool = True,
+        email_on_retry: bool = conf.getboolean('email', 'default_email_on_retry'),
 
 Review comment:
   Hi @kaxil, I updated the code based on your feedback.
   1. Add `fallback=True` to both email_on_retry and email_on_failure
   2. Add a check in the pre-commit config which will fail when it's not `conf.getboolean` (this is my first time to write something about pre-commit using pygrep. Let me if it needs to be modified.
   
   Also, 
   - I added a similar check to the `retreis` argument i.e. fail when it's not `conf.getint`
   - Resort the pre-commit check order: now all checks with id: `base-operator` goes together.
   
   PTAL. 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io commented on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-577972968
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=h1) Report
   > Merging [#7249](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/21a46518d513f4b66ae1be748328b02903034f38?src=pr&el=desc) will **decrease** coverage by `0.27%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7249/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #7249      +/-   ##
   ==========================================
   - Coverage   85.34%   85.07%   -0.28%     
   ==========================================
     Files         791      791              
     Lines       40124    40135      +11     
   ==========================================
   - Hits        34243    34143     -100     
   - Misses       5881     5992     +111
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/models/baseoperator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvYmFzZW9wZXJhdG9yLnB5) | `96.28% <ø> (ø)` | :arrow_up: |
   | [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
   | [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
   | [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
   | [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
   | [...viders/cncf/kubernetes/operators/kubernetes\_pod.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvY25jZi9rdWJlcm5ldGVzL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZC5weQ==) | `76.47% <0%> (-21.18%)` | :arrow_down: |
   | [airflow/jobs/scheduler\_job.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9qb2JzL3NjaGVkdWxlcl9qb2IucHk=) | `89.34% <0%> (ø)` | :arrow_up: |
   | [airflow/bin/cli.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9iaW4vY2xpLnB5) | `94.73% <0%> (ø)` | :arrow_up: |
   | [airflow/ti\_deps/deps/runnable\_exec\_date\_dep.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy90aV9kZXBzL2RlcHMvcnVubmFibGVfZXhlY19kYXRlX2RlcC5weQ==) | `100% <0%> (ø)` | :arrow_up: |
   | [airflow/gcp/operators/bigquery.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9nY3Avb3BlcmF0b3JzL2JpZ3F1ZXJ5LnB5) | `91.62% <0%> (+0.02%)` | :arrow_up: |
   | ... and [2 more](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=footer). Last update [21a4651...44ad8f4](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-577972968
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=h1) Report
   > Merging [#7249](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/21a46518d513f4b66ae1be748328b02903034f38?src=pr&el=desc) will **decrease** coverage by `0.23%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7249/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #7249      +/-   ##
   ==========================================
   - Coverage   85.34%   85.11%   -0.24%     
   ==========================================
     Files         791      791              
     Lines       40124    40137      +13     
   ==========================================
   - Hits        34243    34161      -82     
   - Misses       5881     5976      +95
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/models/baseoperator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvYmFzZW9wZXJhdG9yLnB5) | `96.28% <ø> (ø)` | :arrow_up: |
   | [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
   | [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
   | [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
   | [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
   | [...viders/cncf/kubernetes/operators/kubernetes\_pod.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvY25jZi9rdWJlcm5ldGVzL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZC5weQ==) | `76.47% <0%> (-21.18%)` | :arrow_down: |
   | [airflow/plugins\_manager.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wbHVnaW5zX21hbmFnZXIucHk=) | `87.57% <0%> (-3.11%)` | :arrow_down: |
   | [airflow/jobs/scheduler\_job.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9qb2JzL3NjaGVkdWxlcl9qb2IucHk=) | `89.34% <0%> (ø)` | :arrow_up: |
   | [airflow/providers/redis/hooks/redis.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcmVkaXMvaG9va3MvcmVkaXMucHk=) | `100% <0%> (ø)` | :arrow_up: |
   | [airflow/bin/cli.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9iaW4vY2xpLnB5) | `94.73% <0%> (ø)` | :arrow_up: |
   | ... and [5 more](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=footer). Last update [21a4651...60394ec](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] xinbinhuang commented on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-578629524
 
 
   > Looks almost perfect but it would ideal to have test moved from pre-commit to "tests" folder.
   > 
   > The main reason is it would be ideal to have a test that checks that when a given config is passed, the values returned is boolean.
   > 
   > An example would be:
   > 
   > ```python
   > from tests.test_utils.config import conf_vars
   > 
   > # Some Base class ....
   >     @conf_vars({('email', 'default_email_on_retry'): 'True',
   >                 ('email', 'default_email_on_failure'): 'True'})
   >     def test_default_email_on_actions(self):
   >         # ... Some code to instantiate BaseOperator
   >         self.assertEqual(test_task.default_email_on_retry, True) # or assert test_task.default_email_on_retry == True
   >         self.assertEqual(test_task.default_email_on_failure, True)
   > ```
   > 
   > Something of that sorts
   
   Awesome! I was not aware that it is possible to do this type of testing. (I did start out thinking about testing it in the `tests` folder but could not find a way; should've asked before head next time :) ). 
   
   I will update the PR tomorrow as now it's quite late for PST. Thanks for the advice!

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r370529894
 
 

 ##########
 File path: airflow/models/baseoperator.py
 ##########
 @@ -299,8 +299,8 @@ def __init__(
         task_id: str,
         owner: str = conf.get('operators', 'DEFAULT_OWNER'),
         email: Optional[Union[str, Iterable[str]]] = None,
-        email_on_retry: bool = True,
-        email_on_failure: bool = True,
+        email_on_retry: bool = conf.getboolean('email', 'default_email_on_retry'),
 
 Review comment:
   The main reason for proposing this change is that I believe not every team uses emails to alert on job failure (our team only use slack alert). So every time we create a dag, we need to remember to set these 2 options to `False`, which is error-prone sometimes. Once we does forget to set to False and pass the code review, the error log would be messed with email related error when a task fail. I guess I can also have some some safe guard in CI/CD, but it will be nice if we can override the default setting globally. 
   
   I am not sure what problem would it cause if we set it globally. Can you explain a bit about your concern?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] xinbinhuang commented on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-581151194
 
 
   @kaxil @mik-laj Thanks for reviewing and helping !

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r370530976
 
 

 ##########
 File path: airflow/models/baseoperator.py
 ##########
 @@ -299,8 +299,8 @@ def __init__(
         task_id: str,
         owner: str = conf.get('operators', 'DEFAULT_OWNER'),
         email: Optional[Union[str, Iterable[str]]] = None,
-        email_on_retry: bool = True,
-        email_on_failure: bool = True,
+        email_on_retry: bool = conf.getboolean('email', 'default_email_on_retry'),
 
 Review comment:
   BTW, are you concerned about both settings or just the `email_on_retry` ?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-577972968
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=h1) Report
   > Merging [#7249](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/21a46518d513f4b66ae1be748328b02903034f38?src=pr&el=desc) will **increase** coverage by `0.48%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7249/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #7249      +/-   ##
   ==========================================
   + Coverage   85.34%   85.83%   +0.48%     
   ==========================================
     Files         791      863      +72     
     Lines       40124    40520     +396     
   ==========================================
   + Hits        34243    34779     +536     
   + Misses       5881     5741     -140
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/models/baseoperator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvYmFzZW9wZXJhdG9yLnB5) | `96.51% <ø> (+0.23%)` | :arrow_up: |
   | [airflow/operators/mysql\_to\_hive.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvbXlzcWxfdG9faGl2ZS5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
   | [airflow/operators/s3\_to\_hive\_operator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvczNfdG9faGl2ZV9vcGVyYXRvci5weQ==) | `0% <0%> (-93.97%)` | :arrow_down: |
   | [airflow/contrib/hooks/grpc\_hook.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2hvb2tzL2dycGNfaG9vay5weQ==) | `0% <0%> (-91.94%)` | :arrow_down: |
   | [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
   | [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
   | [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
   | [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
   | [...viders/cncf/kubernetes/operators/kubernetes\_pod.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvY25jZi9rdWJlcm5ldGVzL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZC5weQ==) | `76.47% <0%> (-21.18%)` | :arrow_down: |
   | [airflow/providers/papermill/operators/papermill.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcGFwZXJtaWxsL29wZXJhdG9ycy9wYXBlcm1pbGwucHk=) | `96% <0%> (-4%)` | :arrow_down: |
   | ... and [463 more](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=footer). Last update [21a4651...e29bead](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-577972968
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=h1) Report
   > Merging [#7249](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/21a46518d513f4b66ae1be748328b02903034f38?src=pr&el=desc) will **increase** coverage by `0.36%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7249/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##           master   #7249      +/-   ##
   =========================================
   + Coverage   85.34%   85.7%   +0.36%     
   =========================================
     Files         791     863      +72     
     Lines       40124   40520     +396     
   =========================================
   + Hits        34243   34729     +486     
   + Misses       5881    5791      -90
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/models/baseoperator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvYmFzZW9wZXJhdG9yLnB5) | `96.51% <ø> (+0.23%)` | :arrow_up: |
   | [airflow/operators/mysql\_to\_hive.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvbXlzcWxfdG9faGl2ZS5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
   | [airflow/providers/postgres/operators/postgres.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvcG9zdGdyZXMvb3BlcmF0b3JzL3Bvc3RncmVzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
   | [airflow/operators/s3\_to\_hive\_operator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvczNfdG9faGl2ZV9vcGVyYXRvci5weQ==) | `0% <0%> (-93.97%)` | :arrow_down: |
   | [airflow/contrib/hooks/grpc\_hook.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2hvb2tzL2dycGNfaG9vay5weQ==) | `0% <0%> (-91.94%)` | :arrow_down: |
   | [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
   | [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
   | [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
   | [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
   | [...viders/cncf/kubernetes/operators/kubernetes\_pod.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvY25jZi9rdWJlcm5ldGVzL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZC5weQ==) | `76.47% <0%> (-21.18%)` | :arrow_down: |
   | ... and [466 more](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=footer). Last update [21a4651...e29bead](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r371029259
 
 

 ##########
 File path: airflow/models/baseoperator.py
 ##########
 @@ -299,8 +299,8 @@ def __init__(
         task_id: str,
         owner: str = conf.get('operators', 'DEFAULT_OWNER'),
         email: Optional[Union[str, Iterable[str]]] = None,
-        email_on_retry: bool = True,
-        email_on_failure: bool = True,
+        email_on_retry: bool = conf.getboolean('email', 'default_email_on_retry'),
 
 Review comment:
   Hi @kaxil, I updated the code based on your feedback.
   1. Add `fallback=True` to both email_on_retry and email_on_failure
   2. Add a check in the pre-commit config which will fail when it's not `conf.getboolean` (this is my first time to write something about pre-commit using pygrep. Let me know if it needs to be modified.)
   
   Also, 
   - I added a similar check to the `retreis` argument i.e. fail when it's not `conf.getint`
   - Resort the pre-commit check order: now all checks with id: `base-operator` goes together.
   
   PTAL. 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r373386436
 
 

 ##########
 File path: tests/models/test_baseoperator.py
 ##########
 @@ -248,6 +249,13 @@ def test_custom_resources(self):
         self.assertEqual(task.resources.cpus.qty, 1)
         self.assertEqual(task.resources.ram.qty, 1024)
 
+    @conf_vars({('email', 'default_email_on_retry'): 'True',
+                ('email', 'default_email_on_failure'): 'False'})
+    def test_default_email_on_actions(self):
+        test_task = DummyOperator(task_id='test_default_email_on_actions')
+        assert test_task.email_on_retry is True
+        assert test_task.email_on_failure is False
 
 Review comment:
   https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments 
   Default arguments are cached, so you have to reload the module
   Example:
   ```python
       @conf_vars({("core", "executor"): "CeleryExecutor"})
       def setUpClass(cls):
           importlib.reload(cli)
           cls.parser = cli.CLIFactory.get_parser()
   ```

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-577972968
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=h1) Report
   > Merging [#7249](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/21a46518d513f4b66ae1be748328b02903034f38?src=pr&el=desc) will **increase** coverage by `0.3%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7249/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##           master    #7249     +/-   ##
   =========================================
   + Coverage   85.34%   85.65%   +0.3%     
   =========================================
     Files         791      862     +71     
     Lines       40124    40484    +360     
   =========================================
   + Hits        34243    34675    +432     
   + Misses       5881     5809     -72
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/models/baseoperator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvYmFzZW9wZXJhdG9yLnB5) | `96.28% <ø> (ø)` | :arrow_up: |
   | [airflow/operators/mysql\_to\_hive.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvbXlzcWxfdG9faGl2ZS5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
   | [airflow/operators/s3\_to\_hive\_operator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvczNfdG9faGl2ZV9vcGVyYXRvci5weQ==) | `0% <0%> (-93.97%)` | :arrow_down: |
   | [airflow/contrib/hooks/grpc\_hook.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2hvb2tzL2dycGNfaG9vay5weQ==) | `0% <0%> (-91.94%)` | :arrow_down: |
   | [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
   | [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
   | [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
   | [airflow/providers/mysql/operators/mysql.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvbXlzcWwvb3BlcmF0b3JzL215c3FsLnB5) | `55% <0%> (-45%)` | :arrow_down: |
   | [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
   | [...viders/cncf/kubernetes/operators/kubernetes\_pod.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvY25jZi9rdWJlcm5ldGVzL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZC5weQ==) | `76.47% <0%> (-21.18%)` | :arrow_down: |
   | ... and [462 more](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=footer). Last update [21a4651...5375db7](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] xinbinhuang commented on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-581098112
 
 
   @mik-laj @kaxil  Hi, I refactored the tests and stop trying to use `@conf_vars` to test the change on default_config. 
   
   The reason is that when I try to do `importlib.reload(baseoperator)` to refresh the config, it will also mess up with the test on `TestBaseOperatorMethods::test_cross_downstream` and `TestBaseOperatorMethods::test_chain`. After the reload, it leads to the weird behavior below and I am not sure what's going on.
   
   ```
   # This can be reproduced by adding below after the imports in `test_baseoperator.py` 
   ### import importlib
   ### from airflow.models import baseoperator
   ### importlib.reload(baseoperator)
   
   # It will fails on the below checks in BaseOperator.chain() and BaseOperator._set_relative
   isinstance(task, BaseOperator)  # here task = DummyOperator()
   False
   ```
   
   I would love to fix this if there are more pointers, but I believe right now the test has already served the purpose of testing the default values are boolean.
   
   PTAL and let me know how you think.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r373317405
 
 

 ##########
 File path: tests/models/test_baseoperator.py
 ##########
 @@ -248,6 +249,13 @@ def test_custom_resources(self):
         self.assertEqual(task.resources.cpus.qty, 1)
         self.assertEqual(task.resources.ram.qty, 1024)
 
+    @conf_vars({('email', 'default_email_on_retry'): 'True',
+                ('email', 'default_email_on_failure'): 'False'})
+    def test_default_email_on_actions(self):
+        test_task = DummyOperator(task_id='test_default_email_on_actions')
+        assert test_task.email_on_retry is True
+        assert test_task.email_on_failure is False
 
 Review comment:
   Hi @kaxil, needs some help. When I set `('email', 'default_email_on_failure'): 'False'`, it seems that `test_task.email_on_failure` is still showing as True. 
   
   While when I tried to directly get the config `conf.getboolean('email', 'default_email_on_failure')`, it is actually set as `False`. I cannot tell what's going on.... Do you have any clues?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#issuecomment-577972968
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=h1) Report
   > Merging [#7249](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/21a46518d513f4b66ae1be748328b02903034f38?src=pr&el=desc) will **increase** coverage by `0.53%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7249/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #7249      +/-   ##
   ==========================================
   + Coverage   85.34%   85.88%   +0.53%     
   ==========================================
     Files         791      862      +71     
     Lines       40124    40484     +360     
   ==========================================
   + Hits        34243    34768     +525     
   + Misses       5881     5716     -165
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/models/baseoperator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvYmFzZW9wZXJhdG9yLnB5) | `96.28% <ø> (ø)` | :arrow_up: |
   | [airflow/operators/mysql\_to\_hive.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvbXlzcWxfdG9faGl2ZS5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
   | [airflow/operators/s3\_to\_hive\_operator.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9vcGVyYXRvcnMvczNfdG9faGl2ZV9vcGVyYXRvci5weQ==) | `0% <0%> (-93.97%)` | :arrow_down: |
   | [airflow/contrib/hooks/grpc\_hook.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2hvb2tzL2dycGNfaG9vay5weQ==) | `0% <0%> (-91.94%)` | :arrow_down: |
   | [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
   | [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
   | [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `45.25% <0%> (-46.72%)` | :arrow_down: |
   | [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
   | [...viders/cncf/kubernetes/operators/kubernetes\_pod.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvY25jZi9rdWJlcm5ldGVzL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZC5weQ==) | `76.47% <0%> (-21.18%)` | :arrow_down: |
   | [airflow/plugins\_manager.py](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree#diff-YWlyZmxvdy9wbHVnaW5zX21hbmFnZXIucHk=) | `87.57% <0%> (-3.11%)` | :arrow_down: |
   | ... and [459 more](https://codecov.io/gh/apache/airflow/pull/7249/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=footer). Last update [21a4651...5375db7](https://codecov.io/gh/apache/airflow/pull/7249?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r370553819
 
 

 ##########
 File path: airflow/models/baseoperator.py
 ##########
 @@ -299,8 +299,8 @@ def __init__(
         task_id: str,
         owner: str = conf.get('operators', 'DEFAULT_OWNER'),
         email: Optional[Union[str, Iterable[str]]] = None,
-        email_on_retry: bool = True,
-        email_on_failure: bool = True,
+        email_on_retry: bool = conf.getboolean('email', 'default_email_on_retry'),
 
 Review comment:
   Can we add a test so that if someone modifies this code and changes it to `conf.get` instead of `conf.getboolean`, that test should fail.
   
   Also let's add a fallback value too so `conf.getboolean('email', 'default_email_on_retry', fallback=True)`

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249#discussion_r370552599
 
 

 ##########
 File path: airflow/models/baseoperator.py
 ##########
 @@ -299,8 +299,8 @@ def __init__(
         task_id: str,
         owner: str = conf.get('operators', 'DEFAULT_OWNER'),
         email: Optional[Union[str, Iterable[str]]] = None,
-        email_on_retry: bool = True,
-        email_on_failure: bool = True,
+        email_on_retry: bool = conf.getboolean('email', 'default_email_on_retry'),
 
 Review comment:
   Don't get me wrong, I definitely don't have a serious concern over this. Just want to think it through.
   
   So currently we can set this flag on task level, in default_args so that it applies to all tasks, dag level and with this change config. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil merged pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config

Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #7249: [AIRFLOW-6626] Add email on failure or retry to default config
URL: https://github.com/apache/airflow/pull/7249
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services