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

Events API v2 Support #84

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Events API v2 Support #84

wants to merge 9 commits into from

Conversation

jjinno
Copy link

@jjinno jjinno commented May 21, 2019

No description provided.

jjinno added 5 commits May 21, 2019 16:22
Created Events class compatible with Events API v2 (not to be confused with REST API v2)
New API comes with new documented required fields and new optional fields.
Documentation links provided at top of code for reference to the PD-CEF & Events API v2

Implemented enqueue() to allow for payload to be specified by caller.
This allows direct use of the JSON produced for the request (better for unit-testing enqueue function)

Note that summary/source/severity are still required, and throw a KeyError if not found in either kwargs or payload.
Similar to v1 test: directly tests the enqueue() function with a built-in event_action of "trigger"
@houqp
Copy link

houqp commented Nov 1, 2019

@cugini-dbx any thing blocking this PR from getting merged?

Copy link
Contributor

@cugini-dbx cugini-dbx left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in review. I just have one small comment around a some required field validation, otherwise it looks good to me!

# Required (according to documentation) PD-CEF fields
data = {"event_action": kwargs['event_action']}
data['payload'] = kwargs.get('payload', {})
for key in ['summary', 'source', 'severity']:
Copy link
Contributor

Choose a reason for hiding this comment

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

A teammate pointed out that payload is optional, but looks like this assumes payload is required (by checking for these three keys).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. 👍

Even though payload is now optional, if it is specified, the fields within it are still checked as required
@jjinno
Copy link
Author

jjinno commented Dec 5, 2019

The rest of the Travis issues appear to be not associated with this PR... not sure how best to resolve them

@cugini-dbx
Copy link
Contributor

looks at watch
... Well I totally dropped the ball, apologies for the very late return. The tests have all been fixed, so once you rebase, I'll be happy to merge this in.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sdenef-adeo
Copy link

Hello,

I'm interested in this feature.
It seems all is OK to merge it AFAIU.

Regards,
Sébastien

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.

5 participants