[GitHub] [airflow] dstandish commented on a change in pull request #19572: Simplify ``KubernetesPodOperator``

[GitBox <gi...@apache.org>]

dstandish commented on a change in pull request #19572:
URL: https://github.com/apache/airflow/pull/19572#discussion_r766915248



##########
File path: tests/providers/cncf/kubernetes/operators/test_kubernetes_pod.py
##########
@@ -762,17 +789,27 @@ def test_mark_reattached_pod_if_not_deleted(self, mock_patch_already_checked, mo
             task_id="task",
             is_delete_operator_pod=False,
         )
-        # Run it first to easily get the pod
-        pod = self.run_pod(k)
-
-        # Now try and "reattach"
-        mock_patch_already_checked.reset_mock()
-        mock_delete_pod.reset_mock()
-        self.client_mock.return_value.list_namespaced_pod.return_value.items = [pod]
-        self.monitor_mock.return_value = (State.FAILED, None, None)
+        remote_pod_mock = MagicMock()
+        remote_pod_mock.status.phase = 'Failed'
+        self.await_pod_mock.return_value = remote_pod_mock
 
-        context = self.create_context(k)
+        context = create_context(k)
         with pytest.raises(AirflowException):
             k.execute(context=context)
         mock_patch_already_checked.assert_called_once()
         mock_delete_pod.assert_not_called()
+
+
+def test_suppress_with_logging():
+    with mock.patch('logging.Logger.error') as mock_error:
+
+        class A:
+            log = logging.getLogger()
+
+            def fail(self):
+                with _suppress_with_logging(self, ValueError):
+                    raise ValueError("failure")
+
+        a = A()
+        a.fail()
+        mock_error.assert_called_once_with("failure", exc_info=True)

Review comment:
       OK so yeah I knew this would raise an eyebrow
   
   There are a variety of "cleanup" tasks that we want to do at execution end, whether the run is successful or not:
   * delete operator
   * harvest logs on failure
   * patch already checked
   
   Add to that push xcom, which as you know there's a request to move that in the finally block.
   
   Any one of these distinct cleanup tasks could fail for a variety of reasons.  But I want to attempt each of them, even if th others fail.
   
   So I thought it best to suppress the exceptions when making each call.
   
   But with the built-in `suppress` context mgr, if it _does_ fail, the exceptioned will be swallowed with no logging.  So what this one does is suppress, but also log that there was a failure.
   
   We could achieve the same with multiple try/excepts in the finally, but this is cleaner.
   
   Admittedly it's a bit odd...
   
   WDYT?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org