feat(stepfunctions-tasks): support overriding all properties of CodeBuild StartBuild integration

[zxkane]

closes #10302

---

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

[shivlaks]

thanks for getting this started @zxkane - I think rather than directly translate out all of the potential overrides, we should strive to reuse and simplify wherever we can. take a look at some of my initial observations (I did not dive super deep into this one yet) and let me know what you think.

Also adding @skinny85 as a reviewer as he will have additional insights/feedback on how to structure overrides as the owner of the codebuild module

Pull request has been modified.

[zxkane]

@shivlaks @skinny85 any thought for latest changes?

[zxkane]

@shivlaks @skinny85 any update for the recent changes?

[zxkane]

@shivlaks @skinny85 any feedback for the changes of this PR?

[zxkane]

@shivlaks @SomayaB any feedback for the latest changes?

[shivlaks]

@zxkane I have not had a chance to look at the proposed changes. I will likely only be able to take a pass towards the end of the week. stay tuned!

[skinny85]

Thanks for the contribution @zxkane ! This is a very difficult feature, as it involves a very heavy integration with a different module.

I think the main problem here is the duplication between the CodeBuild and StepFunctions-Tasks modules. Here's my proposed solution to this problem.

Extract a (public) interface in @aws-cdk/aws-codebuild that will contain the property that can be overridden in the StartBuild call, let's call it StartBuildOptions. Make the existing ProjectProps and PipelineProjectProps extend that interface (you might do some interface dance because of CommonProjectProps, but I trust you to figure it out - ping me if you can't).

Then, in the @aws-cdk/aws-stepfunctions-tasks module, you create an interface that contains all those things that you can set for StartBuild, but you can't set when creating the Project:

export interface OverrideProjectProps extends codebuild.StartBuildOptions {
  readonly idempotencyToken?: string;

  // ... other properties that you can set in StartBuild,
  // but can't set when creating a Project
}

Then, in CodeBuildStartBuildProps, you add:

  readonly overrides?: OverrideProjectProps;

And the implementation is basically the same as you currently have.

[gitpod-io]

Pull request has been modified.

[zxkane]

Thanks for the contribution @zxkane ! This is a very difficult feature, as it involves a very heavy integration with a different module.

I think the main problem here is the duplication between the CodeBuild and StepFunctions-Tasks modules. Here's my proposed solution to this problem.

Extract a (public) interface in @aws-cdk/aws-codebuild that will contain the property that can be overridden in the StartBuild call, let's call it StartBuildOptions. Make the existing ProjectProps and PipelineProjectProps extend that interface (you might do some interface dance because of CommonProjectProps, but I trust you to figure it out - ping me if you can't).

Then, in the @aws-cdk/aws-stepfunctions-tasks module, you create an interface that contains all those things that you can set for StartBuild, but you can't set when creating the Project:

export interface OverrideProjectProps extends codebuild.StartBuildOptions {
  readonly idempotencyToken?: string;

  // ... other properties that you can set in StartBuild,
  // but can't set when creating a Project
}

Then, in CodeBuildStartBuildProps, you add:

  readonly overrides?: OverrideProjectProps;

And the implementation is basically the same as you currently have.

@skinny85 Adam, thanks for your comments. There are few properties not supported by CodeBuild L2 construct yet, I leave them not supported in overriding properties as well. I think they should be supported in a separated PR. Let me know what you think.

[skinny85]

I absolutely love this! Great job @zxkane !

A few minor comments, mainly stylistic ones. @shivlaks please review as well!

[shivlaks]

@zxkane nice work!!

I think we're missing a few tests for error scenarios added on the overrides in stepfunctions-tasks. otherwise, made some minor suggestions.

Pull request has been modified.

[zxkane]

@shivlaks @skinny85 could you give your feedback for latest changes?

[shivlaks]

@zxkane thanks for all the work on this PR!! looks good to me.

I added a do-not-merge so @skinny85 can also weigh in before merging it.

[shivlaks]

@zxkane - thanks again!! confirmed with @skinny85 that we're good to merge this one in!!

[mergify]

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]

AWS CodeBuild CI Report

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

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

[mergify]

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

[simboel]

Thanks for the contribution @zxkane. I was about to ask if this feature could be implemented 👍

It seems like this Pull Request has been reverted by @shivlaks because of the issue in aws/jsii#2256. Will this Pull Request merged back into master after that issue has been resolved? We are eager to use the new overrides.

[shivlaks]

@siduenho - we're working on it! the idea is to get it merged back in. we're still sorting out whether this can be resolved at the jsii level. if it cannot, I have an alternative in mind to break up the diamond dependency (probably by adding another interface). stay tuned!

[simboel]

Hi @shivlaks, do you have any updates on this topic?

[shivlaks]

@siduenho since we cannot use diamond dependencies as per the related isssue, we'll pick this back up. It will likely need another interface that doesn't duplicate any properties and to break up the diamond.

[simboel]

@shivlaks are there any updates regarding this issue?

We are currently resorting to multiple CodeBuild job instances in order to compensate the missing attributes. Thus adding quite some complexity and manual work to our workflow.

[jesterhazy]

are there any updates on this issue?