-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(cfnspec): cloudformation spec v22.0.0 #12204
Conversation
…ource with a patch to the spec.
…Formation namespaces.
packages/@aws-cdk/aws-cloudfront/lib/experimental/edge-function.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-dynamodb/test/integ.global-replicas-provisioned.ts
Outdated
Show resolved
Hide resolved
import * as path from 'path'; | ||
import * as cloudwatch from '@aws-cdk/aws-cloudwatch'; | ||
import * as ec2 from '@aws-cdk/aws-ec2'; | ||
import * as iam from '@aws-cdk/aws-iam'; | ||
import * as lambda from '@aws-cdk/aws-lambda'; | ||
// hack, as this is not exported by the Lambda module | ||
import { calculateFunctionHash } from '@aws-cdk/aws-lambda/lib/function-hash'; |
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.
Why not export then instead of reaching internals?
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.
Less surface area for the module?
@@ -9,15 +9,45 @@ export function calculateFunctionHash(fn: LambdaFunction) { | |||
|
|||
// render the cloudformation resource from this function | |||
const config = stack.resolve((functionResource as any)._toCloudFormation()); | |||
// config is of the shape: { Resources: { LogicalId: { Type: 'Function', Properties: { ... } }}} | |||
const resources = config.Resources; | |||
const logicalId = Object.keys(resources)[0]; |
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.
Why is this change needed here? Feels unrelated.
If the CfnFunction update resulted in hash changes and you think this is safe, we can easily update the hash without deploying by running yarn cdk-assert --dry-run
.
We should follow up with a fix to how lambada calculates hash
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.
But reverting these changes means every single customer that updates to the new CDK version will get their Function's Versions replaced...?
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.
Why? Is this due to field order changes?
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.
Yes. The previous properties of the CfnFunction
class result in JSON.stringfy()
from here returning
{"Resources":{"Fn9270CBC0":{"Type":"AWS::Lambda::Function","Properties":{"Code":{"ZipFile":"foo"},"Role":{"Fn::GetAtt":["FnServiceRoleB9001A96","Arn"]},"Handler":"index.handler","Runtime":"nodejs10.x"}}}}
With the new properties (ImageConfig
and PackageType
) added to CfnFunction
, the result is
{"Resources":{"Fn9270CBC0":{"Type":"AWS::Lambda::Function","Properties":{"Code":{"ZipFile":"foo"},"Handler":"index.handler","Role":{"Fn::GetAtt":["FnServiceRoleB9001A96","Arn"]},"Runtime":"nodejs10.x"}}}}
Which results in a different hash.
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.
To correct myself slightly, the addition of the ImageConfig
and PackageType
does not matter here. What changes the order is the fact that the Handler
and Runtime
properties have been made optional (from required) in this spec version, and our code generation takes required properties before optional ones.
…back to required.
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). |
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). |
Fixes #12170
Fixes #11974
Fixes #12114
Fixes #12028