Skip to content
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

[illusive-networks-637] MarketPlace Integration #7772

Merged
merged 7 commits into from
Jul 19, 2020
Merged

[illusive-networks-637] MarketPlace Integration #7772

merged 7 commits into from
Jul 19, 2020

Conversation

adiozer
Copy link
Contributor

@adiozer adiozer commented Jun 29, 2020

Status

  • [] In Progress
  • [X ] Ready
  • In Hold - (Reason for hold)

Related Issues

fixes: link to the issue

Description

A few sentences describing the overall goals of the pull request's commits.

Screenshots

Paste here any images that will help the reviewer

Minimum version of Demisto

  • 4.5.0
  • [X ] 5.0.0
  • 5.5.0

Does it break backward compatibility?

  • Yes
    • Further details:
  • [X ] No

Must have

  • [ X] Tests
  • [ X] Documentation

Demisto Partner?

  • [X ] The title must be in the following format: [YOUR_PARTNER_ID] short description

@content-bot content-bot added the Contribution Thank you! Contributions are always welcome! label Jun 29, 2020
@content-bot content-bot changed the base branch from master to contrib/adiozer_illuisvenetworks_marketplace_integration June 29, 2020 16:29
@content-bot
Copy link
Collaborator

Thank you for your contribution. Your generosity and caring are unrivaled! Rest assured - our content wizard @ronykoz will very shortly look over your proposed changes.

@content-bot
Copy link
Collaborator

The CircleCI check from your latest pushed commit was unsuccessful. @adiozer take a look at the build by clicking this link.


Failed Build Steps

  • Validate Files and Yaml
  • Download Configuration: Verify Base Branch for Contribution
  • Verify Base Branch for Contribution

Try and address the listed CircleCI build step failures at your earliest convenience. This will greatly expedite the process of getting your proposed changes merged into master. Happy coding and may the force be with you.

@fvigo fvigo changed the title IllusiveNetworks MarketPlace Integration [illusive-networks-637] MarketPlace Integration Jun 30, 2020
@content-bot
Copy link
Collaborator

The CircleCI build failed again. @adiozer take a look at the build details here - and try and fix the issues so that we can merge your proposed changes as soon as possible.


Failed Build Steps

  • Validate Files and Yaml

Copy link
Contributor

@ronykoz ronykoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!
I left few comments about the code, let me know if you have any question or if you need my help.

Shortly we will add someone to go over the playbooks and the incident fields

@adiozer
Copy link
Contributor Author

adiozer commented Jul 5, 2020

@ronykoz done fixing + added some comments.
Thanks

@michalgold michalgold requested a review from mayagoldb July 5, 2020 11:52
@mayagoldb
Copy link
Contributor

@adiozer Notes regarding the Illusive-Data-Enrichment playbook:

  1. All "ending tasks" (tasks with no following tasks after them) should be linked to a 'Done' section header
  2. The input validation task - I think there is no need for it since the check for the incident id happens later, you can add the check for ip or fqdn in the other path. Also if you choose to leave it there, why is there a duplicate check? (the and under the task itself)
  3. Following 2 - After task 'Check input - incident id' - in the else path, you can add a check for other input (is there 'fqdn_or_ip')
  4. Input validation error section header - What is the goal here? Would you like to raise error / go to the end of the playbook (Done) / Add a manual task here for the analyst to review?
  5. The 3 illusive-get-forensics-x tasks - no need to duplicate (or actually add them 3 times), you can link all the path to the same 3 tasks since they are accepting the same input.
  6. Task names - relevant for all task names, should not be illusive-get-forensics but Illusive - get forensics
  7. I think there should be a check for incident ID between tasks #5 and #11 (for example if the IP doesn't have any related incidents the following tasks will fail)
  8. For task #15 - Will this always output event ID? if not there should be a check for event ID before the next tasks.

@mayagoldb
Copy link
Contributor

@adiozer Notes regarding the Illusive-Incident-Escalation playbook:

  1. DeleteContext task in the beginning of the playbook - useful for testing, should be removed in the released playbook
  2. Relevant for all set output and set score tasks - these are a bit confusing, it can help understand the playbook's flow if you rename to a clearer task name (even Set context key to True/ False will help for example)
  3. Task names - relevant for all task names, should not be illusive-get-forensics but Illusive - get forensics
  4. Please add a Done section header to end of playbook.

@adiozer
Copy link
Contributor Author

adiozer commented Jul 7, 2020

@mayagoldb Notes regarding the Illusive-Data-Enrichment playbook:
2. the duplicate check is for checking if we're getting only one input. in case of none or both we get to Error (change to Done -> comment #4))
3. I has both checks before but removed the check fqdn task. if I know I get exactly one input (by task#8) and it's not incident id-> it will be for sure fqdn

@adiozer
Copy link
Contributor Author

adiozer commented Jul 8, 2020

@mayagoldb fixed your comments

@content-bot
Copy link
Collaborator

The CircleCI build failed again. @adiozer take a look at the build details here - and try and fix the issues so that we can merge your proposed changes as soon as possible.


Failed Build Steps

  • Validate Files and Yaml
  • Uploading test results: Run Unit Testing and Lint

@ronykoz
Copy link
Contributor

ronykoz commented Jul 8, 2020

@adiozer you can ignore the build failure in the validate files
the error you have there is validation failure:

Packs/IllusiveNetworks/ReleaseNotes/1_0_2.md: [RN107] - No release note entry was found for the layouts "Illusive Networks Incident" in the IllusiveNetworks pack. Please rerun the update-release-notes command without -u to generate an updated template. If you are trying to exclude an item from the release notes, please refer to the documentation found here - https://xsoar.pan.dev/docs/integrations/changelog#excluding-items

And regarding the lint:

IllusiveNetworks
 
/home/circleci/project/Packs/IllusiveNetworks/Integrations/IllusiveNetworks/IllusiveNetworks.py:665:131: E501 line too long (132 > 130 characters)
 
/home/circleci/project/Packs/IllusiveNetworks/Integrations/IllusiveNetworks/IllusiveNetworks.py:746:47: E261 at least two spaces before inline comment
 

@content-bot
Copy link
Collaborator

The CircleCI build failed again. @adiozer take a look at the build details here - and try and fix the issues so that we can merge your proposed changes as soon as possible.


Failed Build Steps

  • Validate Files and Yaml

Copy link
Contributor

@ronykoz ronykoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good from my side, @mayagoldb all yours

@content-bot
Copy link
Collaborator

The CircleCI build failed again. @adiozer take a look at the build details here - and try and fix the issues so that we can merge your proposed changes as soon as possible.


Failed Build Steps

  • Validate Files and Yaml


## Playbook Image
---
![Illusive - Incident Escalation](Insert the link to your image here)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should update the link to your image, and i don't think you are using the two new images

@mayagoldb
Copy link
Contributor

mayagoldb commented Jul 13, 2020

@adiozer Awesome! One last small note for the Illusive - Data Enrichment playbook:

  1. The Done section header - our convention is to create one Done section header and have all the "ending tasks" lead to it, you can look at Palo Alto Networks - Malware Remediation playbook as an example for this

Also this is more of a generic question - I reviewed the classifier & layout and it looks good, my question is if there is a playbook (can be one of the playbooks we have here) that you want to assign to the Illusive Networks Incident type so it will run automatically once an incident of this type is created?

@content-bot
Copy link
Collaborator

The CircleCI build failed again. @adiozer take a look at the build details here - and try and fix the issues so that we can merge your proposed changes as soon as possible.


Failed Build Steps

  • Validate Files and Yaml

@ronykoz
Copy link
Contributor

ronykoz commented Jul 15, 2020

@adiozer got the following error in the validate run:

Packs/IllusiveNetworks/ReleaseNotes/1_0_2.md: [RN107] - No release note entry was found for the layout "Illusive Networks Incident" in the IllusiveNetworks pack. Please rerun the update-release-notes command without -u to generate an updated template. If you are trying to exclude an item from the release notes, please refer to the documentation found here - https://xsoar.pan.dev/docs/integrations/changelog#excluding-items

once it's sorted out we can merge

@content-bot
Copy link
Collaborator

The CircleCI build failed again. @adiozer take a look at the build details here - and try and fix the issues so that we can merge your proposed changes as soon as possible.


Failed Build Steps

  • Validate Files and Yaml

@ronykoz ronykoz merged commit 8c41f67 into demisto:contrib/adiozer_illuisvenetworks_marketplace_integration Jul 19, 2020
ronykoz pushed a commit that referenced this pull request Aug 2, 2020
* [illusive-networks-637] MarketPlace Integration (#7772)

* IllusiveNetworks MarketPlace Integration

* IllusiveNetworks readme fix

* IllusiveNetworks Marketplace CR fixes

* lint fixes

* IllusiveNetworks add images

* IllusiveNetworks CR and description fixes

* fix release note

* updated release notes

* updated readme

* Illusive Networks PR: updated layout and playbooks to fix 6.0 build (#8159)

* updated layout and playbooks to fix 6.0 build

* fixed rel notes

* update rn

* fix yml

* Fix raw response (#8254)

* Fix raw response

* fixed unit tests and linting

* remove space

Co-authored-by: adiozer <[email protected]>
Co-authored-by: ronykoz <[email protected]>
Co-authored-by: Francesco Vigo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution Thank you! Contributions are always welcome!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants