-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
feat(events): make target optional in onXxx()
methods
#2921
Conversation
Make `target` optional in `OnEventOptions` so that it is no required when calling a `onXxx()` method. This allows to use "preconfigured" rules in other constructs. Closes aws#2913
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it valid to provision a Rule without any targets? If not, then maybe we want to implement “validate” at the rule level?
Test for this?
|
Okay, so let’s just add a test |
It’s fishy that nothing broke at the consumption side when this changes from required to optional. I’d expect some null check... |
There's one already |
https://github.com/awslabs/aws-cdk/blob/83eee09e8198ae1921b932c09a6d14c88392800d/packages/%40aws-cdk/aws-events/test/test.rule.ts is full of rules created without targets, what kind of test do you want to add? something in the An extra test in |
Yes sounds good. Just to exercise this specific path |
Options are now not required in `onXxx` methods, but IRepository was not updated to that end. Related #2921
Options are now not required in `onXxx` methods, but IRepository was not updated to that end. Related #2921
Additional fixes like #3036 Options are now not required in onXxx methods, but the interfaces where not updated to that end. Related #2921 Root cause: aws/jsii#548
Make
target
optional inOnEventOptions
so that it is no required when calling aonXxx()
method.
This allows to use "preconfigured" rules in other constructs.
Update
awslint:events-method-signature
to enforce optionaloptions
parameter inonXxx()
method signatures.Closes #2913
Pull Request Checklist
design
folderBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.