-
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32245 +/- ##
=======================================
Coverage 81.48% 81.48%
=======================================
Files 226 226
Lines 13768 13768
Branches 2416 2416
=======================================
Hits 11219 11219
Misses 2271 2271
Partials 278 278
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. |
Co-authored-by: Kenta Goto (k.goto) <[email protected]>
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!
@gracelu0 Thank you for your review! I've addressed all of your comments. |
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, great work!!
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). |
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 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 main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. |
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