rjgoyln commented on code in PR #75479:
URL: https://github.com/apache/airflow/pull/75479#discussion_r4367831850
##########
providers/google/tests/unit/google/cloud/triggers/test_kubernetes_engine.py:
##########
@@ -163,6 +163,54 @@ def
test_serialize_round_trip_preserves_on_finish_action(self):
assert reconstructed_trigger.on_finish_action.value ==
"delete_succeeded_pod"
+ def test_serialize_emits_log_events_on_startup_only_when_disabled(self,
trigger):
+ assert "log_events_on_startup" not in trigger.serialize()[1]
+
+ trigger.log_events_on_startup = False
+
+ _, kwargs_dict = trigger.serialize()
+
+ assert kwargs_dict["log_events_on_startup"] is False
+ reconstructed = GKEStartPodTrigger(**kwargs_dict)
+ assert reconstructed.log_events_on_startup is False
+
+ @pytest.mark.asyncio
+ @pytest.mark.parametrize(
+ ("log_events_on_startup", "expected_calls"),
+ [(True, 1), (False, 0)],
+ )
+ @mock.patch(f"{TRIGGER_KUB_POD_PATH}.define_pod_container_state",
autospec=True)
+ @mock.patch(
+
"airflow.providers.cncf.kubernetes.triggers.pod.AsyncPodManager.await_pod_start",
autospec=True
+ )
+ @mock.patch(
+
"airflow.providers.cncf.kubernetes.triggers.pod.AsyncPodManager.watch_pod_events",
autospec=True
+ )
+ @mock.patch(f"{TRIGGER_KUB_POD_PATH}._get_pod", autospec=True)
+ async def test_wait_for_pod_start_watches_events_only_when_enabled(
Review Comment:
I noticed this test also passes against `main`'s `GKEStartPodTrigger`. It's
a copy of `test_wait_for_pod_start_watches_events_only_when_enabled` in the
cncf-kubernetes `test_pod.py`.
Would it make sense to drop it, along with the EKS copy? The serialize tests
already fail without the change, so they cover the new part.
##########
providers/amazon/src/airflow/providers/amazon/aws/triggers/eks.py:
##########
@@ -170,6 +172,7 @@ def __init__(
last_log_time=last_log_time,
logging_interval=logging_interval,
trigger_kwargs=trigger_kwargs,
+ log_events_on_startup=log_events_on_startup,
Review Comment:
It looks like the amazon provider still allows
`apache-airflow-providers-cncf-kubernetes>=7.2.0`, but this kwarg and
`self.log_events_on_startup` in the operator only exist from 10.25.0.
I tried the PR head against cncf-kubernetes 10.24.0, and the existing
`test_invoke_defer_method_uses_eks_trigger` and `test_serialize_roundtrip` both
fail with `AttributeError` / `TypeError` on `log_events_on_startup`.
Would it make sense to add the `# use next version` marker to that line in
`providers/amazon/pyproject.toml`?
```toml
"apache-airflow-providers-cncf-kubernetes>=7.2.0", # use next version
```
--
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]
