kaxil commented on code in PR #67876:
URL: https://github.com/apache/airflow/pull/67876#discussion_r3337648730
##########
providers/amazon/src/airflow/providers/amazon/aws/triggers/redshift_cluster.py:
##########
@@ -43,10 +43,13 @@ class RedshiftCreateClusterTrigger(AwsBaseWaiterTrigger):
def __init__(
self,
+ *,
cluster_identifier: str,
aws_conn_id: str | None = "aws_default",
+ region_name: str | None = None,
Review Comment:
The `region_name` param (now explicit) and `verify`/`botocore_config`
(accepted via `**kwargs`) aren't listed in this trigger's docstring.
`BatchJobTrigger` documents `region_name` in its own docstring as precedent.
Same gap in the other four triggers in this file. Suggested additions under the
existing `:param` block:
```
:param region_name: The AWS region where the cluster is. Used to build the
hook.
:param verify: Whether or not to verify SSL certificates. Used to build the
hook.
:param botocore_config: Configuration dictionary for the botocore client.
Used to build the hook.
```
##########
providers/amazon/tests/unit/amazon/aws/triggers/test_redshift_cluster.py:
##########
@@ -115,3 +120,105 @@ async def
test_redshift_cluster_sensor_trigger_exception(self, mock_cluster_stat
# so we validate for length of task to be 1
assert len(task) == 1
assert TriggerEvent({"status": "error", "message": "Test exception"})
in task
+
+
+WAITER_TRIGGER_PARAMS = [
+ pytest.param(
+ RedshiftCreateClusterTrigger,
+ 15,
+ 999999,
+ id="RedshiftCreateClusterTrigger",
+ ),
+ pytest.param(
+ RedshiftPauseClusterTrigger,
+ 15,
+ 999999,
+ id="RedshiftPauseClusterTrigger",
+ ),
+ pytest.param(
+ RedshiftCreateClusterSnapshotTrigger,
+ 15,
+ 999999,
+ id="RedshiftCreateClusterSnapshotTrigger",
+ ),
+ pytest.param(
+ RedshiftResumeClusterTrigger,
+ 15,
+ 999999,
+ id="RedshiftResumeClusterTrigger",
+ ),
+ pytest.param(
+ RedshiftDeleteClusterTrigger,
+ 30,
+ 30,
+ id="RedshiftDeleteClusterTrigger",
+ ),
+]
+
+
+class TestRedshiftWaiterTriggers:
+ """Tests for the five Redshift triggers that inherit from
``AwsBaseWaiterTrigger``."""
+
+ @pytest.mark.parametrize(
+ ("trigger_cls", "default_delay", "default_max_attempts"),
+ WAITER_TRIGGER_PARAMS,
+ )
+ def test_serialization(self, trigger_cls, default_delay,
default_max_attempts):
+ trigger = trigger_cls(
+ cluster_identifier="test_cluster",
+ aws_conn_id="aws_default",
+ region_name="us-east-1",
+ )
+
+ classpath, kwargs = trigger.serialize()
+ assert classpath ==
f"airflow.providers.amazon.aws.triggers.redshift_cluster.{trigger_cls.__name__}"
+ assert kwargs == {
+ "cluster_identifier": "test_cluster",
+ "waiter_delay": default_delay,
+ "waiter_max_attempts": default_max_attempts,
+ "aws_conn_id": "aws_default",
+ "region_name": "us-east-1",
+ }
+
+ @pytest.mark.parametrize(
+ ("trigger_cls", "default_delay", "default_max_attempts"),
+ WAITER_TRIGGER_PARAMS,
+ )
+ def test_serialization_with_verify_and_botocore_config(
+ self, trigger_cls, default_delay, default_max_attempts
+ ):
+ trigger = trigger_cls(
+ cluster_identifier="test_cluster",
+ aws_conn_id="aws_default",
+ verify=False,
+ botocore_config={"connect_timeout": 30},
+ )
+
+ _, kwargs = trigger.serialize()
+ assert kwargs["verify"] is False
+ assert kwargs["botocore_config"] == {"connect_timeout": 30}
Review Comment:
`test_serialization_with_verify_and_botocore_config` is the one test that
leaves `region_name` unset, so it's the only place the `region_name=None`
pruning path runs, but it never asserts the outcome. Worth confirming it's
dropped from the serialized kwargs so a regression in the `region_name or None`
/ `prune_dict` handling gets caught:
```suggestion
assert kwargs["botocore_config"] == {"connect_timeout": 30}
assert "region_name" not in kwargs
```
--
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]
