feat(ec2): add ICMPv6 protocol #20626
Conversation
|
|
|
@TheRealAmazonKendra , my change has no logic. That file has no corresponding unit test file, and the surrounding (existing) methods have no unit tests. (Unless I missed them.) So, my PR is following the existing convention. Would you please reconsider? Else please advise on a meaningful unit test for this change. |
|
@TheRealAmazonKendra - ping. |
corymhall
changed the title
TheRealAmazonKendra
changed the title
There was a problem hiding this comment.
The addition of new functions must always have tests. We shouldn't have approved these in the past without them. We need to ensure that the template generated as a result of this function is what you expect it to be. Note that the linter will also fail if no README update is made as well. Port Ranges should be updated to reflect this change.
|
Thanks, @TheRealAmazonKendra . I updated the PR with a new unit test and an updated README. |
mergify
bot
dismissed
TheRealAmazonKendra’s stale review
Pull request has been modified.
philipmw
changed the title
TheRealAmazonKendra
changed the title
| describe('Port class', () => { | ||
| describe('.allIcmpV6', () => { | ||
| it('returns expected instance of Port', () => { | ||
| expect(Port.allIcmpV6()).toEqual(new Port({ |
There was a problem hiding this comment.
Let's check this against the template generated, not against the data structure itself.
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
This allows creation of a security group rule that permits ICMPv6.
There was a problem hiding this comment.
My apologies for not being more clear on what I was asking for in the test. A unit test comparing the output to the produced template would have been sufficient but this is great too. It was probably just more work for you than I intended.
Still, thanks for your hard work on this and for contributing!
|
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). |
This allows creation of a security group rule that permits ICMPv6. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This allows creation of a security group rule that permits
ICMPv6.
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license