feat(codebuild): attribute-based compute type for Fleet #32251

badmintoncryer · 2024-11-22T13:30:19Z

Issue # (if applicable)

None

Reason for this change

AWS Codebuild supports for creating Fleet with attribute based compute type.
https://docs.aws.amazon.com/codebuild/latest/userguide/fleets.html#fleets.attribute-compute

You can specify minimum vCPU, disk and memory sizes. Codebuild automatically selects the instance type based on the compute configuration.

Description of changes

Add computeConfiguraion prop to FleetProps.

Description of how you validated changes

Add both unit and integ tests.

Checklist

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

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.

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

codecov · 2024-11-22T13:53:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.54%. Comparing base (cf18cf6) to head (30f86e7).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32251   +/-   ##
=======================================
  Coverage   80.54%   80.54%           
=======================================
  Files         106      106           
  Lines        6954     6954           
  Branches     1287     1287           
=======================================
  Hits         5601     5601           
  Misses       1175     1175           
  Partials      178      178

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

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

Components	Coverage Δ
packages/aws-cdk	`80.54% <ø> (ø)`

badmintoncryer · 2024-11-28T10:43:24Z

CodeQL job has failed, but this is caused by automatically generated test code and I think this is not a problem.

nmussy · 2024-12-19T05:02:02Z

I swear I searched for issues and PRs before starting #32574 🤦 I'll close mine and give yours a review today

nmussy

Had a quick first look by comparing our implementations, I'll try again with a more neutral outlook later. Thanks!

packages/aws-cdk-lib/aws-codebuild/lib/compute-type.ts

nmussy · 2024-12-19T05:10:47Z

packages/aws-cdk-lib/aws-codebuild/lib/fleet.ts

+        disk: props.computeConfiguration.disk?.toGibibytes(),
+        machineType: props.computeConfiguration.machineType,
+        memory: props.computeConfiguration.memory?.toGibibytes(),
+        vCpu: props.computeConfiguration.vCpu,


Despite what the CloudFormation documentation says, the numeric props are not optional, see https://github.com/nmussy/aws-cdk/blob/6120d8f54103f81c4ec601221c1f56e5a4549236/packages/%40aws-cdk-testing/framework-integ/test/aws-codebuild/test/integ.project-fleet-attribute-based-compute.ts#L67-L73

Could you give them a default value, like this for example, and add an integ with just { machineType: GENERAL }?

nmussy · 2024-12-19T05:13:14Z

packages/aws-cdk-lib/aws-codebuild/lib/fleet.ts

@@ -138,13 +196,37 @@ export class Fleet extends Resource implements IFleet {
      throw new Error('baseCapacity must be greater than or equal to 1');
    }

+    if (props.computeConfiguration) {


I also had this validation to make sure the user saw how ATTRIBUTE_BASED_COMPUTE worked, leaving it up to you if you want to add it as well:

Suggested change

if (props.computeConfiguration) {

if (props.computeType === FleetComputeType.ATTRIBUTE_BASED &&

(!props.computeConfiguration || Object.keys(props.computeConfiguration).length === 0)) {

throw new Error('At least one compute configuration criteria must be specified if computeType is "ATTRIBUTE_BASED"');

}

if (props.computeConfiguration) {

nmussy · 2024-12-19T05:17:55Z

packages/aws-cdk-lib/aws-codebuild/lib/fleet.ts

You're missing a getter for the fromFleetArn Import class:

public get computeConfiguration(): FleetComputeConfiguration | undefined { throw new Error('Cannot retrieve computeConfiguration property from an imported Fleet'); }

nmussy

Additional nits

packages/aws-cdk-lib/aws-codebuild/README.md

nmussy · 2024-12-19T15:50:42Z

Something I was lacking as well was the ability to communicate to the user the exact instance type that will be selected. From what I could tell, it's only given out by the web console, and cannot be retrieved via the API, prior or post fleet creation. Even if we reverse engineered the selection algorithm, we would encounter the following issues:

The availability of instance type is region dependent, see docs
I'm pretty certain the instance types are also account dependent and require contacting the AWS support, as I was getting these kinds of error
The web console selection algorithm is incorrect, or at least differs from what the CloudFormation instance selection is, see this example
And we would need to maintain these instance types/criteria, as they would become stale

So overall, not great DX, but not much we can do from our end

Co-authored-by: Jimmy Gaussen <[email protected]>

badmintoncryer · 2024-12-20T00:05:36Z

Thanks @nmussy! I'll update my PR later as I'm a bit busy right now.

badmintoncryer · 2024-12-22T15:27:27Z

@nmussy Thank you very much for your thorough review and detailed feedback. I believe I have addressed all the points you mentioned. I plan to check later whether the CI passes successfully.

nmussy

More nits, sorry 😅

nmussy · 2024-12-24T10:53:31Z

packages/aws-cdk-lib/aws-codebuild/lib/fleet.ts

+    if (!Token.isUnresolved(vCpu)) {
+      this.validatePositiveInteger(vCpu, 'vCPU count');
+    }


You could move the Token.isUnresolved check to validatePositiveInteger, it's a little cleaner and it can't hurt the other two to validate them as well

You are absolutely right. I have made the corrections!

nmussy · 2024-12-24T10:55:24Z

packages/aws-cdk-lib/aws-codebuild/lib/fleet.ts

@@ -125,6 +191,7 @@ export class Fleet extends Resource implements IFleet {
  public readonly environmentType: EnvironmentType;

  constructor(scope: Construct, id: string, props: FleetProps) {
+    super(scope, id, { physicalName: props.fleetName });


I just noticed this addition, thanks I'd missed the physicalName in my original implementation. Wouldn't something like this be preferable though, I don't see a reason not to give Resource the rest of the props?

super(scope, id, { ...props, physicalName: props.fleetName });

nmussy · 2024-12-24T10:56:34Z

packages/aws-cdk-lib/aws-codebuild/lib/fleet.ts

+    const diskGiB = computeConfig?.disk?.toGibibytes() ?? 0;
+    const memoryGiB = computeConfig?.memory?.toGibibytes() ?? 0;
+    const vCpu = computeConfig?.vCpu ?? 0;


Probably a good idea to add a comment to explain why we need to have these default values here as well

Thanks. Your PR had an excellent comment, so I used it as it was.

// Despite what the CloudFormation schema says, the numeric properties are not optional. // An undefined value will cause the CloudFormation deployment to fail, e.g. // > Cannot invoke "java.lang.Integer.intValue()" because the return value of "software.amazon.codebuild.fleet.ComputeConfiguration.getMemory()" is null

nmussy · 2024-12-24T10:59:30Z

packages/aws-cdk-lib/aws-codebuild/lib/fleet.ts

+  /**
+   * The amount of disk space of the instance type included in your fleet.
+   *
+   * @default 0


I don't know if it's necessary to tell the user we set the default value to 0, same with the CloudFormation bug explanation in the JSDoc. It's a bit confusing and shouldn't affect them. Despite the workaround, in effect we still have an unset value and the instance is selected according to the other criteria

You're right. I also borrowed the description from your PR.

aws-cdk-automation · 2024-12-27T01:50:51Z

AWS CodeBuild CI Report

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

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

badmintoncryer added 4 commits November 20, 2024 22:08

feat: attribute based compute type

4bd1584

compute configuration

83f2140

add integ test

211d820

add unit test

605fd16

github-actions bot added the p2 label Nov 22, 2024

aws-cdk-automation requested a review from a team November 22, 2024 13:30

github-actions bot added the distinguished-contributor [Pilot] contributed 50+ PRs to the CDK label Nov 22, 2024

aws-cdk-automation previously requested changes Nov 22, 2024

View reviewed changes

update readme

8ee1d1c

badmintoncryer marked this pull request as ready for review November 22, 2024 13:41

Merge branch 'main' into compute-config

4ed820d

badmintoncryer added 2 commits November 22, 2024 23:17

Update README.md

47d1de2

fix readme

c8f21ef

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 22, 2024

badmintoncryer added 4 commits November 23, 2024 09:11

Merge branch 'main' into compute-config

b895761

Merge branch 'main' into compute-config

decf3a3

Merge branch 'main' into compute-config

52b4ef6

Merge branch 'main' into compute-config

1359119

github-actions bot mentioned this pull request Dec 1, 2024

Monthly PR metrics report #32340

Open

badmintoncryer added 4 commits December 2, 2024 13:10

Merge branch 'main' into compute-config

8d1bead

Merge branch 'main' into compute-config

d978959

Merge branch 'main' into compute-config

382571d

Merge branch 'main' into compute-config

fffcf47

nmussy mentioned this pull request Dec 19, 2024

feat(codebuild): fleet attribute-based compute type #32574

Closed

1 task

nmussy suggested changes Dec 19, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 19, 2024

nmussy suggested changes Dec 19, 2024

View reviewed changes

packages/aws-cdk-lib/aws-codebuild/README.md Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-codebuild/README.md Outdated Show resolved Hide resolved

badmintoncryer and others added 2 commits December 20, 2024 09:03

Update packages/aws-cdk-lib/aws-codebuild/README.md

43b9bfa

Co-authored-by: Jimmy Gaussen <[email protected]>

Update packages/aws-cdk-lib/aws-codebuild/README.md

14842cc

Co-authored-by: Jimmy Gaussen <[email protected]>

badmintoncryer changed the title ~~feat(codebuild): attribute based compute type for Fleet~~ feat(codebuild): attribute-based compute type for Fleet Dec 20, 2024

badmintoncryer added 4 commits December 22, 2024 22:22

update compute type

1679302

set default value

06080d5

update test

483bedb

Merge branch 'main' into compute-config

cc122af

badmintoncryer added 2 commits December 23, 2024 21:06

fix attribute based compute

83da08e

Merge branch 'main' into compute-config

30f86e7

nmussy suggested changes Dec 24, 2024

View reviewed changes

badmintoncryer added 3 commits December 27, 2024 10:06

Merge remote-tracking branch 'origin/main' into compute-config

222731b

update

629680b

fix unit test

6c45f77

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(codebuild): attribute-based compute type for Fleet #32251

feat(codebuild): attribute-based compute type for Fleet #32251

badmintoncryer commented Nov 22, 2024

aws-cdk-automation left a comment

codecov bot commented Nov 22, 2024 •

edited

Loading

badmintoncryer commented Nov 28, 2024

nmussy commented Dec 19, 2024

nmussy left a comment

nmussy Dec 19, 2024

nmussy Dec 19, 2024

nmussy Dec 19, 2024

nmussy left a comment

nmussy commented Dec 19, 2024 •

edited

Loading

badmintoncryer commented Dec 20, 2024

badmintoncryer commented Dec 22, 2024

nmussy left a comment

nmussy Dec 24, 2024

badmintoncryer Dec 27, 2024

nmussy Dec 24, 2024

nmussy Dec 24, 2024

badmintoncryer Dec 27, 2024

nmussy Dec 24, 2024

badmintoncryer Dec 27, 2024

aws-cdk-automation commented Dec 27, 2024

-    if (props.computeConfiguration) {
+    if (props.computeType === FleetComputeType.ATTRIBUTE_BASED &&
+      (!props.computeConfiguration || Object.keys(props.computeConfiguration).length === 0)) {
+      throw new Error('At least one compute configuration criteria must be specified if computeType is "ATTRIBUTE_BASED"');
+    }
+    if (props.computeConfiguration) {

feat(codebuild): attribute-based compute type for Fleet #32251

Are you sure you want to change the base?

feat(codebuild): attribute-based compute type for Fleet #32251

Conversation

badmintoncryer commented Nov 22, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

badmintoncryer commented Nov 28, 2024

nmussy commented Dec 19, 2024

nmussy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmussy left a comment

Choose a reason for hiding this comment

nmussy commented Dec 19, 2024 • edited Loading

badmintoncryer commented Dec 20, 2024

badmintoncryer commented Dec 22, 2024

nmussy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Dec 27, 2024

AWS CodeBuild CI Report

codecov bot commented Nov 22, 2024 •

edited

Loading

nmussy commented Dec 19, 2024 •

edited

Loading