-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(Public ECR): Added --build-image Options #2725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
db45723
Added Latest Tag Check
CoshUS f6a6dfb
Added --build-image Option
CoshUS 3b109b1
Added Unit Tests
CoshUS 2051d96
Added Help Text and Fixed PyLint
CoshUS ab4be56
Fixed Path Validation for Unit Tests
CoshUS 5bf8347
Updated Type Hintings
CoshUS 240cec2
Added _parse_key_value_pair
CoshUS 00d540f
Updated tag Handling for Image Pulls
CoshUS 6628148
Added Throwing Exception with Invalid Build Images
CoshUS ad03f5b
Fixed PyLint Issue
CoshUS File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missed this during review, will change it to Dict in next pr for consistency |
||
| ) -> None: | ||
| """ | ||
| Initialize the class | ||
|
|
@@ -103,6 +104,8 @@ def __init__( | |
| An optional dictionary of environment variables to pass to the container | ||
| container_env_var_file : Optional[str] | ||
| An optional path to file that contains environment variables to pass to the container | ||
| build_images : Optional[Dict] | ||
| An optional dictionary of build images to be used for building functions | ||
| """ | ||
| self._resources_to_build = resources_to_build | ||
| self._build_dir = build_dir | ||
|
|
@@ -122,6 +125,7 @@ def __init__( | |
| self._colored = Colored() | ||
| self._container_env_var = container_env_var | ||
| self._container_env_var_file = container_env_var_file | ||
| self._build_images = build_images | ||
|
|
||
| def build(self) -> Dict[str, str]: | ||
| """ | ||
|
|
@@ -428,28 +432,23 @@ def _build_layer( | |
|
|
||
| # By default prefer to build in-process for speed | ||
| build_runtime = specified_workflow | ||
| build_method = self._build_function_in_process | ||
| options = ApplicationBuilder._get_build_options(layer_name, config.language, None) | ||
| if self._container_manager: | ||
| build_method = self._build_function_on_container | ||
| if config.language == "provided": | ||
| LOG.warning( | ||
| "For container layer build, first compatible runtime is chosen as build target for container." | ||
| ) | ||
| # Only set to this value if specified workflow is makefile | ||
| # which will result in config language as provided | ||
| build_runtime = compatible_runtimes[0] | ||
| options = ApplicationBuilder._get_build_options(layer_name, config.language, None) | ||
| self._build_function_on_container( | ||
| config, code_dir, artifact_subdir, manifest_path, build_runtime, options, container_env_vars | ||
| ) | ||
| else: | ||
| self._build_function_in_process( | ||
| config, code_dir, artifact_subdir, scratch_dir, manifest_path, build_runtime, options | ||
| ) | ||
|
|
||
| build_method( | ||
CoshUS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| config, | ||
| code_dir, | ||
| artifact_subdir, | ||
| scratch_dir, | ||
| manifest_path, | ||
| build_runtime, | ||
| options, | ||
| container_env_vars, | ||
| ) | ||
| # Not including subfolder in return so that we copy subfolder, instead of copying artifacts inside it. | ||
| return artifact_dir | ||
|
|
||
|
|
@@ -520,21 +519,28 @@ def _build_function( # pylint: disable=R1710 | |
|
|
||
| options = ApplicationBuilder._get_build_options(function_name, config.language, handler) | ||
| # By default prefer to build in-process for speed | ||
| build_method = self._build_function_in_process | ||
| if self._container_manager: | ||
| build_method = self._build_function_on_container | ||
| return build_method( | ||
| image = None | ||
| if self._build_images is not None: | ||
| if function_name in self._build_images: | ||
| image = self._build_images[function_name] | ||
| elif None in self._build_images: | ||
CoshUS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| image = self._build_images[None] | ||
|
|
||
| return self._build_function_on_container( | ||
| config, | ||
| code_dir, | ||
| artifact_dir, | ||
| scratch_dir, | ||
| manifest_path, | ||
| runtime, | ||
| options, | ||
| container_env_vars, | ||
| image, | ||
| ) | ||
|
|
||
| return build_method(config, code_dir, artifact_dir, scratch_dir, manifest_path, runtime, options) | ||
| return self._build_function_in_process( | ||
| config, code_dir, artifact_dir, scratch_dir, manifest_path, runtime, options | ||
| ) | ||
|
|
||
| # pylint: disable=fixme | ||
| # FIXME: we need to throw an exception here, packagetype could be something else | ||
|
|
@@ -572,7 +578,6 @@ def _build_function_in_process( | |
| manifest_path: str, | ||
| runtime: str, | ||
| options: Optional[dict], | ||
| container_env_vars: Optional[Dict] = None, | ||
| ) -> str: | ||
|
|
||
| builder = LambdaBuilder( | ||
|
|
@@ -604,11 +609,11 @@ def _build_function_on_container( | |
| config: CONFIG, | ||
| source_dir: str, | ||
| artifacts_dir: str, | ||
| scratch_dir: str, | ||
| manifest_path: str, | ||
| runtime: str, | ||
| options: Optional[Dict], | ||
| container_env_vars: Optional[Dict] = None, | ||
| build_image: Optional[str] = None, | ||
| ) -> str: | ||
| # _build_function_on_container() is only called when self._container_manager if not None | ||
| if not self._container_manager: | ||
|
|
@@ -642,6 +647,7 @@ def _build_function_on_container( | |
| executable_search_paths=config.executable_search_paths, | ||
| mode=self._mode, | ||
| env_vars=container_env_vars, | ||
| image=build_image, | ||
| ) | ||
|
|
||
| try: | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,7 +126,7 @@ def stop(self, container: Container) -> None: | |
| container.stop() | ||
| container.delete() | ||
|
|
||
| def pull_image(self, image_name, tag="latest", stream=None): | ||
| def pull_image(self, image_name, tag=None, stream=None): | ||
| """ | ||
| Ask Docker to pull the container image with given name. | ||
|
|
||
|
|
@@ -142,6 +142,8 @@ def pull_image(self, image_name, tag="latest", stream=None): | |
| DockerImagePullFailedException | ||
| If the Docker image was not available in the server | ||
| """ | ||
| if tag is None: | ||
| tag = image_name.split(":")[1] if ":" in image_name else "latest" | ||
|
Comment on lines
+145
to
+146
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which tag does it use if |
||
| # use a global lock to get the image lock | ||
| with self._lock: | ||
| image_lock = self._lock_per_image.get(image_name) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?