-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Restore execute_complete functionality TimeSensor when deferrable=True
#53669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Well, I'm not a maintainer, but LGTM. Maybe consider adding a test in def test_execute_complete_accepts_event_argument(self):
"""Ensure execute_complete supports the 'event' kwarg when deferrable=True."""
with DAG(
dag_id="test_execute_complete_event",
schedule=None,
start_date=datetime(2020, 1, 1),
):
op = TimeSensor(task_id="test", target_time=time(10, 0), deferrable=True)
try:
op.execute_complete(context={}, event={"status": "success"})
except TypeError as e:
pytest.fail(f"execute_complete raised TypeError unexpectedly: {e}")What do u think ? @jroachgolf84 |
|
I agree, thanks for the feedback. I've added that test now! |
|
Glad :) Let's see what the maintainers think @potiuk |
|
@sunank200, do you mind taking a look at this? |
|
@amoghrajesh, @rawwar, could you take a look at this? |
providers/standard/src/airflow/providers/standard/sensors/time.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
|
@RNHTTR, can you take a look at this PR? |
1e79418 removed the
eventparameter from theexecute_completemethod in theTimeSensorSensor. This resulted in the exception shown below when theTimeSensorreturned.To test/validate the changes made, the code provided in the issue was tested and executed successfully. The unit tests for this provider were also successfully executed.
closed: #53591