-
Notifications
You must be signed in to change notification settings - Fork 4
Properly orders php layers to allow extension #4
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
Properly orders php layers to allow extension #4
Conversation
… added first This will allow passed in layers to extend the php layer
…yers ConsoleFunction will now extend the cdk construct Function instead of PhpFunction: Without remodelling the functions themselves, this was the easiest pick but it does duplicate nearly all logic from the PhpFunction itself
| expect(layers).length(2); | ||
| expect(layers[0]).to.match(/arn:aws:lambda:us-east-1:534081306603:layer:php-81:\d+/); | ||
| expect(layers[1]).to.match(/arn:aws:lambda:us-east-1:534081306603:layer:php-81:\d+/); | ||
| expect(layers[0]).to.match(/arn:aws:lambda:us-east-1:534081306603:layer:console:\d+/); |
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'm looking into this but I really don't understand how this could be what we want 🤔
The test ensures that the console layer is first, but it should be last, right?
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 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 just tested the deployed function and everything was working correctly. TBH I don't think there is a problem to solve?
Maybe you are on a bugged CDK 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.
Update: another report here: brefphp/bref#1580
Not sure why I can't reproduce this, this is so weird!
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.
Sorry I am (and still am) on vacation, I'll try to keep a closer eye on this conversation :)
Using the ConsoleFunction from the constructs yields the same result that you are seeing, deployed the layers are still reversed for me.
Now the interesting part: If i create a lambda function myself and setting the layers property to use php first and then console, the CF template looks the same(console first, php last), but deployed it is now in the correct order. 😕
I used cdk version 2.8.0 initially and just checked with the latest 2.8.5 and the behaviour is still the same
|
Coming from brefphp/bref #1580 @mnapoli I'm able to reproduce this issue with this script (creates "example" directory in #!/bin/bash
mkdir -p ./example
cd ./example
cdk init app --language typescript
npm install @bref.sh/constructs
echo "#!/usr/bin/env node
import 'source-map-support/register';
import * as cdk from 'aws-cdk-lib';
import { ExampleStack } from '../lib/example-stack';
const app = new cdk.App();
new ExampleStack(app, 'ExampleStack', {
env: { account: process.env.CDK_DEFAULT_ACCOUNT, region: 'us-east-1' },
});" > ./bin/example.ts
echo "import * as cdk from 'aws-cdk-lib';
import { Construct } from 'constructs';
import {ConsoleFunction} from '@bref.sh/constructs';
import {Code} from 'aws-cdk-lib/aws-lambda';
import * as path from 'path';
export class ExampleStack extends cdk.Stack {
constructor(scope: Construct, id: string, props?: cdk.StackProps) {
super(scope, id, props);
new ConsoleFunction(this, 'SymfonyCli', {
code: Code.fromAsset(path.join(__dirname, '../symfony')),
handler: 'bin/console'
});
}
}" > ./lib/example-stack.ts
mkdir -p ./symfony/bin
touch ./symfony/bin/console
cdk synthPartial output of Using $ node -v
v18.14.1
$ npm -v
9.7.2
$ cdk --version
2.85.0 (build 4e0d726) |
|
I don't think this is problem of After some debugging I came across Function.renderLayers which conditionally sort layers based on This flag is enabled by default if you create project via For quick fix you can disable this flag in {
// ....
"context": {
"@aws-cdk/aws-lambda:recognizeLayerVersion": false,
// ....
}
}@mnapoli I don't know how others handle testing of layers, but if you want to reproduce problem in tests, you have to modify export function compileTestStack(definition: (stack: Stack) => void): Template {
const app = new App({ context: { [cx_api.LAMBDA_RECOGNIZE_LAYER_VERSION]: true } });
const stack = new Stack(app, 'app', {
env: { region: 'us-east-1' },
});
definition(stack);
return Template.fromStack(stack);
} |
|
Fixed in #6 thanks both of you! |

Based on this discussion #1560
This changes the merge order of lambda layers, previously the php layer was added last, which lead to the console layer not being able to properly override the php layer.
The confusing part: The order of the array you pass into
addLayers(...ILayerVersion[])is actually reversed when you deploy it.Not sure if this would be a bug or a feature request for the cdk repository, since you would not run into this issue if layers are independent.