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

CloudWatch events - Fixes #52 #69

Merged
merged 5 commits into from
Jun 27, 2016

Conversation

coopernurse
Copy link
Contributor

Add support for CloudWatch events to the event_sources block. Also includes a cron sample that demonstrates usage.

I backed off of my approach of using type + name, in favor of the arn field, but I do think this approach is somewhat brittle as the user has to supply their account id in the arn and (I think) the arn needs to have rule/ in the prefix (e.g. rule/hello-cron)

But I think migration to a simplified event_sources format should be discussed separately, and for now I think it's ok to make this work like all the other sources.

One big thing to review -- in context.py I added this line to deploy():

self.add_event_sources()

This may be wrong, but I didn't see any path that would cause events to be registered -- create() was calling it, but when is that method called?

So I'm happy to remove that line before we merge this if that's incorrect - it will certainly impact code other than CloudWatch. But again, without that I'm not sure how any event sources will get added, but I may have missed how that works.

'State': rule['State']
}
else:
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop the else clause here and just return None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, that's done.

@coopernurse coopernurse mentioned this pull request Jun 13, 2016
@josegonzalez
Copy link
Contributor

Needs docs and tests!

@garnaat
Copy link
Owner

garnaat commented Jun 18, 2016

I originally left the add_event_sources out of deploy on purpose because most of the AWS examples show that as a separate step after doing some sort of mocked testing.

I'm ambivalent about it now. I think a good argument could be made for including it as part of deploy and I'm not opposed to that.

@josegonzalez josegonzalez merged commit eab3559 into garnaat:develop Jun 27, 2016
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

3 participants