-
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
[Marketplace Contribution] Cisco Umbrella cloud security - Content Pack Update #32939
Conversation
Thanks for contributing to a Cortex XSOAR supported pack. To receive credit for your generous contribution, please ask the reviewer to update your information in the pack contributors file. See more information here link |
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.
Finished my review. Let me know if other changes are required.
@@ -29,27 +29,18 @@ script: | |||
- name: destination_list_id | |||
description: The ID of the destination list. Destination lists can be fetched with the `umbrella-destination-lists-list` command. | |||
required: true | |||
isArray: 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.
I did not intentionally move all of these isArray
and required
. Do you want me to add them back in?
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.
it's fine, it could be a result of running the format command, you can keep them.
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.
Thank you for your contribution!
please see my comments, and check the failing check in the pr, it's failing with the unit tests coverage, so please make sure to add unit tests, if you need help, please let me know.
resp_type='response', | ||
ok_codes=(200, 201, 401) | ||
) | ||
try: | ||
res = res.json() | ||
except ValueError as exception: | ||
raise DemistoException('Failed to parse json object from response: {}'.format(res.content), | ||
exception) | ||
if 'error' in res: | ||
return_error( | ||
f'Error occurred while creating an access token. Please check the API key and secret ' | ||
f'and try to run the login command again to generate a new access token as it ' | ||
f'might have expired.\n{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.
no need to provide ok_codes list, 200s status codes are always okay, and 401, is being handled in the test-module command.
for the response type, no need to set it to be response, it's set to be json by default in the http_request function, which also performs the steps done in lines 110 - 119.
in conclusion, for follow up our code writing practice:
- please remove the resp_type and okay_codes params in calling the _http_request method.
- remove the 110 -119 lines as well.
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.
I copied this from the ServiceNow v2 integration source code get_access_token
function. Okay, I made those changes.
@@ -29,27 +29,18 @@ script: | |||
- name: destination_list_id | |||
description: The ID of the destination list. Destination lists can be fetched with the `umbrella-destination-lists-list` command. | |||
required: true | |||
isArray: 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.
it's fine, it could be a result of running the format command, you can keep them.
Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com>
Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com>
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.
see my comments please
...loud-security/Integrations/CiscoUmbrellaCloudSecurityv2/CiscoUmbrellaCloudSecurityv2_test.py
Outdated
Show resolved
Hide resolved
- Ensure that an access token is returned | ||
""" | ||
|
||
response = load_mock_response('token.json') |
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.
no need to add it as a file since it's a small test, define the response as the dict inside the token_json.
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.
Okay, I deleted token.json and added the dict within the function
...loud-security/Integrations/CiscoUmbrellaCloudSecurityv2/CiscoUmbrellaCloudSecurityv2_test.py
Outdated
Show resolved
Hide resolved
Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com>
test_get_access_token() - token.json deleted, so provided response dict containing access_token within function updated "When" docstrings of all test functions
…la-cloud-security' into randomizerxd-contrib-Cisco-umbrella-cloud-security
…CloudSecurityv2/test_data/token.json
012ab61
into
demisto:contrib/xsoar-contrib_randomizerxd-contrib-Cisco-umbrella-cloud-security
…ck Update (#33070) * [Marketplace Contribution] Cisco Umbrella cloud security - Content Pack Update (#32939) * "contribution update to pack "Cisco Umbrella cloud security"" * Revert description * remove try/except around get_access_token call * removed resp_type and ok_codes arguments passed to _http_request * currentVersion to 2.0.8 Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com> * Update Packs/Cisco-umbrella-cloud-security/ReleaseNotes/2_1_0.md Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com> * Rename 2_1_0.md to 2_0_8.md * add test_get_access_token * Create token.json * Apply suggestions from code review Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com> * provide access_token dict and update docstrings test_get_access_token() - token.json deleted, so provided response dict containing access_token within function updated "When" docstrings of all test functions * revert * Delete Packs/Cisco-umbrella-cloud-security/Integrations/CiscoUmbrellaCloudSecurityv2/test_data/token.json * update currentVersion to 2.0.9 * Create 2_0_9.md --------- Co-authored-by: Randy Baldwin <32545292+randomizerxd@users.noreply.github.com> Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com> * fixed lint errors --------- Co-authored-by: xsoar-bot <67315154+xsoar-bot@users.noreply.github.com> Co-authored-by: Randy Baldwin <32545292+randomizerxd@users.noreply.github.com> Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com> Co-authored-by: merit-maita <meretmaayta@gmail.com>
…ck Update (#33070) * [Marketplace Contribution] Cisco Umbrella cloud security - Content Pack Update (#32939) * "contribution update to pack "Cisco Umbrella cloud security"" * Revert description * remove try/except around get_access_token call * removed resp_type and ok_codes arguments passed to _http_request * currentVersion to 2.0.8 Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com> * Update Packs/Cisco-umbrella-cloud-security/ReleaseNotes/2_1_0.md Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com> * Rename 2_1_0.md to 2_0_8.md * add test_get_access_token * Create token.json * Apply suggestions from code review Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com> * provide access_token dict and update docstrings test_get_access_token() - token.json deleted, so provided response dict containing access_token within function updated "When" docstrings of all test functions * revert * Delete Packs/Cisco-umbrella-cloud-security/Integrations/CiscoUmbrellaCloudSecurityv2/test_data/token.json * update currentVersion to 2.0.9 * Create 2_0_9.md --------- Co-authored-by: Randy Baldwin <32545292+randomizerxd@users.noreply.github.com> Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com> * fixed lint errors --------- Co-authored-by: xsoar-bot <67315154+xsoar-bot@users.noreply.github.com> Co-authored-by: Randy Baldwin <32545292+randomizerxd@users.noreply.github.com> Co-authored-by: merit-maita <49760643+merit-maita@users.noreply.github.com> Co-authored-by: merit-maita <meretmaayta@gmail.com>
Status
Contributor
@randomizerxd
Video Link
Short demo video of the Pack usage. Speeds up the review. Optional but recommended. Use a video sharing service such as Google Drive or YouTube.