-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add ability to set State of EventBridge Rule #2524
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
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2524 +/- ##
===========================================
+ Coverage 93.58% 94.43% +0.84%
===========================================
Files 90 99 +9
Lines 6124 7615 +1491
Branches 1260 1598 +338
===========================================
+ Hits 5731 7191 +1460
- Misses 183 205 +22
- Partials 210 219 +9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Thanks!
there are couple of places to change to add to the property dict:
property_types = { |
resource_type = "EventBridgeRule" |
@sebastiankasprzak can we also update the transform tests (both SFN and Lambda event source to include this new property? |
@aahung so Lambda Event source already had the I'll add |
e534699
to
8154199
Compare
@aahung those changes are now be implemented. I have not added integration tests, how would you recommend these should be added? |
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.
LGTM! Thanks for addressing the comments!
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.
One more comment: It would be great if we can use the new "State" in our transform tests, e.g., here, you can run make test to verify if it translates to the current template.
nice! Thanks @aahung Added transform test for one of the Eventbridge tests. Let me know if that's good enough |
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.
LGTM! Thanks for addressing the comments
Issue #, if available:
#2509
Description of changes:
State
property ofAWS::Events::Rule
in SAMDescription of how you validated changes:
Used
bin/sam-translate.py
to generate a CFN template from SAM template which I then successfully deployed in my AWS account with desired changes taking effect.Checklist:
make pr
passesExamples?
Please reach out in the comments, if you want to add an example. Examples will be
added to
sam init
through https://github.com/awslabs/aws-sam-cli-app-templates/By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.