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.

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

cloud watch events support #1126

Merged
merged 3 commits into from Aug 22, 2019
Merged

cloud watch events support #1126

merged 3 commits into from Aug 22, 2019

Conversation

kapilt
Copy link
Contributor

@kapilt kapilt commented May 25, 2019

Issue #1124

Support subscribing functions to cloud watch events.

with this pr the intent is that you can now subscribe to any of the dozen of event types that are sent through cloud watch events https://docs.aws.amazon.com/AmazonCloudWatch/latest/events/EventTypes.html

Example usage, in this case subscribing to ec2 instance state change notifications

@sample_app.on_cw_event({'source': ['aws.ec2']})
def instance_change(event, context):
    print(event)

@kapilt kapilt changed the title cwe support cloud watch events support May 25, 2019
@kapilt kapilt force-pushed the cwe-support branch 2 times, most recently from 728b169 to aad5cb1 Compare May 25, 2019 12:06
@codecov-io
Copy link

codecov-io commented May 25, 2019

Codecov Report

Merging #1126 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1126      +/-   ##
==========================================
+ Coverage   95.98%   96.03%   +0.05%     
==========================================
  Files          28       28              
  Lines        5176     5218      +42     
  Branches      658      665       +7     
==========================================
+ Hits         4968     5011      +43     
+ Misses        135      134       -1     
  Partials       73       73
Impacted Files Coverage Δ
chalice/awsclient.py 94.62% <100%> (+0.06%) ⬆️
chalice/app.py 97.43% <100%> (+0.02%) ⬆️
chalice/deploy/deployer.py 98.32% <100%> (+0.03%) ⬆️
chalice/package.py 100% <100%> (ø) ⬆️
chalice/deploy/planner.py 97.11% <100%> (+0.12%) ⬆️
chalice/deploy/models.py 100% <100%> (+0.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ecde2b...8705c5e. Read the comment docs.

@kapilt kapilt force-pushed the cwe-support branch 2 times, most recently from 202ff5b to 87080f0 Compare May 27, 2019 14:30
@kapilt kapilt force-pushed the cwe-support branch 2 times, most recently from f08c425 to c0b2edc Compare July 2, 2019 02:22
Copy link
Member

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks good! I still need to pull it down and try it out. Otherwise had some comments and suggestions. I think this will be really useful. Also we will probably need to do some rebasing to fix some conflicts.

chalice/deploy/models.py Show resolved Hide resolved
chalice/deploy/planner.py Outdated Show resolved Hide resolved
docs/source/topics/events.rst Show resolved Hide resolved
chalice/app.py Show resolved Hide resolved
@kyleknap
Copy link
Member

Other thing I forgot... It would be good to add a changelog entry for this. You can add it under a new Next Release section in the CHANGELOG.rst file

Copy link
Member

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

I tried it out and it worked well. I just had a couple more bits of feedback on the documentation after trying to use it to test out the feature

docs/source/topics/events.rst Show resolved Hide resolved
docs/source/topics/events.rst Show resolved Hide resolved
Copy link
Member

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks great. Just had one small comment. Otherwise, I say we rebase off of master, add a changelog entry, and we can get this one merged too.

chalice/deploy/planner.py Outdated Show resolved Hide resolved
@kapilt kapilt force-pushed the cwe-support branch 2 times, most recently from e720688 to 99dc613 Compare August 19, 2019 13:43
@kapilt
Copy link
Contributor Author

kapilt commented Aug 21, 2019

rebased and added changelog and terraform packaging support.

Copy link
Member

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Nice! Looks good. Just one a little picky comment: I noticed in the updated docs and in the code, CloudWatch is being spelled out as two words. Could we update it to make it one word (i.e. CloudWatch instead of CloudWatch in the docs and cloudwatch instead of cloud_watch in the code) to make it consistent with the official service branding and how it is exposed in boto3? Otherwise, it should be good to merge.

@kapilt
Copy link
Contributor Author

kapilt commented Aug 22, 2019

wrt to cloudwatch, updated docs and code to reflect the product marketing name.

Copy link
Member

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@kyleknap kyleknap merged commit 0ad1309 into aws:master Aug 22, 2019
@kyleknap kyleknap added this to Done in Chalice Roadmap Aug 22, 2019
@kyleknap kyleknap removed this from To be released in Chalice Roadmap Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants