Skip to content

feat: add ability to pass docker build params in template#8913

Draft
reedham-aws wants to merge 4 commits intoaws:developfrom
reedham-aws:docker-args
Draft

feat: add ability to pass docker build params in template#8913
reedham-aws wants to merge 4 commits intoaws:developfrom
reedham-aws:docker-args

Conversation

@reedham-aws
Copy link
Copy Markdown
Contributor

Which issue(s) does this change fix?

#8656

Why is this change necessary?

It had not been previously possible to pass in standard Docker arguments to the CLI build. While this isn't a big issue, it's a nice quality of life feature to add.

How does it address the issue?

Adds the ability to read extra parameters from the metadata of a SAM template, similar to how we already read build args that go into the Dockerfile. These parameters are then appended to the Docker/Finch CLI subprocess call.

What side effects does this change have?

Should have none because this is opt-in. Integration tests should confirm this.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review has been superseded by a newer review.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Results

Reviewed: 09c3cd5..0ca443e
Files: 6
Comments: 2

platform: Optional[str] = None,
target: Optional[str] = None,
rm: bool = True,
extra_params: Optional[list[str]] = None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BUG] The extra_params parameter is added to both SDKBuildClient.build_image and CLIBuildClient.build_image, but the abstract base class ImageBuildClient.build_image is not updated to include it.

This breaks the interface contract: type checkers (mypy) will flag the subclass signatures as incompatible with the abstract method, and developers reading the base class won't know extra_params exists.

The fix is to add the parameter to the abstract method signature:

@abstractmethod
def build_image(
   self,
   path: str,
   dockerfile: str,
   tag: str,
   buildargs: Optional[Dict[str, str]] = None,
   platform: Optional[str] = None,
   target: Optional[str] = None,
   rm: bool = True,
   extra_params: Optional[list[str]] = None,
) -> Generator[Dict[str, Any], None, None]:

The docstring for the abstract method (lines 44-67) should also be updated to document the new parameter.

docker_tag = f"{image_name.lower()}:{tag}"
docker_build_target = metadata.get("DockerBuildTarget", None)
docker_build_args = metadata.get("DockerBuildArgs", {})
docker_build_extra_params = metadata.get("DockerBuildExtraParams", None)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INPUT_VALIDATION] DockerBuildExtraParams is read from metadata with no type validation, unlike DockerBuildArgs which has an explicit isinstance(docker_build_args, dict) check. If a user provides a non-list value (e.g., a string like "--ssh default"), cmd.extend(extra_params) in CLIBuildClient would iterate over individual characters, producing broken flags. In SDKBuildClient, the truthiness check would pass and the warning would fire, but the inconsistency would be silently ignored.

Add a type check consistent with the existing DockerBuildArgs validation:

docker_build_extra_params = metadata.get("DockerBuildExtraParams", None)

if docker_build_extra_params is not None and not isinstance(docker_build_extra_params, list):
   raise DockerBuildFailed("DockerBuildExtraParams needs to be a list!")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/build sam build command area/local/invoke sam local invoke command area/local/start-api sam local start-api command area/local/start-invoke pr/internal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants