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

Penfield AI Pull Request 2 #15523

Merged
merged 17 commits into from
Nov 30, 2021

Conversation

penfield-chan
Copy link
Contributor

Followup to: #15074

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

  • In Progress
  • Ready
  • In Hold - (Reason for hold)

Related Issues

11 issues were mentioned in the last PR. All should be addressed here. Unclear why test failed in remote environment, may need updated results for CircleCI to determine why.

Description

Resolve the 11 identified issues, perform some other supporting changes.

Screenshots

Paste here any images that will help the reviewer

Minimum version of Cortex XSOAR

  • [X ] 6.0.0
  • 6.1.0
  • 6.2.0
  • 6.5.0

Does it break backward compatibility?

  • Yes
    • Further details:
  • [X ] No

Must have

  • [ X] Tests
  • [X ] Documentation

@content-bot content-bot added the Contribution Thank you! Contributions are always welcome! label Oct 27, 2021
@content-bot content-bot changed the base branch from master to contrib/penfield-chan_master October 27, 2021 17:02
@content-bot
Copy link
Collaborator

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 @ShacharKidor will know he can start review the proposed changes.

@daryakoval daryakoval requested review from daryakoval and removed request for ShacharKidor October 28, 2021 08:58
Packs/PenfieldAI/Integrations/Penfield/Penfield.py Outdated Show resolved Hide resolved
Packs/PenfieldAI/Integrations/Penfield/Penfield.py Outdated Show resolved Hide resolved

if demisto.command() == 'test-module':
result = test_api(client)
if result == 'healthy':
Copy link
Contributor

Choose a reason for hiding this comment

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

The API response can be healthy ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've converted it a more standard 0=healthy and 1=error

Packs/PenfieldAI/Integrations/Penfield/Penfield.py Outdated Show resolved Hide resolved

# get online analyst ids
analysts = demisto.executeCommand('getUsers', {'online': True})[0]['Contents']
analyst_ids = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Is analyst_id supposed to be array? or comma-seperated string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is supposed to be an array of strings.

@daryakoval daryakoval added Partner Contribution Form Filled Whether contribution form filled or not. labels Oct 28, 2021
@edik24 edik24 mentioned this pull request Oct 31, 2021
11 tasks
@penfield-chan
Copy link
Contributor Author

I have added my code changes which should address the questions. Let me know if you spot anything amiss.

Thanks.

@daryakoval
Copy link
Contributor

@penfield-chan Hi, let’s schedule a demo call.

@content-bot
Copy link
Collaborator

This PR is starting to get a little stale. @penfield-chan are there any changes you wanted to make since @daryakoval's last comment?

@daryakoval
Copy link
Contributor

Re-scheduled demo for next week.

@content-bot
Copy link
Collaborator

This PR is starting to get a little stale. @penfield-chan are there any changes you wanted to make since @daryakoval's last comment?

@daryakoval
Copy link
Contributor

Hi, we have couple comments from demo:

  1. Consider to add playbook instead or in addition to script.
  2. Change input for argument analyst_ids from string array to comma separated, for example: admin,darya,penfield. You can use argToList function from CommonServerPython module to change comma-seperated string to list.



def get_assignee(client: Client, args) -> CommandResults:
analyst_ids = args.get('analyst_ids')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
analyst_ids = args.get('analyst_ids')
analyst_ids = argToList(args.get('analyst_ids'))

@content-bot
Copy link
Collaborator

This PR is starting to get a little stale. @penfield-chan are there any changes you wanted to make since @daryakoval's last comment?

L-Tan and others added 4 commits November 23, 2021 13:39
PR review updates: 1) update analyst_ids argument to use comma-separated values in a string instead of list. 2) add basic Penfield Assign playbook
@CLAassistant
Copy link

CLAassistant commented Nov 26, 2021

CLA assistant check
All committers have signed the CLA.

@L-Tan
Copy link
Contributor

L-Tan commented Nov 26, 2021

@daryakoval sorry for the delayed update on this PR. Two main changes are introduced after the demo:

  1. Use comma-separated list for the arg
  2. Add a simple Penfield Assign playbook

Could you please take another look? Thanks a lot.

Update fromversion to 6.0.0

Co-authored-by: Darya Koval <[email protected]>
@daryakoval daryakoval merged commit 354e1a4 into demisto:contrib/penfield-chan_master Nov 30, 2021
@content-bot content-bot mentioned this pull request Nov 30, 2021
7 tasks
daryakoval added a commit that referenced this pull request Nov 30, 2021
* Penfield AI Pull Request 2 (#15523)

* penfield init

* issue 1: remove unowned files from pack-ignore

* address pull request issues

* address pull request issues 2

* remove code comment

* udpate command results

* change healthy response code

* xflake and unittest work

* Update penfield content pack metadata with company info

* Update to use string as analyst_ids arg

* Add Penfield Assign playbook

* Add playbook README with the playbook image (#2)

* use argToList helper instead of split

* Update Packs/PenfieldAI/Playbooks/Penfield_Assign.yml

Update fromversion to 6.0.0

Co-authored-by: Darya Koval <[email protected]>

Co-authored-by: the-naturel <[email protected]>
Co-authored-by: Long <[email protected]>
Co-authored-by: Long Tan <[email protected]>
Co-authored-by: Darya Koval <[email protected]>

* formatting files

Co-authored-by: penfield-chan <[email protected]>
Co-authored-by: the-naturel <[email protected]>
Co-authored-by: Long <[email protected]>
Co-authored-by: Long Tan <[email protected]>
Co-authored-by: Darya Koval <[email protected]>
Co-authored-by: Darya Koval <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution Form Filled Whether contribution form filled or not. Contribution Thank you! Contributions are always welcome! docs-approved Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants