feat(stepfunctions-tasks): add support for CodeBuild StartBuild API #9757

DaWyz · 2020-08-17T04:21:57Z

Implementation

Update package @aws-cdk/aws-stepfunctions-tasks to include support for CodeBuild StartBuild API as per documentation here: https://docs.aws.amazon.com/step-functions/latest/dg/connect-codebuild.html

Includes support for the following Amazon SageMaker API calls:

StartBuild

Closes #8043

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

shivlaks

hey @DaWyz - this is a valiant effort and we appreciate the contribution.
at a cursory glance, it feels like we are duplicating some of the modeling in the aws-codebuild module.

I'd suggest taking a step back and scaffolding out support for this service integration.
as a first pass, how about we take a pass without overrides modeled yet.

We will need to support them as well, but to keep this PR moving, it would help substantially to introduce the ability to start a build, by providing an IProject and any other properties that are not representative of overrides.

what do you think?

packages/@aws-cdk/aws-stepfunctions-tasks/lib/codebuild/start-build.ts

shivlaks · 2020-08-18T22:19:42Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/codebuild/start-build.ts

+import * as sfn from '@aws-cdk/aws-stepfunctions';
+import * as cdk from '@aws-cdk/core';
+
+import { integrationResourceArn, validatePatternSupported } from '../private/task-utils';


this file feels really large, can we break it down?
would any of this apply for StopBuild - if so we should start moving shared types out.

take a look at the EcsRunTask service integration for inspiration on how to break this down

shivlaks · 2020-08-18T22:20:27Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/codebuild/start-build.ts

+          'codebuild:BatchGetBuilds',
+          'codebuild:BatchGetReports',


does StartBuild need these permissions? when would they be used?

Good point. I will remove them as I don't think they are useful for the startBuild part.
Do I need codebuild:StopBuild ? I was thinking it might be useful when stopping a Step Function execution but I'm not actually sure stopping the execution would try to stop the codebuild (Haven't tested this part and just started working with Step Functions a couple of weeks ago).

DaWyz · 2020-08-18T22:57:18Z

@shivlaks. Thanks for the review.

I was kind of hoping you would say that. I was not really happy with the code of this PR (class too big. It was a pain to implement the _renderTask and I knew it was not a good way to do it).

I like the idea of a smaller PR without having the support for all the override properties. Can I suggest implementing environmentVariablesOverride ? It's a small one and probably one of the most useful (that the only thing I needed for my use case). It would also help me get a guideline on how to implement the rest of the Overrides later on.

What is the best place to get some help if I need to ask for some guidelines on how to implement things (like the bind pattern for the _renderTask. I'm not sure how this works) ?

Only allowing to pass the CodeBuild project and environment variables for now. The rest to come in future iteration.

Pull request has been modified.

DaWyz · 2020-08-20T00:12:03Z

@shivlaks, I updated the PR and reduced the scope to only passing the CodeBuild Project and environmentVariables.

Could you have a look at it again ?

Also, Am I allowed to use the CfnProject class to reuse interfaces ? Like in the _renderTask method:

codebuild.CfnProject.EnvironmentVariableProperty

shivlaks

much cleaner! thanks for simplifying this, it's starting to look pretty close to what i had in mind for a first pass

shivlaks · 2020-08-20T07:24:03Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/codebuild/start-build.ts

+
+

nit: don't think these blank lines are needed

shivlaks · 2020-08-20T07:25:02Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/codebuild/start-build.ts

+   * A set of environment variables that overrides, for this build only,
+   * the latest ones already defined in the build project.
+   *
+   * @default No override


Suggested change

* @default No override

* @default - No overrides

shivlaks · 2020-08-20T07:25:27Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/codebuild/start-build.ts

+  readonly project: codebuild.IProject;
+  /**
+   * A set of environment variables that overrides, for this build only,
+   * the latest ones already defined in the build project.


does this describe the default behaviour? maybe it belongs as part of the @default doc string

shivlaks · 2020-08-20T07:31:21Z

packages/@aws-cdk/aws-stepfunctions-tasks/test/codebuild/integ.start-build.ts

+    let startBuild = new tasks.CodeBuildStartBuild(this, 'build-task', {
+      project: project,
+      environmentVariablesOverride: {
+        ZONE: {


curious: any reason why ZONE is all caps? or is it case insensitive when compared to the originally defined environment variables which calls it zone?

I usually set the environment variables in all caps (e.g.: AWS_REGION, NODE_ENV, etc...). Zone is just a random variable name. Happy to change if you think it's confusing.

shivlaks · 2020-08-20T07:32:14Z

packages/@aws-cdk/aws-stepfunctions-tasks/test/codebuild/integ.start-build.ts

+}
+
+const app = new cdk.App();
+new StartBuildStack(app, 'aws-stepfunctions-integ');


probably better to give this stack name an id that specifies it's a codebuild integ test

shivlaks · 2020-08-20T17:53:10Z

packages/@aws-cdk/aws-stepfunctions-tasks/lib/codebuild/start-build.ts

+        EnvironmentVariablesOverride: this.props.environmentVariablesOverride
+          ? codebuild.Project
+            .serializeEnvVariables(this.props.environmentVariablesOverride)
+            .map((environmentVariable: codebuild.CfnProject.EnvironmentVariableProperty) => {


re: your question around codebuild.CfnProject.EnvironmentVariableProperty - since this is not exposed in the public API / surface area that users can interact with, it's acceptable.

I think it's worth asking whether this is the right mechanism to produce the output we want.
What's the alternative? since the function is pretty small, is it maybe better to avoid converting to this Cfn type. Since the module is stable and it's a public static method, I don't think it's an issue since it's not exposed. what do you think?

The thing that makes me wary is that ultimately we're producing ASL and not CloudFormation here so it feels like a roundabout way to get there.

I see what you mean. I was just trying to reuse the existing serializeEnvVariables from the codebuild.Project class. It's a small method and I can probably go with writing my own and it's going to avoid the round trip.

See the implementation below.

private serializeEnvVariables(environmentVariables: { [name: string]: codebuild.BuildEnvironmentVariable }) { return Object.keys(environmentVariables).map(name => ({ Name: name, Type: environmentVariables[name].type || codebuild.BuildEnvironmentVariableType.PLAINTEXT, Value: environmentVariables[name].value, })); }

Pull request has been modified.

shivlaks

awesome turnaround on this! we really appreciate the contribution!!! 🎉

mergify · 2020-08-21T06:16:05Z

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 · 2020-08-21T06:40:41Z

AWS CodeBuild CI Report

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

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

mergify · 2020-08-21T06:41:04Z

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

DaWyz and others added 5 commits August 16, 2020 20:13

feat(aws-stepfunctions-tasks): Add CodeBuild Start Build Task

d15e4a4

Add missing docstring to start-build.ts

47269ac

Fix missing exported error

31e5c1a

Fix build error (invalid ComputeType)

ac9b3c5

Fixing build issue (GitPod working now)

b1ac5b7

SomayaB assigned shivlaks Aug 17, 2020

Improve integration test (add full list of parameters)

a2d69e6

shivlaks previously requested changes Aug 18, 2020

View reviewed changes

Reduce start-build scope

171f1ad

Only allowing to pass the CodeBuild project and environment variables for now. The rest to come in future iteration.

Update README.md

bdac4ea

DaWyz requested a review from shivlaks August 20, 2020 17:09

shivlaks previously requested changes Aug 20, 2020

View reviewed changes

Remove dependency on CfnProject and fix nits

d6430d6

DaWyz requested a review from shivlaks August 21, 2020 02:54

shivlaks approved these changes Aug 21, 2020

View reviewed changes

Merge branch 'master' into DaWyz/sfn-codebuild-start-build-task

ebdbdbd

mergify bot merged commit dae54ec into aws:master Aug 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stepfunctions-tasks): add support for CodeBuild StartBuild API #9757

feat(stepfunctions-tasks): add support for CodeBuild StartBuild API #9757

DaWyz commented Aug 17, 2020 •

edited by shivlaks

Loading

shivlaks left a comment

shivlaks Aug 18, 2020

shivlaks Aug 18, 2020

DaWyz Aug 18, 2020 •

edited

Loading

DaWyz commented Aug 18, 2020

DaWyz commented Aug 20, 2020

shivlaks left a comment

shivlaks Aug 20, 2020

shivlaks Aug 20, 2020

shivlaks Aug 20, 2020

shivlaks Aug 20, 2020

DaWyz Aug 21, 2020

shivlaks Aug 20, 2020

shivlaks Aug 20, 2020

DaWyz Aug 21, 2020

shivlaks left a comment

mergify bot commented Aug 21, 2020

aws-cdk-automation commented Aug 21, 2020

mergify bot commented Aug 21, 2020

feat(stepfunctions-tasks): add support for CodeBuild StartBuild API #9757

feat(stepfunctions-tasks): add support for CodeBuild StartBuild API #9757

Conversation

DaWyz commented Aug 17, 2020 • edited by shivlaks Loading

shivlaks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaWyz Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

DaWyz commented Aug 18, 2020

DaWyz commented Aug 20, 2020

shivlaks 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

Choose a reason for hiding this comment

shivlaks left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 21, 2020

aws-cdk-automation commented Aug 21, 2020

AWS CodeBuild CI Report

mergify bot commented Aug 21, 2020

DaWyz commented Aug 17, 2020 •

edited by shivlaks

Loading

DaWyz Aug 18, 2020 •

edited

Loading