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

[output] Adding support for assign priority to incidents #498

Merged
merged 3 commits into from
Nov 28, 2017

Conversation

javuto
Copy link
Contributor

@javuto javuto commented Nov 27, 2017

to: @ryandeivert @jacknagz
cc: @airbnb/streamalert-maintainers
size: small

Background

New output to use PagerDuty Incidents was added here. Incidents will be created with a low priority by default, unless specified.

Changes

  • Added support to specify a valid priority name in the context.
  • Added testing code.

Testing

$ rm .coverage && ./tests/scripts/unit_tests.sh
...
TOTAL                                                  2573     73    97%
----------------------------------------------------------------------
Ran 470 tests in 8.373s

OK

$ ./tests/scripts/pylint.sh

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

$ python manage.py lambda test --processor rule all
...
StreamAlertCLI [INFO]: (59/59) Successful Tests
StreamAlertCLI [INFO]: (93/93) Alert Tests Passed
StreamAlertCLI [INFO]: Completed

@javuto javuto added this to the 1.6.0 milestone Nov 27, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 96.078% when pulling e423172 on javier-streamalert-priorities into dc1cec8 on master.

incident_priority = {}
_priority = rule_context.get('incident_priority', False)
if _priority:
verified_priority = self._priority_verify(_priority)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: pass the rule_context to the _priority_verify function and have it do all of this, and instead of having _priority_verify return False have it return the default value of {} or dict()..

So your call would just look something like:
incident_priority = self._priority_verify(rule_context)

And everywhere above that returns false would instead return dict()

@javuto javuto force-pushed the javier-streamalert-priorities branch from e423172 to 2a454e5 Compare November 27, 2017 23:08
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 96.078% when pulling 2a454e5 on javier-streamalert-priorities into a5dd27e on master.

@@ -401,6 +441,11 @@ def dispatch(self, **kwargs):
if rule_context:
rule_context = rule_context.get(self.__service__, {})

# Use the priority provided in the context, use it or the incident will be low priority
incident_priority = {}
if rule_context:
Copy link
Contributor

@ryandeivert ryandeivert Nov 28, 2017

Choose a reason for hiding this comment

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

Thanks for the change! Looks great - one last suggestion: don't instantiate the incident_priority here or do the if rule_context check here.. do this all in the _priority_verify and return dict() immediately if the rule_context == None

Copy link
Contributor

@ryandeivert ryandeivert left a comment

Choose a reason for hiding this comment

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

Approving but please see tiny nit comment

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 96.078% when pulling 6099948 on javier-streamalert-priorities into a5dd27e on master.

@javuto javuto merged commit e002194 into master Nov 28, 2017
@javuto javuto deleted the javier-streamalert-priorities branch November 28, 2017 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants