Skip to content
Snippets Groups Projects

Assign approvers based on code owners

Merged Mark Chao requested to merge 1012-assign-code-owner-as-approver into master

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:

  1. overall_approvers method now returns User instead of Approver. This is because it needs to include code owners, which are dynamically calculated therefore does not have a corresponding Approver column.
  2. Due to the above, calls to overall_approvers are changed to work with User instead.
  3. And due to the above, one action is added to enable deleting of approver by using user_id instead. This is because we only have User instead of Approver, 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 using user_id instead.
  4. 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?

Edited by 🤖 GitLab Bot 🤖

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • 1 Error
    :no_entry_sign: 458e4def: Commits that change 30 or more lines in more than three files must describe these changes in the commit body
    3 Warnings
    :warning: This merge request is quite big (more than 633 lines changed), please consider splitting it into multiple merge requests.
    :warning: 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.
    :warning: 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
    :book: 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 :no_entry_sign: Danger

    Edited by 🤖 GitLab Bot 🤖
  • Mark Chao added 4 commits

    added 4 commits

    Compare with previous version

  • Mark Chao changed the description

    changed the description

  • Mark Chao changed the description

    changed the description

  • Mark Chao added 461 commits

    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

    Compare with previous version

  • Mark Chao added 1 commit

    added 1 commit

    • c58ee37d - Fix finding different files between pushes

    Compare with previous version

  • Mark Chao added 1 commit

    added 1 commit

    • 61ab97c8 - Refactor to use diffs to compute owners

    Compare with previous version

  • Author Maintainer

    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 as MergeRequestDiff#head_commit_sha field, so I can just do a db query to find the right MergeRequestDiff. 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 from oldrev to MergeRequestDiff? Thanks!

    Edited by Mark Chao
  • @lulalala I wonder if you're not using the correct oldrev string in this context :thinking: Ideally querying head_commit_sha is the way to go. See this example:

    Screen_Shot_2018-10-18_at_11.26.27

    Note that the previous revision is the first record, which has the correct head_commit_sha:

    Screen_Shot_2018-10-18_at_11.26.41

  • Mark Chao added 3 commits

    added 3 commits

    • 9e6ee6e2 - Add second_to_last method to get previous diff
    • 61790a22 - Fix spec
    • d7cd1a4c - For some reason using association extesion causes spec to fail

    Compare with previous version

  • Mark Chao added 1 commit

    added 1 commit

    • 8b3a430e - For some reason using association extesion causes spec to fail

    Compare with previous version

  • Mark Chao added 2 commits

    added 2 commits

    • 67e09086 - Move MergeRequest affected file paths to model for general use
    • 86b8dcc0 - Fix spec

    Compare with previous version

  • Author Maintainer

    @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

  • Mark Chao marked the checklist item Tests added for this feature/bug as completed

    marked the checklist item Tests added for this feature/bug as completed

  • Mark Chao marked the checklist item Update owner when commit is pushed as completed

    marked the checklist item Update owner when commit is pushed as completed

  • Mark Chao added 411 commits

    added 411 commits

    Compare with previous version

  • Sorry it's not reviewed yet @lulalala. I'll take my time to look at it on my morning, tomorrow!

  • Author Maintainer

    @oswaldo no need to hurry :) Please enjoy the weekend!

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply
Loading