Skip to content

Commit 6ca8f92

Browse files
committed
fix: address PR reviewer findings - remove substring false-positive and add error case tests
- Remove substring check in _force_ecs_deployment that could match wrong services (e.g. my-app:5 matching my-app:50). Use family-name comparison only. - Add unit tests for ContainerName mismatch error handling in both app_builder and packageable_resources. - Clean up unused imports in test file.
1 parent ec83c23 commit 6ca8f92

2 files changed

Lines changed: 43 additions & 8 deletions

File tree

samcli/lib/sync/flows/ecs_container_sync_flow.py

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -184,13 +184,11 @@ def _force_ecs_deployment(self) -> None:
184184
service_name = parts[-1]
185185

186186
svc_response = ecs_client.describe_services(cluster=cluster, services=[service_name])
187+
my_family = physical_id.rsplit("/", 1)[-1].split(":", 1)[0]
187188
for svc in svc_response.get("services", []):
188189
svc_task_def = svc.get("taskDefinition", "")
189-
# Check if this service references our task definition family
190-
if physical_id in svc_task_def or (
191-
svc_task_def.rsplit("/", 1)[-1].split(":", 1)[0]
192-
== physical_id.rsplit("/", 1)[-1].split(":", 1)[0]
193-
):
190+
svc_family = svc_task_def.rsplit("/", 1)[-1].split(":", 1)[0]
191+
if svc_family and svc_family == my_family:
194192
ecs_client.update_service(
195193
cluster=cluster,
196194
service=service_name,

tests/unit/lib/build_module/test_container_build_integration.py

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
"""Tests for ECS/AgentCore container build integration across modules"""
22

33
from unittest import TestCase
4-
from unittest.mock import MagicMock, patch, Mock
5-
from copy import deepcopy
4+
from unittest.mock import MagicMock, patch
65

76
from samcli.lib.build.app_builder import ApplicationBuilder
87
from samcli.lib.build.build_graph import ContainerBuildDefinition
@@ -184,7 +183,7 @@ def test_includes_container_services(
184183
mock_manager.get_repository_mapping.return_value = {"MyFunction": "uri1", "MyAgent": "uri2"}
185184
mock_manager_cls.return_value = mock_manager
186185

187-
result = sync_ecr_stack("template.yaml", "stack", "us-east-1", "bucket", "prefix", {})
186+
sync_ecr_stack("template.yaml", "stack", "us-east-1", "bucket", "prefix", {})
188187

189188
# Verify both function and container service were passed
190189
call_args = mock_manager.set_functions.call_args[0]
@@ -257,3 +256,41 @@ def test_sync_skips_when_no_image(self):
257256
flow._physical_id_mapping = {}
258257
# Should not raise
259258
flow.sync()
259+
260+
261+
class TestContainerNameErrorHandling(TestCase):
262+
def test_update_built_resource_raises_on_container_name_mismatch(self):
263+
from samcli.lib.build.exceptions import DockerBuildFailed
264+
265+
properties = {
266+
"ContainerDefinitions": [
267+
{"Name": "sidecar", "Image": "sidecar:latest"},
268+
{"Name": "web", "Image": "placeholder"},
269+
]
270+
}
271+
metadata = {"ContainerName": "typo"}
272+
with self.assertRaises(DockerBuildFailed):
273+
ApplicationBuilder._update_built_resource(
274+
"myimage:latest", properties, AWS_ECS_TASK_DEFINITION, "/path", metadata
275+
)
276+
277+
def test_get_target_index_raises_on_container_name_mismatch(self):
278+
from samcli.commands.package import exceptions
279+
280+
exporter = ECSTaskDefinitionImageResource.__new__(ECSTaskDefinitionImageResource)
281+
exporter.resource_metadata = {"ContainerName": "typo"}
282+
container_defs = [{"Name": "web"}, {"Name": "sidecar"}]
283+
with self.assertRaises(exceptions.ExportFailedError):
284+
exporter._get_target_index(container_defs)
285+
286+
def test_get_target_index_returns_match(self):
287+
exporter = ECSTaskDefinitionImageResource.__new__(ECSTaskDefinitionImageResource)
288+
exporter.resource_metadata = {"ContainerName": "web"}
289+
container_defs = [{"Name": "sidecar"}, {"Name": "web"}]
290+
self.assertEqual(exporter._get_target_index(container_defs), 1)
291+
292+
def test_get_target_index_defaults_to_zero_without_name(self):
293+
exporter = ECSTaskDefinitionImageResource.__new__(ECSTaskDefinitionImageResource)
294+
exporter.resource_metadata = {}
295+
container_defs = [{"Name": "web"}]
296+
self.assertEqual(exporter._get_target_index(container_defs), 0)

0 commit comments

Comments
 (0)