-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add architecture validations for SAM build LambdaLayer #6322
Conversation
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.
Thanks Jared, LGTM overall. Left a few minor comments.
Let's also write some unit tests for the changes in lib/build to ensure we don't lose coverage there.
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.
LGTM
Add a validation check for LambdaLayer
BuildArchitecture
to ensure that it is a valid architecture and one of theCompatibleArchitectures
listed. If theBuildArchitecture
is invalid, only a warning is displayed to inform the customer. Hard failing is not done for MakefileBuildMethod
to preserve customer freedom in building with their own Makefile.Adds architecture validation for when
--use-container
is specified for building a LambdaLayer. This now gives the customer a much clearer error message regarding the architecture instead of a "failed to pull image" error. (@mndeveci's changes)Adds integration test to verify that a clear error message is displayed in the case of a customer attempting to build a function with an invalid architecture.
Which issue(s) does this change fix?
#6110
Why is this change necessary?
Customers may be confused why LambdaLayers with invalid
BuildArchitecture
s still build just fine. A warning message will help customers catch possible errors in their template.Regarding the
--use-container
option for building a layer, reporting an invalid architecture error addresses the actual root of the problem instead of showing a consequence of having an invalid architecture ("failed to pull image" error). This makes it easier for customers to resolve.How does it address the issue?
BuildMethod
ofMakefile
. Architecture validation for otherBuildMethod
s are already done in LambdaBuilders, but nothing is done forMakefile
.What side effects does this change have?
N/A
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make pr
passesmake update-reproducible-reqs
if dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.