fix: default to empty list for docker build extra params metadata#8941
Conversation
|
https://github.com/aws/aws-sam-cli/actions/runs/25065781894 Edit: Integration tests that had been failing are now passing. Other failures are flaky. |
| SAM_METADATA_DOCKER_CONTEXT_KEY: str(asset_path), | ||
| SAM_METADATA_DOCKER_BUILD_ARGS_KEY: metadata.get(ASSET_DOCKERFILE_BUILD_ARGS_KEY, {}), | ||
| SAM_METADATA_DOCKER_BUILD_EXTRA_PARAMS_KEY: metadata.get(ASSET_DOCKER_BUILD_EXTRA_PARAMS_KEY, None), | ||
| SAM_METADATA_DOCKER_BUILD_EXTRA_PARAMS_KEY: metadata.get(ASSET_DOCKER_BUILD_EXTRA_PARAMS_KEY, []), |
There was a problem hiding this comment.
[TESTING] Consider adding a unit test that verifies the default value of DockerBuildExtraParams when aws:asset:docker-build-extra-params is absent from the metadata. The existing test test_replace_all_resources_that_contain_image_metadata_dockerfile_extensions exercises this path (it omits the key from metadata) but does not assert on the resulting DockerBuildExtraParams value.
Since this PR specifically fixes a TOML serialization failure caused by None being set as the default, a targeted assertion would prevent regressions:
# In test_replace_all_resources_that_contain_image_metadata_dockerfile_extensions,
# after the existing assertions:
self.assertEqual(
[], template_data["Resources"]["Function1"]["Metadata"]["DockerBuildExtraParams"]
)This ensures the default stays as an empty list (TOML-serializable) rather than accidentally reverting to None.
There was a problem hiding this comment.
Not concerned since this is caught in the integration tests. There are unit tests that test other aspects of this.
Which issue(s) does this change fix?
Why is this change necessary?
Integration tests are failing after: #8913.
https://github.com/aws/aws-sam-cli/actions/runs/25045806939/job/73360436775
How does it address the issue?
Changes the default from
Noneto an empty list to avoid trying to convertNoneto TOML.What side effects does this change have?
An empty list would be carried around, but that's not too different from the build args where there's an empty dictionary.
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.