Copilot commented on code in PR #66650:
URL: https://github.com/apache/airflow/pull/66650#discussion_r3215019203
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/triggers/pod.py:
##########
@@ -508,6 +508,18 @@ def define_container_state(self, pod: V1Pod) ->
ContainerState:
return ContainerState.TERMINATED if state_obj.exit_code == 0
else ContainerState.FAILED
return ContainerState.UNDEFINED
+ def define_pod_state(self, pod: V1Pod) -> ContainerState:
+ if pod.status is None:
+ return ContainerState.UNDEFINED
+
+ if pod.status.phase == PodPhase.SUCCEEDED:
+ return ContainerState.TERMINATED
+
+ if pod.status.phase == PodPhase.FAILED:
+ return ContainerState.FAILED
+
+ return self.define_container_state(pod)
Review Comment:
`define_pod_state()` introduces new branching behavior (Succeeded/Failed
short-circuit) but the unit tests only assert the `Failed` branch. Please add
coverage for at least the `Succeeded` branch and a non-terminal phase that
falls back to `define_container_state`, to ensure the new logic behaves as
intended.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/triggers/pod.py:
##########
@@ -307,7 +307,7 @@ async def _wait_for_container_completion(self) ->
TriggerEvent:
time_get_more_logs = time_begin +
datetime.timedelta(seconds=self.logging_interval)
while True:
pod = await self._get_pod()
- container_state = self.define_container_state(pod)
+ container_state = self.define_pod_state(pod)
if container_state == ContainerState.TERMINATED:
return TriggerEvent(
{
Review Comment:
This variable now represents the *pod* terminal state (via
`define_pod_state()`), not just the base container state. Renaming
`container_state` (and corresponding log/message wording where applicable)
would make the control flow easier to follow and reduce misinterpretation
during future changes.
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -925,7 +925,7 @@ def invoke_defer_method(
logging_interval=self.logging_interval,
trigger_kwargs=self.trigger_kwargs,
)
- container_state = trigger.define_container_state(self.pod) if self.pod
else None
+ container_state = trigger.define_pod_state(self.pod) if self.pod else
None
if context and (
container_state == ContainerState.TERMINATED or container_state ==
ContainerState.FAILED
):
Review Comment:
`container_state` is now computed via `define_pod_state(self.pod)` and can
represent a pod-level terminal phase. Renaming this variable (and the related
`mocked_container_state` in tests) to `pod_state`/`terminal_state` would better
reflect what is being checked before deferring.
--
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]
