fix(synthetics): updated handler validation #26569

lpizzinidev · 2023-07-29T15:29:36Z

This fix updates handler validation based on Synthetic rules for Node and Python runtimes.

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

lpizzinidev · 2023-07-29T16:08:34Z

Exemption Request.
All validation cases should be covered by the unit tests.
Let me know if integration tests are needed.

kaizencc

Thanks for taking this on @lpizzinidev. One more thing: I think we should add a basic integ test that tests the folder/fileName.functionName scenario with a recent runtime just to make sure that the docs aren't wrong :) because sometimes they are.

kaizencc · 2023-07-31T13:44:00Z

packages/@aws-cdk/aws-synthetics-alpha/lib/code.ts

-    if (handler !== 'index.handler') {
-      throw new Error(`The handler for inline code must be "index.handler" (got "${handler}")`);
-    }


I'm concerned about removing this. Have you tested this to make sure that it works? IIRC lambda will bundle inline code into an index.js|py file, which means that it should at least always start with index.

I restored the original code since I'm unsure about the expected behavior and couldn't find documentation about it.
Let me know if you think we should dive deeper into this.

I dont see anyone complaining about this, and it looks right to me. So lets keep it for now

packages/@aws-cdk/aws-synthetics-alpha/lib/canary.ts

kaizencc · 2023-07-31T13:45:36Z

packages/@aws-cdk/aws-synthetics-alpha/lib/canary.ts

+    if (
+      runtime === Runtime.SYNTHETICS_PYTHON_SELENIUM_1_0 ||
+      runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_0 ||
+      runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_1 ||
+      runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_2 ||
+      runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_3
+    ) {


Suggested change

if (

runtime === Runtime.SYNTHETICS_PYTHON_SELENIUM_1_0 ||

runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_0 ||

runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_1 ||

runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_2 ||

runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_3

) {

const oldRuntimes = [/*put those runtimes here */];

if (oldRuntimes.includes(runtime)) {

kaizencc · 2023-07-31T13:49:40Z

packages/@aws-cdk/aws-synthetics-alpha/lib/canary.ts

+    } else if (runtime === Runtime.SYNTHETICS_NODEJS_PUPPETEER_3_4) {
+      if (!handler.match(/^[0-9A-Za-z_\\-]+\.[A-Za-z_][A-Za-z0-9_]*$/)) {
+        throw new Error(`Canary Handler must be specified either as \'fileName.handler\' or \'fileName.functionName\' for the \'syn-nodejs-puppeteer-3.4'\ runtime, received ${handler}`);


where did you get the info that 3.4 has it's own specific rule? From the cfn docs it seems this isn't the case:

For syn-python-selenium-1.1, syn-nodejs.puppeteer-3.4, and later runtimes, the handler can be specified as fileName.functionName , or you can specify a folder where canary scripts reside as folder/fileName.functionName .

Thanks for the review. I think I misread this line.
I'll apply the suggested changes and add the required tests.

kaizencc · 2023-07-31T13:50:38Z

packages/@aws-cdk/aws-synthetics-alpha/test/canary.test.ts

+    }).toThrow(/Canary Handler must be specified either as 'fileName.handler' or 'fileName.functionName' for the 'syn-nodejs-puppeteer-3.4' runtime/);
+  });
+
+  testDeprecated('recent runtimes', () => {


can we also unit test that fileName.functionName and folder/fileName.functionName passes the regex for recent runtimes

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

packages/@aws-cdk/aws-synthetics-alpha/lib/canary.ts

kaizencc · 2023-08-02T13:45:47Z

packages/@aws-cdk/aws-synthetics-alpha/lib/code.ts

-    if (handler !== 'index.handler') {
-      throw new Error(`The handler for inline code must be "index.handler" (got "${handler}")`);
-    }


I dont see anyone complaining about this, and it looks right to me. So lets keep it for now

kaizencc

Thanks @lpizzinidev!

mergify · 2023-08-02T14:32:32Z

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

aws-cdk-automation · 2023-08-02T15:06:09Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: fad31f9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-08-02T15:07:47Z

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

fix(synthetics): updated handler validation

6231f82

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Jul 29, 2023

aws-cdk-automation requested a review from a team July 29, 2023 15:29

github-actions bot added the star-contributor [Pilot] contributed between 25-49 PRs to the CDK label Jul 29, 2023

aws-cdk-automation previously requested changes Jul 29, 2023

View reviewed changes

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Jul 29, 2023

kaizencc requested changes Jul 31, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 31, 2023

github-actions bot mentioned this pull request Aug 1, 2023

Monthly PRs metrics report - July 2023 #26583

Closed

lpizzinidev added 2 commits August 2, 2023 09:17

Merge branch 'main' into awsgh-26540

dfa4e47

added integration test, fixed validation and unit tests

2fc7e8f

kaizencc reviewed Aug 2, 2023

View reviewed changes

Update packages/@aws-cdk/aws-synthetics-alpha/lib/canary.ts

2755159

kaizencc approved these changes Aug 2, 2023

View reviewed changes

Merge branch 'main' into awsgh-26540

fad31f9

mergify bot merged commit 1eaec92 into aws:main Aug 2, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(synthetics): updated handler validation #26569

fix(synthetics): updated handler validation #26569

lpizzinidev commented Jul 29, 2023

aws-cdk-automation left a comment

lpizzinidev commented Jul 29, 2023

kaizencc left a comment

kaizencc Jul 31, 2023

lpizzinidev Aug 2, 2023

kaizencc Aug 2, 2023

kaizencc Jul 31, 2023

kaizencc Jul 31, 2023

lpizzinidev Jul 31, 2023

kaizencc Jul 31, 2023

kaizencc Aug 2, 2023

kaizencc left a comment

mergify bot commented Aug 2, 2023

aws-cdk-automation commented Aug 2, 2023

mergify bot commented Aug 2, 2023

fix(synthetics): updated handler validation #26569

fix(synthetics): updated handler validation #26569

Conversation

lpizzinidev commented Jul 29, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lpizzinidev commented Jul 29, 2023

kaizencc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaizencc left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 2, 2023

aws-cdk-automation commented Aug 2, 2023

AWS CodeBuild CI Report

mergify bot commented Aug 2, 2023