-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Absolute V1 #18079
Absolute V1 #18079
Conversation
This pull request introduces 1 alert when merging fc035e5 into 6ce7dc4 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 1e66557 into a07f513 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 5605596 into 264f577 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 052880e into ac3da70 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 038033d into 54d6ce4 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging b30b6ea into af76d58 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 08116ad into 24313fa - view on LGTM.com new alerts:
|
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.
Amazing effort. Please see my comments below.
verify (bool): Whether to check for SSL certificate validity. | ||
proxy (bool): Whether the client should use proxies. | ||
headers (dict): Headers to set when doing a http request. | ||
x_abs_date (str): |
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.
What is this? Please add a description.
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.
done
outputs = parse_device_field_list_response(res) | ||
human_readable = tableToMarkdown(f'{INTEGRATION} Custom device field list', | ||
parse_device_field_list_response_human_readable(outputs), removeNull=True) | ||
return CommandResults(outputs=outputs, outputs_prefix="Absolute.CustomDeviceField", outputs_key_field='DeviceUID', |
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.
Please split this into multi-line instantiation.
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.
done
return offline_time_seconds | ||
|
||
|
||
def raise_error_on_missing_args(msg): |
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.
Seems more like a general error function, rather than a specific missing_args
function. Please make sure to change its naming.
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.
done
payload = {"name": request_name, "message": html_message, "messageName": message_name, | ||
"freezeDefinition": {"deviceFreezeType": device_freeze_type}, "deviceUids": device_ids, | ||
"notificationEmails": notification_emails, "passcodeDefinition": {"option": passcode_type}} |
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.
Please split this into multi-line instantiation.
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.
done
return CommandResults(outputs_prefix='Absolute.DeviceUnenroll', outputs=outputs, readable_output=human_readable, | ||
raw_response=res) |
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.
Please split this into multi-line instantiation.
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.
done
account_uids = remove_duplicates_from_list_arg(args, 'account_uids') | ||
query = add_list_to_filter_string("accountUid", account_uids, query) |
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.
These 2 lines are repeated 9 times. Perhaps extract it to a function?
Better yet, if the order isn't important, perhaps let said function accept something like a dict of source_name: target_name
, and that way you can even further simplify this function, as it'll handle these lines via a loop.
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.
done
else: | ||
parsed_device[key[0].upper() + key[1:]] = val |
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.
Please add a comment here about the expected type of key
- how come can this be a list?
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.
Key is not a list, is a string and we take the substring of the first place till the end. The key is actually the devices fields.
outputs = parse_device_list_response(copy.deepcopy(res), keep_os_in_list=False) | ||
human_readable = tableToMarkdown(f'{INTEGRATION} devices list:', outputs, removeNull=True) | ||
human_readable += f"Above results are with page number: {page} and with size: {limit}." | ||
return CommandResults(outputs_prefix='Absolute.Device', outputs=outputs, outputs_key_field="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.
Please split this into multi-line instantiation.
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.
done
raise DemistoException( | ||
f"{INTEGRATION} at least one of the commands args (device_ids, device_names, local_ips, public_ips) " | ||
f"must be provided.") |
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.
Seems like this could use the generic replacement for raise_error_on_missing_args
.
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.
done
Assigning @jrosenpanw for docs-review. |
@DeanArbel @guykeller Doc review completed. |
Link to the unit tests coverage report: |
Status
Related Issues
fixes: link to the issue
Description
Absolute V1 integration.
Minimum version of Cortex XSOAR
Does it break backward compatibility?
Must have
Leftovers: