Template: Analytics and SIEM #4281
Conversation
- Analytics & SIEM - Authentication - Case Management
- Analytics & SIEM - Authentication - Case Management
creadet authentication
added BaseClient and DemistoException to CommonServerPython
There was a problem hiding this comment.
Work with @kirbles19 to create the Templates/README.md which explains a bit about each template and how to use it.
# Conflicts: # Scripts/CommonServerPython/CommonServerPython.py
I'll work with him also on the Docstring |
There was a problem hiding this comment.
You have duplicate files, please fix that. And arrange the files to be in the structure we discussed.
Templates/Integrations//files...
I've went only through IntegrationTemplates/AnalyticsAndSIEM/AnalyticsAndSIEM.py and the commonServerPython, if you think that there are some other things in the other files that might be relevant please let me know.
And please note that i've not mentioned repeated comments, please fix them everywhere.
| ''' GLOBALS/PARAMS ''' | ||
| INTEGRATION_NAME: str = 'Analytics & SIEM Integration' | ||
| # lowercase with `-` dividers | ||
| INTEGRATION_NAME_COMMAND: str = 'analytics-and-siem' |
There was a problem hiding this comment.
| INTEGRATION_NAME_COMMAND: str = 'analytics-and-siem' | |
| INTEGRATION_COMMAND_NAME: str = 'analytics-and-siem' |
| from CommonServerPython import * | ||
| from CommonServerUserPython import * | ||
|
|
||
| ''' IMPORTS ''' |
There was a problem hiding this comment.
My own preference which makes the code a bit more readable is to order the imports(And anywhere else where the order doesn't matter) by the line length(like the ones for the demistomock)
| INTEGRATION_NAME_CONTEXT: str = 'AnalyticsAndSIEM' | ||
|
|
||
|
|
||
| class Client: |
There was a problem hiding this comment.
Add class docstring which will explain it usage, it should also contain the documentation for the class attributes
|
|
||
| class Client: | ||
| def __init__(self, server: str, use_ssl: bool, fetch_time: Optional[str] = None): | ||
| self._server: str = server.rstrip(chars='/') |
There was a problem hiding this comment.
seems like an overkill with the argument types, you've mentioned them in the function itself, no need to overdo it.
It makes it less readable and you should use it only when needed.
| def _http_request(self, method: str, url_suffix: str, full_url: str = None, headers: Dict = None, | ||
| auth: Tuple = None, params: Dict = None, data: Dict = None, files: Dict = None, | ||
| timeout: float = 10, resp_type: str = 'json') -> Any: | ||
| """A wrapper for requests lib to send our requests and handle requests |
There was a problem hiding this comment.
Try to read the PEP257 guide https://www.python.org/dev/peps/pep-0257/ and https://google.github.io/styleguide/pyguide.html
You have this:
The docstring is a phrase ending in a period. It prescribes the function or method's effect as a command ("Do this", "Return that"), not as a description; e.g. don't write "Returns the pathname ...".
And
Args:
List each parameter by name. A description should follow the name, and be separated by a colon and a space. If the description is too long to fit on a single 80-character line, use a hanging indent of 2 or 4 spaces (be consistent with the rest of the file).
The description should include required type(s) if the code does not contain a corresponding type annotation.
If a function accepts *foo (variable length argument lists) and/or **bar (arbitrary keyword arguments), they should be listed as *foo and **bar.
| @@ -0,0 +1,443 @@ | |||
| import demistomock as demisto | |||
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
Add some description here, and mention the usage of it
| verify, | ||
| proxy | ||
| ): | ||
| """Base Client for use in new integrations |
There was a problem hiding this comment.
Should be the class docsting
| integration_name_command, | ||
| integration_name_context, | ||
| verify, | ||
| proxy |
| self._proxies = handle_proxy() | ||
| else: | ||
| self._proxies = None | ||
| self._base_url = '{}{}'.format(self._server, base_suffix) |
There was a problem hiding this comment.
move it before the proxy handling
added @Property to vars in BaseClient added default to __init__ in BaseClient
* Added build_dbot_entry * Added build_malicious_dbot_entry
* Added build_dbot_entry * Added build_malicious_dbot_entry
renamed readme.md to README.md
|
we can use @logger. |
|
|
||
| # Switch case | ||
| commands = { | ||
| 'test-module': test_module, |
There was a problem hiding this comment.
it appears that if your return_outputs('ok', None, None) it will fail the integration Test. So change return_outputs to support this option
| } | ||
| try: | ||
| if command == 'fetch-incidents': | ||
| incidents, new_last_run = commands[command](client, last_run=demisto.getLastRun()) |
There was a problem hiding this comment.
no need to do it generically when you know exactly which function your are going to execute. Replace it with fetch_incidents
| return f'{INTEGRATION_NAME} - Could not find any events.', {}, {} | ||
|
|
||
|
|
||
| def get_event(client: Client, args: Dict) -> Tuple[str, Dict, Dict]: |
There was a problem hiding this comment.
change get_event to be get_event_command , do the same for other functions. And remove _request from Client functions.
There was a problem hiding this comment.
we've discussed @ the first presentation and that's what agreed upon all of us.
| ''' COMMANDS ''' | ||
|
|
||
|
|
||
| def test_module(client: Client, *_) -> Tuple[str, Dict, Dict]: |
There was a problem hiding this comment.
what is *_ and test_module dont have demisto.args(), the user will decide which parameters to pass to test_module
There was a problem hiding this comment.
*_ are ignored args.
| def fetch_incidents( | ||
| client: Client, | ||
| fetch_time: str, | ||
| last_run: Optional[datetime] = None) -> Tuple[List, datetime]: |
There was a problem hiding this comment.
last_run should be str not datetime. you cant save datetime in setLastRun() AFAIK
|
|
||
| def main(): # pragma: no cover | ||
| params = demisto.params() | ||
| base_url = f"{params.get('url', '').rstrip('/')}'/api/v2/'" |
There was a problem hiding this comment.
I dont think we need to provide default value of '' url. We should return error in that case
use urljoin(params.get('url'), '/api/v2/')
| proxy = params.get('proxy') | ||
| client = Client(base_url=base_url, verify=verify_ssl, proxy=proxy) | ||
| command = demisto.command() | ||
| demisto.info(f'Command being called is {command}') |
There was a problem hiding this comment.
No need to info this. change to debug
Status
Ready
Description
New integration templates
Does it break backward compatibility?
Must have