potiuk commented on code in PR #73323:
URL: https://github.com/apache/airflow/pull/73323#discussion_r4056997757
##########
providers/amazon/src/airflow/providers/amazon/aws/operators/emr.py:
##########
@@ -1815,9 +1819,17 @@ def __init__(
self.wait_for_delete_completion = False if deferrable else
wait_for_completion
def execute(self, context: Context) -> None:
- # super stops the app (or makes sure it's already stopped)
+ # super stops the app (or makes sure it's already stopped). In
deferrable mode it defers
+ # instead of returning, and the task resumes in
``delete_stopped_application``.
super().execute(context)
+ self._delete_application()
+
+ def delete_stopped_application(self, context: Context, event: dict[str,
Any] | None = None) -> None:
+ # Validates the stop trigger event and raises if the application
failed to stop.
Review Comment:
This comment restates what the parent does. The bit worth recording is why
it is `super().execute_complete(...)` and not `self.execute_complete(...)`:
this class overrides `execute_complete` to handle the delete trigger's event.
Rewording to that (fixup incoming).
##########
providers/amazon/tests/unit/amazon/aws/operators/test_emr_serverless.py:
##########
@@ -1390,15 +1394,69 @@ def test_delete_application_waiter_params(
@mock.patch.object(EmrServerlessHook, "conn")
def test_delete_application_deferrable(self, mock_conn):
- mock_conn.delete_application.return_value = {"ResponseMetadata":
{"HTTPStatusCode": 200}}
+ operator = EmrServerlessDeleteApplicationOperator(
+ task_id=task_id,
+ application_id=application_id,
+ deferrable=True,
+ )
+ with pytest.raises(TaskDeferred) as defer:
+ operator.execute(None)
+
+ assert isinstance(defer.value.trigger,
EmrServerlessStopApplicationTrigger)
+ assert defer.value.method_name == "delete_stopped_application"
+
mock_conn.stop_application.assert_called_once_with(applicationId=application_id)
+ mock_conn.delete_application.assert_not_called()
+ @mock.patch.object(EmrServerlessHook, "cancel_running_jobs")
Review Comment:
`@mock.patch.object(EmrServerlessHook, "cancel_running_jobs")` could take
`autospec=True` per the "use `spec`/`autospec` when mocking" guideline. The
rest of the file patches `conn` without spec too, so purely optional (fixup
incoming).
--
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]
