fix: If image functions have same metadata, only 1st one is in the result #2624
Conversation
This file contains 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
aahung
changed the title
aahung
changed the title
aahung
changed the title
sriram-mv
suggested changes
Feb 12, 2021
| @@ -193,6 +193,30 @@ def test_build_layers_and_functions(self, mock_copy_tree, mock_path): | |||
| str(mock_path(given_build_dir, self.function1_2.name)), | |||
| ) | |||
|
|
|||
| def test_build_single_function_definition_image_functions_with_same(self, mock_copy_tree, mock_path): | |||
There was a problem hiding this comment.
should we also add an integ test for this?
mndeveci
suggested changes
Feb 12, 2021
samcli/lib/build/build_strategy.py
Outdated
Comment on lines
128
to
131
| if build_definition.packagetype == ZIP: | ||
| for function in build_definition.functions: | ||
| if function.name is not single_function_name: | ||
| for function in build_definition.functions: | ||
| if function.name != single_function_name: | ||
| if build_definition.packagetype == ZIP: | ||
| # for zip function we need to copy over the artifacts |
There was a problem hiding this comment.
Why we need to change the order here? This will check the if statement for every function in the build definition (no matter it is ZIP or IMAGE), but the previous order will have 2 separate flow for IMAGE and ZIP from the beginning.
There was a problem hiding this comment.
right, good catch! I optimized for more less code. This indeed introduced more checks. Reverted
aahung
force-pushed
the
fix-image-functions-with-same-image
branch
from
06788b5 to
c0142f4
Compare
aahung
commented
Feb 12, 2021
| @@ -127,12 +127,18 @@ def build_single_function_definition(self, build_definition: FunctionBuildDefini | |||
| # copy results to other functions | |||
| if build_definition.packagetype == ZIP: | |||
| for function in build_definition.functions: | |||
| if function.name is not single_function_name: | |||
| if function.name != single_function_name: | |||
There was a problem hiding this comment.
we should use != instead of is not to compare strings.
sriram-mv
approved these changes
Feb 12, 2021
mndeveci
approved these changes
Feb 12, 2021
elbayaaa
added a commit
that referenced
this pull request
Feb 19, 2021
* Revert "fix: If image functions have same metadata, only 1st one is in the result (#2624)" This reverts commit 9440f15. * Revert "fix: SamFunctionProvider could miss functions with same name but in different stacks (#2609)" This reverts commit 1382eaf. * Revert "fix: Nested stack cannot be extracted if not in working dir (#2618)" This reverts commit d7bcfc6.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…s built
Which issue(s) does this change fix?
Why is this change necessary?
How does it address the issue?
What side effects does this change have?
Checklist
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.