-
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 80.54% 80.54%
=======================================
Files 106 106
Lines 6954 6954
Branches 1287 1287
=======================================
Hits 5601 5601
Misses 1175 1175
Partials 178 178
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. |
a0af52a
to
bf3d3d4
Compare
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.
66ad184
to
6cd56c0
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
The third argument of the Consequently, the specifications for the arguments of the |
@nmussy Thank you for your review!! I've addressed all of your comments. |
@@ -684,7 +689,7 @@ export class Canary extends cdk.Resource implements ec2.IConnectable { | |||
|
|||
// Only check runtime family is Node.js because versions prior to `syn-nodejs-puppeteer-3.3` are deprecated and can no longer be configured. | |||
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}\`.`); |
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.
Would you be able to add a test for Playwright in integ.canary-artifact-s3-encryption
to confirm this works as expected? Thanks
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