fix(ecs): DeploymentAlarms property is specified for ECS service with CODE_DEPLOY and EXTERNAL deployment controller by tam0ri · Pull Request #26317 · aws/aws-cdk

tam0ri · 2023-07-11T10:48:50Z

From #25840, ECS L2 construct sets the default configuration for the CfnService.deploymentConfiguration.alarms property to:

alarmNames: [],
rollback: false,
enable: false,

However, alarm based rollback feature is only supported for ECS services that use the rolling update (ECS) deployment controller.
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/deployment-alarm-failure.html

Due to this limitation, when deploymentController is set to CODE_DEPLOY or EXTERNAL, creation for the service will fail by conflict with deploymentConfiguration.alarms property.

This PR solves the issue by skipping to set default configuration for the CfnService.deploymentConfiguration.alarms property for CODE_DEPLOY and EXTERNAL deployment controller.

Closes #26307

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

… CODE_DEPLOY and EXTERNAL deployment controller

gitpod-io · 2023-07-11T10:48:58Z

aws-cdk-automation

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.

tam0ri · 2023-07-11T10:52:22Z

Exemption Request for this quick fix. If integration tests and snapshot are still needed, please let me know.

tam0ri · 2023-07-11T13:05:19Z

I checked failed build. I want to take a new snapshot for @aws-cdk-testing/framework-integ/test/aws-codedeploy/test/ecs/integ.deployment-group.ts, but I can't execute integ-runner now. npx lerna run build --scope=@aws-cdk-testing/framework-integ failed with the following error.

$ cdk-build
       
       /workspace/aws-cdk/packages/aws-cdk/lib/commands/migrate.ts
         5:24  error  Unable to resolve path to module '../vendor/noctilucent'  import/no-unresolved
       
       ✖ 1 problem (1 error, 0 warnings)
       
       Error: /workspace/aws-cdk/node_modules/eslint/bin/eslint.js . --ext=.ts --resolve-plugins-relative-to=/workspace/aws-cdk/tools/@aws-cdk/cdk-build-tools/lib exited with error code 1

I usually use devcontainer, so I tried to use Gitpod, but the problem does not solved.

mrgrain · 2023-07-11T13:14:06Z

@tam0ri Is your branch up-to-date? We made a change to how we import noctilucent recently.
Also did you receive any other errors during the build?

mrgrain · 2023-07-11T13:17:12Z

packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

+    } else if (props.deploymentController?.type !== DeploymentControllerType.CODE_DEPLOY &&
+      props.deploymentController?.type !== DeploymentControllerType.EXTERNAL && this.deploymentAlarmsAvailableInRegion()) {


Any particular reason you are checking for the negative here (not CODE_DEPLOY, not EXTERNAL) and not if the controller is ECS?

There is no particular reason. I fix this not to use negative condition.

mrgrain

Code looks good. Thank you! Just the one small question. ~~If you add the test~~ I can run the snapshots for you.

Edit: Just realized you only need a new snapshot. I can do that.

tam0ri · 2023-07-11T13:39:19Z

I pushed the fix. If you have any comment, please feel free to point it out!

Is your branch up-to-date? We made a change to how we import noctilucent recently.

Yes, I already pulled the latest main branch. I'll dig it further more later, but it really helpful that you add a new snapshot.

tam0ri · 2023-07-11T15:50:57Z

I could execute integ-runner on Gitpod, so I added a new snapshot by myself.

The previous snapshot includes the invalid configuration (alarm based rollback + CODE_DEPLOY deployment controller). This led creation failure for CFn stack when I executed integration test.

aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codedeploy/test/ecs/integ.deployment-group.js.snapshot/aws-cdk-codedeploy-ecs-dg.template.json

Lines 445 to 462 in 9fca790

    
           "FargateServiceAC2B3B85": { 
        
            "Type": "AWS::ECS::Service", 
        
            "Properties": { 
        
             "Cluster": { 
        
              "Ref": "EcsCluster97242B84" 
        
             }, 
        
             "DeploymentConfiguration": { 
        
              "Alarms": { 
        
               "AlarmNames": [], 
        
               "Enable": false, 
        
               "Rollback": false 
        
              }, 
        
              "MaximumPercent": 200, 
        
              "MinimumHealthyPercent": 50 
        
             }, 
        
             "DeploymentController": { 
        
              "Type": "CODE_DEPLOY" 
        
             },

So, I created the new snapshot by --disable-update-workflow the option.

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

mergify · 2023-07-12T10:46:20Z

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-cdk-automation · 2023-07-12T11:13:09Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 1bcdeb9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-07-12T11:13:32Z

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

… CODE_DEPLOY and EXTERNAL deployment controller (aws#26317) From aws#25840, ECS L2 construct sets the default configuration for the `CfnService.deploymentConfiguration.alarms` property to: ``` alarmNames: [], rollback: false, enable: false, ``` However, alarm based rollback feature is only supported for ECS services that use the rolling update (ECS) deployment controller. https://docs.aws.amazon.com/AmazonECS/latest/developerguide/deployment-alarm-failure.html Due to this limitation, when deploymentController is set to CODE_DEPLOY or EXTERNAL, creation for the service will fail by conflict with `deploymentConfiguration.alarms` property. This PR solves the issue by skipping to set default configuration for the `CfnService.deploymentConfiguration.alarms` property for CODE_DEPLOY and EXTERNAL deployment controller. Closes aws#26307 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

fix(ecs): DeploymentAlarms property is specified for ECS service with…

0be8efb

… CODE_DEPLOY and EXTERNAL deployment controller

aws-cdk-automation requested a review from a team July 11, 2023 10:49

github-actions bot added bug This issue is a bug. p1 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK labels Jul 11, 2023

aws-cdk-automation previously requested changes Jul 11, 2023

View reviewed changes

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jul 11, 2023

mrgrain reviewed Jul 11, 2023

View reviewed changes

mrgrain requested changes Jul 11, 2023

View reviewed changes

fix not to use negaative conditions

6d44c35

add a new snapshot

3021a97

mrgrain added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jul 11, 2023

tam0ri requested a review from mrgrain July 12, 2023 00:34

mrgrain approved these changes Jul 12, 2023

View reviewed changes

Merge branch 'main' into fix/ecs-alarm-rollback-codedeploy

1bcdeb9

mergify bot merged commit b799c82 into aws:main Jul 12, 2023

github-actions bot mentioned this pull request Aug 1, 2023

Monthly PRs metrics report - July 2023 #26583

Closed

		} else if (props.deploymentController?.type !== DeploymentControllerType.CODE_DEPLOY &&
		props.deploymentController?.type !== DeploymentControllerType.EXTERNAL && this.deploymentAlarmsAvailableInRegion()) {

Conversation

tam0ri commented Jul 11, 2023

Uh oh!

gitpod-io bot commented Jul 11, 2023

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

tam0ri commented Jul 11, 2023

Uh oh!

tam0ri commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrgrain commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrgrain Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

tam0ri Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

mrgrain left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tam0ri commented Jul 11, 2023

Uh oh!

tam0ri commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Jul 12, 2023

Uh oh!

aws-cdk-automation commented Jul 12, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tam0ri commented Jul 11, 2023 •

edited

Loading

mrgrain commented Jul 11, 2023 •

edited

Loading

mrgrain left a comment •

edited

Loading

tam0ri commented Jul 11, 2023 •

edited

Loading