Skip to content

chore(ec2): deprecate security group name accessor#16116

Merged
mergify[bot] merged 9 commits intoaws:masterfrom
mprencipe:fix-13774
Sep 21, 2021
Merged

chore(ec2): deprecate security group name accessor#16116
mergify[bot] merged 9 commits intoaws:masterfrom
mprencipe:fix-13774

Conversation

@mprencipe
Copy link
Copy Markdown
Contributor

@mprencipe mprencipe commented Aug 18, 2021

The security group name accessor will (almost) always return the security group ID, rather than the name. This change deprecates the name accessor to make it clearer to users that this is not a supported use case.

fixes #13774


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

@gitpod-io
Copy link
Copy Markdown

gitpod-io bot commented Aug 18, 2021

@peterwoodworth peterwoodworth added effort/small Small work item – less than a day of effort p1 @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud labels Aug 19, 2021
njlynch
njlynch previously requested changes Sep 3, 2021
Copy link
Copy Markdown
Contributor

@njlynch njlynch 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 the contribution!

I absolutely agree we should deprecate the property, given how misleading it is. However, I disagree with the suggestion in the issue that we should make it throw. There is a chance that folks have built apps using this field (albeit incorrectly), and the change to make it throw would break them. While logically a small change, it's still a breaking change. I'd prefer we simply deprecate, which will cause it to be removed in the next major version (CDKv2).

@mergify mergify bot dismissed njlynch’s stale review September 6, 2021 06:44

Pull request has been modified.

@mprencipe
Copy link
Copy Markdown
Contributor Author

Hi, thanks for the comment. By making the accessor throw, I was following the advice in the issue:

In fact it seems like it's not possible to obtain a security group's name, so we should probably deprecate the accessor and make it throw.
I've updated the pull request by just marking the property as deprecated.

njlynch
njlynch previously approved these changes Sep 10, 2021
@mergify mergify bot dismissed njlynch’s stale review September 10, 2021 10:51

Pull request has been modified.

@njlynch njlynch changed the title fix: Deprecate security group name accessor and make it throw an error. Fixes issue #13774. fix(ec2): deprecate security group name accessor Sep 10, 2021
@njlynch njlynch changed the title fix(ec2): deprecate security group name accessor chore(ec2): deprecate security group name accessor Sep 10, 2021
njlynch
njlynch previously approved these changes Sep 10, 2021
@mergify mergify bot dismissed njlynch’s stale review September 10, 2021 10:54

Pull request has been modified.

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Sep 21, 2021

Thank you for contributing! Your pull request will be updated from master 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
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 5a2e390
  • 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 ac503cb into aws:master Sep 21, 2021
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Sep 21, 2021

Thank you for contributing! Your pull request will be updated from master 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

@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/small Small work item – less than a day of effort p1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(aws-ec2): securityGroupName returns id of sg and not name

4 participants