fix(events): allow adding same target to rule multiple times #3187
Conversation
As rule targets can have different input configurations adding the same target multiple times must be possible. Move rule target id generation to `aws-events` where it's easy to simply increment a counter. This id is passed as an argument to `bind()` for targets that need to know their given id (e.g. `EcsTask`). Add `permissionsNode` on `IFunction` to handle permission checks for both functions and singleton functions. Fixes aws#3173
| const id = sanitizeId(targetProps.id); | ||
| const inputProps = targetProps.input && targetProps.input.bind(this); | ||
| // Simply increment id for each `addTarget` call. This is guaranteed to be unique. | ||
| const id = `Target${this.targets.length}`; |
There was a problem hiding this comment.
I'm not sure this will work correctly if we update a list of 2 elements to a list of 2 different elements.
I believe what will happen is CloudFormation will call addTarget("Rule1", { ... }) (the new Rule1), and then removeTarget("Rule1") (intending to remove the old rule1).
Anything that depends on addition order is super scary to me. I'd rather have a different consistent way of generating IDs.
There was a problem hiding this comment.
Either an optional "id" field on every target that can be used in lieu of the default id, or we hash in the target configuration to make the ID unique.
There was a problem hiding this comment.
Are you sure? CF will likely use PutTargets which updates everything in one go?
There was a problem hiding this comment.
OK I get what you mean, let's hash the configuration.
There was a problem hiding this comment.
How do you suggest passing the id to the EcsTask target (which needs the id for its AwsCustomResource) if we hash it in aws-events after receiving the full configuration?
There was a problem hiding this comment.
I'm not sure this will work correctly if we update a list of 2 elements to a list of 2 different elements.
Actually I tested this and it works fine. If this was the case we would already have a problem with the current implementation because when you change a target configuration (let's say message for targets.SnsTopic) you are basically updating the target but keeping the same id. Rule targets are not CF resources by themselves, they are properties of AWS::Events::Rule.
You can try the following:
First depoy:
event.addTarget(new targets.SqsQueue(queue));
event.addTarget(new targets.SnsTopic(topic));Second deploy:
event.addTarget(new targets.SnsTopic(topic));
event.addTarget(new targets.SqsQueue(queue));After second deploy targets are correctly inverted.
Let me know if you still think that the id generation should be updated.
There was a problem hiding this comment.
Why do they have an ID then... :x
|
Oh well I guess this breaks API compatibility now. |
|
This is rather technical/internal... what do you suggest? |
|
I have to context switch to get back into it... but offhand: Not as good as an optional |
|
@rix0rrr this is now non breaking. (shouldn't |
Not if the field is on a struct that is returned from a function. Consider the following: interface OutputStruct {
value: string;
}
const struct : OutputStruct = giveMeTheStruct();
functionThatExpectsAString(struct.value);Now change |
|
After merging it it broke |
|
This is a better pR: #3353 |
As rule targets can have different input configurations adding the same target multiple times must be possible. Move rule target id generation to `aws-events` where it's easy to simply increment a counter. This id is passed as an argument to `bind()` for targets that need to know their given id (e.g. `EcsTask`). Add `permissionsNode` on `IFunction` to handle permission checks for both functions and singleton functions. Fixes #3173
As rule targets can have different input configurations adding the same target multiple times
must be possible.
Move rule target id generation to
aws-eventswhere it's easy to simply increment a counter. Thisid is passed as an argument to
bind()for targets that need to know their given id (e.g.EcsTask).Add
permissionsNodeonIFunctionto handle permission checks for both functions and singletonfunctions.
Fixes #3173
Please read the contribution guidelines and follow the pull-request checklist.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license