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

[Feedly] Initial Integration #29822

Conversation

Mathieu4141
Copy link
Contributor

@Mathieu4141 Mathieu4141 commented Sep 21, 2023

Contributing to Cortex XSOAR Content

Status

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

Related Issues

N/A

Description

Adding an integration with Feedly. The integration will pull articles with entities, IoCs and their relationships, and create the corresponding indicators in XSOAR.

Must have

  • Tests
  • Documentation

@CLAassistant
Copy link

CLAassistant commented Sep 21, 2023

CLA assistant check
All committers have signed the CLA.

@MichaelYochpaz
Copy link
Contributor

Hi @Mathieu4141, thank you for your contribution.
Your PR review will be slightly delayed since it is the holiday season in Israel, and we have a holiday this weekend (21th-25th of September), I apologize in advance.

@MichaelYochpaz
Copy link
Contributor

Hey @Mathieu4141, thank you for the contribution.
Please fill the contribution form. Without it, we cannot start the contribution process.

@MichaelYochpaz MichaelYochpaz added the pending-contributor The PR is pending the response of its creator label Sep 26, 2023
@Mathieu4141
Copy link
Contributor Author

Hey @Mathieu4141, thank you for the contribution. Please fill the contribution form. Without it, we cannot start the contribution process.

Done! Sorry I was missing some details to fill it before
Let me know if you need anything more before reviewing

@MichaelYochpaz MichaelYochpaz removed the pending-contributor The PR is pending the response of its creator label Sep 26, 2023
@MichaelYochpaz
Copy link
Contributor

@Mathieu4141 Hey, looks like it's good to go for a review now, and you've completed all the required tasks :)
Please note that there might be a few days delay in the review process since the contribution PRs queue is a bit overloaded.
I apologize in advance

Copy link
Contributor

@MichaelYochpaz MichaelYochpaz left a comment

Choose a reason for hiding this comment

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

Hey, I'm still reviewing the code, but there are a few notes I made that can be addressed in the meantime.

Please also pull and merge from master to your branch, and fix the errors here:
https://github.com/demisto/content/actions/runs/6263315560/job/17011882198?pr=29822

Feel free to reach out to me here or on the DFIR Slack if you need any help.

Packs/Feedly/Integrations/Feedly/Feedly.py Outdated Show resolved Hide resolved
Packs/Feedly/Integrations/Feedly/README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
https://api.feedly.com
https://feedly.com
base_url=FEEDLY_BASE_URL,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that added here? Is this line detected as a secret?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it was detected as a secret

Packs/Feedly/Integrations/Feedly/Feedly.py Outdated Show resolved Hide resolved
Packs/Feedly/Integrations/Feedly/Feedly_test.py Outdated Show resolved Hide resolved
@MichaelYochpaz MichaelYochpaz added the pending-contributor The PR is pending the response of its creator label Oct 2, 2023
Copy link
Contributor

@MichaelYochpaz MichaelYochpaz left a comment

Choose a reason for hiding this comment

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

Some additional notes

Packs/Feedly/Integrations/Feedly/Feedly.yml Outdated Show resolved Hide resolved
Packs/Feedly/Integrations/Feedly/Feedly.yml Outdated Show resolved Hide resolved
Packs/Feedly/Integrations/Feedly/Feedly.yml Outdated Show resolved Hide resolved
Packs/Feedly/Integrations/Feedly/Feedly.py Outdated Show resolved Hide resolved
Packs/Feedly/Integrations/Feedly/Feedly.yml Outdated Show resolved Hide resolved
Packs/Feedly/Integrations/Feedly/Feedly.py Outdated Show resolved Hide resolved
Packs/Feedly/Integrations/Feedly/Feedly.py Outdated Show resolved Hide resolved
Copy link
Contributor

@MichaelYochpaz MichaelYochpaz left a comment

Choose a reason for hiding this comment

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

Some additional notes following the last changes. (seems like you've replaced the name of the files using the IDE, and it changed them in some unwanted places?)

Please also note the failing validations and linting error.

Packs/FeedFeedly/pack_metadata.json Outdated Show resolved Hide resolved
Packs/FeedFeedly/README.md Outdated Show resolved Hide resolved
Packs/FeedFeedly/Integrations/FeedFeedly/README.md Outdated Show resolved Hide resolved
Packs/FeedFeedly/Integrations/FeedFeedly/README.md Outdated Show resolved Hide resolved
Packs/FeedFeedly/Integrations/FeedFeedly/FeedFeedly.py Outdated Show resolved Hide resolved
Packs/FeedFeedly/Integrations/FeedFeedly/FeedFeedly.py Outdated Show resolved Hide resolved
Packs/FeedFeedly/Integrations/FeedFeedly/FeedFeedly.py Outdated Show resolved Hide resolved
@MichaelYochpaz MichaelYochpaz removed the pending-contributor The PR is pending the response of its creator label Oct 3, 2023
@MichaelYochpaz MichaelYochpaz changed the title [feedly] initial integration [Feedly] Initial Integration Oct 4, 2023
@MichaelYochpaz MichaelYochpaz added the pending-contributor The PR is pending the response of its creator label Oct 5, 2023
@MichaelYochpaz
Copy link
Contributor

MichaelYochpaz commented Oct 8, 2023

@Mathieu4141 Hey, you have 2 failing linting / unit-test validations:

  1. A mypy error:
/home/circleci/project/Packs/FeedFeedly/Integrations/FeedFeedly/FeedFeedly.py:691:72: error:
Argument 1 to "is_valid_type" of "RelationshipsTypes" has incompatible type
"Optional[Any]"; expected "str"  [arg-type]
    ...ntityRelationship.RelationshipsTypes.is_valid_type(relationship_type):
                                                          ^~~~~~~~~~~~~~~~~
Found 1 error in 1 file (checked 1 source file)
  1. Your unit-test coverage is too low. We require a minimum of 70% coverage (currently at 59.8%).

Please fix :)

@MichaelYochpaz MichaelYochpaz added ready-for-instance-test In contribution PRs, this label will cause a trigger of a build with a modified pack from the PR. and removed pending-contributor The PR is pending the response of its creator labels Oct 9, 2023
@Ni-Knight
Copy link
Contributor

Ni-Knight commented Oct 12, 2023

Hi @Mathieu4141 I would put it a bit different
image

Main on the left is a single field section, only Description MD, all the rest on the left in that order. That is also XSOAR best practice.

JSON:
layoutscontainer-Feedly_Report.json

And maybe an image of Unit42 report next time and not Sentinel One ;)

@MichaelYochpaz
Copy link
Contributor

@Mathieu4141 Also note that I don't see you've addressed the previously mentioned limit parameter issue in your recent changes, so I'm assuming it is still unfixed?

Adding @melamedbn to the review since files that are reviewed by our security team (custom layout and indicator type) were added.

@melamedbn melamedbn self-assigned this Oct 15, 2023
@MichaelYochpaz
Copy link
Contributor

@Mathieu4141 Please fix the following failing validations.
All should be easily fixed by running:

demisto-sdk format -i "Packs/FeedFeedly/Layouts/layoutscontainer-FeedlyReport.json" 

Other than that, it looks good to me and ready for a merge (assuming the others don't have any additional notes).
Good job :)

@MichaelYochpaz MichaelYochpaz added the pending-contributor The PR is pending the response of its creator label Oct 16, 2023
Copy link
Contributor

@MichaelYochpaz MichaelYochpaz left a comment

Choose a reason for hiding this comment

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

Looks good to me, good job :)
Please make sure to re-test the layout & reports and that they work properly following your last changes.

Waiting for @melamedbn's and @Ni-Knight's approvals for a merge.

@MichaelYochpaz MichaelYochpaz removed the pending-contributor The PR is pending the response of its creator label Oct 16, 2023
@Mathieu4141
Copy link
Contributor Author

Looks good to me, good job :) Please make sure to re-test the layout & reports and that they work properly following your last changes.

Yes everything works correctly!

Thanks for all your reviews!

Copy link
Contributor

@Ni-Knight Ni-Knight left a comment

Choose a reason for hiding this comment

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

Only one comment about tags.

Packs/FeedFeedly/pack_metadata.json Outdated Show resolved Hide resolved
Copy link
Contributor

@Ni-Knight Ni-Knight left a comment

Choose a reason for hiding this comment

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

Well done.

@MichaelYochpaz MichaelYochpaz added ready-for-instance-test In contribution PRs, this label will cause a trigger of a build with a modified pack from the PR. and removed ready-for-instance-test In contribution PRs, this label will cause a trigger of a build with a modified pack from the PR. labels Oct 17, 2023
@melamedbn
Copy link
Contributor

Hi @Mathieu4141,

We'll be ready if you can fix the panels' positioning in the layout grid, so we won't have blank spaces and add a layout description.
Take the following screenshot as an optional example:

image

Adjusted
image

Best regards,
Ben

@MichaelYochpaz MichaelYochpaz added ready-for-instance-test In contribution PRs, this label will cause a trigger of a build with a modified pack from the PR. and removed ready-for-instance-test In contribution PRs, this label will cause a trigger of a build with a modified pack from the PR. labels Oct 22, 2023
@MichaelYochpaz MichaelYochpaz merged commit befe3f8 into demisto:contrib/Mathieu4141_feedly/create-integration Oct 22, 2023
22 of 24 checks passed
MichaelYochpaz added a commit that referenced this pull request Oct 22, 2023
* [Feedly] Initial Integration (#29822)

* [feedly] initial integration

* [feedly] pr.review

* [feedly] address pre commit checks

* [feedly] fix image sizes

* [feedly] pr.review

* [feedly] pr.review

* [feedly] pr.review

* [feedly] update image

* [feedly] increase coverage and fix typing

* [feedly] fix relationships and test it

* [feedly] fix indicators imit

* [feedly] add TTP and threat tags

* [feedly] order of params

* [feedly] Add custom FeedlyReport for better visualization

* [feedly] fixes

* [feedly] fix feedyl report layout

* [feedly] add to free feeds

* [feedly] (layout) pr.review

* [feedly] format layout file

* [feedly] update image

---------

Co-authored-by: Michael Yochpaz <8832013+MichaelYochpaz@users.noreply.github.com>

* Bump Docker image

---------

Co-authored-by: Mathieu Béligon <beligonmathieu@gmail.com>
Co-authored-by: Michael Yochpaz <8832013+MichaelYochpaz@users.noreply.github.com>
sapirshuker pushed a commit that referenced this pull request Dec 21, 2023
* [Feedly] Initial Integration (#29822)

* [feedly] initial integration

* [feedly] pr.review

* [feedly] address pre commit checks

* [feedly] fix image sizes

* [feedly] pr.review

* [feedly] pr.review

* [feedly] pr.review

* [feedly] update image

* [feedly] increase coverage and fix typing

* [feedly] fix relationships and test it

* [feedly] fix indicators imit

* [feedly] add TTP and threat tags

* [feedly] order of params

* [feedly] Add custom FeedlyReport for better visualization

* [feedly] fixes

* [feedly] fix feedyl report layout

* [feedly] add to free feeds

* [feedly] (layout) pr.review

* [feedly] format layout file

* [feedly] update image

---------

Co-authored-by: Michael Yochpaz <8832013+MichaelYochpaz@users.noreply.github.com>

* Bump Docker image

---------

Co-authored-by: Mathieu Béligon <beligonmathieu@gmail.com>
Co-authored-by: Michael Yochpaz <8832013+MichaelYochpaz@users.noreply.github.com>
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! External PR Partner Partner-Approved post-demo ready-for-instance-test In contribution PRs, this label will cause a trigger of a build with a modified pack from the PR. Security Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants