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

chore(lambda): allow the architecture property to be readable from the Function #17121

Merged
merged 8 commits into from
Nov 16, 2021

Conversation

jcstorms1
Copy link
Contributor

@jcstorms1 jcstorms1 commented Oct 22, 2021

We currently have layers that are designed specifically for arm64 and x86 based functions, and a single stack may have multiple functions with varying architectures. Applying the correct layer to a large list of functions can be a tedious process that may result in applying the incorrect layer to a function. Exposing the architecture at the Function level would allow a user to easily apply the correct layer automatically during deploy by checking the value and pulling the appropriate layer for that architecture. This would eliminate or greatly reduce the possibility of applying the wrong layer type.


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

@gitpod-io
Copy link

gitpod-io bot commented Oct 22, 2021

@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Oct 22, 2021
@mergify
Copy link
Contributor

mergify bot commented Oct 22, 2021

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@jcstorms1 jcstorms1 changed the title Feat(Lambda) Allow the architecture property to be readable from the Function feat(lambda) Allow the architecture property to be readable from the Function Oct 22, 2021
@jcstorms1 jcstorms1 changed the title feat(lambda) Allow the architecture property to be readable from the Function feat(lambda): Allow the architecture property to be readable from the Function Oct 22, 2021
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR.

Can you update the PR description on the motivation for this change?

@jcstorms1
Copy link
Contributor Author

I've updated the description to clarify the motivation for this change. Thanks for taking a look!

packages/@aws-cdk/aws-lambda/lib/function.ts Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/test/function.test.ts Outdated Show resolved Hide resolved
@nija-at nija-at changed the title feat(lambda): Allow the architecture property to be readable from the Function chore(lambda): allow the architecture property to be readable from the Function Oct 26, 2021
@mergify mergify bot dismissed nija-at’s stale review October 26, 2021 14:24

Pull request has been modified.

@jcstorms1
Copy link
Contributor Author

I have updated the Architecture property to return the correct default value. This led to several changes with multiple tests. I was able to get most of them passing but the following two are not. It looks like both are using hashing and I was hoping for some guidance on how to get them to pass as well. Thanks!

  1. function-hash.test.js
  2. lambda-version.test.js

@jcstorms1
Copy link
Contributor Author

Hi @nija-at I was curious if you saw my last comment and if you had any advice on those tests? Thanks!

nija-at
nija-at previously requested changes Nov 5, 2021
@@ -672,7 +677,7 @@ export class Function extends FunctionBase {
if (props.architectures && props.architectures.length > 1) {
throw new Error('Only one architecture must be specified.');
}
const architecture = props.architecture ?? (props.architectures && props.architectures[0]);
const architecture = props.architecture ?? (props.architectures && props.architectures[0]) ?? Architecture.X86_64;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no, this is not what I meant by my previous comment. I meant only about fixing the getter.
The change you have now will cause every CDK user's stack to change (undesirable).

However, I just decided in a different PR to leave such getters as undefined if the user has not supplied it. Let's revert this back to returning undefined if not specified.

Apologies for the churn.

@mergify mergify bot dismissed nija-at’s stale review November 9, 2021 16:57

Pull request has been modified.

Co-authored-by: Niranjan Jayakar <nija@amazon.com>
@jcstorms1
Copy link
Contributor Author

jcstorms1 commented Nov 9, 2021

@nija-at apologies for the confusion. I've gone ahead and reverted the last two commits as requested and fixed the test assertion method.

nija-at
nija-at previously approved these changes Nov 10, 2021
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @jcstorms1

@mergify mergify bot dismissed nija-at’s stale review November 10, 2021 19:16

Pull request has been modified.

@jcstorms1
Copy link
Contributor Author

Hey @nija-at. I was curious if there was anything else you needed from me to get this merged? I'm hoping to have this feature available soon if possible. Thanks again!

@nija-at
Copy link
Contributor

nija-at commented Nov 16, 2021

@jcstorms1 - my previous approval was discarded of the manual branch update you triggered. Don't touch the PR once approved. Our bots will do the needed changes and merge.

@mergify
Copy link
Contributor

mergify bot commented Nov 16, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 35f4483
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 1b81c20 into aws:master Nov 16, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 16, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mpvosseller pushed a commit to mpvosseller/aws-cdk that referenced this pull request Nov 16, 2021
…e Function (aws#17121)

We currently have layers that are designed specifically for arm64 and x86 based functions, and a single stack may have multiple functions with varying architectures. Applying the correct layer to a large list of functions can be a tedious process that may result in applying the incorrect layer to a function. Exposing the architecture at the Function level would allow a user to easily apply the correct layer automatically during deploy by checking the value and pulling the appropriate layer for that architecture. This would eliminate or greatly reduce the possibility of applying the wrong layer type.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…e Function (aws#17121)

We currently have layers that are designed specifically for arm64 and x86 based functions, and a single stack may have multiple functions with varying architectures. Applying the correct layer to a large list of functions can be a tedious process that may result in applying the incorrect layer to a function. Exposing the architecture at the Function level would allow a user to easily apply the correct layer automatically during deploy by checking the value and pulling the appropriate layer for that architecture. This would eliminate or greatly reduce the possibility of applying the wrong layer type.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda Related to AWS Lambda
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants