feat(ecs-patterns): add `containerCpu` and `containerMemoryLimitMiB` property to `ApplicationLoadBalancedFargateService`

[ren-yamanashi]

Issue #20638

Closes #20638

Reason for this change

ApplicationLoadBalancedFargateService did not allow you to specify the CPU and memory of the container.

Description of changes

• Add containerCpu property to ApplicationLoadBalancedFargateServiceProps
• Add containerMemoryLimitMiB property to ApplicationLoadBalancedFargateServiceProps

Description of how you validated changes

Added both unit and integration tests.

Checklist

• [x] My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[ren-yamanashi]

@go-to-k

Could you change the PR title from "feat(application-load-balanced-fargate-service): ..." to "feat(ecs-patterns): add containerCpu and containerMemoryLimitMiB property to ApplicationLoadBalancedFargateService"? Because we should specify the module name, not the construct name.

Thank you for the suggestion. I have updated the PR title. Please check the changes at your convenience.

[ren-yamanashi]

@go-to-k

Thanks for the review! I have fixed it, so please check it out!

(A link to the fix commit is provided in reply to the thread.)

[ren-yamanashi]

If the containerCpu and the containerMemoryLimitMiB are larger than (or equal to) the cpu and the memoryLimitMiB, will CloudFormation deploy errors occur? If so, it is good to add validations for them. (and also good to add corresponding unit tests)

for example:

      if (
        props.containerCpu &&
        !Token.isUnresolved(props.containerCpu) &&
        props.cpu &&
        !Token.isUnresolved(props.cpu) &&
        props.containerCpu > props.cpu // or `props.containerCpu >= props.cpu`
      ) {
        throw new Error('containerCpu must be less than or equal to cpu');
      }
      // The same applies to memory.
      // ...

see: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts#L108-L119

As with the validateHealthyPercentage method, it may also be possible to check for values that are not negative. (If CFn fails for negative values.)

@go-to-k

Thank you for your comment!

We have confirmed that the deployment fails in the following cases, so we have add validation.

• If containerCpu is greater than cpu
• If containerCpu is not positive integer
• If containerMemoryLimitMiB is greater than memoryLimitMiB
• if containerMemoryLimitMiB is not positive integer

Please confirm.

[ren-yamanashi]

@go-to-k

We have responded to comments and fixed the implementation, so please confirm!

[ren-yamanashi]

You can resolve threads that have been resolved, as the number of threads has increased (I don't have the permission to do that).

@go-to-k

I resolved threads that have been resolved!

[go-to-k]

I approved it, so it should have a pr/needs-maintainer-review label, but apparently it doesn't. Could you try to pull diffs from the main branch?

[ren-yamanashi]

@go-to-k

I approved it, so it should have a pr/needs-maintainer-review label, but apparently it doesn't. Could you try to pull diffs from the main branch?

When I pulled the main branch, CI build failed...

I'll investigate this failure and comment as soon as the CI build succeeds.

[ren-yamanashi]

@go-to-k

I checked the build logs and it looks like the following tests failed (Detailed log output is described below.)

• diff.test.ts
• cdk-toolkit.test.ts

I wasn't sure if this failure was due to this MR implementation...

Is there a place where the implementation of this MR is insufficient? Please some advice to fix it.

FAIL test/diff.test.ts (13.346 s)
aws-cdk:   笳� imports 窶ｺ imports render correctly for a nonexistant stack without creating a changeset
aws-cdk:     expect(received).toContain(expected) // indexOf
aws-cdk:     Expected substring: "Stack A
aws-cdk:     Parameters and rules created during migration do not affect resource configuration.
aws-cdk:     Resources
aws-cdk:     [竊疹 AWS::SQS::Queue Queue import
aws-cdk:     [竊疹 AWS::SQS::Queue Queue2 import
aws-cdk:     [竊疹 AWS::S3::Bucket Bucket import
aws-cdk:     "
aws-cdk:     Received string:    "Stack A
aws-cdk:     Resources
aws-cdk:     [+] AWS::SQS::Queue Queueﾂｷ
aws-cdk:     [+] AWS::SQS::Queue Queue2ﾂｷ
aws-cdk:     [+] AWS::S3::Bucket Bucketﾂｷﾂｷﾂｷ
aws-cdk:     笨ｨ  Number of stacks with differences: 1
aws-cdk:     "
aws-cdk:       179 |     const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '');
aws-cdk:       180 |     expect(cfn.createDiffChangeSet).not.toHaveBeenCalled();
aws-cdk:     > 181 |     expect(plainTextOutput).toContain(`Stack A
aws-cdk:           |                             ^
aws-cdk:       182 | Parameters and rules created during migration do not affect resource configuration.
aws-cdk:       183 | Resources
aws-cdk:       184 | [竊疹 AWS::SQS::Queue Queue import
aws-cdk:       at Object.<anonymous> (test/diff.test.ts:181:29)
aws-cdk:   笳� imports 窶ｺ imports render correctly for a nonexistant stack without creating a changeset
aws-cdk:     ENOENT: no such file or directory, lstat 'migrate.json'
aws-cdk:       161 |
aws-cdk:       162 |   afterEach(() => {
aws-cdk:     > 163 |     fs.rmSync('migrate.json');
aws-cdk:           |        ^
aws-cdk:       164 |   });
aws-cdk:       165 |
aws-cdk:       166 |   test('imports render correctly for a nonexistant stack without creating a changeset', async () => {
aws-cdk:       at Object.<anonymous> (test/diff.test.ts:163:8)

FAIL test/cdk-toolkit.test.ts (20.524 s)
aws-cdk:   笳� readCurrentTemplate 窶ｺ do not print warnings if lookup role not provided in stack artifact
aws-cdk:     expect(received).toEqual(expected) // deep equality
aws-cdk:     - Expected  - 1
aws-cdk:     + Received  + 1
aws-cdk:       Object {
aws-cdk:     -   "assumeRoleArn": undefined,
aws-cdk:     +   "assumeRoleArn": "bloop:here:123456789012",
aws-cdk:         "assumeRoleExternalId": undefined,
aws-cdk:       }
aws-cdk:       388 |     expect(mockCloudExecutable.sdkProvider.sdk.ssm).not.toHaveBeenCalled();
aws-cdk:       389 |     expect(mockForEnvironment.mock.calls.length).toEqual(2);
aws-cdk:     > 390 |     expect(mockForEnvironment.mock.calls[0][2]).toEqual({
aws-cdk:           |                                                 ^
aws-cdk:       391 |       assumeRoleArn: undefined,
aws-cdk:       392 |       assumeRoleExternalId: undefined,
aws-cdk:       393 |     });
aws-cdk:       at Object.<anonymous> (test/cdk-toolkit.test.ts:390:49)
aws-cdk: Test Suites: 2 failed, 68 passed, 70 total
aws-cdk: Tests:       2 failed, 851 passed, 853 total
aws-cdk: Snapshots:   0 total
aws-cdk: Time:        32.131 s
aws-cdk: Ran all test suites.
aws-cdk: Error: /codebuild/output/src1370003057/src/github.com/aws/aws-cdk/node_modules/jest/bin/jest.js exited with error code 1
aws-cdk: Tests failed. Total time (32.8s) | /codebuild/output/src1370003057/src/github.com/aws/aws-cdk/node_modules/jest/bin/jest.js (32.8s)

[go-to-k]

This would have nothing to do with this PR. However, it would be curious to know if this is just a coincidence or if it happens with other PRs. It might be good to wait and see a bit more.

[ren-yamanashi]

This would have nothing to do with this PR. However, it would be curious to know if this is just a coincidence or if it happens with other PRs. It might be good to wait and see a bit more.

I pulled the main branch again and CI went passed. Thanks you!

[ren-yamanashi]

@go-to-k

PR Linter / validate-pr succeeded!

Run ./tools/@aws-cdk/prlint
  env:
    GITHUB_TOKEN: ***
    PR_NUMBER: 
    PR_SHA: 
    REPO_ROOT: /home/runner/work/aws-cdk/aws-cdk
⌛  Fetching PR number 30920
⌛  Fetching files for PR number 30920
⌛  Validating...
Deleting PR Linter Comment now
✅  Success
CodeBuild Commit Statuses:  [ 'success', 'pending' ]
PR code build job SUCCESSFUL

Regarding the label change, it seems that it has not been approved, so could you please approve?

If the label is still not granted, I will try runnning PR Linter or CodeBuild CI again.

[go-to-k]

I approved, but the PR doesn't have the label.

Could you run PR Linter again?

[go-to-k]

Could you try making some changes to the description of this PR without running CodeBuild CI?

[ren-yamanashi]

@go-to-k

I ran PR Linter without runnning CodeBuild again, but the PR doesn't have the label... Looking at other PRs, it was labeled successfully, but is there a problem with what I submitted?

[go-to-k]

It appears that the status is CHANGES_REQUESTED and the communityApproved is false even though it has been approved: https://github.com/aws/aws-cdk/actions/runs/10669155243/job/29570426536

raw data: 
{
  "go-to-k": {
    "id": 2225629631,
    "node_id": "PRR_kwDOBk6Df86EqGm_",
    "user": {
      "login": "go-to-k",
      "id": 24818752,
      "node_id": "MDQ6VXNlcjI0ODE4NzUy",
      "avatar_url": "https://avatars.githubusercontent.com/u/24818752?u=e284689a061d1e9378ffd7b4e76bdc5320bfc45a&v=4",
      "gravatar_id": "",
      "url": "https://api.github.com/users/go-to-k",
      "html_url": "https://github.com/go-to-k",
      "followers_url": "https://api.github.com/users/go-to-k/followers",
      "following_url": "[https://api.github.com/users/go-to-k/following{/other_user}](https://api.github.com/users/go-to-k/following%7B/other_user%7D)",
      "gists_url": "[https://api.github.com/users/go-to-k/gists{/gist_id}](https://api.github.com/users/go-to-k/gists%7B/gist_id%7D)",
      "starred_url": "[https://api.github.com/users/go-to-k/starred{/owner}{/repo}](https://api.github.com/users/go-to-k/starred%7B/owner%7D%7B/repo%7D)",
      "subscriptions_url": "https://api.github.com/users/go-to-k/subscriptions",
      "organizations_url": "https://api.github.com/users/go-to-k/orgs",
      "repos_url": "https://api.github.com/users/go-to-k/repos",
      "events_url": "[https://api.github.com/users/go-to-k/events{/privacy}](https://api.github.com/users/go-to-k/events%7B/privacy%7D)",
      "received_events_url": "https://api.github.com/users/go-to-k/received_events",
      "type": "User",
      "site_admin": false
    },
    "body": "",
    "state": "CHANGES_REQUESTED",
    "html_url": "https://github.com/aws/aws-cdk/pull/30920#pullrequestreview-2225629631",
    "pull_request_url": "https://api.github.com/repos/aws/aws-cdk/pulls/30920",
    "author_association": "CONTRIBUTOR",
    "_links": {
      "html": {
        "href": "https://github.com/aws/aws-cdk/pull/30920#pullrequestreview-2225629631"
      },
      "pull_request": {
        "href": "https://api.github.com/repos/aws/aws-cdk/pulls/30920"
      }
    },
    "submitted_at": "2024-08-07T16:14:56Z",
    "commit_id": "ef494bd1fe8f15fcb4d5d60e4b804cdfc8123366"
  }
}

evaluation:  {
  "draft": false,
  "mergeable_state": "blocked",
  "maintainerRequestedChanges": false,
  "maintainerApproved": false,
  "communityRequestedChanges": true,
  "communityApproved": false,
  "userRequestsExemption": false
}

Apparently there was too much history to get all the latest data.

https://github.com/aws/aws-cdk/blob/main/tools/@aws-cdk/prlint/lint.ts#L376

  private async assessNeedsReview(
    pr: Pick<GitHubPr, 'mergeable_state' | 'draft' | 'labels' | 'number'>,
  ): Promise<void> {
    const reviews = await this.client.pulls.listReviews(this.prParams);
    console.log(JSON.stringify(reviews.data));

I will consult with the CDK team.

P.S. I will take the problem in the PR: https://github.com/aws/aws-cdk/pull/31290

[ren-yamanashi]

Thank you very much!

[go-to-k]

@ren-yamanashi

Hi, the fix PR has been merged!

https://github.com/aws/aws-cdk/pull/31290

I approved it again and the review label was successfully attached.

[ren-yamanashi]

Thank you for your quick response!

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 80.84%. Comparing base (fd9462c) to head (f6993ab).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #30920   +/-   ##
=======================================
  Coverage   80.84%   80.84%           
=======================================
  Files         236      236           
  Lines       14230    14230           
  Branches     2487     2487           
=======================================
  Hits        11504    11504           
  Misses       2442     2442           
  Partials      284      284           

Flag	Coverage Δ
suite.unit	80.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	79.64% <ø> (ø)
packages/aws-cdk-lib/core	82.14% <ø> (ø)

[aws-cdk-automation]

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

[mergify[bot]]

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

[mergify[bot]]

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

[gracelu0]

@Mergifyio update

[mergify[bot]]

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally. GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/analytics-metadata-updater.yml without workflows permission

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[mergify[bot]]

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

[github-actions[bot]]

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.