scala-steward icon indicating copy to clipboard operation
scala-steward copied to clipboard

Support merge request level approval rules to set required approvals #2794

Open endertunc opened this issue 2 years ago • 5 comments

Resolves #2794

Gitlab introduced what they call Merge Request Level Approval Rules which you can define on repo level whcih would apply to all matching merge request and can be adjusted per merge request. You can read more about it here: https://docs.gitlab.com/ee/user/project/merge_requests/approvals/rules.html

Technical details

Here is an example merge request approval rules defined on project level (see https://docs.gitlab.com/ee/api/merge_request_approvals.html#get-project-level-rules);

[
    {
        "id": 63,
        "name": "All Members",
        "rule_type": "any_approver",
        "eligible_approvers": [],
        "approvals_required": 1,
        "users": [],
        "groups": [],
        "contains_hidden_groups": false,
        "protected_branches": [],
        "applies_to_all_protected_branches": false
    },
    {
        "id": 131,
        "name": "scala-steward",
        "rule_type": "regular",
        "eligible_approvers": [],
        "approvals_required": 1,
        "users": [],
        "groups": [],
        "contains_hidden_groups": false,
        "protected_branches": [],
        "applies_to_all_protected_branches": false
    }
]

And this is how it would like on Gitlab Merge Request Settings UI;

image

When a merge request created in said project matching approval rules will be added to merge request level approval rules. Given the fact that two rules defined above don't target any special branch, they will be applied to all branches. Here is an example of merge request level rules with above two rules attached automatically (see https://docs.gitlab.com/ee/api/merge_request_approvals.html#get-merge-request-level-rules);

[
    {
        "id": 17897,
        "name": "All Members",
        "rule_type": "any_approver",
        "eligible_approvers": [],
        "approvals_required": 1,
        "users": [],
        "groups": [],
        "contains_hidden_groups": false,
        "section": null,
        "source_rule": {
            "approvals_required": 1
        },
        "overridden": false
    },
    {
        "id": 18120,
        "name": "scala-steward",
        "rule_type": "regular",
        "eligible_approvers": [],
        "approvals_required": 1,
        "users": [],
        "groups": [],
        "contains_hidden_groups": false,
        "section": null,
        "source_rule": {
            "approvals_required": 1
        },
        "overridden": false
    }
]

And this is how it would like on Gitlab Merge Request UI;

image

With the new flow, we can not say that we want only one approval in this merge request anymore. Instead, we need to specify this behavior on merge request level approval rules individually. Let's say we don't want any approvals for Scala Steward merge request, then we need to set approvals_required to 0 in all the defined merge request level approval rules.

Here is what I did to support the new flow

  • I added configuration option in following format --merge-request-level-approval-rule MERGE_REQUEST_LEVEL_APPROVAL_RULE_NAME:REQUIRED_APPROVALS. For example, --merge-request-level-approval-rule scala-steward:0 --merge-request-level-approval-rule All Members:0.

  • Query to merge request level rules to list all the active merge request level rules attached to this merge request

  • Combine the configuration provided by user with the information retrieved from the API to decide what merge request level rules to update and then update the merge request level rules with configuration given by user.

Notes

  • As you might already realize, we match/find merge request level rules by it's name. This is due to the fact that merge request level approval rules doesn't reference to original rule by it's id. The only connection is the name therefore we request name of the approval rule from user. I guess this is somewhat user-friendly since they don't need to deal with ids.

  • There is one default-can-not-be-deleted approval rule which is called - hmm I don't know actually, because in UI it's called All eligible users however in API it's called All Members. Users who wish to update this rule has to configure All Members rather than All eligible users. I guess we need to document it somewhere.

I am still trying to test this manually on our company Gitlab but I am having some permissions on our Gitlab instance. I will let you know once I manage to test this manually as well.

endertunc avatar Mar 19 '23 11:03 endertunc

Codecov Report

Patch coverage: 89.39% and project coverage change: -0.02 :warning:

Comparison is base (75b49de) 91.09% compared to head (8766c3b) 91.08%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3014      +/-   ##
==========================================
- Coverage   91.09%   91.08%   -0.02%     
==========================================
  Files         160      160              
  Lines        3336     3387      +51     
  Branches      303      313      +10     
==========================================
+ Hits         3039     3085      +46     
- Misses        297      302       +5     
Impacted Files Coverage Δ
...ala/org/scalasteward/core/application/Config.scala 50.00% <ø> (ø)
...a/org/scalasteward/core/forge/ForgeSelection.scala 100.00% <ø> (ø)
.../scalasteward/core/forge/gitlab/GitLabApiAlg.scala 90.68% <84.44%> (-1.57%) :arrow_down:
.../scala/org/scalasteward/core/application/Cli.scala 100.00% <100.00%> (ø)
...scala/org/scalasteward/core/forge/gitlab/Url.scala 93.75% <100.00%> (+0.89%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Mar 19 '23 12:03 codecov[bot]

I will cover the missing lines reported by Codecov.

endertunc avatar Mar 24 '23 18:03 endertunc

I added missing test cases related to the changes in this pull request.

endertunc avatar Mar 31 '23 15:03 endertunc

@fthomas just following up on this one. Do you think this is worth merging?

As described in the ticket, implementation in main depends on deprecated API. However, I am not sure if there is enough interest from your side or community to have this PR merged.

I liked working on the issue. I learnt few things from the code base and I also learned few things from Gitlab API so I overall it was a nice experience for me and I wouldn't want to push forward if there is no interest.

We can keep it around for a while and see if people show any interest. If you wish to close the PR now without merging it I am also fine with it :)

endertunc avatar Jul 28 '23 20:07 endertunc

I came here to do this work so that we can have approvers set by SS on Gitlab and was happy to find your MR @endertunc - I've thrown in a few review comments, hopefully that's a help. It'd be really great to have this after that @fthomas 🙏🏻 . Happy to help in any way I can.

henricook avatar Nov 16 '23 13:11 henricook