scala-steward
scala-steward copied to clipboard
Support merge request level approval rules to set required approvals #2794
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;
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;
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 usershowever in API it's calledAll Members. Users who wish to update this rule has to configureAll Membersrather thanAll 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.
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.
I will cover the missing lines reported by Codecov.
I added missing test cases related to the changes in this pull request.
@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 :)
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.