feat(codebuild): add ability to customize build status reporting for third-party Git sources #19408

Lilja · 2022-03-15T18:32:58Z

The idea is to be able to customise the Github "check" through the context parameter I linked in the AWS doc.

#19403

gitpod-io · 2022-03-15T18:33:01Z

skinny85

Thanks for the PR @Lilja, but I don't love the current modeling of these capabilities.

Here are the docs of these properties:

        /**
         * Specifies the context of the build status CodeBuild sends to the source provider. The usage of this parameter depends on the source provider.
         *
         * - **Bitbucket** - This parameter is used for the `name` parameter in the Bitbucket commit status. For more information, see [build](https://docs.aws.amazon.com/https://developer.atlassian.com/bitbucket/api/2/reference/resource/repositories/%7Bworkspace%7D/%7Brepo_slug%7D/commit/%7Bnode%7D/statuses/build) in the Bitbucket API documentation.
         * - **GitHub/GitHub Enterprise Server** - This parameter is used for the `context` parameter in the GitHub commit status. For more information, see [Create a commit status](https://docs.aws.amazon.com/https://developer.github.com/v3/repos/statuses/#create-a-commit-status) in the GitHub developer guide.
         *
         * @link http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-codebuild-project-buildstatusconfig.html#cfn-codebuild-project-buildstatusconfig-context
         */
        readonly context?: string;
        /**
         * Specifies the target url of the build status CodeBuild sends to the source provider. The usage of this parameter depends on the source provider.
         *
         * - **Bitbucket** - This parameter is used for the `url` parameter in the Bitbucket commit status. For more information, see [build](https://docs.aws.amazon.com/https://developer.atlassian.com/bitbucket/api/2/reference/resource/repositories/%7Bworkspace%7D/%7Brepo_slug%7D/commit/%7Bnode%7D/statuses/build) in the Bitbucket API documentation.
         * - **GitHub/GitHub Enterprise Server** - This parameter is used for the `target_url` parameter in the GitHub commit status. For more information, see [Create a commit status](https://docs.aws.amazon.com/https://developer.github.com/v3/repos/statuses/#create-a-commit-status) in the GitHub developer guide.
         *
         * @link http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-codebuild-project-buildstatusconfig.html#cfn-codebuild-project-buildstatusconfig-targeturl
         */
        readonly targetUrl?: string;

So, what should happen is:

The properties used for creating the BitBucketSource, BitBucketSourceProps, should get a new, optional property called name, of type string.
The properties for creating the GitHubSource and GitHubEnterpriseSource will need to get a new, optional property, called context, of type string. (You might have to introduce a common superinterface to share between GitHubEnterpriseSourceProps and GitHubSourceProps.
ThirdPartyGitSourceProps should get a new, optional property, called targetUrl, of type string.
You fill the buildStatusConfig property of the SourceProperty interface that's returned from the bind() method of all of these Sources (through the sourceProperty property of the SourceConfig returned from bind()).

Let me know if this makes sense!

Thanks,
Adam

skinny85 · 2022-03-31T00:57:07Z

Maybe buildStatusName and buildStatusContext and buildStatusUrl are better names for the properties.

Feel free to propose better names, of course 🙂.

Pull request has been modified.

Lilja · 2022-03-31T13:20:56Z

Hey @skinny85. Thanks for the feedback. I've tried to adress it in my latest commit.

I've added GithubContextProps that is a new interface between github and github enterprise.
The buildStatusConfig prop is created in ThirdPartyGitSource, I didn't exactly understand how you would construct it in the SourceProvider since it doesn't have any super config of the more concrete Sources, so it doesn't really know the context/targetUrl.
I don't really know how to do the defaults here. It's probably not a good idea to require these to be sent in the props of the concrete github/bitbucket sources, so buildStatusName: string should probably be buildStatusName?: string. The problem with this is that there is some tooling in this library that requires @default documentation of something that is optional. I have no clue what the actual default value is. You can see it in this PR, as name is AWS CodeBuild us-east-1 (AutoBuildProject89A8053A-LhjRyN9kxr8o). So it would seem like the default of context would be something like AWS Codebuild $AWS_REGION ($SOME_SORT_OF_ID), but I'm only guessing here. Should we really write a default here when in reality, not supplying buildStatusConfig makes the CodeBuild API pick some very opaque default.

skinny85

We're getting there, thanks for all of your work on this @Lilja!

I hope my review answers all of your questions, if something is still unclear, let me know!

skinny85 · 2022-03-31T20:18:30Z

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

+  /**
+ * This parameter is used for the `url`/`target_url` parameter in the Bitbucket/GitHub commit status.
+ * BitBucket: `url`
+ * Github: `target_url`
+ *
+ * @default ???????????
+ */


Docs are misaligned here:

Suggested change

/**

* This parameter is used for the `url`/`target_url` parameter in the Bitbucket/GitHub commit status.

* BitBucket: `url`

* Github: `target_url`

*

* @default ???????????

*/

/**

* This parameter is used for the `url`/`target_url` parameter in the Bitbucket/GitHub commit status.

* BitBucket: `url`

* Github: `target_url`

*

* @default ???????????

*/

I don't think we need to talk about what exact property this fills for each type. It should be obvious from the name.

Let's change the docs to say something like "The URL that the build will report back to the source provider. Can use built-in CodeBuild variables, like $AWS_REGION". Something like that.

Let's also add an @example, and make @default say something like "- link to the AWS Console for CodeBuild to a particular build execution".

skinny85 · 2022-03-31T20:37:01Z

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

+/**
+ * wip
+ */
+export interface GithubContextProps extends ThirdPartyGitSourceProps {


This name isn't the best long-term I'm fairly sure. Let's change it to CommonGitHubSourceProps.

Also, let's not make it exported (let's keep it module-private).

skinny85 · 2022-03-31T20:40:21Z

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

+/**
+ * wip
+ */


Once you make this interface not exported anymore, you'll be able to remove these JSDocs.

skinny85 · 2022-03-31T20:41:37Z

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

+    const buildStatusConfig = {
+        targetUrl: this.buildStatusUrl
+    }
+    if (typeof superConfig["buildStatusContext"] !== "undefined") {
+        // Github/GitHubEnterprise
+        buildStatusConfig["context"] = superConfig["buildStatusContext"]
+    } else if (typeof superConfig["buildStatusName"] !== "undefined") {
+        // BitBucket
+        buildStatusConfig["context"] = superConfig["buildStatusName"]
+    }


Since we need to handle buildStatusConfig separately in the two subclasses anyway, let's just remove all of this code.

skinny85 · 2022-03-31T20:42:00Z

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


    return {
      sourceProperty: {
        ...superConfig.sourceProperty,
        reportBuildStatus: this.reportBuildStatus,
+        buildStatusConfig,


Let's remove this too (will be handled by the subclasses).

skinny85 · 2022-03-31T20:48:36Z

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

+  /**
+   * This parameter is used for the `context` parameter in the GitHub commit status.
+   *
+   * @example 'My build #$CODEBUILD_BUILD_NUMBER'


Let's use double quotes for the example, renders better in other languages.

skinny85 · 2022-03-31T20:51:05Z

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

+        buildStatusConfig: {
+            context: this.buildStatusContext,
+            targetUrl: this.buildStatusUrl,
+        },


Seems like we need to introduce a new, module-private superclass here, CommonGitHubSource, that deals with the buildStatusConfig property, so that this code is not duplicated between GitHubEnterpriseSource and GitHubSource.

skinny85 · 2022-03-31T20:53:28Z

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

+  /**
+   * This parameter is used for the `name` parameter in the Bitbucket commit status.
+   *
+   * @example 'My build #$CODEBUILD_BUILD_NUMBER'
+   */


Again, let's brush up on the docs here: "The name that the build will report back to the BitBucket source provider. Can use built-in CodeBuild variables, like $AWS_REGION". Something like that.

Also, double quotes for @example.

And @default "AWS CodeBuild $AWS_REGION ($PROJECT_NAME)".

skinny85 · 2022-03-31T20:56:02Z

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

@@ -776,6 +824,7 @@ export interface BitBucketSourceProps extends ThirdPartyGitSourceProps {
 class BitBucketSource extends ThirdPartyGitSource {
  public readonly type = BITBUCKET_SOURCE_TYPE;
  private readonly httpsCloneUrl: any;
+  private readonly buildStatusName: string;


We need to assign this field in the constructor.

Then, in bind(), do:

return { sourceProperty: { ...superConfig.sourceProperty, location: this.httpsCloneUrl, buildStatusConfig: { context: this.buildStatusName, targetUrl: this.buildStatusUrl, }, }, sourceVersion: superConfig.sourceVersion, buildTriggers: superConfig.buildTriggers, };

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

Pull request has been modified.

skinny85

Looks great @Lilja, but you need to fix the formatting to get the PR build to succeed, so we can merge this in!

skinny85 · 2022-04-05T16:56:11Z

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

@@ -499,6 +499,14 @@ interface ThirdPartyGitSourceProps extends GitSourceProps {
   * @default every push and every Pull Request (create or update) triggers a build
   */
  readonly webhookFilters?: FilterGroup[];
+
+  /**
+   * This parameter is used for the `url`/`target_url` parameter in the Bitbucket/GitHub commit status.


Again - I don't think we need to specify exactly which parameter this fills. I think it's obvious from the name.

Suggested change

* This parameter is used for the `url`/`target_url` parameter in the Bitbucket/GitHub commit status.

* The URL that the build will report back to the source provider.

* Can use built-in CodeBuild variables, like $AWS_REGION.

skinny85 · 2022-04-05T17:01:51Z

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

+  /**
+   * This parameter is used for the `url`/`target_url` parameter in the Bitbucket/GitHub commit status.
+   *
+   * @example "The URL that the build will report back to the source provider. Can use built-in CodeBuild variables, like $AWS_REGION"


This is a very weird example 😃.

Suggested change

* @example "The URL that the build will report back to the source provider. Can use built-in CodeBuild variables, like $AWS_REGION"

* @example "$CODEBUILD_PUBLIC_BUILD_URL"

skinny85 · 2022-04-05T17:02:57Z

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

+   * This parameter is used for the `url`/`target_url` parameter in the Bitbucket/GitHub commit status.
+   *
+   * @example "The URL that the build will report back to the source provider. Can use built-in CodeBuild variables, like $AWS_REGION"
+   * @default "link to the AWS Console for CodeBuild to a particular build execution"


No need for the quotes here:

Suggested change

* @default "link to the AWS Console for CodeBuild to a particular build execution"

* @default - link to the AWS Console for CodeBuild to a particular build execution

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

skinny85 · 2022-04-05T17:17:44Z

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

+        new codebuild.Project(stack, 'MyProject', {
+          source,
+        })
+        Template.fromStack(stack).hasResourceProperties(
+            'AWS::CodeBuild::Project', {
+                'Source': {
+                    'buildStatusConfig': {
+                        'context': context,
+                    }
+                }
+            }
+        )
+    })


Same comments here (inline source, wrong formatting).

skinny85 · 2022-04-05T17:17:55Z

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

+        new codebuild.Project(stack, 'MyProject', {
+          source,
+        })
+        Template.fromStack(stack).hasResourceProperties(
+            'AWS::CodeBuild::Project', {
+                'Source': {
+                    'buildStatusConfig': {
+                        'context': context,
+                        'targetUrl': targetUrl,
+                    }
+                }
+            }
+        )


Same comments here (inline source, wrong formatting).

skinny85 · 2022-04-05T17:18:11Z

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

+})
+
+describe('sources with customised build status configuration', () => {
+    test('github with targetUrl', () => {


Suggested change

test('github with targetUrl', () => {

test('GitHub with targetUrl', () => {

skinny85 · 2022-04-05T17:18:43Z

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

+            'AWS::CodeBuild::Project', {
+                'Source': {
+                    'buildStatusConfig': {
+                        'context': context,


Since this test checks context too, can you actually remove the first test, and just leave this one? The first test is redundant currently.

Good point, my thought was to have a unit test only having either one of those parameters. The tests above only uses context and name, so my thought was to create a unit test with only targetUrl.

skinny85 · 2022-04-05T17:19:03Z

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

+    })
+})


Missing semiicolons:

Suggested change

})

})

});

});

Pull request has been modified.

skinny85

We're almost there!

skinny85 · 2022-04-07T18:08:43Z

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

@@ -499,6 +499,17 @@ interface ThirdPartyGitSourceProps extends GitSourceProps {
   * @default every push and every Pull Request (create or update) triggers a build
   */
  readonly webhookFilters?: FilterGroup[];
+
+  /**
+   * The URL that the build will report back to the source provider. Can use built-in CodeBuild variables, like $AWS_REGION


Multi-line, and full sentences, please:

Suggested change

* The URL that the build will report back to the source provider. Can use built-in CodeBuild variables, like $AWS_REGION

* The URL that the build will report back to the source provider.

* Can use built-in CodeBuild variables, like $AWS_REGION.

skinny85 · 2022-04-07T18:14:49Z

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

+        buildStatusConfig: {
+          context: this.buildStatusContext,
+          targetUrl: this.buildStatusUrl,
+        },


We need to render this property only if at least one of this.buildStatusContext or this.buildStatusUrl is defined:

Suggested change

buildStatusConfig: {

context: this.buildStatusContext,

targetUrl: this.buildStatusUrl,

},

buildStatusConfig: this.buildStatusContext !== undefined || this.buildStatusUrl !== undefined

? {

context: this.buildStatusContext,

targetUrl: this.buildStatusUrl,

}

: undefined,

This is why the integration test is failing in the build right now:

@aws-cdk/aws-codebuild: Verifying integ.github-webhook-batch.js against integ.github-webhook-batch.expected.json ... CHANGED. @aws-cdk/aws-codebuild: Resources @aws-cdk/aws-codebuild: [~] AWS::CodeBuild::Project MyProject39F7B0AE @aws-cdk/aws-codebuild: â””â”€ [~] Source @aws-cdk/aws-codebuild: â””â”€ [+] Added: .BuildStatusConfig @aws-cdk/aws-codebuild: Verifying integ.github.js against integ.github.expected.json ... CHANGED. @aws-cdk/aws-codebuild: Resources @aws-cdk/aws-codebuild: [~] AWS::CodeBuild::Project MyProject39F7B0AE @aws-cdk/aws-codebuild: â””â”€ [~] Source @aws-cdk/aws-codebuild: â””â”€ [+] Added: .BuildStatusConfig

skinny85 · 2022-04-07T18:17:47Z

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

+        buildStatusConfig: {
+          context: this.buildStatusName,
+          targetUrl: this.buildStatusUrl,
+        },


Same here - we need to render this property only if at least one of this.buildStatusName or this.buildStatusUrl is defined:

Suggested change

buildStatusConfig: {

context: this.buildStatusName,

targetUrl: this.buildStatusUrl,

},

buildStatusConfig: this.buildStatusName !== undefined || this.buildStatusUrl !== undefined

? {

context: this.buildStatusName,

targetUrl: this.buildStatusUrl,

}

: undefined,

Pull request has been modified.

Lilja · 2022-04-13T17:42:12Z

Thanks for being so patient with me @skinny85!

Fixed compliance with earlier test cases and rebased the branch. Now, there's only the remaining test case of testing the target_url. Is it interesting or should we remove it completely?

skinny85

Last 3 cosmetic changes, and this will be merged in!

Fixed compliance with earlier test cases and rebased the branch. Now, there's only the remaining test case of testing the target_url. Is it interesting or should we remove it completely?

Nah, we're good. Leave it as-is.

Thanks for being so patient with me @skinny85!

My pleasure, thanks for all of your hard work in contributing this feature!

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

skinny85

Looks great @Lilja!

mergify · 2022-04-14T16:15:49Z

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 · 2022-04-14T16:59:42Z

AWS CodeBuild CI Report

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

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

mergify · 2022-04-14T16:59:57Z

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

…third-party Git sources (aws#19408) [Documentation](https://docs.aws.amazon.com/codebuild/latest/userguide/create-project-cli.html#cli.source.buildstatusconfig.context) The idea is to be able to customise the Github "check" through the context parameter I linked in the AWS doc. aws#19403

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

github-actions bot assigned skinny85 Mar 15, 2022

skinny85 previously requested changes Mar 31, 2022

View reviewed changes

skinny85 previously requested changes Apr 5, 2022

View reviewed changes

skinny85 changed the title ~~feat(aws-codebuild): Add buildStatusConfig to ThirdPartySourceProps~~ feat(codebuild): add ability to customize build status reporting for third-party Git sources Apr 5, 2022

github-actions bot added the p2 label Apr 5, 2022

skinny85 added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Apr 5, 2022

skinny85 previously requested changes Apr 7, 2022

View reviewed changes

feat(aws-codebuild): Add buildStatusConfig to ThirdPartySourceProps

8594731

skinny85 reviewed Apr 13, 2022

View reviewed changes

packages/@aws-cdk/aws-codebuild/lib/source.ts Show resolved Hide resolved

packages/@aws-cdk/aws-codebuild/lib/source.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-codebuild/lib/source.ts Show resolved Hide resolved

correct arg syntax, pr comments

52972d4

skinny85 approved these changes Apr 14, 2022

View reviewed changes

Merge branch 'master' into master

fe25f02

mergify bot merged commit 423d72f into aws:master Apr 14, 2022

	* This parameter is used for the `url`/`target_url` parameter in the Bitbucket/GitHub commit status.
	* The URL that the build will report back to the source provider.
	* Can use built-in CodeBuild variables, like $AWS_REGION.

	* @example "The URL that the build will report back to the source provider. Can use built-in CodeBuild variables, like $AWS_REGION"
	* @example "$CODEBUILD_PUBLIC_BUILD_URL"

	* @default "link to the AWS Console for CodeBuild to a particular build execution"
	* @default - link to the AWS Console for CodeBuild to a particular build execution

	test('github with targetUrl', () => {
	test('GitHub with targetUrl', () => {

-    })
-})
+    });
+});

feat(codebuild): add ability to customize build status reporting for third-party Git sources #19408

feat(codebuild): add ability to customize build status reporting for third-party Git sources #19408

Conversation

Lilja commented Mar 15, 2022 • edited

gitpod-io bot commented Mar 15, 2022

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 commented Mar 31, 2022

Lilja commented Mar 31, 2022

skinny85 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

skinny85 Mar 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 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

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Apr 7, 2022 • edited

Choose a reason for hiding this comment

Lilja commented Apr 13, 2022

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Apr 14, 2022

aws-cdk-automation commented Apr 14, 2022

AWS CodeBuild CI Report

mergify bot commented Apr 14, 2022

Lilja commented Mar 15, 2022 •

edited

skinny85 Mar 31, 2022 •

edited

skinny85 Apr 7, 2022 •

edited