-
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
Get trail status #32960
Get trail status #32960
Conversation
Thank you for your contribution. Your generosity and caring are unrivaled! Make sure to register your contribution by filling the Contribution Registration form, so our content wizard @YuvHayun will know the proposed changes are ready to be reviewed. |
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 @kcelovic, thx for your contribution!
Looks good in general, please make sure to add tests, sign the CLA, and address my comments.
Let me know if you have any questions.
Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.yml
Outdated
Show resolved
Hide resolved
Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.yml
Outdated
Show resolved
Hide resolved
Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.py
Outdated
Show resolved
Hide resolved
Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Yuval Hayun <[email protected]>
…l.yml Co-authored-by: Yuval Hayun <[email protected]>
…l.yml Co-authored-by: Yuval Hayun <[email protected]>
…l.py Co-authored-by: Yuval Hayun <[email protected]>
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.
The try & except comment is still relevant.
Also, tests are still needed.
Let me know when you're ready for re-review.
@YuvHayun what kind of tests are needed? playbook test? |
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 @kcelovic, I left some comments.
I'm talking about unit tests in the AWS-CloudTrail_test.py file.
Also, please make sure to update the readme to include the new command.
This can be done easily using the command `demisto-sdk generate-docs AWS-CloudTrail.yml'
Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.py
Outdated
Show resolved
Hide resolved
Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.py
Outdated
Show resolved
Hide resolved
Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.yml
Outdated
Show resolved
Hide resolved
'''EXECUTION BLOCK''' | ||
try: | ||
if demisto.command() == 'test-module': | ||
test_function() | ||
if demisto.command() == 'aws-cloudtrail-create-trail': | ||
create_trail(demisto.args()) | ||
if demisto.command() == 'aws-cloudtrail-delete-trail': | ||
delete_trail(demisto.args()) | ||
if demisto.command() == 'aws-cloudtrail-describe-trails': | ||
describe_trails(demisto.args()) | ||
if demisto.command() == 'aws-cloudtrail-update-trail': | ||
update_trail(demisto.args()) | ||
if demisto.command() == 'aws-cloudtrail-start-logging': | ||
start_logging(demisto.args()) | ||
if demisto.command() == 'aws-cloudtrail-stop-logging': | ||
stop_logging(demisto.args()) | ||
if demisto.command() == 'aws-cloudtrail-lookup-events': | ||
lookup_events(demisto.args()) | ||
if demisto.command() == 'aws-cloudtrail-get-trail-status': | ||
get_trail_status(demisto.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.
Why did you remove the main()?
Please revert this change.
Also, the video you send me is not working for some reason.. please attach a new one.
Let's make sure to whole build flow is passing and that the video is attached and I'll move on with the pr.
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.
changes reverted. new video attached.
Screen.Recording.2024-03-05.mov
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.
@YuvHayun I am unsure why the tests are failing, let me know if additional changes are needed.
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 @kcelovic, I left some comments about places in the code that I think are failing for you.
Please also add tests to the new command you've added.
Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.py
Outdated
Show resolved
Hide resolved
def handle_returning_date_to_string(date_obj: datetime | str | None) -> str: | ||
"""Gets date object to string""" | ||
# if the returning date is a string leave it as is. | ||
if isinstance(date_obj, str): | ||
# if the returning date is a string or None, leave it as is. | ||
if date_obj is None or isinstance(date_obj, 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.
Something is odd about this logic, if date_obj is None and we return it then the function should support None return (in that case let's also make sure it doesn't break any b/c).
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 changes do you recommend here?
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 would remove the None option from the argument anotation.
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.
And the unit tests.
def handle_returning_date_to_string(date_obj: datetime | str | None) -> str: | ||
"""Gets date object to string""" | ||
# if the returning date is a string leave it as is. | ||
if isinstance(date_obj, str): | ||
# if the returning date is a string or None, leave it as is. | ||
if date_obj is None or isinstance(date_obj, 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.
I would remove the None option from the argument anotation.
Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.py
Outdated
Show resolved
Hide resolved
…l.py Co-authored-by: Yuval Hayun <[email protected]>
Great job @kcelovic! |
f3fdb70
into
demisto:contrib/kcelovic_get-trail-status
Thank you for your contribution. Your external PR has been merged and the changes are now included in an internal PR for further review. The internal PR will be merged to the master branch within 3 business days. |
* Get trail status (#32960) * "contribution update to pack "AWS - CloudTrail"" * made requested changes * Update Packs/AWS-CloudTrail/ReleaseNotes/1_1_0.md Co-authored-by: Yuval Hayun <[email protected]> * Update Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.yml Co-authored-by: Yuval Hayun <[email protected]> * Update Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.yml Co-authored-by: Yuval Hayun <[email protected]> * Update Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.py Co-authored-by: Yuval Hayun <[email protected]> * removed try & except under get_trail_status function * fixed indent * fixed typo * fixed typo * made requested changes * updated docker version * updated docker * fixed typos * reverted change on package-lock.json * reverted changes as requested * revert package-lock.json * update dockerimage * Update 1_1_0.md * Update AWS-CloudTrail.py * Update AWS-CloudTrail.yml * Update 1_1_0.md * Update AWS-CloudTrail.py * Update AWS-CloudTrail.py * Update AWS-CloudTrail.py * Update AWS-CloudTrail.py * Update Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.py Co-authored-by: Yuval Hayun <[email protected]> * Update AWS-CloudTrail_test.py * Update 1_1_0.md * Update AWS-CloudTrail.yml --------- Co-authored-by: xsoar-bot <[email protected]> Co-authored-by: Yuval Hayun <[email protected]> * revert package-lcok * remove docker line --------- Co-authored-by: kcelovic <[email protected]> Co-authored-by: xsoar-bot <[email protected]> Co-authored-by: Yuval Hayun <[email protected]> Co-authored-by: YuvHayun <[email protected]>
* Get trail status (demisto#32960) * "contribution update to pack "AWS - CloudTrail"" * made requested changes * Update Packs/AWS-CloudTrail/ReleaseNotes/1_1_0.md Co-authored-by: Yuval Hayun <[email protected]> * Update Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.yml Co-authored-by: Yuval Hayun <[email protected]> * Update Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.yml Co-authored-by: Yuval Hayun <[email protected]> * Update Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.py Co-authored-by: Yuval Hayun <[email protected]> * removed try & except under get_trail_status function * fixed indent * fixed typo * fixed typo * made requested changes * updated docker version * updated docker * fixed typos * reverted change on package-lock.json * reverted changes as requested * revert package-lock.json * update dockerimage * Update 1_1_0.md * Update AWS-CloudTrail.py * Update AWS-CloudTrail.yml * Update 1_1_0.md * Update AWS-CloudTrail.py * Update AWS-CloudTrail.py * Update AWS-CloudTrail.py * Update AWS-CloudTrail.py * Update Packs/AWS-CloudTrail/Integrations/AWS-CloudTrail/AWS-CloudTrail.py Co-authored-by: Yuval Hayun <[email protected]> * Update AWS-CloudTrail_test.py * Update 1_1_0.md * Update AWS-CloudTrail.yml --------- Co-authored-by: xsoar-bot <[email protected]> Co-authored-by: Yuval Hayun <[email protected]> * revert package-lcok * remove docker line --------- Co-authored-by: kcelovic <[email protected]> Co-authored-by: xsoar-bot <[email protected]> Co-authored-by: Yuval Hayun <[email protected]> Co-authored-by: YuvHayun <[email protected]>
Contributing to Cortex XSOAR Content
Make sure to register your contribution by filling the contribution registration form
The Pull Request will be reviewed only after the contribution registration form is filled.
Status
Related Issues
fixes: link to the issue
Description
A few sentences describing the overall goals of the pull request's commits.
Must have