-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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(ecs,lambda,rds): specify allowAllOutbound when importing security groups #3833
Conversation
I suspect there's also something to do here: aws-cdk/packages/@aws-cdk/aws-ecs/lib/cluster.ts Lines 585 to 587 in 3a0cde0
but this import takes a |
Pull Request Checklist
|
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.
Test please.
* | ||
* @default true | ||
*/ | ||
readonly securityGroupAllowsAllOutbound?: 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.
I feel like the right thing would have been to specify ISecurityGroup
instead of a security group ID but this ship has sailed...
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.
How about deprecating in favor of a securityGroup: ISecurityGroup
?
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.
Yes!
*/ | ||
readonly securityGroupId: string; | ||
readonly securityGroupId?: string; |
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.
same
BREAKING CHANGE: `securityGroupId: string` replaced by `securityGroup: ISecurityGroup` when importing a cluster/instance in `@aws-cdk/aws-rds`
7477570
to
fe9eba0
Compare
Thank you for contributing! Your pull request is now being automatically merged. |
@eladb I added the |
Thank you for contributing! Your pull request is now being automatically merged. |
Some imported resource were not included in #3386.
BREAKING CHANGE:
securityGroupId: string
replaced bysecurityGroup: ISecurityGroup
when importing a cluster/instance in@aws-cdk/aws-rds
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license