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

endertunc · 2023-03-19T11:27:03Z

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 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 · 2023-03-19T12:41:49Z

modules/core/src/main/scala/org/scalasteward/core/application/Cli.scala

@@ -44,6 +46,21 @@ object Cli {
    val processTimeout = "process-timeout"
  }

+  implicit val mergeRequestApprovalsConfigArgument: Argument[MergeRequestApprovalRulesCfg] =
+    Argument.from("approvals_rule_name=required_approvals") { s =>
+      s.split(":").toList match {


I split with : but I can switch to = like we do in env variables. I don't have any preferences.

= makes more sense I guess so I changed it already.

endertunc · 2023-03-19T12:45:23Z

modules/core/src/main/scala/org/scalasteward/core/application/Config.scala

  final case class GitLabCfg(
      mergeWhenPipelineSucceeds: Boolean,
-      requiredReviewers: Option[Int],
+      requiredApprovals: Option[Either[Int, Nel[MergeRequestApprovalRulesCfg]]],


Here is how to read this;
Left side the old flow, user just provides the old configuration via --gitlab-required-reviewers <integer>
Right side is the new flow, user provides the new configuration via --merge-request-level-approval-rule <approvals_rule_name=required_approvals>

I also renamed the name of this variable. It's not reviewers, it's actually approvals.

I can wrap left side to a value class. It might be more explanatory then just Int. WDYT?

@endertunc I'd suggest a gitlab- prefix for this new config param perhaps, for consistency?

Make sense I will add the prefix 👍

endertunc · 2023-03-19T12:46:11Z

modules/core/src/main/scala/org/scalasteward/core/forge/gitlab/GitLabApiAlg.scala

+              setApprovalRules(repo, mrWithStatus, approvalRules)
+            case Some(Left(requiredReviewers)) =>
+              setReviewers(repo, mrWithStatus, requiredReviewers)
+            case None => F.unit


Here we call one flow or the other depending on the configuration provided.

codecov · 2023-03-19T12:46:35Z

Codecov Report

Patch coverage: 89.39% and project coverage change: -0.02 ⚠️

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%)`	⬇️
.../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%)`	⬆️

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

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

endertunc · 2023-03-24T18:49:39Z

I will cover the missing lines reported by Codecov.

…ource-branch

endertunc · 2023-03-31T15:05:59Z

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

endertunc · 2023-07-28T20:01:44Z

@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 :)

henricook · 2023-11-16T13:45:04Z

docs/help.md

@@ -80,6 +80,8 @@ Options and flags:
        Whether to merge a gitlab merge request when the pipeline succeeds
    --gitlab-required-reviewers <integer>
        When set, the number of required reviewers for a merge request will be set to this number (non-negative integer).  Is only used in the context of gitlab-merge-when-pipeline-succeeds being enabled, and requires that the configured access token have the appropriate privileges.  Also requires a Gitlab Premium subscription.
+    --merge-request-level-approval-rule <approvals_rule_name=required_approvals>
+        Additional repo config file (can be used multiple times)


Is this description of the flag accurate?

No it's not :) I will update it 👍

henricook · 2023-11-16T13:49:30Z

modules/core/src/main/scala/org/scalasteward/core/forge/gitlab/GitLabApiAlg.scala

-        for {
-          _ <- logger.info(
-            s"Setting number of required reviewers on ${mrOut.webUrl} to $requiredReviewers"
+  private def setReviewers(


It might be worth adding a comment here reminding readers that this is retained only for Gitlab versions older than... 12.3 I think?

I think we should diffidently mention it in the description of the flag that enable this behavior. I wouldn't mind mentioning it here as well.

henricook · 2023-11-16T13:50:06Z

modules/core/src/test/scala/org/scalasteward/core/application/CliTest.scala

    val params = minimumRequiredParams ++ List(
      List("--gitlab-merge-when-pipeline-succeeds"),
+      List("--gitlab-remove-source-branch"),


Is this a drive-by? Or a bad merge with main?

I think there were some missing test cases (config flags in this case) so I think I just did a bit of improvement here if I remember correctly.

henricook · 2023-11-16T13:51:43Z

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.

endertunc force-pushed the support-merge-request-level-approval-rules branch from 42e72d3 to d2c7bd7 Compare March 19, 2023 12:34

endertunc marked this pull request as ready for review March 19, 2023 12:34

endertunc commented Mar 19, 2023

View reviewed changes

endertunc changed the title ~~update configuration settings and integrate new API calls to gitlab flow~~ Support merge request level approval rules to set required approvals #2794 Mar 19, 2023

Ender Tunc added 4 commits March 31, 2023 16:51

update configuration settings and integrate new API calls to gitlab flow

9295c9d

finalize the implementation and add tests

11dac1c

run sbt docs/mdoc and add small update in tests for --gitlab-remove-s…

c6cde8b

…ource-branch

add more tests

8766c3b

endertunc force-pushed the support-merge-request-level-approval-rules branch from 38ec23d to 8766c3b Compare March 31, 2023 14:52

henricook reviewed Nov 16, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

endertunc commented Mar 19, 2023 •

edited

Loading

endertunc Mar 19, 2023

endertunc Mar 31, 2023

endertunc Mar 19, 2023

henricook Nov 16, 2023 •

edited

Loading

endertunc Dec 3, 2023

endertunc Mar 19, 2023

codecov bot commented Mar 19, 2023 •

edited

Loading

endertunc commented Mar 24, 2023

endertunc commented Mar 31, 2023

endertunc commented Jul 28, 2023

henricook Nov 16, 2023

endertunc Dec 3, 2023

henricook Nov 16, 2023

endertunc Dec 3, 2023

henricook Nov 16, 2023

endertunc Dec 3, 2023

henricook commented Nov 16, 2023

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

Are you sure you want to change the base?

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

Conversation

endertunc commented Mar 19, 2023 • edited Loading

Technical details

Here is what I did to support the new flow

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henricook Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 19, 2023 • edited Loading

Codecov Report

endertunc commented Mar 24, 2023

endertunc commented Mar 31, 2023

endertunc commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henricook commented Nov 16, 2023

endertunc commented Mar 19, 2023 •

edited

Loading

henricook Nov 16, 2023 •

edited

Loading

codecov bot commented Mar 19, 2023 •

edited

Loading