-
Notifications
You must be signed in to change notification settings - Fork 61
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
Make Credential File Optional #125
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.
Looks really good! There's a couple tests in SentinelOne that are failing that I can help troubleshoot and figure out what has changed but wanted to go ahead and get this review out the door.
Explaining S1 changes/updates:@xC0uNt3r7hr34t Noted in #129 that there were unnecessary changes made to the pagination within S1, which should never have been altered. To address this bug and maintain limits in accordance with the S1 console and documentation, the pagination limit has been reset to 1000, which was the setting prior to the limit feature being introduced into Surveyor. The limits for Deep Visibility and Power Query have been maintained at 20000 and 1000, respectively. These values are updated in the request parameters, which is the only value that should have been changed. Testing passed for in and out of bounds ranges for each mode. |
products/sentinel_one.py
Outdated
self.creds_file = kwargs['creds_file'] if 'creds_file' in kwargs else None | ||
self._raw = kwargs['raw'] if 'raw' in kwargs else self._raw | ||
limit = (kwargs['limit']) if 'limit' in kwargs else 0 | ||
self._pq = pq # This supports command-line options, will default to Deep Visibility |
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 could be considered a breaking change making DV the default instead of PQ
In PR #94 , there was the design decision to make PQ the default due to accuracy and performance
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.
PQ should remain the default as DV will be sunsetting in the next 6 months to a year.
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.
That comment has since been corrected. Thanks! @xC0uNt3r7hr34t
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!
Changes
click
output message withprint
in S1 product.closes #121
closes #128