Merge pull request #2528 from dkichler/issue-2519-gitlab-minimum-reviewers

feat:  Adding support for setting required number of reviewers on a S…

Author: exoego

docs/help.md

@@ -5,7 +5,7 @@ All command line arguments for the `scala-steward` application.
 ```
 Usage:
     scala-steward validate-repo-config
-    scala-steward --workspace <file> --repos-file <file> [--git-author-name <string>] --git-author-email <string> [--git-author-signing-key <string>] --git-ask-pass <file> [--sign-commits] [--vcs-type <vcs-type>] [--vcs-api-host <uri>] --vcs-login <string> [--do-not-fork] [--add-labels] [--ignore-opts-files] [--env-var <name=value>]... [--process-timeout <duration>] [--whitelist <string>]... [--read-only <string>]... [--enable-sandbox | --disable-sandbox] [--max-buffer-size <integer>] [--repo-config <uri>]... [--disable-default-repo-config] [--scalafix-migrations <uri>]... [--disable-default-scalafix-migrations] [--artifact-migrations <uri>]... [--disable-default-artifact-migrations] [--cache-ttl <duration>] [--bitbucket-server-use-default-reviewers] [--gitlab-merge-when-pipeline-succeeds] [--github-app-id <integer> --github-app-key-file <file>] [--url-checker-test-url <uri>] [--default-maven-repo <string>] [--refresh-backoff-period <duration>]
+    scala-steward --workspace <file> --repos-file <file> [--git-author-name <string>] --git-author-email <string> [--git-author-signing-key <string>] --git-ask-pass <file> [--sign-commits] [--vcs-type <vcs-type>] [--vcs-api-host <uri>] --vcs-login <string> [--do-not-fork] [--add-labels] [--ignore-opts-files] [--env-var <name=value>]... [--process-timeout <duration>] [--whitelist <string>]... [--read-only <string>]... [--enable-sandbox | --disable-sandbox] [--max-buffer-size <integer>] [--repo-config <uri>]... [--disable-default-repo-config] [--scalafix-migrations <uri>]... [--disable-default-scalafix-migrations] [--artifact-migrations <uri>]... [--disable-default-artifact-migrations] [--cache-ttl <duration>] [--bitbucket-server-use-default-reviewers] [--gitlab-merge-when-pipeline-succeeds] [--gitlab-required-reviewers <integer>] [--github-app-id <integer> --github-app-key-file <file>] [--url-checker-test-url <uri>] [--default-maven-repo <string>] [--refresh-backoff-period <duration>]
 
 
 
@@ -70,6 +70,8 @@ Options and flags:
         Whether to assign the default reviewers to a bitbucket pull request; default: false
     --gitlab-merge-when-pipeline-succeeds
         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.
     --github-app-id <integer>
         GitHub application id
     --github-app-key-file <file>

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

@@ -226,8 +226,14 @@ object Cli {
       "Whether to merge a gitlab merge request when the pipeline succeeds"
     ).orFalse
 
+  private val gitlabRequiredReviewers: Opts[Option[Int]] =
+    option[Int](
+      "gitlab-required-reviewers",
+      "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."
+    ).validate("Required reviewers must be non-negative")(_ >= 0).orNone
+
   private val gitLabCfg: Opts[GitLabCfg] =
-    gitlabMergeWhenPipelineSucceeds.map(GitLabCfg.apply)
+    (gitlabMergeWhenPipelineSucceeds, gitlabRequiredReviewers).mapN(GitLabCfg.apply)
 
   private val githubAppId: Opts[Long] =
     option[Long]("github-app-id", "GitHub application id")

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

@@ -133,7 +133,8 @@ object Config {
   )
 
   final case class GitLabCfg(
-      mergeWhenPipelineSucceeds: Boolean
+      mergeWhenPipelineSucceeds: Boolean,
+      requiredReviewers: Option[Int]
   )
 
   sealed trait StewardUsage

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

@@ -62,6 +62,10 @@ final private[gitlab] case class MergeRequestOut(
   val pullRequestOut: PullRequestOut = PullRequestOut(webUrl, state, iid, title)
 }
 
+final private[gitlab] case class MergeRequestApprovalsOut(
+    approvalsRequired: Int
+)
+
 final private[gitlab] case class CommitId(id: Sha1) {
   val commitOut: CommitOut = CommitOut(id)
 }
@@ -109,6 +113,13 @@ private[gitlab] object GitLabJsonCodec {
     } yield MergeRequestOut(webUrl, state, title, iid, mergeStatus)
   }
 
+  implicit val mergeRequestApprovalsOutDecoder: Decoder[MergeRequestApprovalsOut] =
+    Decoder.instance { c =>
+      for {
+        requiredReviewers <- c.downField("approvals_required").as[Int]
+      } yield MergeRequestApprovalsOut(requiredReviewers)
+    }
+
   implicit val projectIdDecoder: Decoder[ProjectId] = deriveDecoder
   implicit val mergeRequestPayloadEncoder: Encoder[MergeRequestPayload] = deriveEncoder
   implicit val updateStateEncoder: Encoder[UpdateState] = Encoder.instance { newState =>
@@ -181,34 +192,64 @@ final class GitLabApiAlg[F[_]](
     val updatedMergeRequest =
       if (!gitLabCfg.mergeWhenPipelineSucceeds)
         mergeRequest
-      else
-        mergeRequest
-          .flatMap(mr => waitForMergeRequestStatus(mr.iid))
-          .flatMap {
-            case mr if mr.mergeStatus === GitLabMergeStatus.CanBeMerged =>
-              for {
-                _ <- logger.info(s"Setting ${mr.webUrl} to merge when pipeline succeeds")
-                res <-
-                  client
-                    .put[MergeRequestOut](
-                      url.mergeWhenPiplineSucceeds(repo, mr.iid),
-                      modify(repo)
-                    )
-                    // it's possible that our status changed from can be merged already,
-                    // so just handle it gracefully and proceed without setting auto merge.
-                    .recoverWith { case UnexpectedResponse(_, _, _, status, _) =>
-                      logger
-                        .warn(s"Unexpected gitlab response setting auto merge: $status")
-                        .as(mr)
-                    }
-              } yield res
-            case mr =>
-              logger.info(s"Unable to automatically merge ${mr.webUrl}").map(_ => mr)
-          }
+      else {
+        for {
+          mr <- mergeRequest
+          mrWithStatus <- waitForMergeRequestStatus(mr.iid)
+          _ <- maybeSetReviewers(repo, mrWithStatus)
+          mergedUponSuccess <- mergePipelineUponSuccess(repo, mrWithStatus)
+        } yield mergedUponSuccess
+      }
 
     updatedMergeRequest.map(_.pullRequestOut)
   }
 
+  private def mergePipelineUponSuccess(repo: Repo, mr: MergeRequestOut): F[MergeRequestOut] =
+    mr match {
+      case mr if mr.mergeStatus === GitLabMergeStatus.CanBeMerged =>
+        for {
+          _ <- logger.info(s"Setting ${mr.webUrl} to merge when pipeline succeeds")
+          res <-
+            client
+              .put[MergeRequestOut](
+                url.mergeWhenPiplineSucceeds(repo, mr.iid),
+                modify(repo)
+              )
+              // it's possible that our status changed from can be merged already,
+              // so just handle it gracefully and proceed without setting auto merge.
+              .recoverWith { case UnexpectedResponse(_, _, _, status, _) =>
+                logger
+                  .warn(s"Unexpected gitlab response setting auto merge: $status")
+                  .as(mr)
+              }
+        } yield res
+      case mr =>
+        logger.info(s"Unable to automatically merge ${mr.webUrl}").map(_ => mr)
+    }
+
+  private def maybeSetReviewers(repo: Repo, mrOut: MergeRequestOut): F[MergeRequestOut] =
+    gitLabCfg.requiredReviewers match {
+      case Some(requiredReviewers) =>
+        for {
+          _ <- logger.info(
+            s"Setting number of required reviewers on ${mrOut.webUrl} to $requiredReviewers"
+          )
+          _ <-
+            client
+              .put[MergeRequestApprovalsOut](
+                url.requiredApprovals(repo, mrOut.iid, requiredReviewers),
+                modify(repo)
+              )
+              .map(_ => ())
+              .recoverWith { case UnexpectedResponse(_, _, _, status, body) =>
+                logger
+                  .warn(s"Unexpected response setting required reviewers: $status:  $body")
+                  .as(())
+              }
+        } yield mrOut
+      case None => F.pure(mrOut)
+    }
+
   override def closePullRequest(repo: Repo, number: PullRequestNumber): F[PullRequestOut] =
     client
       .putWithBody[MergeRequestOut, UpdateState](

modules/core/src/main/scala/org/scalasteward/core/vcs/gitlab/Url.scala

@@ -42,6 +42,10 @@ class Url(apiHost: Uri) {
     (existingMergeRequest(repo, number) / "merge")
       .withQueryParam("merge_when_pipeline_succeeds", "true")
 
+  def requiredApprovals(repo: Repo, number: PullRequestNumber, approvalsRequired: Int): Uri =
+    (existingMergeRequest(repo, number) / "approvals")
+      .withQueryParam("approvals_required", approvalsRequired)
+
   def listMergeRequests(repo: Repo, source: String, target: String): Uri =
     mergeRequest(repo)
       .withQueryParam("source_branch", source)

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

@@ -62,17 +62,22 @@ class CliTest extends FunSuite {
     )
     assertEquals(obtained.githubApp, Some(GitHubApp(12345678L, File("example_app_key"))))
     assertEquals(obtained.refreshBackoffPeriod, 1.day)
+    assert(!obtained.gitLabCfg.mergeWhenPipelineSucceeds)
+    assertEquals(obtained.gitLabCfg.requiredReviewers, None)
   }
 
+  val minimumRequiredParams = List(
+    List("--workspace", "a"),
+    List("--repos-file", "b"),
+    List("--git-author-email", "d"),
+    List("--vcs-login", "e"),
+    List("--git-ask-pass", "f"),
+    List("--disable-sandbox")
+  )
+
   test("parseArgs: minimal example") {
     val Success(StewardUsage.Regular(obtained)) = Cli.parseArgs(
-      List(
-        List("--workspace", "a"),
-        List("--repos-file", "b"),
-        List("--git-author-email", "d"),
-        List("--vcs-login", "e"),
-        List("--git-ask-pass", "f")
-      ).flatten
+      minimumRequiredParams.flatten
     )
 
     assert(!obtained.processCfg.sandboxCfg.enableSandbox)
@@ -144,6 +149,27 @@ class CliTest extends FunSuite {
     assert(clue(obtained).startsWith("Usage"))
   }
 
+  test("parseArgs: non-default GitLab arguments") {
+    val params = minimumRequiredParams ++ List(
+      List("--gitlab-merge-when-pipeline-succeeds"),
+      List("--gitlab-required-reviewers", "5")
+    )
+    val Success(StewardUsage.Regular(obtained)) = Cli.parseArgs(params.flatten)
+
+    assert(obtained.gitLabCfg.mergeWhenPipelineSucceeds)
+    assertEquals(obtained.gitLabCfg.requiredReviewers, Some(5))
+  }
+
+  test("parseArgs: invalid GitLab required reviewers") {
+    val params = minimumRequiredParams ++ List(
+      List("--gitlab-merge-when-pipeline-succeeds"),
+      List("--gitlab-required-reviewers", "-3")
+    )
+    val Error(errorMsg) = Cli.parseArgs(params.flatten)
+
+    assert(clue(errorMsg).startsWith("Required reviewers must be non-negative"))
+  }
+
   test("parseArgs: validate-repo-config") {
     val Success(StewardUsage.ValidateRepoConfig(file)) = Cli.parseArgs(
       List(

modules/core/src/test/scala/org/scalasteward/core/vcs/gitlab/GitLabApiAlgTest.scala

@@ -29,6 +29,8 @@ class GitLabApiAlgTest extends FunSuite {
   object MergeWhenPipelineSucceedsMatcher
       extends QueryParamDecoderMatcher[Boolean]("merge_when_pipeline_succeeds")
 
+  object RequiredReviewersMatcher extends QueryParamDecoderMatcher[Int]("approvals_required")
+
   val routes: HttpRoutes[IO] =
     HttpRoutes.of[IO] {
       case GET -> Root / "projects" / "foo/bar" =>
@@ -66,6 +68,14 @@ class GitLabApiAlgTest extends FunSuite {
           )
         )
 
+      case PUT -> Root / "projects" / "foo/bar" / "merge_requests" / "150" / "approvals"
+          :? RequiredReviewersMatcher(requiredApprovers) =>
+        Ok(
+          putMrApprovals.deepMerge(
+            json""" { "iid": 150, "approvals_required": $requiredApprovers, "web_url": "https://gitlab.com/foo/bar/merge_requests/150" } """
+          )
+        )
+
       case POST -> Root / "projects" / "foo/bar" / "merge_requests" / "150" / "notes" =>
         Ok(json"""  {
             "body": "Superseded by #1234"
@@ -80,7 +90,11 @@ class GitLabApiAlgTest extends FunSuite {
   implicit val httpJsonClient: HttpJsonClient[IO] = new HttpJsonClient[IO]
   implicit val logger: Logger[IO] = Slf4jLogger.getLogger[IO]
   val gitlabApiAlg =
-    new GitLabApiAlg[IO](config.vcsCfg, GitLabCfg(mergeWhenPipelineSucceeds = false), _ => IO.pure)
+    new GitLabApiAlg[IO](
+      config.vcsCfg,
+      GitLabCfg(mergeWhenPipelineSucceeds = false, requiredReviewers = None),
+      _ => IO.pure
+    )
 
   private val data = UpdateData(
     RepoData(Repo("foo", "bar"), dummyRepoCache, RepoConfig.empty),
@@ -109,7 +123,7 @@ class GitLabApiAlgTest extends FunSuite {
   test("createPullRequest -- no fork") {
     val gitlabApiAlgNoFork = new GitLabApiAlg[IO](
       config.vcsCfg.copy(doNotFork = true),
-      GitLabCfg(mergeWhenPipelineSucceeds = false),
+      GitLabCfg(mergeWhenPipelineSucceeds = false, requiredReviewers = None),
       _ => IO.pure
     )
     val prOut = gitlabApiAlgNoFork
@@ -144,7 +158,7 @@ class GitLabApiAlgTest extends FunSuite {
   test("createPullRequest -- auto merge") {
     val gitlabApiAlgNoFork = new GitLabApiAlg[IO](
       config.vcsCfg.copy(doNotFork = true),
-      GitLabCfg(mergeWhenPipelineSucceeds = true),
+      GitLabCfg(mergeWhenPipelineSucceeds = true, requiredReviewers = None),
       _ => IO.pure
     )
 
@@ -176,7 +190,7 @@ class GitLabApiAlgTest extends FunSuite {
 
     val gitlabApiAlgNoFork = new GitLabApiAlg[IO](
       config.vcsCfg.copy(doNotFork = true),
-      GitLabCfg(mergeWhenPipelineSucceeds = true),
+      GitLabCfg(mergeWhenPipelineSucceeds = true, requiredReviewers = None),
       _ => IO.pure
     )(errorJsonClient, implicitly, implicitly)
 
@@ -194,6 +208,59 @@ class GitLabApiAlgTest extends FunSuite {
     assertEquals(prOut, expected)
   }
 
+  test("createPullRequest -- reduce required reviewers") {
+    val gitlabApiAlgLessReviewersRequired = new GitLabApiAlg[IO](
+      config.vcsCfg.copy(doNotFork = true),
+      GitLabCfg(mergeWhenPipelineSucceeds = true, requiredReviewers = Some(0)),
+      _ => IO.pure
+    )
+
+    val prOut = gitlabApiAlgLessReviewersRequired
+      .createPullRequest(Repo("foo", "bar"), newPRData)
+      .unsafeRunSync()
+
+    val expected = PullRequestOut(
+      uri"https://gitlab.com/foo/bar/merge_requests/150",
+      PullRequestState.Open,
+      PullRequestNumber(150),
+      "title"
+    )
+
+    assertEquals(prOut, expected)
+  }
+
+  test("createPullRequest -- no fail upon required reviewers error") {
+    val errorClient: Client[IO] =
+      Client.fromHttpApp(
+        (HttpRoutes.of[IO] {
+          case PUT -> Root / "projects" / "foo/bar" / "merge_requests" / "150" / "approvals"
+              :? RequiredReviewersMatcher(requiredReviewers) =>
+            BadRequest(s"Cannot set requiredReviewers to $requiredReviewers")
+        } <+> routes).orNotFound
+      )
+    val errorJsonClient: HttpJsonClient[IO] =
+      new HttpJsonClient[IO]()(errorClient, Concurrent[IO])
+
+    val gitlabApiAlgLessReviewersRequiredNoError = new GitLabApiAlg[IO](
+      config.vcsCfg.copy(doNotFork = true),
+      GitLabCfg(mergeWhenPipelineSucceeds = true, requiredReviewers = Some(0)),
+      _ => IO.pure
+    )(errorJsonClient, implicitly, implicitly)
+
+    val prOut = gitlabApiAlgLessReviewersRequiredNoError
+      .createPullRequest(Repo("foo", "bar"), newPRData)
+      .unsafeRunSync()
+
+    val expected = PullRequestOut(
+      uri"https://gitlab.com/foo/bar/merge_requests/150",
+      PullRequestState.Open,
+      PullRequestNumber(150),
+      "title"
+    )
+
+    assertEquals(prOut, expected)
+  }
+
   test("commentPullRequest") {
     val reference = gitlabApiAlg.referencePullRequest(PullRequestNumber(1347))
     assertEquals(reference, "!1347")
@@ -365,4 +432,33 @@ class GitLabApiAlgTest extends FunSuite {
         "packages_enabled": true
       }
     """
+
+  val putMrApprovals =
+    json"""
+      {
+        "id": 138691106,
+        "iid": 4,
+        "project_id": 466,
+        "title": "Standard ScalaSteward configured MR title",
+        "description": "Standard ScalaSteward configured MR description",
+        "state": "opened",
+        "created_at": "2022-02-04T17:43:57.363Z",
+        "updated_at": "2022-02-11T22:38:28.645Z",
+        "merge_status": "can_be_merged",
+        "approved": true,
+        "approvals_required": 0,
+        "approvals_left": 0,
+        "require_password_to_approve": false,
+        "approved_by": [],
+        "suggested_approvers": [],
+        "approvers": [],
+        "approver_groups": [],
+        "user_has_approved": false,
+        "user_can_approve": true,
+        "approval_rules_left": [],
+        "has_approval_rules": true,
+        "merge_request_approvers_available": true,
+        "multiple_approval_rules_available": true
+     }
+     """
 }