aws-lambda Added CodeBuild CloudWatch state event by ivanmartos · Pull Request #45490 · DefinitelyTyped/DefinitelyTyped

ivanmartos · 2020-06-15T14:47:37Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change. (Run with npm test.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: <>
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
Include tests for your changes
If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

Related https://docs.aws.amazon.com/codebuild/latest/userguide/sample-build-notifications.html

typescript-bot · 2020-06-15T14:47:41Z

@ivanmartos Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — keep an eye on this comment as I'll be updating it with information as things progress.

Code Reviews

Because this PR edits the configuration file, it can be merged once it's reviewed by a DT maintainer.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
❌ A DT maintainer needs to merge changes which affect module config files

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 45490,
  "author": "ivanmartos",
  "owners": [
    "darbio",
    "skarum",
    "StefH",
    "tobyhede",
    "buggy",
    "y13i",
    "wwwy3y3",
    "OrthoDex",
    "MichaelMarner",
    "daniel-cottone",
    "kostya-misura",
    "coderbyheart",
    "palmithor",
    "daniloraisi",
    "simonbuchan",
    "Haydabase",
    "repl-chris",
    "aneilbaboo",
    "jeznag",
    "louislarry",
    "dpapukchiev",
    "ohookins",
    "trevor-leach",
    "jagregory",
    "dalen",
    "loikg",
    "skyzenr",
    "redlickigrzegorz",
    "juancarbonel",
    "pwmcintyre",
    "alex-bolenok-centralreach",
    "marianzange",
    "apepper",
    "apalumbo",
    "SachinShekhar"
  ],
  "dangerLevel": "ScopedAndConfiguration",
  "headCommitAbbrOid": "7cbc1f3",
  "headCommitOid": "7cbc1f3b3cf2f0087f9e7b78b0df90d4083715a1",
  "mergeIsRequested": false,
  "stalenessInDays": 6,
  "lastCommitDate": "2020-06-17T08:45:35.000Z",
  "lastCommentDate": "2020-06-17T07:33:12.000Z",
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45490/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": true,
  "popularityLevel": "Popular",
  "anyPackageIsNew": false,
  "packages": [
    "aws-lambda"
  ],
  "files": [
    {
      "filePath": "types/aws-lambda/aws-lambda-tests.ts",
      "kind": "test",
      "package": "aws-lambda"
    },
    {
      "filePath": "types/aws-lambda/index.d.ts",
      "kind": "definition",
      "package": "aws-lambda"
    },
    {
      "filePath": "types/aws-lambda/test/codebuild-tests.ts",
      "kind": "test",
      "package": "aws-lambda"
    },
    {
      "filePath": "types/aws-lambda/trigger/codebuild-cloudwatch-state.d.ts",
      "kind": "definition",
      "package": "aws-lambda"
    },
    {
      "filePath": "types/aws-lambda/tsconfig.json",
      "kind": "package-meta",
      "package": "aws-lambda"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-06-17T09:00:48.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

typescript-bot · 2020-06-15T14:47:42Z

🔔 @darbio @skarum @StefH @tobyhede @buggy @y13i @wwwy3y3 @OrthoDex @MichaelMarner @daniel-cottone @kostya-misura @coderbyheart @palmithor @daniloraisi @simonbuchan @Haydabase @repl-chris @aneilbaboo @jeznag @louislarry @dpapukchiev @ohookins @trevor-leach @jagregory @dalen @loikg @skyzenr @redlickigrzegorz @juancarbonel @pwmcintyre @alex-bolenok-centralreach @marianzange @apepper @apalumbo @SachinShekhar - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

typescript-bot · 2020-06-15T16:51:08Z

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊

	master	#45490	diff
Batch compilation
Memory usage (MiB)	72.0	74.9	+4.1%
Type count	10754	10947	+2%
Assignability cache size	2339	2362	+1%

Language service
Samples taken	2501	2366	-5%
Identifiers in tests	2884	2916	+1%
`getCompletionsAtPosition`
Mean duration (ms)	294.2	303.4	+3.1%
Mean CV	9.5%	9.1%
Worst duration (ms)	374.4	381.9	+2.0%
Worst identifier	event	CustomHandler
`getQuickInfoAtPosition`
Mean duration (ms)	295.2	305.6	+3.5%
Mean CV	9.7%	9.6%	-1.3%
Worst duration (ms)	385.8	389.3	+0.9%
Worst identifier	typedCallback	cognitoIdentityPoolId

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

simonbuchan

Thanks for all this work! A few notes and quibbles, but this looks fine to merge overall to me.

types/aws-lambda/index.d.ts

simonbuchan · 2020-06-15T23:21:42Z

types/aws-lambda/trigger/codebuild-cloudwatch-state.d.ts

+    | 'BUILD_GENERAL1_2XLARGE';
+export type CodeBuildEnvironmentVariableType = 'PARAMETER_STORE' | 'PLAINTEXT';
+
+export interface CodeBuildCloudWatchStateEvent {


This should be able to use the CloudWatchEvent<> (?) template, passing in the detail type and detail.

not sure what you mean here. Could you please explain a bit more? I based this PR on the codepipeline-cloudwatch* types. Can't seem to find if there

Sorry, was me being on a phone again. I was referring to the EventBridgeEvent<TDetailType extends string, TDetail> in trigger/eventbridge.d.ts which seems to be what this structure is based on. It's more for users to be able to compose their own custom event structures though.

types/aws-lambda/trigger/codebuild-cloudwatch-state.d.ts

types/aws-lambda/trigger/codebuild-cloudwatch.d.ts

simonbuchan · 2020-06-15T23:33:22Z

types/aws-lambda/test/codebuild-tests.ts

+/* CodeBuild CloudWatch Events
+ * see https://docs.aws.amazon.com/codebuild/latest/userguide/sample-build-notifications.html
+ */
+const CodeBuildCloudWatchStateEvent: CodeBuildCloudWatchStateEvent = {


Ideally tests should be using the handler types, like a user would, but we still have several in this style. It's much less important when there isn't a result type though.

after I gave it a bit of thought it makes more sense to remove those Codebuild specific handlers. In my project I'm currently receiving these types as a message of SNSEvent triggered by CloudWatch event. That means that if I needed to create a handler, I would be just replicating SNS types
I already removed those handlers
Thanks

Sorry, wrote the first review on a phone, so I was a bit terse. I'm referring to writing the test that uses the types as a user would use the types, e.g. writing a handler that reads the fields. This catches a lot of broken changes or unexpected warts in using types.

So for this package that would be writing a handler type, normally something like:

const someHandler: SomeHandler = async (event) => { strOrUndefined = event.someOptionalField; ... };

though it's a bit more complicated with a result type.

I understand what you mean about the test of handler. What I wanted to say is that handler does not directly receive CodeBuildCloudWatchStateEvent, but it receives SNSEvent with record that contains CodeBuildCloudWatchStateEvent as a JSON string in Message

Something like

import { CodeBuildCloudWatchStateEvent, SNSEvent } from 'aws-lambda' export const handleCodebuildEvent = async (event: SNSEvent) => { const record = event.Records[0] const codebuildEvent: CodeBuildCloudWatchStateEvent = JSON.parse(record.Sns.Message) }

I haven't tried integration with EventBridge, only via CloudWatch Event and SNS.
Basically the use case is that you have CloudWatch event defined that checks CodeBuild state changes. Upon event it pushes a message to SNS topic and there is lambda subscribed to this SNS topic. This lambda consumes the messages as in a code of block above.

I already have working monitoring of CodePipelines in the same way - CloudWatch event checks CodePipeline executions and upon event it pushes messages to SNS topic and there is lambda subscribed to that topic

import { CodePipelineCloudWatchActionEvent, SNSEvent } from 'aws-lambda' export const handlePipelineEvent = async (event: SNSEvent) => { const record = event.Records[0] const pipelineEvent: CodePipelineCloudWatchActionEvent = JSON.parse(record.Sns.Message) }

In this use case I'm not sure it makes sense to create a test for handler, cause I would be testing the SNSEvent in the handler, not the CodeBuildCloudWatchStateEvent - since handler receives type of SNSEvent and the CodeBuildCloudWatchStateEvent is just a stringified JSON in the SNSEvent
However if you feel strong about it I can add the test that will contain SNSEvent as an event type of handler and it will contain JSON string of CodeBuildCloudWatchStateEvent and in the end I will just check whether the Message of SNSEvent consists the same stringified JSON

EventBridge is just CloudWatch Events renamed. Presumably you can just send these events straight to lambda: (I haven't messed with anything other than scheduled events), this would be the handler type.

I would somewhat strongly prefer that the test was reading from the event, not creating one, as that catches problems with changes to the structure.

With SNS usage as the test, this might look something like:

export const handlePipelineEvent = async (snsEvent: SNSEvent) => { const event: CodePipelineCloudWatchActionEvent = JSON.parse(snsEvent.Records[0].Sns.Message) str = event.version; str = event.id; // and so on for the rest of the fields, using types from `aws-lambda-tests.ts` where appropriate. }

but obviously, that's quite a decent bit of work, so I'm not going to block on that, much better to have this merged.

For comparison, here's an example of a test parsing data from the event to get further data:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/aws-lambda/test/cloudwatch-tests.ts#L13-L15

Note that unless you're getting this as an event directly in a handler, this type shouldn't have the ...Event suffix, maybe something like ...MessageData? But I think you can and thus should use the EventBridge handler structure.

ok, no problem
I'll modify it and add the tests 👍

simonbuchan · 2020-06-15T23:37:25Z

types/aws-lambda/trigger/codebuild-cloudwatch-state.d.ts

+    | 'BUILD_GENERAL1_MEDIUM'
+    | 'BUILD_GENERAL1_LARGE'
+    | 'BUILD_GENERAL1_2XLARGE';
+export type CodeBuildEnvironmentVariableType = 'PARAMETER_STORE' | 'PLAINTEXT';


I'm not a big fan of calling types "types", but really it's more important to be consistent with the property name, documentation and the rest of the package, in that order, and it's not really that important.

I based the naming on the sdk documentation and there they explicitly use type
https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/CodeBuild.html

typescript-bot · 2020-06-16T07:15:19Z

@simonbuchan Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

simonbuchan

I'm going to put this on hold for a second, as I'm a bit confused about the handler type being removed.

According to my understanding of the CodeBuild notification docs, these structures are what are delivered to SNS, not to Lambda, and this would only be the structure if the SNS template is a passthrough. But other documentation refers to the event being delivered to EventBridge:

In Detail type, choose Basic if you want only the information provided to Amazon EventBridge included in the notification.

But I can't seem to find the documentation for the actual EventBridge stuff that lambda could receive, which is what this package generally is focused on, structures defined by AWS, and for lambda triggers, not just structures that you could happen to use in lambda. I think there are a few examples of structures that are delivered as JSON in an event, but they're pretty rare.

Just to reiterate, this isn't a no: just a check that this does actually make sense for this to be added in this package.

simonbuchan · 2020-06-16T10:13:03Z

types/aws-lambda/test/codebuild-tests.ts

+/* CodeBuild CloudWatch Events
+ * see https://docs.aws.amazon.com/codebuild/latest/userguide/sample-build-notifications.html
+ */
+const CodeBuildCloudWatchStateEvent: CodeBuildCloudWatchStateEvent = {


Sorry, wrote the first review on a phone, so I was a bit terse. I'm referring to writing the test that uses the types as a user would use the types, e.g. writing a handler that reads the fields. This catches a lot of broken changes or unexpected warts in using types.

So for this package that would be writing a handler type, normally something like:

const someHandler: SomeHandler = async (event) => { strOrUndefined = event.someOptionalField; ... };

though it's a bit more complicated with a result type.

simonbuchan · 2020-06-16T10:26:37Z

types/aws-lambda/trigger/codebuild-cloudwatch-state.d.ts

+    | 'BUILD_GENERAL1_2XLARGE';
+export type CodeBuildEnvironmentVariableType = 'PARAMETER_STORE' | 'PLAINTEXT';
+
+export interface CodeBuildCloudWatchStateEvent {


Sorry, was me being on a phone again. I was referring to the EventBridgeEvent<TDetailType extends string, TDetail> in trigger/eventbridge.d.ts which seems to be what this structure is based on. It's more for users to be able to compose their own custom event structures though.

types/aws-lambda/trigger/codebuild-cloudwatch-state.d.ts

types/aws-lambda/trigger/codebuild-cloudwatch.d.ts

typescript-bot · 2020-06-16T10:57:02Z

@ivanmartos One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

…BuildCloudWatchStateHandler

simonbuchan

That was fast! Thanks for putting up with me through this!

typescript-bot · 2020-06-17T10:48:56Z

Updated numbers for you here from 7ab9466.

Comparison details 📊

	master	#45490	diff
Batch compilation
Memory usage (MiB)	73.2	74.2	+1.4%
Type count	10754	11010	+2%
Assignability cache size	2339	2365	+1%

Language service
Samples taken	2405	2461	+2%
Identifiers in tests	2884	3074	+7%
`getCompletionsAtPosition`
Mean duration (ms)	299.9	320.0	+6.7%
Mean CV	9.0%	8.2%
Worst duration (ms)	391.7	510.9	+30.4% 🔸
Worst identifier	iamRolesToOverride	triggerSource
`getQuickInfoAtPosition`
Mean duration (ms)	301.1	318.0	+5.6%
Mean CV	9.0%	8.3%	-7.9%
Worst duration (ms)	396.3	398.3	+0.5%
Worst identifier	Callback	request

Looks like there were a couple significant differences—take a look at worst-case duration for getting completions at a position to make sure everything looks ok.

typescript-bot · 2020-06-24T11:48:36Z

I just published @types/aws-lambda@8.10.57 to npm.

) * added type and handler for CodeBuild Cloudwatch State event * added type and handler for CodeBuild Cloudwatch event * added test for codebuild cloudwatch event * extended definitions block * added SECRETS_MANAGER as environment variable type * removed handlers * removed unnecessary general codebuild types * CodeBuildCloudWatchStateEvent extends EventBridgeEvent and added CodeBuildCloudWatchStateHandler * added general type strArrayOrUndefined for tests * added tests for CodeBuildCloudWatchStateHandler

ivanmartos added 4 commits June 15, 2020 16:42

added type and handler for CodeBuild Cloudwatch State event

d8163c5

added type and handler for CodeBuild Cloudwatch event

1444a06

added test for codebuild cloudwatch event

608717a

extended definitions block

2fdd58c

typescript-bot added Where is GH Actions? GH Actions didn't give a response to this PR Popular package This PR affects a popular package (as counted by NPM download counts). labels Jun 15, 2020

typescript-bot removed the Where is GH Actions? GH Actions didn't give a response to this PR label Jun 15, 2020

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Jun 15, 2020

simonbuchan approved these changes Jun 15, 2020

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Jun 15, 2020

ivanmartos added 2 commits June 16, 2020 09:10

added SECRETS_MANAGER as environment variable type

5754c69

removed handlers

7492030

typescript-bot removed the Owner Approved A listed owner of this package signed off on the pull request. label Jun 16, 2020

simonbuchan suggested changes Jun 16, 2020

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Jun 16, 2020

ivanmartos added 4 commits June 17, 2020 10:44

removed unnecessary general codebuild types

67401e9

CodeBuildCloudWatchStateEvent extends EventBridgeEvent and added Code…

3443f6a

…BuildCloudWatchStateHandler

added general type strArrayOrUndefined for tests

5f2ba1c

added tests for CodeBuildCloudWatchStateHandler

7cbc1f3

typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Jun 17, 2020

simonbuchan approved these changes Jun 17, 2020

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Jun 17, 2020

typescript-bot added Perf: Worse typescript-bot determined that this PR has a negative impact on compilation performance. and removed Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. labels Jun 17, 2020

typescript-bot added the Check Config Changes a module config files label Jun 24, 2020

elibarzilay merged commit 413fe84 into DefinitelyTyped:master Jun 24, 2020

Conversation

ivanmartos commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Reviews

Status

Uh oh!

typescript-bot commented Jun 15, 2020

Uh oh!

typescript-bot commented Jun 15, 2020

Uh oh!

simonbuchan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Jun 16, 2020

Uh oh!

simonbuchan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

typescript-bot commented Jun 16, 2020

Uh oh!

simonbuchan left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Jun 17, 2020

Uh oh!

typescript-bot commented Jun 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ivanmartos commented Jun 15, 2020 •

edited

Loading

typescript-bot commented Jun 15, 2020 •

edited

Loading