jscheffl commented on code in PR #68507:
URL: https://github.com/apache/airflow/pull/68507#discussion_r3408587722
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -1228,12 +1228,20 @@ def cleanup(
istio_enabled = self.is_istio_enabled(remote_pod)
pod_phase = remote_pod.status.phase if hasattr(remote_pod, "status")
else None
- # if the pod fails or success, but we don't want to delete it
- if (
- pod_phase != PodPhase.SUCCEEDED
- or self.on_finish_action == OnFinishAction.KEEP_POD
- or self.on_finish_action == OnFinishAction.DELETE_ACTIVE_POD
- ):
+ should_delete = (
+ self.on_finish_action == OnFinishAction.DELETE_POD
+ or (
+ self.on_finish_action == OnFinishAction.DELETE_SUCCEEDED_POD
+ and pod_phase == PodPhase.SUCCEEDED
+ )
+ or (
+ self.on_finish_action == OnFinishAction.DELETE_ACTIVE_POD
+ and pod_phase in {PodPhase.RUNNING, PodPhase.PENDING}
+ )
+ )
+
+ # Patch the pod unless it is a succeeded pod that will be deleted.
+ if not (pod_phase == PodPhase.SUCCEEDED and should_delete):
Review Comment:
I was reading 3 times and it is both the existing as well as the new code
hard to de-chipher. Can you attempt to make it w/o negation? For example like:
```suggestion
if pod_phase != PodPhase.SUCCEEDED or should_keep):
```
--
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]
