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

Triggers: executed on update even executeOnHandlerChange: false #25939

Closed
ctapobep opened this issue Jun 12, 2023 · 2 comments · Fixed by #26676
Closed

Triggers: executed on update even executeOnHandlerChange: false #25939

ctapobep opened this issue Jun 12, 2023 · 2 comments · Fixed by #26676
Labels
@aws-cdk/triggers Related to the triggers package bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@ctapobep
Copy link

ctapobep commented Jun 12, 2023

Describe the bug

Even if I set executeOnHandlerChange: false, the function gets executed on every deployment. This happens regardless of whether I just changed executeOnHandlerChange to false or if I first deployed with this flag, and then changed my function.

It doesn't seems like this property is used anywhere within CDK.

Expected Behavior

The handler is invoked just once - when Stack is originally created. Alternatively, just remove the option so that it doesn't confuse.

Current Behavior

The handler is executed every time I run cdk deploy

Reproduction Steps

Here's the CDK script:

const app = new App();
const mainStack = new Stack(app, 'Stas-HelloWorld', {env});
let scriptDir = 'Lambda-HelloWorld';
new Trigger(mainStack, 'InitDbTriggerForLambda', {
    handler: new Function(mainStack, scriptDir, {
        runtime: Runtime.NODEJS_16_X,
        code: Code.fromAsset(scriptDir),
        handler: 'lambda.handler',
        timeout: Duration.seconds(120),
        logRetention: RetentionDays.ONE_DAY
    }),
    executeAfter: [mainStack],
    executeOnHandlerChange: false
});

Originally the function did nothing, just declared the handler. Then I changed it so that it throws an error to be sure that it's actually invoked (this made the deployment fail):

 exports.handler = async function(event) {
    throw new Error("ooops");
 }

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.82.0

Framework Version

No response

Node.js Version

v18.16.0

OS

MacOS

Language

Typescript

Language Version

No response

Other information

No response

@ctapobep ctapobep added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 12, 2023
@github-actions github-actions bot added the @aws-cdk/triggers Related to the triggers package label Jun 12, 2023
@peterwoodworth
Copy link
Contributor

Yeah it appears this prop does nothing 😬

private determineHandlerArn(props: TriggerProps) {
return props.handler.currentVersion.functionArn;
// const executeOnHandlerChange = props.executeOnHandlerChange ?? true;
// if (executeOnHandlerChange) {
// }
// return props.handler.functionArn;
}

Thanks for reporting!

@peterwoodworth peterwoodworth added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Jun 12, 2023
@mergify mergify bot closed this as completed in #26676 Aug 10, 2023
mergify bot pushed a commit that referenced this issue Aug 10, 2023
… false (#26676)

Currently, trigger is re-executed every time when the handler function code or configuration changes even if executeOnHandlerChange is set to false. This is because executeOnHandlerChange prop is never used in the current implementation. 

This PR solves the issue by referring executeOnHandlerChange prop and skipping function invocation when executeOnHandlerChange is set to false.

Closes #25939

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/triggers Related to the triggers package bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants