-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(globalaccelerator): add support for ip addresses and type #28055
Conversation
Thank you for the work on this so far. |
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 this PR! I commented some points.
b69f399
to
25b3969
Compare
@go-to-k updated per your feedback. Thanks! |
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 update!
I have commented on some adjustments, please take a look.
...g/framework-integ/test/aws-globalaccelerator/test/integ.globalaccelerator-ip-address-type.ts
Outdated
Show resolved
Hide resolved
Updated—thanks! |
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. |
Is this ready? I'm not been part of a issue and PR here. How long does it usually take for one to get deployed into production? |
There will be a maintainer review after this. I don't know when it will be merged, as it is also up to the maintainer. |
packages/aws-cdk-lib/awslint.json
Outdated
"from-method:aws-cdk-lib.aws_apigatewayv2.WebSocketRoute", | ||
"props-default-doc:aws-cdk-lib.aws_globalaccelerator.AcceleratorAttributes.dualStackDnsName", | ||
"props-default-doc:aws-cdk-lib.aws_globalaccelerator.AcceleratorAttributes.ipv4Addresses", | ||
"props-default-doc:aws-cdk-lib.aws_globalaccelerator.AcceleratorAttributes.ipv6Addresses" |
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.
@msambol why were these added to awslint.json?
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 get a linter error if they're not in this file.
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 linter error happens when there is no@default
value specified for the props.
Please include:
/**
* @default <value>
*/
in the code comment for your props values and remove the changes to awslint.
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.
@sumupitchayan Pinged you on Slack. These are for imports, I think they'd all be 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.
Updated.
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). |
Closes #28051, #28209.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license