o-nikolas commented on code in PR #36722:
URL: https://github.com/apache/airflow/pull/36722#discussion_r1448022745
##########
tests/providers/amazon/aws/executors/ecs/test_ecs_executor.py:
##########
@@ -943,8 +944,10 @@ def test_provided_values_override_defaults(self,
assign_subnets):
assert task_kwargs["platformVersion"] == templated_version
- def test_count_can_not_be_modified_by_the_user(self, assign_subnets):
+ @mock.patch.object(EcsHook, "conn")
+ def test_count_can_not_be_modified_by_the_user(self, mock_conn,
assign_subnets):
"""The ``count`` parameter must always be 1; verify that the user can
not override this value."""
+ mock_conn.describe_clusters.return_value = {"clusters": [{"status":
"ACTIVE"}]}
Review Comment:
Was this test failing before this change?
##########
airflow/providers/amazon/aws/executors/ecs/ecs_executor_config.py:
##########
@@ -60,6 +61,26 @@ def build_task_kwargs() -> dict:
task_kwargs = _fetch_config_values()
task_kwargs.update(_fetch_templated_kwargs())
+ try:
+ has_launch_type: bool = task_kwargs.get("launch_type") is not None
+ has_capacity_provider: bool =
task_kwargs.get("capacity_provider_strategy") is not None
+
+ if has_capacity_provider and has_launch_type:
+ raise ValueError(
+ "capacity_provider_strategy and launch_type are mutually
exclusive, you can not provide both."
+ )
+ elif not (has_capacity_provider or has_launch_type):
+ # Default API behavior if neither is provided is to fall back on
the default capacity
+ # provider if it exists. Since it is not a required value, check
if there is one
+ # before using it, and if there is not then use the FARGATE
launch_type as
+ # the final fallback.
+ cluster =
EcsHook().conn.describe_clusters(clusters=[task_kwargs["cluster"]])["clusters"][0]
+ if not cluster.get("defaultCapacityProviderStrategy"):
+ task_kwargs["launch_type"] = "FARGATE"
+
+ except KeyError:
+ pass
Review Comment:
I see you using `get` in all cases. What is throwing the `KeyError` here?
##########
airflow/providers/amazon/aws/executors/ecs/ecs_executor_config.py:
##########
@@ -60,6 +61,26 @@ def build_task_kwargs() -> dict:
task_kwargs = _fetch_config_values()
task_kwargs.update(_fetch_templated_kwargs())
+ try:
+ has_launch_type: bool = task_kwargs.get("launch_type") is not None
+ has_capacity_provider: bool =
task_kwargs.get("capacity_provider_strategy") is not None
Review Comment:
```suggestion
has_launch_type: bool = "launch_type" in task_kwargs
has_capacity_provider: bool = "capacity_provider_strategy" in
task_kwargs
```
##########
tests/providers/amazon/aws/executors/ecs/test_ecs_executor.py:
##########
@@ -1086,3 +1089,31 @@ def test_start_health_check_config(self, set_env_vars):
executor.start()
ecs_mock.stop_task.assert_not_called()
+
+ def test_providing_both_capacity_provider_and_launch_type_fails(self,
set_env_vars):
+ os.environ[
+
f"AIRFLOW__{CONFIG_GROUP_NAME}__{AllEcsConfigKeys.CAPACITY_PROVIDER_STRATEGY}".upper()
+ ] = "[{'capacityProvider': 'cp1', 'weight': 5}, {'capacityProvider':
'cp2', 'weight': 1}]"
+ expected_error = (
+ "capacity_provider_strategy and launch_type are mutually
exclusive, you can not provide both."
+ )
+
+ with pytest.raises(ValueError, match=expected_error):
+ AwsEcsExecutor()
+
+ def test_providing_capacity_provider(self, set_env_vars):
+ valid_capacity_provider = (
+ "[{'capacityProvider': 'cp1', 'weight': 5}, {'capacityProvider':
'cp2', 'weight': 1}]"
+ )
+
+ os.environ[
+
f"AIRFLOW__{CONFIG_GROUP_NAME}__{AllEcsConfigKeys.CAPACITY_PROVIDER_STRATEGY}".upper()
+ ] = valid_capacity_provider
+
os.environ.pop(f"AIRFLOW__{CONFIG_GROUP_NAME}__{AllEcsConfigKeys.LAUNCH_TYPE}".upper())
Review Comment:
We should add two more test cases where you don't provide a capacity
provider and also remove this launch type config. And ensure that the two layer
defaulting works (one test where the cluster default is used and another where
the ultimate fallback of fargate launch type is used).
Another option is to no longer set the launch type in `set_env_vars` and let
the defaulting put it in, then you wouldn't need to test that specific case,
since it'd be tested everywhere (but you still need one for the cluster default
in that case).
--
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]
