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

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

Merged
merged 9 commits into from
Sep 21, 2021
Merged

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

merged 9 commits into from
Sep 21, 2021

Conversation

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

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