Assign approvers based on code owners
The CE port is at https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/22513
What does this MR do?
Assign approvers based on code owners
-
Only allow assigning when code owner feature is available -
Update owner when commit is pushed -
Give code owner as option when creating merge request -
Remove code owner as approver suggestion
Context
The major changes are:
-
overall_approvers
method now returnsUser
instead ofApprover
. This is because it needs to include code owners, which are dynamically calculated therefore does not have a correspondingApprover
column. - Due to the above, calls to
overall_approvers
are changed to work withUser
instead. - And due to the above, one action is added to enable deleting of approver by using
user_id
instead. This is because we only haveUser
instead ofApprover
, and if we want to get an user's approver it would cause extra database queries. At the end I decided to allow deleting of approvers by usinguser_id
instead. - For code push, we need to compute the difference in code owners, before and after the push.
What are the relevant issue numbers?
Closes #1012 (closed)
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated -
Tests added for this feature/bug -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides -
EE specific content should be in the top level /ee
folder -
For a paid feature, have we considered GitLab.com plans, how it works for groups, and is there a design for promoting it to users who aren't on the correct plan?
Merge request reports
Activity
1 Error 458e4def: Commits that change 30 or more lines in more than three files must describe these changes in the commit body 3 Warnings This merge request is quite big (more than 633 lines changed), please consider splitting it into multiple merge requests. e3890a26: This commit’s subject line could be improved. Commit subjects are ideally no longer than roughly 50 characters, though we allow up to 72 characters in the subject. If possible, try to reduce the length of the subject to roughly 50 characters. 893d751d: This commit’s subject line could be improved. Commit subjects are ideally no longer than roughly 50 characters, though we allow up to 72 characters in the subject. If possible, try to reduce the length of the subject to roughly 50 characters. 1 Message This merge request adds or changes files that require a review from the docs team. Docs Review
The following files require a review from the Documentation team:
doc/user/project/merge_requests/merge_request_approvals.md
To make sure these changes are reviewed, mention
@gl-docsteam
in a separate comment, and explain what needs to be reviewed by the team. Please don't mention the team until your changes are ready for review.Commit message standards
One or more commit messages do not meet our Git commit message standards. For more information on how to write a good commit message, take a look at How to Write a Git Commit Message.
Here is an example of a good commit message:
Reject ruby interpolation in externalized strings When using ruby interpolation in externalized strings, they can't be detected. Which means they will never be presented to be translated. To mix variables into translations we need to use `sprintf` instead. Instead of: _("Hello #{subject}") Use: _("Hello %{subject}") % { subject: 'world' }
This is an example of a bad commit message:
updated README.md
This commit message is bad because although it tells us that README.md is updated, it doesn't tell us why or how it was updated.
Generated by
DangerEdited by 🤖 GitLab Bot 🤖added 461 commits
-
1a814d64...84e95169 - 448 commits from branch
master
- 890f6b55 - Move MergeRequest affected file paths to model for general use
- 6bdc5a3f - Allow destroying approver by user_id
- 02f78985 - Allow overall_approver to return User instead of Approver
- b44691f5 - Update approvers when refreshing merge requests
- 0eae0c35 - code owners
- c3eb88d1 - approvers controller
- ee8e082e - merge request diff fie paths
- 8ed7c207 - overall_approver
- 423451ae - changelog
- 97d0e75e - Fix for_merge_request
- 7f9f2633 - Remove code owner from approver suggestions
- 5ba0f813 - Remove unused feature flag
- 94fd727f - Fix finding different files between pushes
Toggle commit list-
1a814d64...84e95169 - 448 commits from branch
Hi @oswaldo just wondering if I can ask you a few questions, as Douwe said you have a lot of experience around this area.
During a code push for a MR, I am trying to find the MergeRequestDiff prior to code push. This way I can get the previous file paths.
I have
oldrev
at https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/7933/diffs#d3f5b9fe133b2be4af9333e15dcdddf2fbfc5af3_28_35, so I assume it is the same asMergeRequestDiff#head_commit_sha
field, so I can just do a db query to find the rightMergeRequestDiff
. However it seems to be not the case. Do you know why it is not the case, and what's a good way to get fromoldrev
toMergeRequestDiff
? Thanks!Edited by Mark Chao@lulalala I wonder if you're not using the correct oldrev string in this context
Ideally queryinghead_commit_sha
is the way to go. See this example:Note that the previous revision is the first record, which has the correct
head_commit_sha
:added 1 commit
- 8b3a430e - For some reason using association extesion causes spec to fail
added 2 commits
@oswaldo thanks, that was caused by the way the spec props are setup. I decided to just fetch the second to last diff instead. It is easier to understand anyways.
Could you review this? I have add additional information at description to make this easier to understand. Thanks!
assigned to @oswaldo
marked the checklist item Tests added for this feature/bug as completed
Sorry it's not reviewed yet @lulalala. I'll take my time to look at it on my morning, tomorrow!
@oswaldo no need to hurry :) Please enjoy the weekend!