Re: [PR] Fix SDK to read default_email_on_failure and default_email_on_retry from config [airflow]
amoghrajesh commented on code in PR #59912:
URL: https://github.com/apache/airflow/pull/59912#discussion_r2655407366
##
task-sdk/src/airflow/sdk/definitions/_internal/abstractoperator.py:
##
@@ -76,7 +76,8 @@
DEFAULT_TASK_EXECUTION_TIMEOUT: datetime.timedelta | None = conf.gettimedelta(
"core", "default_task_execution_timeout"
)
-
+DEFAULT_EMAIL_ON_FAILURE: bool = conf.getboolean("email",
"default_email_on_failure", fallback=True)
+DEFAULT_EMAIL_ON_RETRY: bool = conf.getboolean("email",
"default_email_on_retry", fallback=True)
Review Comment:
I agree with your judgement
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fix SDK to read default_email_on_failure and default_email_on_retry from config [airflow]
boring-cyborg[bot] commented on PR #59912: URL: https://github.com/apache/airflow/pull/59912#issuecomment-3702131729 Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fix SDK to read default_email_on_failure and default_email_on_retry from config [airflow]
potiuk commented on code in PR #59912:
URL: https://github.com/apache/airflow/pull/59912#discussion_r2655344965
##
task-sdk/src/airflow/sdk/definitions/_internal/abstractoperator.py:
##
@@ -76,7 +76,8 @@
DEFAULT_TASK_EXECUTION_TIMEOUT: datetime.timedelta | None = conf.gettimedelta(
"core", "default_task_execution_timeout"
)
-
+DEFAULT_EMAIL_ON_FAILURE: bool = conf.getboolean("email",
"default_email_on_failure", fallback=True)
+DEFAULT_EMAIL_ON_RETRY: bool = conf.getboolean("email",
"default_email_on_retry", fallback=True)
Review Comment:
I think not worth testing it in this case. Those are just default values
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fix SDK to read default_email_on_failure and default_email_on_retry from config [airflow]
potiuk merged PR #59912: URL: https://github.com/apache/airflow/pull/59912 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Fix SDK to read default_email_on_failure and default_email_on_retry from config [airflow]
amoghrajesh commented on code in PR #59912:
URL: https://github.com/apache/airflow/pull/59912#discussion_r2654875088
##
task-sdk/src/airflow/sdk/definitions/_internal/abstractoperator.py:
##
@@ -76,7 +76,8 @@
DEFAULT_TASK_EXECUTION_TIMEOUT: datetime.timedelta | None = conf.gettimedelta(
"core", "default_task_execution_timeout"
)
-
+DEFAULT_EMAIL_ON_FAILURE: bool = conf.getboolean("email",
"default_email_on_failure", fallback=True)
+DEFAULT_EMAIL_ON_RETRY: bool = conf.getboolean("email",
"default_email_on_retry", fallback=True)
Review Comment:
Good call, I wouldnt encourage reloading too. If we monkeypatch those vars,
it wouldn't validate the intended flow
I do not have a better proposal as of now, maybe @potiuk does?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fix SDK to read default_email_on_failure and default_email_on_retry from config [airflow]
justinpakzad commented on code in PR #59912:
URL: https://github.com/apache/airflow/pull/59912#discussion_r2653344811
##
task-sdk/src/airflow/sdk/definitions/_internal/abstractoperator.py:
##
@@ -76,7 +76,8 @@
DEFAULT_TASK_EXECUTION_TIMEOUT: datetime.timedelta | None = conf.gettimedelta(
"core", "default_task_execution_timeout"
)
-
+DEFAULT_EMAIL_ON_FAILURE: bool = conf.getboolean("email",
"default_email_on_failure", fallback=True)
+DEFAULT_EMAIL_ON_RETRY: bool = conf.getboolean("email",
"default_email_on_retry", fallback=True)
Review Comment:
So I've been trying to add some tests using `conf_vars` but I'm running into
some issues. The issue (I think) is that the email defaults are read as
module-level constants at import time, so using `conf_vars` requires reloading
the modules to pick up config changes. But when I do that, it causes other test
files that have already imported operators (which inherit from the original
BaseOperator) to fail. Do you know if there are any workarounds for this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fix SDK to read default_email_on_failure and default_email_on_retry from config [airflow]
potiuk commented on code in PR #59912:
URL: https://github.com/apache/airflow/pull/59912#discussion_r2652815477
##
task-sdk/src/airflow/sdk/definitions/_internal/abstractoperator.py:
##
@@ -76,7 +76,8 @@
DEFAULT_TASK_EXECUTION_TIMEOUT: datetime.timedelta | None = conf.gettimedelta(
"core", "default_task_execution_timeout"
)
-
+DEFAULT_EMAIL_ON_FAILURE: bool = conf.getboolean("email",
"default_email_on_failure", fallback=True)
+DEFAULT_EMAIL_ON_RETRY: bool = conf.getboolean("email",
"default_email_on_retry", fallback=True)
Review Comment:
Yep.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fix SDK to read default_email_on_failure and default_email_on_retry from config [airflow]
amoghrajesh commented on code in PR #59912:
URL: https://github.com/apache/airflow/pull/59912#discussion_r2652335798
##
task-sdk/src/airflow/sdk/definitions/_internal/abstractoperator.py:
##
@@ -76,7 +76,8 @@
DEFAULT_TASK_EXECUTION_TIMEOUT: datetime.timedelta | None = conf.gettimedelta(
"core", "default_task_execution_timeout"
)
-
+DEFAULT_EMAIL_ON_FAILURE: bool = conf.getboolean("email",
"default_email_on_failure", fallback=True)
+DEFAULT_EMAIL_ON_RETRY: bool = conf.getboolean("email",
"default_email_on_retry", fallback=True)
Review Comment:
We should have some test coverage for this. The test:
`test_default_email_on_actions` only checks for true. Maybe use `conf_vars` to
explicitly set to false and assert 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Fix SDK to read default_email_on_failure and default_email_on_retry from config [airflow]
justinpakzad commented on PR #59912: URL: https://github.com/apache/airflow/pull/59912#issuecomment-3697808705 > LGTM, but I wonder if this is really intended. It looks like it should be. Also it would be great to get some tests covering it ? So there are already two tests in the `tests/task_sdk/bases/test_operator.py` file that cover the defaults and setting email_on_retry to False. I assumed that should be sufficient but happy to add more if needed. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
