-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix: set the correct port for the SES SMTP servie endpoint #34750
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
base: main
Are you sure you want to change the base?
Conversation
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.
(This review is outdated)
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
208f2f8 to
852e448
Compare
|
I restricted the VPC integ test to us-west-2 as the SES endpoint is not available in all subnets of us-east-1. |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Hi @johnf |
|
@johnf |
|
@alvazjor I'll give it another try over the weekend. I was having issues running the tests. Also at least when I run them there are lots of broken tests (at least there were a few weeks back) |
852e448 to
2497045
Compare
|
I'm recreating the integration snapshots now. Please note This is due to the region change mentioned above |
|
@alvazjor I can't manage to get the integration tests to run I keep getting |
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.
(This review is outdated)
|
@johnf I will try to reproduce the change in my end and deploy the tests to see if I also get the same issue |
|
Hi @johnf ! Sorry for the late reply, can you run the integ test with the |
2497045 to
853ccd8
Compare
Done |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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.
Following snapshot needs to be updated:
@aws-cdk-testing/framework-integ: CHANGED aws-ec2/test/integ.vpc-endpoint.lit 4.354s
@aws-cdk-testing/framework-integ: Security Group Changes
@aws-cdk-testing/framework-integ: ┌───┬──────────────────────────────────────────────────────────┬─────┬────────────┬────────────────────────────┐
@aws-cdk-testing/framework-integ: │ │ Group │ Dir │ Protocol │ Peer │
@aws-cdk-testing/framework-integ: ├───┼──────────────────────────────────────────────────────────┼─────┼────────────┼────────────────────────────┤
@aws-cdk-testing/framework-integ: │ + │ ${MyVpcCrossRegionEndpointSecurityGroup371BDCEC.GroupId} │ In │ TCP 443 │ ${MyVpcF9F0CA6F.CidrBlock} │
@aws-cdk-testing/framework-integ: │ + │ ${MyVpcCrossRegionEndpointSecurityGroup371BDCEC.GroupId} │ Out │ Everything │ Everyone (IPv4) │
@aws-cdk-testing/framework-integ: └───┴──────────────────────────────────────────────────────────┴─────┴────────────┴────────────────────────────┘
@aws-cdk-testing/framework-integ: (NOTE: There may be security-related changes not in this list. See https://github.com/aws/aws-cdk/issues/1299)
@aws-cdk-testing/framework-integ:
@aws-cdk-testing/framework-integ: Resources
@aws-cdk-testing/framework-integ: [+] AWS::EC2::SecurityGroup MyVpcCrossRegionEndpointSecurityGroup371BDCEC
@aws-cdk-testing/framework-integ: [+] AWS::EC2::VPCEndpoint MyVpcCrossRegionEndpoint9A9ABB87
Issue # (if applicable)
I didn't create an issue went stright to PR
Reason for this change
The endpoint helper automatically creates a security group with port 443.
The EMAIL_SMPT helper should set this to 587
Description of changes
I've updated the definition to pass in the correct port overriding the default.
I've worked around this in my own code by not using the predefined helper
Describe any new or updated permissions being added
N/A
Description of how you validated changes
Tested by hand
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license