feat(Public ECR): Added --build-image Options #2725
Conversation
|
More of a general comment; we should test the following cases:
|
| required=False, | ||
| help="Container image URIs for building functions. " | ||
| "You can specify for all functions with just the image URI " | ||
| "(--build-image public.ecr.aws/sam/build-nodejs14.x:latest). " |
There was a problem hiding this comment.
Why not have two options for this? We have seen in the past overriding options for different cases can become complex quickly. Seems like having one option for all and one option to specify per function might be safer to have.
Side question: Did we consider putting this data on the function in the template Metadata like we do for other build information?
There was a problem hiding this comment.
This was done for consistency with the env var option as both of them have the same format and similar effect. We can bring this into the UX review for a further discussion.
Metadata in the template is not in scope of this change, but we can explore it in the future.
There was a problem hiding this comment.
I believe right now this is safe to have them in one option. Previously the container env var option have the the same overriding and precedence pattern which we feel that is safe and would not be too complex to cause any issues. Anyways we have a ux review set up on Friday as well, @jfuss if you can join to discuss this it would be nice as well!
There was a problem hiding this comment.
Did we consider putting this data on the function in the template Metadata like we do for other build information?
I think this is a good call-out. We should have a consistent story about how parameters are passed. How do we choose between the template or CLI arguments? Why are CLI arguments are preferred here versus the template?
samcli/commands/build/command.py
Outdated
| for build_image_string in image_args: | ||
| function_name, image_uri = _parse_key_value_pair(build_image_string) | ||
| if not image_uri: | ||
| LOG.error("Invalid command line --build-image input %s, skipped.", build_image_string) |
There was a problem hiding this comment.
Why should we be skipping on invalid input rather than failing the command?
There was a problem hiding this comment.
Yep I would agree on failing the command here. This behaviour here is to align with how we parse the env vars. But I would agree that the building should not happen if the image cannot be parsed properly
| @@ -67,6 +67,7 @@ def __init__( | |||
| docker_client: Optional[docker.DockerClient] = None, | |||
| container_env_var: Optional[Dict] = None, | |||
| container_env_var_file: Optional[str] = None, | |||
| build_images: Optional[dict] = None, | |||
There was a problem hiding this comment.
Missed this during review, will change it to Dict in next pr for consistency
| if tag is None: | ||
| tag = image_name.split(":")[1] if ":" in image_name else "latest" |
There was a problem hiding this comment.
Which tag does it use if image_name has a tag and tag is set?
* feat(Public ECR): Changed Build Image Registry to Public ECR (#2708) * Changed Build Image Registry to Public ECR * Added _IMAGE_URI_PREFIX * Fixed Unit Tests * Updated Image Format * feat(Public ECR): Updated Test Image Name and Added Latest Tag Check (#2720) * Added Tests for Latest Tag * Added use_container Check for Verify Latest Tag Tests * Fixed DotNet and Go Build Tests * Added Missing use_container Check for test_build_function_and_layer * feat(Public ECR): Changed Build Image Registry to Public ECR (#2708) * Changed Build Image Registry to Public ECR * Added _IMAGE_URI_PREFIX * Fixed Unit Tests * Updated Image Format * feat(Public ECR): Updated Test Image Name and Added Latest Tag Check (#2720) * Added Tests for Latest Tag * Added use_container Check for Verify Latest Tag Tests * Fixed DotNet and Go Build Tests * Added Missing use_container Check for test_build_function_and_layer * feat(Public ECR): Added --build-image Options (#2725) * Added Latest Tag Check * Added --build-image Option * Added Unit Tests * Added Help Text and Fixed PyLint * Fixed Path Validation for Unit Tests * Updated Type Hintings * Added _parse_key_value_pair * Updated tag Handling for Image Pulls * Added Throwing Exception with Invalid Build Images * Fixed PyLint Issue * Feat: added integration tests, container tag check and layer support (#2780) * Added integration tests * Added click requirement on -u presence for container based options * Added click class that checks -u presence for container based options * Reformatting and fixing test cases * Fix for integration test failures * Added support for layers * Resolve conflict * Refactoring file location * Addressing review comments, refactoring * Reformatting logging sentence * Refactoring container check for simplicity * Clarifying test case * Added unit tests on layer builds * Improving readability * Refactoring to simplify * Removing unnecessary lines * Refactoring integration test * Shortening error message * Adjusting test behaviour * Added new unit tests * Test refactoring * Added new test class * Removed unused import * Improving help text Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>
* feat(Public ECR): Changed Build Image Registry to Public ECR (aws#2708) * Changed Build Image Registry to Public ECR * Added _IMAGE_URI_PREFIX * Fixed Unit Tests * Updated Image Format * feat(Public ECR): Updated Test Image Name and Added Latest Tag Check (aws#2720) * Added Tests for Latest Tag * Added use_container Check for Verify Latest Tag Tests * Fixed DotNet and Go Build Tests * Added Missing use_container Check for test_build_function_and_layer * feat(Public ECR): Changed Build Image Registry to Public ECR (aws#2708) * Changed Build Image Registry to Public ECR * Added _IMAGE_URI_PREFIX * Fixed Unit Tests * Updated Image Format * feat(Public ECR): Updated Test Image Name and Added Latest Tag Check (aws#2720) * Added Tests for Latest Tag * Added use_container Check for Verify Latest Tag Tests * Fixed DotNet and Go Build Tests * Added Missing use_container Check for test_build_function_and_layer * feat(Public ECR): Added --build-image Options (aws#2725) * Added Latest Tag Check * Added --build-image Option * Added Unit Tests * Added Help Text and Fixed PyLint * Fixed Path Validation for Unit Tests * Updated Type Hintings * Added _parse_key_value_pair * Updated tag Handling for Image Pulls * Added Throwing Exception with Invalid Build Images * Fixed PyLint Issue * Feat: added integration tests, container tag check and layer support (aws#2780) * Added integration tests * Added click requirement on -u presence for container based options * Added click class that checks -u presence for container based options * Reformatting and fixing test cases * Fix for integration test failures * Added support for layers * Resolve conflict * Refactoring file location * Addressing review comments, refactoring * Reformatting logging sentence * Refactoring container check for simplicity * Clarifying test case * Added unit tests on layer builds * Improving readability * Refactoring to simplify * Removing unnecessary lines * Refactoring integration test * Shortening error message * Adjusting test behaviour * Added new unit tests * Test refactoring * Added new test class * Removed unused import * Improving help text Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>
Feature Branch PR
Notes
Integration tests will be added in another PR.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.