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.

Sign up for GitHub

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

Jump to bottom

fix(aws-ecs-patterns): only create an A record if LB is public #6895

Merged
merged 5 commits into from
Mar 20, 2020
Merged

fix(aws-ecs-patterns): only create an A record if LB is public #6895

merged 5 commits into from
Mar 20, 2020

Conversation

hencrice
Copy link
Contributor

Before this change, we unnecessarily create A record when the load balancer(s) is non public-facing. This commit fixes #6702.

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

@hencrice
fix(aws-ecs-patterns): only create an A record if LB is public
e8195ac 
Before this change, we unnecessarily create A record when the load balancer(s) is non public-facing. This commit fixes #6702.
@hencrice hencrice requested review from iamhopaul123 and bvtujo March 20, 2020 17:22
@hencrice hencrice self-assigned this Mar 20, 2020
@hencrice
Copy link
Contributor Author

For the failed PR linkter check, I'm a bit confused on what kind of tests it's looking for. It only says:

Run ./.github/actions/prlinter
  with:
    check: MANDATORY_CHANGES
  env:
    GITHUB_TOKEN: ***
Creating authenticated GitHub Client
⌛  Fetching PR number 6895
⌛  Fetching files for PR number 6895
⌛  Validating...
##[error]Fixes must contain a change to a test file

@hencrice
Copy link
Contributor Author

For the failed PR linkter check, I'm a bit confused on what kind of tests it's looking for. It only says:

Run ./.github/actions/prlinter
  with:
    check: MANDATORY_CHANGES
  env:
    GITHUB_TOKEN: ***
Creating authenticated GitHub Client
⌛  Fetching PR number 6895
⌛  Fetching files for PR number 6895
⌛  Validating...
##[error]Fixes must contain a change to a test file

Found it. I suppose it just wants me to at least update some tests: https://github.com/aws/aws-cdk/blob/master/tools/prlint/index.js#L59

iamhopaul123
iamhopaul123 previously approved these changes Mar 20, 2020
View reviewed changes
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.

LGTM, thank you for fixing this!

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecs-patterns/lib/base/network-multiple-target-groups-service-base.ts Show resolved Hide resolved
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: e8195ac
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 9edfd74
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot dismissed iamhopaul123’s stale review March 20, 2020 22:45

Pull request has been modified.

@hencrice hencrice requested a review from iamhopaul123 March 20, 2020 22:47
@hencrice hencrice requested a review from piradeepk March 20, 2020 22:53
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 44effbf
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 63e0d6d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: c1a1dad
  • 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 Mar 20, 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 f31f4e1 into aws:master Mar 20, 2020
@hencrice hencrice mentioned this pull request Mar 24, 2020
Merged
@hencrice hencrice mentioned this pull request Apr 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamhopaul123 iamhopaul123 iamhopaul123 approved these changes

@bvtujo bvtujo Awaiting requested review from bvtujo

@piradeepk piradeepk Awaiting requested review from piradeepk

Assignees

@hencrice hencrice

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ARecord is created even with publicLoadBalancer is set to false in ApplicationLoadBalancedFargateService
3 participants
@hencrice @aws-cdk-automation @iamhopaul123