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

feat(ecs-patterns): add option to create cname instead of alias record #10812

Merged
merged 10 commits into from
Nov 6, 2020
Merged

feat(ecs-patterns): add option to create cname instead of alias record #10812

merged 10 commits into from
Nov 6, 2020

Conversation

hoegertn
Copy link
Contributor


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

@SomayaB SomayaB added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Oct 12, 2020
@hoegertn
Copy link
Contributor Author

@uttarasridhar Hi, do you see any chance we can get this merged? I have a client that cannot use the patterns module because of this.

Comment on lines 182 to 183
* Specifies whether the Route53 record should be a CNAME or an A record using the Alias feature.
* This is useful if you need to work with DNS systems that do not support alias records.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to create an issue to describe why we need this feature and link this PR to that issue.

The PR LGTM from code perspective. I just don't understand the scenario. Will be great to elaborate it in the issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah agree with @pahud, could you create an issue elaborating the use case that this PR mainly addresses? It would also increase visibility for the other users with similar problem.

@gitpod-io
Copy link

gitpod-io bot commented Oct 30, 2020

Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks great to me! A couple of questions though:

  1. Could you enable this feature in our NLB patterns as well, since we want to keep feature parity if it makes sense.
  2. Would you mind to manually test it works when users specify ALIAS/CNAME/NONE?

@hoegertn
Copy link
Contributor Author

hoegertn commented Nov 5, 2020

I added the feature to NLBs and tested it locally

Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@mergify
Copy link
Contributor

mergify bot commented Nov 5, 2020

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

@hoegertn
Copy link
Contributor Author

hoegertn commented Nov 5, 2020

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Nov 5, 2020

@hoegertn is not allowed to run commands

@iamhopaul123
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Nov 5, 2020

Command update: success

Branch has been successfully updated

@mergify
Copy link
Contributor

mergify bot commented Nov 6, 2020

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: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: fb2650e
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Nov 6, 2020

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

@mergify mergify bot merged commit 89a5055 into aws:master Nov 6, 2020
@hoegertn hoegertn deleted the hoegertn-cname branch November 6, 2020 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants