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

fix(ecs): remove default deployment alarm setting #28329

Closed
wants to merge 22 commits into from

Conversation

sumupitchayan
Copy link
Contributor

@sumupitchayan sumupitchayan commented Dec 11, 2023

Originally in this PR #25840, we added default deployment alarm settings to fix an issue where adding deployment alarms, deploying your CFN stack, then removing the deployment alarms from the CFN template, and deploying again WILL NOT remove the deployment alarms from the service.

ECS now already supports default deployment alarm settings. We will remove the default setting of deploymentAlarms. Reason for removing this default behaviour is for region build where the deployment alarm service may not be available in new regions but is set by default by CDK.

Internal ticket tracking V1142791950


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

Signed-off-by: Sumu <sumughan@amazon.com>
Signed-off-by: Sumu <sumughan@amazon.com>
Signed-off-by: Sumu <sumughan@amazon.com>
@aws-cdk-automation aws-cdk-automation requested a review from a team December 11, 2023 14:57
@github-actions github-actions bot added the p2 label Dec 11, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 11, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@sumupitchayan sumupitchayan added pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Dec 11, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review December 11, 2023 15:01

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@sumupitchayan sumupitchayan added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Dec 11, 2023
@paulhcsun paulhcsun added pr/do-not-merge This PR should not be merged at this time. pr-linter/do-not-close The PR linter will not close this PR while this label is present labels Mar 7, 2024
@gracelu0 gracelu0 marked this pull request as ready for review April 4, 2024 23:33
@xazhao xazhao removed pr/do-not-merge This PR should not be merged at this time. pr-linter/do-not-close The PR linter will not close this PR while this label is present labels Apr 18, 2024
Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

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

Can you update the PR description to indicate why we're doing this? This change needs a feature flag, since it's a breaking change.

packages/aws-cdk-lib/aws-ecs/test/base-service.test.ts Outdated Show resolved Hide resolved
@GavinZZ
Copy link
Contributor

GavinZZ commented May 14, 2024

I discussed with @sumupitchayan and I don't think he'll have the capacity to work on this in the near future. I'll discuss with the team and see what we can do to get this merged first.

@GavinZZ GavinZZ requested a review from comcalvi May 14, 2024 23:02
@GavinZZ GavinZZ added p1 and removed pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-integ-test The PR linter will not require integ test changes p2 labels May 14, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation dismissed their stale review May 14, 2024 23:09

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 9396770
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@GavinZZ
Copy link
Contributor

GavinZZ commented May 15, 2024

Going to close this PR since the original author doesn't have the capacity to work on it. I'll take over and create a new PR with feature flag to guarantee backward compatibility.

#30217

@GavinZZ GavinZZ closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants