-
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
feat(synthetics): node playwright 1.0 and python selenium 4.1 runtime #32245
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32245 +/- ##
==========================================
- Coverage 81.52% 81.38% -0.15%
==========================================
Files 222 222
Lines 13717 13695 -22
Branches 2417 2412 -5
==========================================
- Hits 11183 11145 -38
- Misses 2254 2271 +17
+ Partials 280 279 -1
Flags with carried forward coverage won't be shown. Click here to find out 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.
Thank you for the PR! Could we name this PR with feat
instead of chore
and add to this integ test: https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/framework-integ/test/aws-synthetics/test/integ.canary.ts?
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 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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Pull request has been modified.
@gracelu0 Thank you for your review! I have some problems. Currently, the code cannot deploy the - private validateCanaryAsset(scope: Construct, handler: string, family: RuntimeFamily) {
+ private validateCanaryAsset(scope: Construct, handler: string, runtime: Runtime) { To align with this change, the - public bind(scope: Construct, handler: string, family: RuntimeFamily): CodeConfig {
+ public bind(scope: Construct, handler: string, runtime: Runtime): CodeConfig { However, this would introduce a breaking change to a public function, causing the CI to fail. API elements with incompatible changes:
err - METHOD aws-cdk-lib.aws_synthetics.AssetCode.bind: argument family, takes aws-cdk-lib.aws_synthetics.Runtime (formerly aws-cdk-lib.aws_synthetics.RuntimeFamily): aws-cdk-lib.aws_synthetics.RuntimeFamily is an enum different from aws-cdk-lib.aws_synthetics.Runtime [incompatible-argument:aws-cdk-lib.aws_synthetics.AssetCode.bind]
err - METHOD aws-cdk-lib.aws_synthetics.Code.bind: argument family, takes aws-cdk-lib.aws_synthetics.Runtime (formerly aws-cdk-lib.aws_synthetics.RuntimeFamily): aws-cdk-lib.aws_synthetics.RuntimeFamily is an enum different from aws-cdk-lib.aws_synthetics.Runtime [incompatible-argument:aws-cdk-lib.aws_synthetics.Code.bind]
err - METHOD aws-cdk-lib.aws_synthetics.InlineCode.bind: argument _family, takes aws-cdk-lib.aws_synthetics.Runtime (formerly aws-cdk-lib.aws_synthetics.RuntimeFamily): aws-cdk-lib.aws_synthetics.RuntimeFamily is an enum different from aws-cdk-lib.aws_synthetics.Runtime [incompatible-argument:aws-cdk-lib.aws_synthetics.InlineCode.bind]
err - METHOD aws-cdk-lib.aws_synthetics.S3Code.bind: argument _family, takes aws-cdk-lib.aws_synthetics.Runtime (formerly aws-cdk-lib.aws_synthetics.RuntimeFamily): aws-cdk-lib.aws_synthetics.RuntimeFamily is an enum different from aws-cdk-lib.aws_synthetics.Runtime [incompatible-argument:aws-cdk-lib.aws_synthetics.S3Code.bind]
Checking region info facts... OK.
Some packages seem to have undergone breaking API changes. Please avoid. Could you provide some advice on how to approach this modification? |
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 didn't have a chance to go over everything, but from what little testing I ran before being made aware of my duplicate implementation:
-
activeTracing
is not (yet?) supported for Playwright. Despite being allowed as a Node.js Runtime by the CDK, CloudFormation will throw on deployment
aws-cdk/packages/aws-cdk-lib/aws-synthetics/lib/canary.ts
Lines 592 to 594 in 27619cc
if (props.activeTracing && !cdk.Token.isUnresolved(props.runtime.family) && props.runtime.family !== RuntimeFamily.NODEJS) { throw new Error('You can only enable active tracing for canaries that use canary runtime version `syn-nodejs-2.0` or later.'); } -
I'm not sure about
artifactS3EncryptionMode
, but the CDK allows the use of the property as well, so it would at least require updating the error message and adding integration coverage
aws-cdk/packages/aws-cdk-lib/aws-synthetics/lib/canary.ts
Lines 686 to 688 in 27619cc
if (!isNodeRuntime && props.artifactS3EncryptionMode) { throw new Error(`Artifact encryption is only supported for canaries that use Synthetics runtime version \`syn-nodejs-puppeteer-3.3\` or later, got \`${props.runtime.name}\`.`); }
const puppeteer80 = createCanaryByRuntimes(Runtime.SYNTHETICS_NODEJS_PUPPETEER_8_0); | ||
const playwright10 = createCanaryByRuntimes(Runtime.SYNTHETICS_NODEJS_PLAYWRIGHT_1_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.
Could you add tests for SYNTHETICS_NODEJS_PUPPETEER_9_0
and SYNTHETICS_NODEJS_PUPPETEER_9_1
? They weren't covered when they were added
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've added!
const hasValidExtension = playwrightValidExtensions.some(ext => fs.existsSync(path.join(assetPath, `${filename}${ext}`))); | ||
if (runtime.family === RuntimeFamily.NODEJS && runtime.name.includes('puppeteer') && !fs.existsSync(path.join(assetPath, 'nodejs', 'node_modules', nodeFilename))) { |
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.
Playwright canaries should support multiple directories (e.g. a/b/c/entrypoint.mjs
), see docs:
Optionally, you can also store your entry point file in a folder structure of your choice. However, be sure that the folder path is specified in your handler name.
I think the validation would handle it fine, but could you add a unit test and an integ to make sure we support it correctly?
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.
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
// Validation can be performed using both `RuntimeFamily` and `runtimeName` arguments, | ||
// but for backward compatibility of `Code.bind()`, it is also designed to accept only `RuntimeFamily`. | ||
if (runtimeName && !Token.isUnresolved(runtimeName)) { | ||
const filename = handler.split('.')[0]; | ||
const nodeFilename = `${filename}.js`; | ||
const pythonFilename = `${filename}.py`; | ||
const playwrightValidExtensions = ['.cjs', '.mjs', '.js']; | ||
const hasValidExtension = playwrightValidExtensions.some(ext => fs.existsSync(path.join(assetPath, `${filename}${ext}`))); | ||
if (family === RuntimeFamily.NODEJS && runtimeName.includes('puppeteer') && !fs.existsSync(path.join(assetPath, 'nodejs', 'node_modules', nodeFilename))) { | ||
throw new Error(`The canary resource requires that the handler is present at "nodejs/node_modules/${nodeFilename}" but not found at ${this.assetPath} (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Nodejs.html)`); | ||
} | ||
if (family === RuntimeFamily.NODEJS && runtimeName.includes('playwright') && !hasValidExtension) { | ||
throw new Error(`The canary resource requires that the handler is present at one of the following extensions: ${playwrightValidExtensions.join(', ')} but not found at ${this.assetPath}`); | ||
} | ||
if (family === RuntimeFamily.PYTHON && !fs.existsSync(path.join(assetPath, 'python', pythonFilename))) { | ||
throw new Error(`The canary resource requires that the handler is present at "python/${pythonFilename}" but not found at ${this.assetPath} (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Python.html)`); | ||
} | ||
} else { | ||
const filename = handler.split('.')[0]; | ||
const nodeFilename = `${filename}.js`; | ||
const pythonFilename = `${filename}.py`; | ||
if (family === RuntimeFamily.NODEJS && !fs.existsSync(path.join(assetPath, 'nodejs', 'node_modules', nodeFilename))) { | ||
throw new Error(`The canary resource requires that the handler is present at "nodejs/node_modules/${nodeFilename}" but not found at ${this.assetPath} (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Nodejs.html)`); | ||
} | ||
if (family === RuntimeFamily.PYTHON && !fs.existsSync(path.join(assetPath, 'python', pythonFilename))) { | ||
throw new Error(`The canary resource requires that the handler is present at "python/${pythonFilename}" but not found at ${this.assetPath} (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Python.html)`); | ||
} |
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.
It would be good to simplify it a bit more to make it more readable.
One example:
// Validation can be performed using both `RuntimeFamily` and `runtimeName` arguments, | |
// but for backward compatibility of `Code.bind()`, it is also designed to accept only `RuntimeFamily`. | |
if (runtimeName && !Token.isUnresolved(runtimeName)) { | |
const filename = handler.split('.')[0]; | |
const nodeFilename = `${filename}.js`; | |
const pythonFilename = `${filename}.py`; | |
const playwrightValidExtensions = ['.cjs', '.mjs', '.js']; | |
const hasValidExtension = playwrightValidExtensions.some(ext => fs.existsSync(path.join(assetPath, `${filename}${ext}`))); | |
if (family === RuntimeFamily.NODEJS && runtimeName.includes('puppeteer') && !fs.existsSync(path.join(assetPath, 'nodejs', 'node_modules', nodeFilename))) { | |
throw new Error(`The canary resource requires that the handler is present at "nodejs/node_modules/${nodeFilename}" but not found at ${this.assetPath} (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Nodejs.html)`); | |
} | |
if (family === RuntimeFamily.NODEJS && runtimeName.includes('playwright') && !hasValidExtension) { | |
throw new Error(`The canary resource requires that the handler is present at one of the following extensions: ${playwrightValidExtensions.join(', ')} but not found at ${this.assetPath}`); | |
} | |
if (family === RuntimeFamily.PYTHON && !fs.existsSync(path.join(assetPath, 'python', pythonFilename))) { | |
throw new Error(`The canary resource requires that the handler is present at "python/${pythonFilename}" but not found at ${this.assetPath} (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Python.html)`); | |
} | |
} else { | |
const filename = handler.split('.')[0]; | |
const nodeFilename = `${filename}.js`; | |
const pythonFilename = `${filename}.py`; | |
if (family === RuntimeFamily.NODEJS && !fs.existsSync(path.join(assetPath, 'nodejs', 'node_modules', nodeFilename))) { | |
throw new Error(`The canary resource requires that the handler is present at "nodejs/node_modules/${nodeFilename}" but not found at ${this.assetPath} (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Nodejs.html)`); | |
} | |
if (family === RuntimeFamily.PYTHON && !fs.existsSync(path.join(assetPath, 'python', pythonFilename))) { | |
throw new Error(`The canary resource requires that the handler is present at "python/${pythonFilename}" but not found at ${this.assetPath} (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Python.html)`); | |
} | |
const filename = handler.split('.')[0]; | |
const nodeFilename = `${filename}.js`; | |
const pythonFilename = `${filename}.py`; | |
// Validation can be performed using both `RuntimeFamily` and `runtimeName` arguments, | |
// but for backward compatibility of `Code.bind()`, it is also designed to accept only `RuntimeFamily`. | |
if (runtimeName && !Token.isUnresolved(runtimeName)) { | |
const playwrightValidExtensions = ['.cjs', '.mjs', '.js']; | |
const hasValidExtension = playwrightValidExtensions.some(ext => fs.existsSync(path.join(assetPath, `${filename}${ext}`))); | |
if (family === RuntimeFamily.NODEJS && runtimeName.includes('puppeteer') && !fs.existsSync(path.join(assetPath, 'nodejs', 'node_modules', nodeFilename))) { | |
throw new Error(`The canary resource requires that the handler is present at "nodejs/node_modules/${nodeFilename}" but not found at ${this.assetPath} (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Nodejs.html)`); | |
} | |
if (family === RuntimeFamily.NODEJS && runtimeName.includes('playwright') && !hasValidExtension) { | |
throw new Error(`The canary resource requires that the handler is present at one of the following extensions: ${playwrightValidExtensions.join(', ')} but not found at ${this.assetPath}`); | |
} | |
} else if (family === RuntimeFamily.NODEJS && !fs.existsSync(path.join(assetPath, 'nodejs', 'node_modules', nodeFilename))) { | |
throw new Error(`The canary resource requires that the handler is present at "nodejs/node_modules/${nodeFilename}" but not found at ${this.assetPath} (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Nodejs.html)`); | |
} | |
if (family === RuntimeFamily.PYTHON && !fs.existsSync(path.join(assetPath, 'python', pythonFilename))) { | |
throw new Error(`The canary resource requires that the handler is present at "python/${pythonFilename}" but not found at ${this.assetPath} (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Python.html)`); | |
} |
However, I am not sure if the following validation is good when runtimeName is a token, i.e., when there is a possibility that it may be a playwright instead of a puppeteer.
// If runtimeName is a Token and the following is true:
} else if (family === RuntimeFamily.NODEJS && !fs.existsSync(path.join(assetPath, 'nodejs', 'node_modules', nodeFilename))) {
throw new Error(`The canary resource requires that the handler is present at "nodejs/node_modules/${nodeFilename}" but not found at ${this.assetPath} (https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Canaries_WritingCanary_Nodejs.html)`);
}
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.
Thank you for your great suggestion.
However, I am not sure if the following validation is good when runtimeName is a token, i.e., when there is a possibility that it may be a playwright instead of a puppeteer.
I have removed this validation because it would be an issue if the token was "playwright" and caused an error.
Co-authored-by: Kenta Goto (k.goto) <24818752+go-to-k@users.noreply.github.com>
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.
LGTM.
@badmintoncryer Hi, would you mind taking a look at the failed check and update the branch? Thanks :) |
@liuxiax Hello, CodeQL job has failed but this is caused by auto generated snapshots code. I think this is not a problem. |
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.
Hi @badmintoncryer , thank you for all your effort on this! And thank you to @go-to-k and @nmussy for your thorough review :) Just a few more nit comments and then I'm good to approve.
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.
nit: for L183 and L184, should be See the Note below for directory structure
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.
Where is above😁 Thanks!
if (runtimeName && !Token.isUnresolved(runtimeName)) { | ||
const playwrightValidExtensions = ['.cjs', '.mjs', '.js']; | ||
const hasValidExtension = playwrightValidExtensions.some(ext => fs.existsSync(path.join(assetPath, `${filename}${ext}`))); | ||
if (family === RuntimeFamily.NODEJS && runtimeName.includes('puppeteer') && !fs.existsSync(path.join(assetPath, 'nodejs', 'node_modules', nodeFilename))) { |
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.
Could we add the comments about the validation checks (e.g. * Requires asset directory to have the structure 'nodejs/node_modules' for puppeteer runtime.
) inside the function here to make it a bit easier to follow?
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.
Thank you for the nice suggestion. I've updated.
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.
Thanks! Could you repeat the same for the other validation checks (python, playwright)?
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've added!
@gracelu0 Thank you for your review! I've addressed all of your comments. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
None
Reason for this change
AWS Synthetics begins supporting the NodeJS Playwright runtime.
https://aws.amazon.com/about-aws/whats-new/2024/11/amazon-cloudwatch-synthetics-playwright-runtime-canaries-nodejs/
And Python Selenium runtime v4.1 is also released.
https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_Library_python_selenium.html#CloudWatch_Synthetics_runtimeversion-syn-python-selenium-4.1
Description of changes
Add two runtimes to
Runtime
classDescription of how you validated changes
Execute describe-runtime AWS CLI.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license