Skip to content

Commit 854065b

Browse files
singledigitclaude
andcommitted
fix: address bnusunny PR review findings for ECS/AgentCore container sync
- sam sync now captures the ECR URI returned by ECRUploader.upload and registers a new TaskDefinition revision with the updated image, then updates matching ECS services to that revision (fixes silent no-op) - Tighten _force_ecs_deployment → _update_services_to_task_definition to require both the arn:aws:ecs: prefix and :service/ segment, preventing false matches on App Runner / VPC Lattice / AppMesh ARNs - Split AgentCore out of ECSContainerSyncFlow: GENERATOR_MAPPING now routes AWS::BedrockAgentCore::Runtime to _create_agentcore_flow which logs a clear 'not yet supported' warning and returns None (UpdateAgentRuntime not yet implemented) - Add AWS::ECS::TaskDefinition to SyncCodeResources._accepted_resources so sam sync --code-resource AWS::ECS::TaskDefinition is accepted by Click - Add ApiCallTypes.UPDATE_CONTAINER_IMAGE enum value; use it in ECSContainerSyncFlow._get_resource_api_calls instead of UPDATE_FUNCTION_CODE - Fix multi-container TaskDefinition safety: raise DockerBuildFailed when >1 ContainerDefinitions and no Metadata.ContainerName (app_builder.py) - Fix AgentCore _update_built_resource: replace setdefault chain (which corrupts intrinsics) with direct dict assignment - Fix isinstance(metadata, dict) guards in _update_built_resource, _has_container_build_metadata, and ECSTaskDefinitionImageResource - Update ECSTaskDefinitionImageResource docstring and add LOG.warning for multi-container fallback (packageable_resources.py) - Remove dead BuildGraph.CONTAINER_BUILD_DEFINITIONS constant and get/put_container_build_definition methods (zero callers, not serialized in _read/_write) - Add test_ecs_container_sync_flow.py with 17 unit tests covering the new sync path, ARN filtering, error handling, and tag propagation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 2915d43 commit 854065b

9 files changed

Lines changed: 348 additions & 28 deletions

File tree

samcli/lib/build/app_builder.py

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -420,7 +420,7 @@ def _update_built_resource(
420420
if resource_type == AWS_ECS_TASK_DEFINITION:
421421
container_defs = resource_properties.get("ContainerDefinitions", [])
422422
if container_defs:
423-
target_name = metadata.get("ContainerName") if metadata else None
423+
target_name = metadata.get("ContainerName") if isinstance(metadata, dict) else None
424424
if target_name:
425425
for container_def in container_defs:
426426
if container_def.get("Name") == target_name:
@@ -431,12 +431,17 @@ def _update_built_resource(
431431
f"Metadata.ContainerName '{target_name}' does not match any "
432432
f"container in ContainerDefinitions"
433433
)
434+
elif len(container_defs) > 1:
435+
raise DockerBuildFailed(
436+
f"TaskDefinition has {len(container_defs)} containers but Metadata.ContainerName is not set. "
437+
f"Add 'ContainerName: <name>' to the resource Metadata to specify which container to build."
438+
)
434439
else:
435440
container_defs[0]["Image"] = path
436441
if resource_type == AWS_BEDROCK_AGENTCORE_RUNTIME:
437-
artifact = resource_properties.setdefault("AgentRuntimeArtifact", {})
438-
container_config = artifact.setdefault("ContainerConfiguration", {})
439-
container_config["ContainerUri"] = path
442+
resource_properties["AgentRuntimeArtifact"] = {
443+
"ContainerConfiguration": {"ContainerUri": path}
444+
}
440445

441446
def _build_lambda_image(self, function_name: str, metadata: Dict, architecture: str) -> str:
442447
"""

samcli/lib/build/build_graph.py

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -200,14 +200,12 @@ class BuildGraph:
200200
# global table build definitions key
201201
FUNCTION_BUILD_DEFINITIONS = "function_build_definitions"
202202
LAYER_BUILD_DEFINITIONS = "layer_build_definitions"
203-
CONTAINER_BUILD_DEFINITIONS = "container_build_definitions"
204203

205204
def __init__(self, build_dir: str) -> None:
206205
# put build.toml file inside .aws-sam folder
207206
self._filepath = Path(build_dir).parent.joinpath(DEFAULT_BUILD_GRAPH_FILE_NAME)
208207
self._function_build_definitions: List["FunctionBuildDefinition"] = []
209208
self._layer_build_definitions: List["LayerBuildDefinition"] = []
210-
self._container_build_definitions: List["ContainerBuildDefinition"] = []
211209
self._atomic_read()
212210

213211
def get_function_build_definitions(self) -> Tuple["FunctionBuildDefinition", ...]:
@@ -216,21 +214,6 @@ def get_function_build_definitions(self) -> Tuple["FunctionBuildDefinition", ...
216214
def get_layer_build_definitions(self) -> Tuple["LayerBuildDefinition", ...]:
217215
return tuple(self._layer_build_definitions)
218216

219-
def get_container_build_definitions(self) -> Tuple["ContainerBuildDefinition", ...]:
220-
return tuple(self._container_build_definitions)
221-
222-
def put_container_build_definition(self, container_build_definition: "ContainerBuildDefinition") -> None:
223-
"""
224-
Puts the newly read container build definition into existing build graph.
225-
Each container build definition is unique per resource identifier.
226-
"""
227-
if container_build_definition not in self._container_build_definitions:
228-
LOG.debug(
229-
"Adding container build definition: %s",
230-
container_build_definition,
231-
)
232-
self._container_build_definitions.append(container_build_definition)
233-
234217
def get_function_build_definition_with_full_path(
235218
self, function_full_path: str
236219
) -> Optional["FunctionBuildDefinition"]:

samcli/lib/package/packageable_resources.py

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -694,7 +694,8 @@ def export(self, resource_id: str, resource_dict: Optional[Dict], parent_dir: st
694694
class ECSTaskDefinitionImageResource(ResourceImage):
695695
"""
696696
Represents an ECS TaskDefinition with a container image that needs to be pushed to ECR.
697-
The image reference is at ContainerDefinitions[0].Image.
697+
For single-container TaskDefinitions the first container is used by default; for
698+
multi-container TaskDefinitions Metadata.ContainerName must be set.
698699
"""
699700

700701
RESOURCE_TYPE = AWS_ECS_TASK_DEFINITION
@@ -703,8 +704,8 @@ class ECSTaskDefinitionImageResource(ResourceImage):
703704

704705
def _get_target_index(self, container_defs):
705706
"""Find the target container index using ContainerName from Metadata."""
706-
metadata = getattr(self, "resource_metadata", None) or {}
707-
target_name = metadata.get("ContainerName")
707+
metadata = getattr(self, "resource_metadata", None)
708+
target_name = metadata.get("ContainerName") if isinstance(metadata, dict) else None
708709
if target_name:
709710
for i, cd in enumerate(container_defs):
710711
if cd.get("Name") == target_name:
@@ -714,9 +715,15 @@ def _get_target_index(self, container_defs):
714715
property_name=self.PROPERTY_NAME,
715716
property_value=target_name,
716717
ex=ValueError(
717-
f"Metadata.ContainerName '{target_name}' does not match any " f"container in ContainerDefinitions"
718+
f"Metadata.ContainerName '{target_name}' does not match any container in ContainerDefinitions"
718719
),
719720
)
721+
if len(container_defs) > 1:
722+
LOG.warning(
723+
"TaskDefinition has multiple containers but Metadata.ContainerName is not set; "
724+
"packaging the first container. Add 'ContainerName: <name>' to the resource Metadata "
725+
"to avoid ambiguity."
726+
)
720727
return 0
721728

722729
def export(self, resource_id, resource_dict, parent_dir):

samcli/lib/providers/sam_container_provider.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ def _extract_container_services(self) -> None:
8181
@staticmethod
8282
def _has_container_build_metadata(metadata: Optional[Dict]) -> bool:
8383
"""Check if metadata contains the required fields for a container image build."""
84-
if not metadata:
84+
if not isinstance(metadata, dict):
8585
return False
8686
return bool(metadata.get("Dockerfile") and metadata.get("DockerContext"))
8787

samcli/lib/sync/sync_flow.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ class ApiCallTypes(Enum):
4343
UPDATE_FUNCTION_CONFIGURATION = "UpdateFunctionConfiguration"
4444
UPDATE_FUNCTION_CODE = "UpdateFunctionCode"
4545
PUBLISH_VERSION = "PublishVersion"
46+
UPDATE_CONTAINER_IMAGE = "UpdateContainerImage"
4647

4748

4849
class ResourceAPICall(NamedTuple):

samcli/lib/sync/sync_flow_factory.py

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ class SyncCodeResources:
7575
AWS_APIGATEWAY_V2_API,
7676
AWS_SERVERLESS_STATEMACHINE,
7777
AWS_STEPFUNCTIONS_STATEMACHINE,
78+
AWS_ECS_TASK_DEFINITION,
7879
]
7980

8081
@classmethod
@@ -356,6 +357,19 @@ def _create_ecs_container_flow(
356357
application_build_result,
357358
)
358359

360+
def _create_agentcore_flow(
361+
self,
362+
resource_identifier: ResourceIdentifier,
363+
application_build_result: Optional[ApplicationBuildResult],
364+
) -> Optional[SyncFlow]:
365+
# AgentCore sync requires UpdateAgentRuntime, which is not yet implemented.
366+
LOG.warning(
367+
"sam sync is not yet supported for AWS::BedrockAgentCore::Runtime resource '%s'. "
368+
"Use sam deploy to update AgentCore resources.",
369+
str(resource_identifier),
370+
)
371+
return None
372+
359373
GeneratorFunction = Callable[
360374
["SyncFlowFactory", ResourceIdentifier, Optional[ApplicationBuildResult]], Optional[SyncFlow]
361375
]
@@ -371,7 +385,7 @@ def _create_ecs_container_flow(
371385
AWS_SERVERLESS_STATEMACHINE: _create_stepfunctions_flow,
372386
AWS_STEPFUNCTIONS_STATEMACHINE: _create_stepfunctions_flow,
373387
AWS_ECS_TASK_DEFINITION: _create_ecs_container_flow,
374-
AWS_BEDROCK_AGENTCORE_RUNTIME: _create_ecs_container_flow,
388+
AWS_BEDROCK_AGENTCORE_RUNTIME: _create_agentcore_flow,
375389
}
376390

377391
# SyncFlow mapping between resource type and creation function

0 commit comments

Comments
 (0)