[gitlabcomments] Add enricher to handle gitlab comments#881
[gitlabcomments] Add enricher to handle gitlab comments#881vchrombie wants to merge 1 commit intochaoss:mainfrom
Conversation
|
The PR has a lot of pending work, I will be working on this. I will mark ready for review after completing some good work. Please let me know if you are having any suggestions. |
|
Hi @vchrombie , please ping me when I can review this PR. I had a look at the current status and the start looks promising :) WRT tests (if this can be of any help), you should run Perceval and copy some docs to a test file, which will be finally put in the folder tests/data. The name is important since it is used to automatically identify the file and load its content (https://github.com/chaoss/grimoirelab-elk/blob/master/tests/base.py#L137) If needed you can tweak the test data (e.g., changing value attributes) to make sure that all code is covered when running the tests. |
|
Hi @valeriocos |
|
Thanks for the update @vchrombie |
|
I will mark this PR ready-for-review once I complete the |
95edbc8 to
c61362e
Compare
Pull Request Test Coverage Report for Build 1227039055
💛 - Coveralls |
valeriocos
left a comment
There was a problem hiding this comment.
Hi @vchrombie, thank you for advancing with this PR. I tested it on a battery of gitlab repos (taken from the gitlab:issue section at https://gitlab.com/Bitergia/c/gitlab/sources/-/blob/master/projects.json).
After fixing some minor errors (check comments), the execution was working fine.
I also checked that the url generated for comments work fine and allows to jump on the original comment on gitlab:
"sub_type" : "issue_comment",
"body" : "@sabbap \"This merge request at least does something with the chevron until @JobV returns from his vacation when he can implement the scrolling.\"\r\n\r\n@JobV your call if this is good enough or not",
"body_analyzed" : "@sabbap \"This merge request at least does something with the chevron until @JobV returns from his vacation when he can implement the scrolling.\"\r\n\r\n@JobV your call if this is good enough or not",
"url" : "https://gitlab.com/gitlab-com/www-gitlab-com/issues/272#note_948621", <----
"comment_updated_at" : "2015-03-13T00:16:20.314Z",
"id" : "160818_merge_comment_948621",
"grimoire_creation_date" : "2015-03-13T00:16:20.314000+00:00",
"is_gitlab_issue_comment" : 1,
Can you add the tests and schema? Thanks
|
Thanks for the review @valeriocos.
Sure, I will be working on it and add them here soon. |
705ed10 to
2d14167
Compare
|
Hi @valeriocos Recent updates to the PR
I'm right now checking the coverage, will update the PR if needed. Once the PR is perfect, I can squash the commits and rebase them with the latest changes too. |
240e81d to
c2a40cb
Compare
valeriocos
left a comment
There was a problem hiding this comment.
Hi @vchrombie , thank you for working on this PR. Overall it looks good, I left some minor comments.
I also successfully executed the enricher on a short list of gitlab repos.
|
Thanks @valeriocos. |
|
No worries, thank you for working on this PR |
schema/gitlabcomments_issues.csv
Outdated
| assignee_user_name,keyword,true,"Assignee user name from SortingHat." | ||
| assignee_uuid,keyword,true,"Assignee UUID from SortingHat." | ||
| author_bot,boolean,true,"True if the given author is identified as a bot, from SortingHat profile." | ||
| author_domain,keyword,true,"Domain associated to the author in SortingHat profile." |
There was a problem hiding this comment.
Hi @valeriocos
I was confused about this deletion.
I didn't see this (author_domain) in the mapping too, but assignee_domain and merged_by_domain are present.
Can you check test the enricher once and let me know if you have this?
The same deletion is present in the merge_request too.
There was a problem hiding this comment.
*_domain fields are added by SortingHat. By default GitLab users don't have their emails exposed on the API. However, if you execute sortinghat with a matching algorithm that joins the identities based on username and email (the algorithms are available here), the email information can be collected. The *_domain fields should be filled and thus they will appear in the mapping.
There was a problem hiding this comment.
Okay, got it. So, I will add them back to the schema then.
There was a problem hiding this comment.
Perfect, let me know when I can review the PR, thanks!
|
Also, I have made a small script to generate schema file of an es index, generate-es-index-schema-py. The script can be improved by adding the idea of default field and adding descriptions. Please leave your comments in the gist or you can open an issue here, vchrombie/grimoirelab-scripts. Feedback is highly welcomed. ^^ |
1e30dfe to
10bf53e
Compare
|
Hi @valeriocos |
There was a problem hiding this comment.
Hi @vchrombie ,
The PR is almost ready to be merged, thank you for your work! Please check the comments (for the ones concerning the test coverage, try to address them if possible).
Please check the actions below too:
- create a yaml file to provide a high-level description of this new addition (https://github.com/Bitergia/release-tools#changelog). As a reference you can check the PR for rocketchat
- create an issue in https://github.com/Bitergia/release-tools#changelog to give feedback on the use of the changelog (you can check the issues 14 and 15 in that repo as a reference)
- update the readme at https://github.com/chaoss/grimoirelab-elk#enriched-data
- submit a PR to mordred to document how to execute the gitlabcomments: https://github.com/chaoss/grimoirelab-sirmordred#supported-data-sources-
- squash the commits of this PR in just one
- sync this branch with master
| "type": "text", | ||
| "index": true | ||
| }, | ||
| "id": { |
|
|
||
| item = self.items[0] | ||
| eitem = enrich_backend.get_rich_item(item) | ||
| self.assertEqual(item['category'], 'issue') |
There was a problem hiding this comment.
can you add an assert to check the item_type? This applies also to the other items in this test
| rich_mr.update(self.get_item_project(rich_mr)) | ||
|
|
||
| if 'project' in item: | ||
| rich_mr['project'] = item['project'] |
|
please @vchrombie ping me when the PR is ready for review, thanks |
10bf53e to
b41798a
Compare
|
Coming from outside and having dismissed GrimoireLab previously for lacking GitLab support, I would still like to see it support additional code forges other than GitHub, as per the arguments in inhttps://github.com/chaoss/grimoirelab/issues/284#issuecomment-827155190). Is there something me as an outsider can contribute to the formal aspects of the PR, in so the remaining technical questions can be answered more easily? Speaking of:
|
|
Hi @almereyda, thanks for your comment.
Sorry about it, but the current GrimoireLab supports analyzing issues and merge requests from the gitlab projects (reference). This PR is an additional enricher which can analyze the comments of the gitlab issues and mrs. I couldn't complete it because I completely forgot about this.
Thanks for offering help, I appreciate it. But, I think I can complete the work on this PR. I can pull some time during the coming weekend and complete the pending work. |
78a9e7f to
84580a1
Compare
This commit adds a new enricher to handle gitlab comments data. This is the github2 version for gitlab. The tests are added accordingly. Signed-off-by: Venu Vardhan Reddy Tekula <[email protected]>
84580a1 to
d0bd0dc
Compare
This PR proposes a new gitlab enricher to handle gitlab comments. The old enricher is left in place to avoid breaking changes with existing customer dashboards.
Ref: chaoss/grimoirelab#208