-
Notifications
You must be signed in to change notification settings - Fork 121
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
[gitlabcomments] Add enricher to handle gitlab comments #881
base: master
Are you sure you want to change the base?
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -11,10 +11,10 @@ assignee_org_name,keyword,true,"Assignee organization name." | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, got it. So, I will add them back to the schema then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this mapping needed?
|
||
item = self.items[0] | ||
eitem = enrich_backend.get_rich_item(item) | ||
self.assertEqual(item['category'], 'issue') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not covered by the tests
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