-
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
Fix Erroneous "No BuildArchitecture specified in Layer" Warning #6508
Conversation
51fea81
to
e302c4f
Compare
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!
samcli/lib/build/app_builder.py
Outdated
LOG.warning( | ||
"WARNING: No BuildArchitecture specified in Layer `%s`" + " Metadata. Defaulting to x86_64.", | ||
layer_name, | ||
) |
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 we want to print this warning message if layer has arm64
architecture as CompatibleArchitectures
but it doesn't have BuildArchitecture
property
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.
There is another warning in the LayerVersion constructor that should take care of this.
It would output
WARNING: Layer `Layer` has BuildArchitecture `x86_64`, which is not listed in CompatibleArchitectures.
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 may be slightly confusing if the customer doesn't know that x86 is the default? Maybe we can adjust the messaging here
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.
My concern with this one is; if I have a layer which is supposed to be built with x86
I will keep getting these warning messages all the time, which can be annoying.
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.
Right. As discussed, going to restructure according to these rules:
There are two cases where we'd like to warn the customer
- Compatible Architectures is only x86 (or not present) but Build Architecture is arm64
- Build Architecture is x86 (or not present) but Compatible Architectures is only arm64
Which issue(s) does this change fix?
#6444
Why is this change necessary?
Customers were reporting the warning of
No BuildArchitecture specified in Layer
when building Lambda Functions which referenced existing Lambda Layers. The error message also incorrectly appears while building Lambda Layers without a BuildMethod at all.How does it address the issue?
Moved the logic which checks for missing BuildArchitecture from the LayerVersion constructor to the app builder
build_layer
function, so that warnings are not printed for layers that are not built.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.