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

feat(synthetics): support runtime nodejs puppeteer 4.0 #25553

Merged

Conversation

WinterYukky
Copy link
Contributor

@WinterYukky WinterYukky commented May 12, 2023

What change

Add CloudWatch Synthetics runtime nodejs puppeteer 4.0.

Others

I changed integ test to using integ-tests assertions.

Closes #25493


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

@github-actions github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label May 12, 2023
@gitpod-io
Copy link

gitpod-io bot commented May 12, 2023

@github-actions github-actions bot added the p2 label May 12, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team May 12, 2023 05:16
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 12, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This blueprint was renewed in AWS Synthetics service console and failed run, so I replaced this script to latest script.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

conn.close()


def main():

url = 'https://example.com/'
url = os.environ['URL']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this dummy url to actual URL that created in integ test stack.

Comment on lines +87 to +100
// Assertion that all Canary's are Passed
[
inlineAsset,
directoryAsset,
zipAsset,
puppeteer39,
puppeteer40,
selenium13,
].forEach((canary) => test.assertions
.awsApiCall('Synthetics', 'getCanaryRuns', {
Name: canary.canaryName,
})
.assertAtPath('CanaryRuns.0.Status.State', ExpectedResult.stringLikeRegexp('PASSED'))
.waitForAssertions({ totalTimeout: cdk.Duration.minutes(5) }));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion can test that status of canaries is passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This blueprint was renewed in AWS Synthetics service console and failed run, so I replaced this script to latest script.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean by this? does the old canary runtime code not work? We intentionally did not use AWS Synthetics blueprints because we don't want to be in charge of making sure that our copy is the same as the blueprint.

If the current code fails, i would prefer to fix it in the most basic way possible than replace it with the actual blueprint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. old canary runtime code was not work. I got failed status.
However, I understood your think, so I changed runtime code to close to minimum changes.

@github-actions github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. labels May 12, 2023
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the new runtime and updating the integ test! My only comments are about our test canaries themselves -- they are intentionally not supposed to mirror any blueprints that AWS Synthetics provides

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean by this? does the old canary runtime code not work? We intentionally did not use AWS Synthetics blueprints because we don't want to be in charge of making sure that our copy is the same as the blueprint.

If the current code fails, i would prefer to fix it in the most basic way possible than replace it with the actual blueprint.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@WinterYukky
Copy link
Contributor Author

Thanks @kaizencc for your reviewing!
I changed canary runtime code to close to minimum changes. Could you re:review this PR?

Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @WinterYukky!

@mergify
Copy link
Contributor

mergify bot commented May 30, 2023

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 1d7a9a8 into aws:main May 30, 2023
@mergify
Copy link
Contributor

mergify bot commented May 30, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admired-contributor [Pilot] contributed between 13-24 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-synthetics-alpha): support latest canary runtime for NodeJS v4.0
3 participants