feat(codebuild): add support for setting a BuildEnvironment Certificate by mneil · Pull Request #15738 · aws/aws-cdk

mneil · 2021-07-23T16:16:00Z

Add certificate as optional parameter to codebuild project
Environment. The certificate option supports including an additional
PEM certificate for on-prem DNS / SSL of codebuild projects.

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

gitpod-io · 2021-07-23T16:16:03Z

rot26 · 2021-07-23T16:25:29Z

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


+  /**
+   * The ARN of the Amazon S3 bucket, path prefix, and object key that contains the
+   * PEM-encoded certificate for the build project


Optional: you could add an example s3 arn.

Example: arn:aws:s3:::bucket_name/key/path/to/ca.pem

rot26

This is something we need at my job. 👍

skinny85

Thanks for the contribution @mneil! Small suggestion on the design of this new API.

Also, we need to add an example to the module's ReadMe file.

skinny85 · 2021-07-23T17:51:05Z

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

+   *
+   * @default - No external certificate is added to the project
+   */
+  readonly certificate?: string;


I'm wondering whether a string is the best way to model this...? Seems a little looks for my taste.

I see 2 possible alternatives:

Introduce a dedicated interface for this property, something like BuildCertificate, that takes in an s3.IBucket, a string for the key, and maybe an optional version?

Use the existing Location interface from the S3 module. There's a small downside that it takes in a bucketName as just a string, but maybe that's fine - we can show an example in the ReadMe of importing a Bucket by ARN, and then using it to pass the bucketName property as bucket.bucketName.

I'll let you decide which option you like more!

Ah thanks! I was looking for something better but couldn't find existing classes that met the need and wasn't sure about adding another interface. I like option 2 better since it already exists.

However, arns do not support versions and the BuildEnvironment certificate at the cloudformation level only accepts a string value that is an arn. It seems like there's no way to support a version at the moment for the certificate. Because of this I'll create a new interface that takes in an IBucket and key only so people don't think they can use a version when it's not supported.

Let me know if I'm wrong about the version? If so I'll use the existing interface. Won't be hard to do either way.

Yeah, let's go with a new interface here.

Pull request has been modified.

mneil · 2021-07-26T16:45:30Z

@skinny85 Any additional feedback? I've added a new interface and documentation.

skinny85 · 2021-07-26T17:25:27Z

I haven't reviewed the latest iteration. Make sure to re-request my review after you're done pushing your changes (there's a button next to my avatar in the top-right corner of the PR page).

skinny85

Looks great @mneil! A few minor comments.

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

skinny85 · 2021-07-26T23:01:16Z

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

    props: ProjectProps,
    projectVars: { [name: string]: BuildEnvironmentVariable } = {}): CfnProject.EnvironmentProperty {

+    let certificate: string | undefined = undefined;


Can we inline this variable? I don't think it adds much.

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

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

Add certificate as optional parameter to codebuild project Environment. The certificate option supports including an additional PEM certificate for on-prem DNS / SSL of codebuild projects.

Pull request has been modified.

mneil · 2021-07-27T15:35:30Z

I should probably stop force pushing so you can see the changes are resolved :). If there's anymore feedback I'll do that and squash later. I just realized I'm clobbering the diff on your requested changes.

skinny85

Looks great @mneil, thanks for the contribution!

mergify · 2021-07-27T16:48: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-07-27T17:26:21Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 99e5fef
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-07-27T22:36:33Z

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

…te (aws#15738) fixes aws#15701 Add certificate as optional parameter to codebuild project Environment. The certificate option supports including an additional PEM certificate for on-prem DNS / SSL of codebuild projects. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

rot26 reviewed Jul 23, 2021

View reviewed changes

rot26 previously approved these changes Jul 23, 2021

View reviewed changes

mneil dismissed rot26’s stale review via 64f3735 July 23, 2021 17:05

mneil force-pushed the codebuild-environment-certificate branch from 2a72d0e to 64f3735 Compare July 23, 2021 17:05

skinny85 self-assigned this Jul 23, 2021

skinny85 previously requested changes Jul 23, 2021

View reviewed changes

mneil force-pushed the codebuild-environment-certificate branch from 64f3735 to 5abe2bd Compare July 23, 2021 19:31

mneil force-pushed the codebuild-environment-certificate branch 2 times, most recently from d7a1d2c to 76a5717 Compare July 23, 2021 19:34

mneil requested a review from skinny85 July 26, 2021 19:40

skinny85 previously requested changes Jul 26, 2021

View reviewed changes

skinny85 changed the title ~~feat(aws-codebuild): Add support for BuildEnvironment Certificate~~ feat(codebuild): add support for BuildEnvironment Certificate Jul 26, 2021

skinny85 changed the title ~~feat(codebuild): add support for BuildEnvironment Certificate~~ feat(codebuild): add support for setting a BuildEnvironment Certificate Jul 26, 2021

feat(aws-codebuild): add support for build environment certificate

fc7fb32

Add certificate as optional parameter to codebuild project Environment. The certificate option supports including an additional PEM certificate for on-prem DNS / SSL of codebuild projects.

mneil force-pushed the codebuild-environment-certificate branch from 76a5717 to fc7fb32 Compare July 27, 2021 15:34

mneil requested a review from skinny85 July 27, 2021 15:36

Remove commented out import

415bc4f

skinny85 previously approved these changes Jul 27, 2021

View reviewed changes

Merge branch 'master' into codebuild-environment-certificate

99e5fef

mergify bot dismissed skinny85’s stale review via 99e5fef July 27, 2021 16:48

skinny85 approved these changes Jul 27, 2021

View reviewed changes

mergify bot merged commit 76fb481 into aws:master Jul 27, 2021

mneil deleted the codebuild-environment-certificate branch July 28, 2021 15:43

Conversation

mneil commented Jul 23, 2021

Uh oh!

gitpod-io bot commented Jul 23, 2021

Uh oh!

rot26 Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

rot26 left a comment

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

skinny85 Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

mneil Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

skinny85 Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

mneil commented Jul 26, 2021

Uh oh!

skinny85 commented Jul 26, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

skinny85 Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mneil commented Jul 27, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jul 27, 2021

Uh oh!

aws-cdk-automation commented Jul 27, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants