Skip to content
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

Warn for missing BuildArchitecture during Layer build #6355

Merged
merged 9 commits into from
Nov 28, 2023

Conversation

bentvelj
Copy link
Contributor

NOTE: I've moved the check for BuildArchitectureCompatibleArchitectures from PR #6322 (related issue: #6110) into the Layer build, so that it's not exclusive to Makefile Layer builds.

To summarize:

  • All Layer builds will warn if BuildArchitecture is missing
  • All Layer builds will warn if BuildArchitecture is not in CompatibleArchitectures
  • Only Makefile Layer builds will check and warn on invalid (not x86_64 or arm64) BuildArchitecture. Other BuildMethods will verify this in LambdaBuilders already, and fail on bad architecture.

Which issue(s) does this change fix?

#3747

Why is this change necessary?

Customer is confused as to why the build does not use their CompatibleArchitectures when they omit BuildArchitecture. It has already been decided to default to the case of x86_64 in the case of missing BuildArchitecture, so we can warn the customer to add it if it is missing.

How does it address the issue?

Adds a warning to Layer builds when BuildArchitecture is missing, and that x86_64 is being used as a default.
Also warns if the BuildArchitecture specified is not in CompatibleArchitectures.

What side effects does this change have?

N/A

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@bentvelj bentvelj requested a review from a team as a code owner November 28, 2023 18:37
@github-actions github-actions bot added area/build sam build command pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Nov 28, 2023
Copy link
Contributor

@sidhujus sidhujus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bentvelj bentvelj enabled auto-merge November 28, 2023 19:14
@bentvelj bentvelj disabled auto-merge November 28, 2023 19:14
@lucashuy lucashuy added pr/internal and removed pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Nov 28, 2023
@bentvelj bentvelj added this pull request to the merge queue Nov 28, 2023
Merged via the queue into develop with commit fd75e84 Nov 28, 2023
@bentvelj bentvelj deleted the bentvels_fix_3747 branch November 28, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build sam build command pr/internal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants