Add support for scheduled events #393

jamesls · 2017-06-29T17:31:06Z

Proposal: #390

Sample app:

from chalice import Chalice
import datetime

app = Chalice(app_name='testevents')


@app.route('/')
def index():
    return {'hello': 'world'}


@app.schedule('rate(1 minute)')
def every_minute(event):
    print(event.to_dict())

The implementation piggy backs off of the multi lambda support from the built in authorizers. The only unexpected change was an update to the deployed.json file from:

    "lambda_functions": {
      "testevents-dev-every_minute": "arn:lambda-foo"
    },

to:

    "lambda_functions": {
      "testevents-dev-every_minute": {
        "type": "scheduled_event",
        "arn": "arn:lambda-foo"
      }
    },

So we can differentiate the different types of lambda functions.

The other caveat is that right now you still need to deploy an API gateway API. There isn't support yet for only deploying scheduled events. That will be in a follow up PR.

Review Notes

The deployer module has some duplication that I'm working on removing. The logic itself won't change but I'm refactoring the code. Everything else (app.py, awsclient.py, all the tests) are ready for review.

codecov-io · 2017-06-29T17:36:06Z

Codecov Report

Merging #393 into master will increase coverage by 0.47%.
The diff coverage is 97.77%.

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
+ Coverage   92.28%   92.75%   +0.47%     
==========================================
  Files          18       18              
  Lines        2464     2569     +105     
  Branches      323      334      +11     
==========================================
+ Hits         2274     2383     +109     
+ Misses        141      137       -4     
  Partials       49       49

Impacted Files	Coverage Δ
chalice/__init__.py	`100% <ø> (ø)`	⬆️
chalice/deploy/swagger.py	`100% <100%> (ø)`	⬆️
chalice/config.py	`97.29% <100%> (+0.15%)`	⬆️
chalice/awsclient.py	`92.9% <96.77%> (+1.41%)`	⬆️
chalice/deploy/deployer.py	`90.37% <96.87%> (+1.07%)`	⬆️
chalice/app.py	`97.06% <98.46%> (+0.21%)`	⬆️

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 0f13291...83eafe1. Read the comment docs.

kyleknap

Looks good. Just some early feedback on the user interface to start before I get into the deployer part of the code.

kyleknap · 2017-06-29T17:50:50Z

chalice/app.py

+
+class CloudWatchEvent(object):
+    def __init__(self, event_dict):
+        self.version = event_dict['version']


Will all of these be guaranteed to be present in the event? If it is a direct lookup and a key is missing from the event it would cause issues.

kyleknap · 2017-06-29T17:56:56Z

chalice/app.py

@@ -765,3 +779,82 @@ class AuthRoute(object):
    def __init__(self, path, methods):
        self.path = path
        self.methods = methods
+
+
+class EventSource(object):


I think the name EventSource along with Chalice.event_sources may be a bit ambiguous here because event source (thinking from the lambda perspective) could mean any event from any service as show by their docs but this seems to be specific to cloud watch events. We may want to either make the name more specific to cloudwatch event sources or make this class more generalized for any lambda event source (like repurpose the schedule_expression to something general to lambda event sources).

It's intentional to have an EventSource type. The idea would be this is used for all event types as support more types is added. I don't envision adding a new public attr to chalice.Chalice for every new event source, that wouldn't be maintainable. I can extract out a separate CloudWatchEventSource class with EventSource as its base type. Seemed like overkill for now, but I don't mind adding it.

Makes sense. I like the idea of having EventSource be the base class and CloudWatchEventSource subclass from it to include any of the cloudwatch event specific logic.

I pushed an update. Let me know what you think.

kyleknap · 2017-06-29T17:57:39Z

chalice/app.pyi

+
+
+class Rate(ScheduleExpression):
+    unit = ... # type: str


The type here should be an integer. Or union of str and integer to be similar to Cron? I think I would prefer integer though if you can only provide whole numbers to rate.

That should be an integer, I'll update.

kyleknap

Looks good. Just had a couple more comments. I think we should be good once we have docs up.

kyleknap · 2017-06-29T20:20:46Z

tests/functional/test_awsclient.py

+                         'AWS:SourceArn': source_arn,
+                     }
+                 },
+                 'Effect': 'Allow',


Without looking at what got removed in the diff, it is not very easy to tell what portion of the policy needs to be present for it to be skipped. Maybe separate this into tests where one tests that a specific policy entry skips over the add_permission call but the other entries causes add_permission to be triggered?

I refactored the test a bit to make this more clear.

kyleknap · 2017-06-29T20:34:02Z

chalice/app.py

+        return 'rate(%s %s)' % (self.value, unit)
+
+
+class Cron(ScheduleExpression):


Did we want to add Cron and Rate to the __init__?

I don't follow what you're asking. Can you elaborate?

Oh so I have to import those two from chalice.app right now, but I'm use to importing all of the user facing classes from chalice instead when I right my application.

ohhhh, chalice/__init__.py. Gotcha. Yeah I can add that.

kyleknap · 2017-06-29T22:26:00Z

Cool. Looks good once docs are included, it should be good to merge. I also noticed that there was some missing coverage according to the codecov report. Do you know what that may be?

jamesls · 2017-06-30T00:27:23Z

Yeah it was some of the new code I added for the base class refactoring. I added a isinstance check that's missing a testcase if you don't pass an instance of CloudWatchEventSource. Right now there are no other kinds so I can make one up for the test.

jamesls · 2017-07-03T18:31:04Z

OK I think everything has been addressed, and docs have been added. Let me know if there's any more feedback.

cc @kyleknap @stealthycoin

kyleknap

Looks good. Just had a small comment. Otherwise, 🚢

kyleknap · 2017-07-03T21:55:47Z

docs/source/api.rst

+      .. code-block:: python
+
+          @app.schedule('cron(15 10 ? * 6L 2002-2005)')
+          def handler(event):


We may want to have all of these handler names share the function name handler() just in case if someone tries to literally copy and paste all of this. Maybe make these handler names more specific like cron_handler()?

You mean not share?

yes not share

OK I'll update that before merging.

Defines the API a user would interact with to configure and invoke scheduled events in their chalice app.

jamesls requested review from kyleknap and stealthycoin June 29, 2017 17:31

kyleknap reviewed Jun 29, 2017

View reviewed changes

kyleknap approved these changes Jul 3, 2017

View reviewed changes

stealthycoin approved these changes Jul 4, 2017

View reviewed changes

jamesls force-pushed the scheduled-events branch from c89a24e to 28149ca Compare July 4, 2017 02:58

jamesls added 5 commits July 5, 2017 10:39

Add user facing API for scheduled events

fb8a37c

Defines the API a user would interact with to configure and invoke scheduled events in their chalice app.

Update awsclient with needed cloudwatch events APIs

253abff

Add support for deploying scheduled events

65f4c35

Add documentation for scheduled events

2efb976

Add scheduled events feature to changelog

83eafe1

jamesls force-pushed the scheduled-events branch from 2a79e20 to 83eafe1 Compare July 5, 2017 17:39

jamesls merged commit 83eafe1 into aws:master Jul 5, 2017

jamesls mentioned this pull request Jul 5, 2017

[proposal] Add support for scheduled events #390

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for scheduled events #393

Add support for scheduled events #393

jamesls commented Jun 29, 2017

codecov-io commented Jun 29, 2017 •

edited

kyleknap left a comment

kyleknap Jun 29, 2017

jamesls Jun 29, 2017

kyleknap Jun 29, 2017

jamesls Jun 29, 2017

kyleknap Jun 29, 2017

jamesls Jun 29, 2017

kyleknap Jun 29, 2017

jamesls Jun 29, 2017

kyleknap left a comment

kyleknap Jun 29, 2017

jamesls Jun 29, 2017

kyleknap Jun 29, 2017

jamesls Jun 29, 2017

kyleknap Jun 29, 2017

jamesls Jun 29, 2017

kyleknap commented Jun 29, 2017

jamesls commented Jun 30, 2017

jamesls commented Jul 3, 2017

kyleknap left a comment

kyleknap Jul 3, 2017

jamesls Jul 3, 2017

kyleknap Jul 3, 2017

jamesls Jul 3, 2017

		return 'rate(%s %s)' % (self.value, unit)


		class Cron(ScheduleExpression):

Add support for scheduled events #393

Add support for scheduled events #393

Conversation

jamesls commented Jun 29, 2017

codecov-io commented Jun 29, 2017 • edited

Codecov Report

kyleknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyleknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyleknap commented Jun 29, 2017

jamesls commented Jun 30, 2017

jamesls commented Jul 3, 2017

kyleknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 29, 2017 •

edited