feat(aws-codebuild): add `enableBatchBuilds()` to Project #12531

tjenkinson · 2021-01-15T12:33:53Z

In order for a CodeBuild to run in batch mode, a batch service role is needed, as described here in the docs.

Batch builds introduce a new security role in the batch configuration. This new role is required as CodeBuild must be able to call the StartBuild, StopBuild, and RetryBuild actions on your behalf to run builds as part of a batch. Customers should use a new role, and not the same role they use in their build...

At first I thought lets add this by default, but then I realised when BatchConfiguration is set to something, in the aws console the default 'start build' button behaviour changes to start a batch build by default instead :/

So now this adds a new supportBatchBuildType option, which when true adds minimum amount of BatchConfiguration needed for batch builds to run.

I also updated the doc blocks for the webhook option and CodePipeline action option, because users of those also need to set this option. It would be nice to auto-enable this if a webhook or CodeBuild action is configured, but that sounds pretty complicated.

I'm not sure why anyone would need to customise this role, given it appears to only be used internally to do those 3 things, so this PR does not make it configurable. My thinking is that this could be added later if needed, but this PR just gets batch builds working.

In the future if people want control of the other BatchConfiguration options I was thinking these could be added and would require supportBatchBuildType to be true.

related: aws-cloudformation/cloudformation-coverage-roadmap#621

gitpod-io · 2021-01-15T12:33:56Z

skinny85

Thanks a lot for the contribution @tjenkinson !

May I suggest something? 🙂.

Your change currently says, "if you use webhookTriggersBatchBuild, you have to use supportBatchBuildType too". I would posit that that's actually backwards, and what you want is actually: "if you use webhookTriggersBatchBuild, CDK will make sure that that supportBatchBuildType is set!" 🙂.

Given that, I'd suggest the following changes:

Make the buildBatchConfig in the CfnProject be set with a Lazy (like encryptionKey is, for example).
Remove the supportBatchBuildType property from ProjectProps.
Add a method enableBatchBuilds(): BatchBuildConfig | undefined to IProject. It will return undefined for imported Projects, where this cannot be changed. For new Projects, it will create a new Role like you do today, and set the buildBatchConfig private property of the L1 so that the above Lazy can set it. BatchBuildConfig is a very simple new interface that only contains a role property. You should return the newly created Role there, so that customers can update it if necessary. The new method also gives us flexibility in adding an optional role argument to it in the future.
In the ThirdPartyGitSource, if webhookTriggersBatchBuild was set to true, call the enableBatchBuilds() method on the IProject instance it gets in bind().
Similarly in CodeBuildAction: if executeBatchBuild was set to true, call the enableBatchBuilds() method on the IProject it gets in the properties.

Let me know if this description is clear, or if you need any help implementing it!

tjenkinson · 2021-01-15T18:48:47Z

Makes sense to me. Will update.

Pull request has been modified.

tjenkinson · 2021-01-18T15:42:45Z

@skinny85 updated.

I left the supportBatchBuildType option in for now because I think it's something you also might want to enable for triggering a build manually? Otherwise you'd need to construct the project and then immediately call enableBatchBuilds().

Also I made the batchRole a public readonly property because this is then inline with the existing role property, and because of that thought enableBatchBuilds() didn't really need to return anything.

Thoughts?

Finally I updated the action integration test and added a new integration test for a batch webhook trigger.

skinny85

Looks great @tjenkinson ! Some minor comments below. I see where you're coming from, but I have reasons to stay with the design I've initially proposed - I've explained them in the review comments. Hope you agree!

packages/@aws-cdk/aws-codebuild/lib/project.ts

packages/@aws-cdk/aws-codebuild/lib/source.ts

packages/@aws-cdk/aws-codebuild/test/integ.github-webhook-batch.ts

packages/@aws-cdk/aws-codebuild/test/test.codebuild.ts

because it is not set when sources are bound

Pull request has been modified.

tjenkinson · 2021-01-22T15:11:28Z

packages/@aws-cdk/aws-codebuild/lib/project.ts

+/**
+ * The return value for the {@link Project} `enableBatchBuilds()` method.
+ */
+export interface IEnableBatchBuildsReturn {


@skinny85 I named this this instead of BatchBuildConfig because I didn't want it to get confused with the actual buildBatchConfig in L1

Meh. The type on the L1 level is called ProjectBuildBatchConfigProperty, so I'm not too worried about this one being called BatchBuildConfig.

The name is not a coincidence - it's actually a convention we use for return types (see examples [1], [2]).

A name ending in Return seems a little weird given that, so if you could change it, I would appreciate it 🙂.

No problem will update then. Thanks for the examples

skinny85

Looks great @tjenkinson ! One more rename to go, and some clarifying questions. But this will be shipped very soon!

skinny85 · 2021-01-22T18:04:57Z

packages/@aws-cdk/aws-codebuild/lib/project.ts

+/**
+ * The return value for the {@link Project} `enableBatchBuilds()` method.
+ */
+export interface IEnableBatchBuildsReturn {


Meh. The type on the L1 level is called ProjectBuildBatchConfigProperty, so I'm not too worried about this one being called BatchBuildConfig.

The name is not a coincidence - it's actually a convention we use for return types (see examples [1], [2]).

A name ending in Return seems a little weird given that, so if you could change it, I would appreciate it 🙂.

packages/@aws-cdk/aws-codebuild/lib/project.ts

skinny85 · 2021-01-22T18:09:20Z

packages/@aws-cdk/aws-codebuild/lib/project.ts

+          produce: () => this.projectArn,
+        })],
+        actions: ['codebuild:StartBuild', 'codebuild:StopBuild', 'codebuild:RetryBuild'],
+      }));


Don't we need some batch permissions here? Like this? https://github.com/aws/aws-cdk/pull/12181/files?file-filters%5B%5D=.ts#diff-7902b8e1bfd8571028c1c97116a58c4f6e090f92d7df3490e16e12defbfa2fab

I don't think so. The docs don't mention it and it worked when I tested this in a different account.

Think this is so that internally non batch builds can be triggered as part of a batch. Don't think a batch can trigger another batch?

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Pull request has been modified.

skinny85

Thanks so much for the contribution and working on the PR iterations @tjenkinson!

tjenkinson · 2021-01-26T20:04:25Z

No problem thanks for the help! Hopefully this is the last PR we need to get batch builds working 🤞

mergify · 2021-01-26T20:05:15Z

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

aws-cdk-automation · 2021-01-26T20:41:00Z

AWS CodeBuild CI Report

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

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

mergify · 2021-01-26T20:41:23Z

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

github-actions bot added the @aws-cdk/aws-codebuild Related to AWS CodeBuild label Jan 15, 2021

github-actions bot assigned skinny85 Jan 15, 2021

feat(aws-codebuild): add supportBatchBuildType option to Project

8eade4c

tjenkinson force-pushed the codebuild-batch-service-role branch from 20f197e to 8eade4c Compare January 15, 2021 14:03

Tom Jenkinson added 2 commits January 15, 2021 14:08

update executeBatchBuild doc

dddcb69

more docs

9981c8a

tjenkinson changed the title ~~fix(aws-codebuild): configure batch service role~~ feat(aws-codebuild): add supportBatchBuildType option to Project Jan 15, 2021

tjenkinson and others added 2 commits January 15, 2021 14:47

Merge branch 'master' into codebuild-batch-service-role

88339ad

rename stack in integration test

e98e12d

tjenkinson marked this pull request as ready for review January 15, 2021 14:57

Merge branch 'master' into codebuild-batch-service-role

0ec5784

skinny85 previously requested changes Jan 15, 2021

View reviewed changes

Merge branch 'master' into codebuild-batch-service-role

48617a2

tjenkinson marked this pull request as draft January 18, 2021 13:19

add enableBatchBuilds() and auto enable when needed

2aa8ccf

tjenkinson force-pushed the codebuild-batch-service-role branch from a854534 to 2aa8ccf Compare January 18, 2021 15:38

tjenkinson marked this pull request as ready for review January 18, 2021 15:42

tjenkinson requested a review from skinny85 January 18, 2021 15:42

Merge branch 'master' into codebuild-batch-service-role

5275bc3

skinny85 previously requested changes Jan 19, 2021

View reviewed changes

changes from review, and lazy retrieve projectArn

26fde2e

because it is not set when sources are bound

Tom Jenkinson added 2 commits January 22, 2021 14:57

lint

c445ca7

test project.enableBatchBuilds() returns role

769472b

tjenkinson commented Jan 22, 2021

View reviewed changes

tjenkinson requested a review from skinny85 January 22, 2021 15:12

Merge branch 'master' into codebuild-batch-service-role

c5d7ce2

skinny85 previously requested changes Jan 22, 2021

View reviewed changes

Update packages/@aws-cdk/aws-codebuild/lib/project.ts

5481254

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Tom Jenkinson added 3 commits January 26, 2021 17:25

rename to BuildBatchConfig

74d758f

remove roleName from BatchServiceRole

aeda32a

shuffle order

0d7d183

tjenkinson changed the title ~~feat(aws-codebuild): add supportBatchBuildType option to Project~~ feat(aws-codebuild): add enableBatchBuilds() to Project Jan 26, 2021

Merge branch 'master' into codebuild-batch-service-role

1a1244b

tjenkinson requested a review from skinny85 January 26, 2021 18:31

Break the line in ReadMe.md

ecf030d

skinny85 approved these changes Jan 26, 2021

View reviewed changes

Merge branch 'master' into codebuild-batch-service-role

339e0ea

mergify bot merged commit 0568390 into aws:master Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-codebuild): add `enableBatchBuilds()` to Project #12531

feat(aws-codebuild): add `enableBatchBuilds()` to Project #12531

tjenkinson commented Jan 15, 2021 •

edited

Loading

gitpod-io bot commented Jan 15, 2021 •

edited

Loading

skinny85 left a comment

tjenkinson commented Jan 15, 2021

tjenkinson commented Jan 18, 2021

skinny85 left a comment

tjenkinson Jan 22, 2021

skinny85 Jan 22, 2021

tjenkinson Jan 22, 2021

skinny85 left a comment

skinny85 Jan 22, 2021

skinny85 Jan 22, 2021

tjenkinson Jan 22, 2021

skinny85 left a comment

tjenkinson commented Jan 26, 2021

mergify bot commented Jan 26, 2021

aws-cdk-automation commented Jan 26, 2021

mergify bot commented Jan 26, 2021

feat(aws-codebuild): add enableBatchBuilds() to Project #12531

feat(aws-codebuild): add enableBatchBuilds() to Project #12531

Conversation

tjenkinson commented Jan 15, 2021 • edited Loading

gitpod-io bot commented Jan 15, 2021 • edited Loading

skinny85 left a comment

Choose a reason for hiding this comment

tjenkinson commented Jan 15, 2021

tjenkinson commented Jan 18, 2021

skinny85 left a comment

Choose a reason for hiding this comment

tjenkinson Jan 22, 2021

Choose a reason for hiding this comment

skinny85 Jan 22, 2021

Choose a reason for hiding this comment

tjenkinson Jan 22, 2021

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Jan 22, 2021

Choose a reason for hiding this comment

skinny85 Jan 22, 2021

Choose a reason for hiding this comment

tjenkinson Jan 22, 2021

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

tjenkinson commented Jan 26, 2021

mergify bot commented Jan 26, 2021

aws-cdk-automation commented Jan 26, 2021

AWS CodeBuild CI Report

mergify bot commented Jan 26, 2021

feat(aws-codebuild): add `enableBatchBuilds()` to Project #12531

feat(aws-codebuild): add `enableBatchBuilds()` to Project #12531

tjenkinson commented Jan 15, 2021 •

edited

Loading

gitpod-io bot commented Jan 15, 2021 •

edited

Loading