-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(synthetics): safe canary update #34608
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
Conversation
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 contribution.
Some minor comments.
| } | ||
|
|
||
| private validateDryRunAndUpdate(runtime: Runtime, dryRunAndUpdate?: boolean) { | ||
| if (dryRunAndUpdate === undefined) { |
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.
Is validation needed when dryRunAndUpdate is false?
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.
You're right! I've updated to validate when only dryRunAndUpdate is true.
| } | ||
| } | ||
|
|
||
| private validateDryRunAndUpdate(runtime: Runtime, dryRunAndUpdate?: boolean) { |
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.
IMO, this validation seems a bit overkilled. But I'd like to wait for the maintainers' opinion on this.
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 validation adds value, but it could be simplified. Could you please explain why you did not use the available runtime values to check against? Having a collection of the supported runtime versions and then checking if the provided one is any of the supported ones would likely be a simpler approach.
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 feature is defined as being available in versions syn-nodejs-puppeteer-10.0+, syn-nodejs-playwright-2.0+, and syn-python-selenium-5.1+. Currently, only puppeteer-10.0, playwright-2.0, and selenium-5.1 are the available versions, but they should continue to increase in the future.
I think it would be quite cumbersome to keep updating the available versions each time this happens.
What are your thoughts on this?
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.
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.
Updating the collection to add future supported versions may be cumbersome, but the same applies to handling future deprecated versions. At some point, the minimum supported version could be deprecated, requiring an update to the logic. This is likely an unavoidable aspect of the project.
With that in mind, I'd still rather have a simpler logic in place. So what do you think about using a regex expression that separates the runtimeName string into the runtime family ('syn-nodejs-puppeteer', 'syn-nodejs-playwright', etc) and the version number? We could keep a single object (let's say it's called MIN_SUPPORTED_VERSIONS or something similar) that has the supported family names as the keys, and the minimum version as the values. Then, you will just need to match the runtimeName to the regex and use the object to see if the matched family exists in the MIN_SUPPORTED_VERSIONS and if the version part is equal to or greater than the value of that key.
Something like:
const RUNTIME_REGEX = /^syn-(nodejs-puppeteer|nodejs-playwright|python-selenium)-(\d+\.\d+)$/;
const MIN_SUPPORTED_VERSIONS: { [family: string]: number } = {
'nodejs-puppeteer': 10.0,
'nodejs-playwright': 2.0,
'python-selenium': 5.1
};
const match = runtimeName.match(RUNTIME_REGEX);
// If the runtime name doesn't match our expected pattern, it's not supported
if (!match || match.length < 3) {
return false;
}
const family = match[1];
const version = parseFloat(match[2]);
const minVersion = MIN_SUPPORTED_VERSIONS[family];
// Check if this family is in our object and if the version meets the minimum
return minVersion !== undefined && version >= minVersion;
what do you think?
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.
@alvazjor Thank you for your suggestion. I've updated my implementation like that. Could you please confirm again?
|
@mazyu36 Thank you for your review! I've updated my implementation.
I can understand your opinion. I'll also wait for maintainer's review. |
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. LGTM.
|
@badmintoncryer Thank you for your contribution. Added one comment. |
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 for the contribution
|
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 CloudWatch synthetics supports for performing safe canary update.
This feature cannot be configurable from AWS CDK L2 construct.
Description of changes
dryRunAndUpdateprop tocanaryPropsDescribe any new or updated permissions being added
None
Description of how you validated changes
Add both unit and integ tets.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license