-
Notifications
You must be signed in to change notification settings - Fork 4k
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(lambda): function version ignores layer version changes #20150
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.
Reviving this PR that has been sitting in my local copy of cdk for a while. It's still a bit rough around the edges but I think it's worth getting out there.
@@ -640,7 +641,7 @@ export class Function extends FunctionBase { | |||
|
|||
protected readonly canCreatePermissions = true; | |||
|
|||
private readonly layers: ILayerVersion[] = []; | |||
public readonly layers: ILayerVersion[] = []; |
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.
The alternative to making this public is to send the layers directly into the calculateFunctionHash()
like: calculateFunctionHash(fn, fn.layers)
. That seems awkward to me so I chose to make this public.
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.
calculateFunctionHash
is a private function, so I don't really care about its signature. I care more about making this public, which this change does... so I'd rather you didn't.
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.
Marked @internal instead. I need this to be publicly accessible for the aspect as well, and if my understanding of @internal
is correct, this makes the most sense.
…nto conroy/lambdalayer
@@ -291,8 +291,15 @@ by default. | |||
Existing users will need to enable the [feature flag] | |||
`@aws-cdk/aws-lambda:recognizeVersionProps`. Since CloudFormation does not | |||
allow duplicate versions, they will also need to make some modification to | |||
their function so that a new version can be created. Any trivial change such as |
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.
updated this readme
* version. This aspect will change the function description in these cases, | ||
* which "validates" the new function hash. | ||
*/ | ||
export class FunctionVersionUpgrade implements IAspect { |
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.
added this Aspect that should help users update their existing lambdas
@@ -31,4 +32,8 @@ alias.addFunctionUrl({ | |||
authType: lambda.FunctionUrlAuthType.NONE, | |||
}); | |||
|
|||
// Changes the function description when the feature flag is present | |||
// to validate the changed function hash. | |||
cdk.Aspects.of(stack).add(new lambda.FunctionVersionUpgrade(LAMBDA_RECOGNIZE_LAYER_VERSION)); |
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.
Added this aspect here, so that yarn integ --update-on-failed
succeeds. For the integ test updates in other modules, I just ran yarn integ --update-on-failed --disable-update-workflow --dry-run
.
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.
Conditionally approved
@@ -640,7 +641,7 @@ export class Function extends FunctionBase { | |||
|
|||
protected readonly canCreatePermissions = true; | |||
|
|||
private readonly layers: ILayerVersion[] = []; | |||
public readonly layers: ILayerVersion[] = []; |
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.
calculateFunctionHash
is a private function, so I don't really care about its signature. I care more about making this public, which this change does... so I'd rather you didn't.
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
modify cdk.json to enable this feature, pasting this inside the context: |
Fixes #19098.
This introduces two bug fixes that are hidden behind a feature flag to preserve the current hash:
I also added a few more tests around this area to confirm the current behavior which should help demonstrate what the feature flag will change.
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license