[backend] Add Rocket.Chat backend #661

animeshk08 · 2020-05-02T17:53:34Z

Fixes: #543
This commit adds support to fetch
messages from a Rocket.Chat channel.

The tests have been added accordingly.
The usage docs have been updated.

Signed-off-by: Animesh Kumar [email protected]

coveralls · 2020-05-02T17:56:01Z

Coverage increased (+0.04%) to 97.833% when pulling a4155de on animeshk08:rocket-chat into 7f4cceb on chaoss:master.

animeshk08 · 2020-05-02T18:02:04Z

perceval/backends/core/rocketchat.py

+        if not self.from_archive:
+            self.sleep_for_rate_limit()
+
+        response = super().fetch(url, payload, headers=headers)


Hi @valeriocos, Please have a look at this line.
In case a user provides a wrong Channel Name or User Id, an HTTP error is raised here. In that case, the log includes error.response.reason which is not that deductive. The actual cause for the error is returned in the body of the response. Hence I initially made the below change.

try: response = super().fetch(url, payload, headers=headers) except requests.exceptions.HTTPError as error: logger.error(error.response.text) raise error

However, I think such a situation may happen in other APIs as well.(I found it in Pagure, Gitter, Groups.io when working with them) Hence, instead of addressing this in the backend a change in the core client should be addressed in separate PR.
WDYT?

I like the idea @animeshk08! I understand that the improvement should be made at https://github.com/chaoss/grimoirelab-perceval/blob/master/perceval/client.py#L182

Please open a new issue when you have time, and we can discuss there how to proceed, thanks!

Yes, I saw many error messages that were confusing in the backends.

Thank you @lukaszgryglicki for the feedback. The PR #672 tries to fix the problem, but the solution is still not good.

It would be great if you can add to the corresponding issue (#671), some examples of error messages that are confusing for you. Thanks

animeshk08 · 2020-05-02T18:02:50Z

Hi @valeriocos. Please have a look at this PR when you have time :)

valeriocos

Thank you @animeshk08 for taking over this PR. I left some comments, please add also the release note, thanks!

valeriocos · 2020-05-03T07:19:34Z

perceval/backends/core/rocketchat.py

+
+
+class RocketChat(Backend):
+    """Rocket Chat backend.


The upstream server is mentioned in the code as Rocket Chat, Rocket.chat and RocketChat. For consistency with the README, we should use Rocket.Chat.

This comment applies to the rest of the code (please look for Rocket in the code)

valeriocos · 2020-05-03T07:31:28Z

perceval/backends/core/rocketchat.py

+
+        This method parses a JSON stream, containing the history of
+        a channel, and returns a list with the parsed data. It also
+        returns if there are more messages that are not included on


The second sentence isn't clear, I guess it refers to total. If this is the case, a possible rewriting could be:

This method parses a JSON stream, containing the history of a channel. It returns a list of messages and the total messages count in that channel.

WDYT?

valeriocos · 2020-05-03T07:32:21Z

perceval/backends/core/rocketchat.py

+
+    @staticmethod
+    def parse_channel_info(raw_channel_info):
+        """Parse a user's info JSON stream.


The docstring isn't aligned with the title and logic of the method

valeriocos · 2020-05-03T07:42:38Z

perceval/backends/core/rocketchat.py

+    def has_resuming(cls):
+        """Returns whether it supports to resume the fetch process.
+
+        :returns: this backend does supports items resuming


typo does supports

valeriocos · 2020-05-03T07:52:08Z

perceval/backends/core/rocketchat.py

+        return response
+
+    def messages(self, channel, from_date, offset):
+        """Fetch messages from a channel."""


Could you add to this docstring the details how the messages are fetched (for instance, in ascending order based on the time they were last updated)? Thanks

I'm asking this because the value 1 in the sort params isn't clear. Info about that value is here https://rocket.chat/docs/developer-guides/rest-api/offset-and-count-and-sort-info/

valeriocos · 2020-05-03T08:03:10Z

perceval/backends/core/rocketchat.py

+        if not self.from_archive:
+            self.sleep_for_rate_limit()
+
+        response = super().fetch(url, payload, headers=headers)


I like the idea @animeshk08! I understand that the improvement should be made at https://github.com/chaoss/grimoirelab-perceval/blob/master/perceval/client.py#L182

Please open a new issue when you have time, and we can discuss there how to proceed, thanks!

valeriocos · 2020-05-03T08:09:02Z

perceval/backends/core/rocketchat.py

+    An API token and a User Id is required to access the server.
+
+    :param url: server url from where messages are to be fetched
+    :param user_id: generated user-id using your RocketChat account


Can we switch user_id with channel? The current sequence url, user_id, channel doesn't seem logical to me.

valeriocos · 2020-05-03T08:09:57Z

perceval/backends/core/rocketchat.py

+        action.required = True
+
+        # Required argument
+        parser.parser.add_argument('-u', '--user_id',


Why --user_id is separated from the other positional arguments? What's the difference?

When running perceval rocketchat --help, the user_id appears as an optional argument (see below), however it doesn't seem possible to fetch messages without providing an user_id.

[2020-05-03 11:06:14,424] - Sir Perceval is on his quest. usage: perceval [-h] [--category CATEGORY] [--tag TAG] [--filter-classified] [--from-date FROM_DATE] -t API_TOKEN [--archive-path ARCHIVE_PATH] [--no-archive] [--fetch-archive] [--archived-since ARCHIVED_SINCE] [--no-ssl-verify] [-o OUTFILE] [--json-line] [-u USER_ID] [--max-items MAX_ITEMS] [--sleep-for-rate] [--min-rate-to-sleep MIN_RATE_TO_SLEEP] url channel positional arguments: url URL of the RocketChat server channel RocketChat channel(room) name optional arguments: -h, --help show this help message and exit -u USER_ID, --user_id USER_ID User Id to fetch messages ...

If line 377 is changed to the code below, the user_id appears as a positional argument

parser.parser.add_argument('user_id', help="User Id to fetch messages")

valeriocos · 2020-05-03T08:16:33Z

tests/test_rocketchat.py

+        self.assertEqual(backend.max_items, MAX_ITEMS)
+
+        # When tag is empty or None it will be set to
+        # the value in uri


URI should be URL

valeriocos · 2020-05-03T08:57:00Z

tests/test_rocketchat.py

+        self.assertEqual(client.min_rate_to_sleep, 1)
+
+    @httpretty.activate
+    def test_message(self):


missing s at the end.

animeshk08 · 2020-05-03T11:34:30Z

Hi @valeriocos I have made the updates requested, please have a look :)

valeriocos

Thank you @animeshk08 for addressing the previous comments! Please have a look at the last ones!

Feel free to start the development of the ELK connectors, so we can have an early evaluation of this new backend (and provide corrections if needed before merging this PR).

Thanks!

valeriocos · 2020-05-03T15:51:08Z

perceval/backends/core/rocketchat.py

+        action.required = True
+
+        # Required positional arguments
+        parser.parser.add_argument('user_id',


This should be after channel to be consistent with the position of the params at https://github.com/chaoss/grimoirelab-perceval/pull/661/files#diff-5933efe10c89e406b289947bcf03c4e5R73

valeriocos · 2020-05-03T15:52:20Z

README.md

+
+Rocket.Chat backend needs an API token and a User Id to authenticate to the server.
+```
+$ perceval rocketchat -t 'abchdefghij' --from-date '2020-05-02' 1234abcd https://open.rocket.chat general


1234abcd should be at the end

animeshk08 · 2020-05-03T16:22:40Z

Thank you for your reviews @valeriocos. I have made the changes!

Feel free to start the development of the ELK connectors, so we can have an early evaluation of this new backend (and provide corrections if needed before merging this PR).

Sure I am on it!

valeriocos

Hi @animeshk08 , I hope you are fine!

I understand that you are working on chaoss/grimoirelab-elk#876, and I have some feeback regarding it.

I made a quick test to see how the current Perceval implementation will shape how to pass the params via the projects.json. If we keep it as it is, we should include the user id in the projects.json as follows:

       "rocketchat": [
            "https://open.rocket.chat general AvCH4MNPFbXQ6nSQA"
        ]

However, since the same user_id can be used to fetch more channels, the projects.json will contain redundant information (the user_id will appear for every channel we are fetching). I would suggest to apply the change below to the user_id argument (as similarly done for groupsio).

diff --git a/perceval/backends/core/rocketchat.py b/perceval/backends/core/rocketchat.py
index 5b9a933..b41f302 100644
--- a/perceval/backends/core/rocketchat.py
+++ b/perceval/backends/core/rocketchat.py
@@ -385,7 +385,7 @@ class RocketChatCommand(BackendCommand):
         parser.parser.add_argument('channel',
                                    help="Rocket.Chat channel(room) name")
 
-        parser.parser.add_argument('user_id',
+        parser.parser.add_argument('-u', '--user-id', dest='user_id',
                                    help="User Id to fetch messages")

With this modification, the projects.json will look like the one below, and we will be able to put the user_id in the setup.cfg, WDYT?

       "rocketchat": [
            "https://open.rocket.chat general"
        ]

[rocketchat]
api-token = ...
raw_index = rocketchat_today_raw
user-id = AvCH4MNPFbXQ6nSQA
enriched_index = rocketchat_today_enriched
sleep-for-rate = true

Thanks for working on the support for RocketChat!

valeriocos · 2020-05-23T08:59:53Z

perceval/backends/core/rocketchat.py

+        'channel_id': ['channel_info', '_id']
+    }
+
+    def __init__(self, url=None, channel=None, user_id=None, api_token=None,


The following params are mandatory, the default value can be removed
def __init__(self, url, channel, user_id, api_token, ..)

WDYT?

Thank you, I have updated it :)

valeriocos · 2020-05-23T09:02:23Z

perceval/backends/core/rocketchat.py

+    PCOUNT = "count"
+    POLDEST = "oldest"
+
+    def __init__(self, url=None, user_id=None, api_token=None, max_items=MAX_ITEMS,


Similar comment as above, if url, user_id and api_token are mandatory, the default values are not needed.

Thank you, I have updated it :)

This commit adds support to fetch messages from a Rocket.Chat channel. The tests have been added accordingly. The usage docs have been updated. Release notes have also been added. Signed-off-by: Animesh Kumar <[email protected]>

animeshk08 · 2020-05-26T21:55:14Z

Reply #661 (review)

I had the same thought process, while initially working on this backend(#661 (comment)).
I missed to justify the reason at that time, Thank you for pointing this out :)

valeriocos

LGTM, thanks @animeshk08 ! Nice work!

valeriocos · 2020-05-27T08:40:40Z

Coveralls reports that the test coverage went down after merging, however the build reports a different info https://coveralls.io/builds/31056088

I have run the coverage locally and rocketchat is fully covered:

Name                                                                                                                                             Stmts   Miss  Cover   Missing
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/slimbook/Escritorio/sources/perceval/perceval/backends/core/rocketchat.py                                                                    133      0   100%

animeshk08 commented May 2, 2020

View reviewed changes

valeriocos reviewed May 3, 2020

View reviewed changes

animeshk08 force-pushed the rocket-chat branch from 902958b to 2b88893 Compare May 3, 2020 11:31

valeriocos reviewed May 3, 2020

View reviewed changes

animeshk08 force-pushed the rocket-chat branch from 2b88893 to adb8602 Compare May 3, 2020 16:18

vchrombie mentioned this pull request May 17, 2020

[Rocket.Chat] Add rocketchat backend support to ELK chaoss/grimoirelab-elk#876

Closed

valeriocos self-requested a review May 23, 2020 08:57

valeriocos reviewed May 23, 2020

View reviewed changes

animeshk08 force-pushed the rocket-chat branch 2 times, most recently from a6e883a to 4b2496e Compare May 26, 2020 21:48

[backend] Add Rocket.Chat backend

a4155de

This commit adds support to fetch messages from a Rocket.Chat channel. The tests have been added accordingly. The usage docs have been updated. Release notes have also been added. Signed-off-by: Animesh Kumar <[email protected]>

animeshk08 force-pushed the rocket-chat branch from 4b2496e to a4155de Compare May 26, 2020 21:52

animeshk08 mentioned this pull request May 26, 2020

[rocketchat] Add support for RocketChat chaoss/grimoirelab-elk#882

Closed

valeriocos self-requested a review May 27, 2020 08:20

valeriocos approved these changes May 27, 2020

View reviewed changes

valeriocos closed this in 48eff53 May 27, 2020

valeriocos merged commit 48eff53 into chaoss:master May 27, 2020

valeriocos mentioned this pull request Jun 16, 2020

Integrating taiga to Perceval fioddor/taiga-perceval-backend#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[backend] Add Rocket.Chat backend #661

[backend] Add Rocket.Chat backend #661

animeshk08 commented May 2, 2020

coveralls commented May 2, 2020 •

edited

Loading

animeshk08 May 2, 2020

valeriocos May 3, 2020

lukaszgryglicki Jun 16, 2020

valeriocos Jun 16, 2020

animeshk08 commented May 2, 2020

valeriocos left a comment

valeriocos May 3, 2020

valeriocos May 3, 2020

valeriocos May 3, 2020

valeriocos May 3, 2020

valeriocos May 3, 2020

valeriocos May 3, 2020

valeriocos May 3, 2020

valeriocos May 3, 2020

valeriocos May 3, 2020

valeriocos May 3, 2020

valeriocos May 3, 2020

animeshk08 commented May 3, 2020

valeriocos left a comment

valeriocos May 3, 2020

valeriocos May 3, 2020

animeshk08 commented May 3, 2020

valeriocos left a comment •

edited

Loading

valeriocos May 23, 2020

animeshk08 May 26, 2020

valeriocos May 23, 2020

animeshk08 May 26, 2020

animeshk08 commented May 26, 2020

valeriocos left a comment

valeriocos commented May 27, 2020 •

edited

Loading

[backend] Add Rocket.Chat backend #661

[backend] Add Rocket.Chat backend #661

Conversation

animeshk08 commented May 2, 2020

coveralls commented May 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

animeshk08 commented May 2, 2020

valeriocos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

animeshk08 commented May 3, 2020

valeriocos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

animeshk08 commented May 3, 2020

valeriocos left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

animeshk08 commented May 26, 2020

valeriocos left a comment

Choose a reason for hiding this comment

valeriocos commented May 27, 2020 • edited Loading

coveralls commented May 2, 2020 •

edited

Loading

valeriocos left a comment •

edited

Loading

valeriocos commented May 27, 2020 •

edited

Loading