-
Notifications
You must be signed in to change notification settings - Fork 334
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
[outputs] Komand support added (contribution from @0xdabbad00) #608
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @0xdabbad00 thanks for the contribution! I had a couple comments, could you also add unit tests to verify this output?
@ryandeivert if you have a second to review that'd be great
if not creds: | ||
return self._log_status(False) | ||
|
||
headers = {"Authorization": creds['komand_auth_token']} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of double quotes instead of single quotes
LOGGER.debug('sending alert to Komand') | ||
|
||
success = False | ||
if container_id: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this if
block seems confusing, are container_id
and artifact*
variables copy pasta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes /me hangs head in shame, will fix.
OutputProperty(description='a short and unique descriptor for this ' | ||
'Komand integration')), | ||
('komand_auth_token', | ||
OutputProperty(description='the auth token for this Komand integration. Example: 00000000-0000-0000-0000-000000000000', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this description line and the one below will exceed the pylint line len threshold of 100, consider breaking into multiple likes like the preceding 'descriptor'
block
hey @0xdabbad00 looking good! I know this is a small class, but (as jack mentioned) some simple unit tests would be excellent :) |
|
||
LOGGER.debug('sending alert to Komand') | ||
|
||
success = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this declaration as it's unnecessary
@0xdabbad00 can you fix the current conflict in |
so close! @0xdabbad00 - some pylint errors:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢
to: @airbnb/streamalert-maintainers
size: small
resolves #602
Background
This is basically a copy/paste job of the Phantom integration, but even simpler. Komand has a web hook, which you hit with an authorization token, and just sent it your data.
Testing
Deployed and tested in our environment.