Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws-cdk-events: jsonTemplate versus textTemplate (take 2) #27

Merged
merged 6 commits into from
Jun 5, 2018
Merged

aws-cdk-events: jsonTemplate versus textTemplate (take 2) #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0016b96
feat(aws-cdk-events): allow tokens in input templates
Jun 3, 2018
7a97742
Allow either strings or objects to be passed
Jun 3, 2018
b202880
Alpha-sort imports
Jun 4, 2018
93aaf3c
Merge remote-tracking branch 'origin/master' into benisrae/events-tem…
Jun 5, 2018
77f914a
jsonTemplate versus textTemplate
Jun 5, 2018
493bea9
squash: Fix a couple of tests
Jun 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/aws-cdk-codebuild/test/integ.project-events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ project.onStateChange('StateChange', topic);
// The phase will be extracted from the "completed-phase" field of the event
// details.
project.onPhaseChange('PhaseChange').addTarget(topic, {
template: `Build phase changed to <phase>`,
textTemplate: `Build phase changed to <phase>`,
pathsMap: {
phase: '$.detail.completed-phase'
}
Expand All @@ -31,7 +31,7 @@ project.onPhaseChange('PhaseChange').addTarget(topic, {
// trigger a build when a commit is pushed to the repo
const onCommitRule = repo.onCommit('OnCommit', project, 'master');
onCommitRule.addTarget(topic, {
template: 'A commit was pushed to the repository <repo> on branch <branch>',
textTemplate: 'A commit was pushed to the repository <repo> on branch <branch>',
pathsMap: {
branch: '$.detail.referenceName',
repo: '$.detail.repositoryName'
Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk-codepipeline/test/integ.pipeline-events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ const repository = new Repository(stack, 'CodeCommitRepo', { repositoryName: 'fo
const project = new BuildProject(stack, 'BuildProject', { source: new CodePipelineSource() });

const sourceAction = new CodeCommitSource(sourceStage, 'CodeCommitSource', { artifactName: 'Source', repository });
new CodeBuildAction(buildStage, 'CodeBuildAction', { source: sourceAction, project });
new CodeBuildAction(buildStage, 'CodeBuildAction', { inputArtifact: sourceAction.artifact, project });

const topic = new Topic(stack, 'MyTopic');
topic.subscribeEmail('benisrae', '[email protected]');

pipeline.onStateChange('OnPipelineStateChange').addTarget(topic, {
template: 'Pipeline <pipeline> changed state to <state>',
textTemplate: 'Pipeline <pipeline> changed state to <state>',
pathsMap: {
pipeline: '$.detail.pipeline',
state: '$.detail.state'
Expand Down
33 changes: 31 additions & 2 deletions packages/aws-cdk-events/lib/input-options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,38 @@ export interface TargetInputTemplate {
* inputPathsMap to customize the data sent to the target. Enclose each
* InputPathsMaps value in brackets: <value>
*
* The value will be serialized as a JSON object (JSON.stringify).
* The value passed here will be double-quoted to indicate it's a string value.
* This option is mutually exclusive with `jsonTemplate`.
*
* @example
*
* {
* textTemplate: 'Build <buildid> started',
* pathsMap: {
* buildid: '$.detail.id'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope for this PR, but can we model this as well.

For example, how am I to know that the current event source has a "detail" member which has an "id" member?

Should be not that hard to slap together some classes that produce the same string, but in a way that can be Ctrl-space autocompleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The surface area is pretty big given events can potentially have tons of data associated with them and users will potentially want to extract any of it and transform it when they trigger their target. We might be able to provide some helpers with in the source construct (the one that has onXxx methods) which make it easier to work with certain fields of events emitted by the construct.

At any rate, I've opened #38 to track.

* }
* }
*/
textTemplate?: any;

/**
* Input template where you can use the values of the keys from
* inputPathsMap to customize the data sent to the target. Enclose each
* InputPathsMaps value in brackets: <value>
*
* This option is mutually exclusive with `textTemplate`.
*
* @example
*
* {
* jsonTemplate: '{ "commands": <commandsToRun> }' ,
* pathsMap: {
* commandsToRun: '$.detail.commands'
* }
* }
*
*/
template?: any;
jsonTemplate?: any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, confusing. If jsonTemplate is already supposed to be a JSON-ified object, type it as string, not any. This looks like I can pass in a JSON object.

But at the same time, why CAN'T I pass in a JSON object? That is so much more user-friendly than a pre-stringified object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, it's any instead of string because it also needs to be able to accept a Token, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:sad: tokens are evil (unless maybe we can make them behave like strings... #24)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That only helps insofar as that now we maybe could type this field as string and have users call token.toString() to make that work out.

Let me just leave this notion of Value<string> here again... :)


/**
* Map of JSON paths to be extracted from the event. These are key-value
Expand Down
38 changes: 31 additions & 7 deletions packages/aws-cdk-events/lib/rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Construct, resolve, Token } from 'aws-cdk';
import { Construct, FnConcat, Token } from 'aws-cdk';
import { events } from 'aws-cdk-resources';
import { EventPattern } from './event-pattern';
import { TargetInputTemplate } from './input-options';
Expand Down Expand Up @@ -107,13 +107,37 @@ export class EventRule extends EventRuleRef {

this.targets.push({
...target.eventRuleTarget,
inputTransformer: inputOptions && {
inputTemplate: typeof(inputOptions.template) === 'string'
? JSON.stringify(inputOptions.template)
: resolve(inputOptions.template),
inputPathsMap: inputOptions.pathsMap
},
inputTransformer: renderTransformer(),
});

function renderTransformer(): events.RuleResource.InputTransformerProperty | undefined {
if (!inputOptions) {
return undefined;
}

if (inputOptions.jsonTemplate && inputOptions.textTemplate) {
throw new Error('"jsonTemplate" and "textTemplate" are mutually exclusive');
}

if (!inputOptions.jsonTemplate && !inputOptions.textTemplate) {
throw new Error('One of "jsonTemplate" or "textTemplate" are required');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message could cover both cases. So simplification (warning, nasty code ahead using !!, just because I can):

if (!!inputOptions.jsonTemplate === !!inputOptions.textTemplate) {
    throw new Error('Exactly one of "jsonTemplate" or "textTemplate" is required');
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really a simplification?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, meh. Honestly I don't care.

But it is one fewer if and it's exactly as helpful to users.

The implementation is moderately more complex, using the "bang-bang operator" (a legitimate JavaScript hack I believe) and the fact that we're comparing two booleans for equality.

}

let inputTemplate: any;

if (inputOptions.jsonTemplate) {
inputTemplate = inputOptions.jsonTemplate;
} else if (typeof(inputOptions.textTemplate) === 'string') {
inputTemplate = JSON.stringify(inputOptions.textTemplate);
} else {
inputTemplate = new FnConcat('"', inputOptions.textTemplate, '"');
}

return {
inputPathsMap: inputOptions.pathsMap,
inputTemplate
};
}
}

/**
Expand Down
126 changes: 87 additions & 39 deletions packages/aws-cdk-events/test/test.rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { App, Arn, FnConcat, Stack } from 'aws-cdk';
import { App, Arn, FnConcat, FnJoin, Stack } from 'aws-cdk';
import { expect } from 'aws-cdk-assert';
import { iam } from 'aws-cdk-resources';
import { Test } from 'nodeunit';
Expand Down Expand Up @@ -158,7 +158,7 @@ export = {
});

rule.addTarget(t2, {
template: 'This is <bla>',
textTemplate: 'This is <bla>',
pathsMap: {
bla: '$.detail.bla'
}
Expand Down Expand Up @@ -201,67 +201,115 @@ export = {
'input template can contain tokens'(test: Test) {
const stack = new Stack();
const t1: IEventRuleTarget = {
eventRuleTarget: {
id: 'T1',
arn: new Arn('ARN1'),
kinesisParameters: { partitionKeyPath: 'partitionKeyPath' }
}
};
const t2: IEventRuleTarget = {
eventRuleTarget: {
id: 'T2',
arn: new Arn('ARN2'),
roleArn: new iam.RoleArn('IAM-ROLE-ARN')
}
eventRuleTarget: { id: 'T1', arn: new Arn('ARN1'), kinesisParameters: { partitionKeyPath: 'partitionKeyPath' } }
};

const t2: IEventRuleTarget = { eventRuleTarget: { id: 'T2', arn: new Arn('ARN2'), roleArn: new iam.RoleArn('IAM-ROLE-ARN') } };
const t3: IEventRuleTarget = { eventRuleTarget: { id: 'T3', arn: new Arn('ARN3') } };
const t4: IEventRuleTarget = { eventRuleTarget: { id: 'T4', arn: new Arn('ARN4') } };

const rule = new EventRule(stack, 'EventRule');

// a plain string should just be stringified (i.e. double quotes added and escaped)
rule.addTarget(t2, {
textTemplate: 'Hello, "world"'
});

// tokens are used here (FnConcat), but this is a text template so we
// expect it to be wrapped in double quotes automatically for us.
rule.addTarget(t1, {
template: new FnConcat('a', 'b')
textTemplate: new FnConcat('a', 'b')
});
rule.addTarget(t2, {
template: 'Hello, world'

// jsonTemplate can be used to format JSON documents with replacements
rule.addTarget(t3, {
jsonTemplate: '{ "foo": <bar> }',
pathsMap: {
bar: '$.detail.bar'
}
});

// tokens can also used for JSON templates, but that means escaping is
// the responsibility of the user.
rule.addTarget(t4, {
jsonTemplate: new FnJoin(' ', '"', 'hello', '\"world\"', '"'),
});

expect(stack).toMatch({
"Resources": {
"EventRule5A491D2C": {
"Type": "AWS::Events::Rule",
"Properties": {
"State": "ENABLED",
"Targets": [
{
"Arn": "ARN1",
"Id": "T1",
"InputTransformer": {
"InputTemplate": {
"State": "ENABLED",
"Targets": [
{
"Arn": "ARN2",
"Id": "T2",
"InputTransformer": {
"InputTemplate": "\"Hello, \\\"world\\\"\""
},
"RoleArn": "IAM-ROLE-ARN"
},
{
"Arn": "ARN1",
"Id": "T1",
"InputTransformer": {
"InputTemplate": {
"Fn::Join": [
"",
[
"a",
"b"
"\"",
{
"Fn::Join": [
"",
[
"a",
"b"
]
]
},
"\""
]
]
}
},
"KinesisParameters": {
"PartitionKeyPath": "partitionKeyPath"
}
},
"KinesisParameters": {
"PartitionKeyPath": "partitionKeyPath"
}
},
{
"Arn": "ARN2",
"Id": "T2",
{
"Arn": "ARN3",
"Id": "T3",
"InputTransformer": {
"InputTemplate": "\"Hello, world\""
},
"RoleArn": "IAM-ROLE-ARN"
}
]
"InputPathsMap": {
"bar": "$.detail.bar"
},
"InputTemplate": "{ \"foo\": <bar> }"
}
},
{
"Arn": "ARN4",
"Id": "T4",
"InputTransformer": {
"InputTemplate": {
"Fn::Join": [
" ",
[
"\"",
"hello",
"\"world\"",
"\""
]
]
}
}
}
]
}
}
}
});

test.done();
}
};
};