Skip to content

Commit 42e72d3

Browse files
author
Ender Tunc
committed
finalize the implementation and add tests
1 parent 58e6793 commit 42e72d3

File tree

7 files changed

+320
-48
lines changed

7 files changed

+320
-48
lines changed

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

+9-9
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import com.monovore.decline.Opts.{flag, option, options}
2323
import com.monovore.decline._
2424
import org.http4s.Uri
2525
import org.http4s.syntax.literals._
26-
import org.scalasteward.core.application.Cli.gitlabMergeRequestApprovalsConfig
2726
import org.scalasteward.core.application.Config._
2827
import org.scalasteward.core.data.Resolver
2928
import org.scalasteward.core.forge.ForgeType
@@ -47,15 +46,15 @@ object Cli {
4746
val processTimeout = "process-timeout"
4847
}
4948

50-
implicit val mergeRequestApprovalsConfigArgument: Argument[MergeRequestApprovalsConfig] =
49+
implicit val mergeRequestApprovalsConfigArgument: Argument[MergeRequestApprovalRulesCfg] =
5150
Argument.from("approvals_rule_name=required_approvals") { s =>
5251
s.split(":").toList match {
5352
case approvalRuleName :: requiredApprovalsAsString :: Nil =>
5453
Try(requiredApprovalsAsString.trim.toInt) match {
5554
case Failure(_) =>
5655
s"[$requiredApprovalsAsString] is not a valid Integer".invalidNel
5756
case Success(requiredApprovals) =>
58-
new MergeRequestApprovalsConfig(approvalRuleName.trim, requiredApprovals).validNel
57+
MergeRequestApprovalRulesCfg(approvalRuleName.trim, requiredApprovals).validNel
5958
}
6059
case _ =>
6160
s"The value is expected in the following format: APPROVALS_RULE_NAME:REQUIRED_APPROVALS.".invalidNel
@@ -294,21 +293,22 @@ object Cli {
294293

295294
private val gitlabRequiredReviewers: Opts[Option[Int]] =
296295
option[Int](
297-
"gitlabRequiredReviewers",
296+
"gitlab-required-reviewers",
298297
"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."
299298
).validate("Required reviewers must be non-negative")(_ >= 0).orNone
300299

301-
private val gitlabMergeRequestApprovalsConfig: Opts[Option[Nel[MergeRequestApprovalsConfig]]] =
302-
options[MergeRequestApprovalsConfig](
300+
private val gitlabMergeRequestApprovalsConfig: Opts[Option[Nel[MergeRequestApprovalRulesCfg]]] =
301+
options[MergeRequestApprovalRulesCfg](
303302
"merge-request-level-approval-rule",
304303
s"Additional repo config file $multiple"
305304
)
306-
// ToDo better message
307-
.validate("")(_.forall(_.requiredApproves >= 0) == true)
305+
.validate("Merge request level required approvals must be non-negative")(
306+
_.forall(_.requiredApprovals >= 0) == true
307+
)
308308
.orNone
309309

310310
private val gitlabReviewersAndApprovalsConfig
311-
: Opts[Option[Either[Int, Nel[MergeRequestApprovalsConfig]]]] =
311+
: Opts[Option[Either[Int, Nel[MergeRequestApprovalRulesCfg]]]] =
312312
((gitlabRequiredReviewers, gitlabMergeRequestApprovalsConfig).tupled.mapValidated {
313313
case (None, None) => None.validNel
314314
case (None, Some(gitlabMergeRequestApprovalsConfig)) =>

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -156,11 +156,11 @@ object Config {
156156
final case class GitHubCfg(
157157
) extends ForgeSpecificCfg
158158

159-
final case class MergeRequestApprovalsConfig(approvalRuleName: String, requiredApproves: Int)
159+
final case class MergeRequestApprovalRulesCfg(approvalRuleName: String, requiredApprovals: Int)
160160

161161
final case class GitLabCfg(
162162
mergeWhenPipelineSucceeds: Boolean,
163-
requiredReviewers: Option[Either[Int, Nel[MergeRequestApprovalsConfig]]]
163+
requiredApprovals: Option[Either[Int, Nel[MergeRequestApprovalRulesCfg]]]
164164
) extends ForgeSpecificCfg
165165

166166
final case class GiteaCfg(

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

+39-26
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import io.circe._
2222
import io.circe.generic.semiauto._
2323
import io.circe.syntax._
2424
import org.http4s.{Request, Status, Uri}
25-
import org.scalasteward.core.application.Config.{ForgeCfg, GitLabCfg, MergeRequestApprovalsConfig}
25+
import org.scalasteward.core.application.Config.{ForgeCfg, GitLabCfg, MergeRequestApprovalRulesCfg}
2626
import org.scalasteward.core.data.Repo
2727
import org.scalasteward.core.forge.ForgeApiAlg
2828
import org.scalasteward.core.forge.data._
@@ -152,7 +152,7 @@ private[gitlab] object GitLabJsonCodec {
152152
Decoder.instance { c =>
153153
for {
154154
id <- c.downField("id").as[Int]
155-
name <- c.downField("string").as[String]
155+
name <- c.downField("name").as[String]
156156
} yield MergeRequestLevelApprovalRuleOut(id, name)
157157
}
158158

@@ -238,7 +238,7 @@ final class GitLabApiAlg[F[_]: Parallel](
238238
for {
239239
mr <- mergeRequest
240240
mrWithStatus <- waitForMergeRequestStatus(mr.iid)
241-
_ <- gitLabCfg.requiredReviewers match {
241+
_ <- gitLabCfg.requiredApprovals match {
242242
case Some(Right(approvalRules)) =>
243243
setApprovalRules(repo, mrWithStatus, approvalRules)
244244
case Some(Left(requiredReviewers)) =>
@@ -302,11 +302,11 @@ final class GitLabApiAlg[F[_]: Parallel](
302302
private def setApprovalRules(
303303
repo: Repo,
304304
mrOut: MergeRequestOut,
305-
approvalsConfig: Nel[MergeRequestApprovalsConfig]
305+
approvalRulesCfg: Nel[MergeRequestApprovalRulesCfg]
306306
): F[MergeRequestOut] =
307307
for {
308308
_ <- logger.info(
309-
s"Adjusting merge request approvals rules on ${mrOut.webUrl} with following config: $approvalsConfig"
309+
s"Adjusting merge request approvals rules on ${mrOut.webUrl} with following config: $approvalRulesCfg"
310310
)
311311
activeApprovalRules <-
312312
client
@@ -315,34 +315,47 @@ final class GitLabApiAlg[F[_]: Parallel](
315315
modify(repo)
316316
)
317317
.recoverWith { case UnexpectedResponse(_, _, _, status, body) =>
318-
// ToDo better log
319318
logger
320-
.warn(s"Unexpected response setting required reviewers: $status: $body")
319+
.warn(s"Unexpected response getting merge request approval rules: $status: $body")
321320
.as(List.empty)
322321
}
323-
approvalRuleNamesFromConfig = approvalsConfig.map(_.approvalRuleName)
324-
approvalRulesToUpdate = activeApprovalRules.intersect(approvalRuleNamesFromConfig.toList)
322+
approvalRulesToUpdate = calculateRulesToUpdate(activeApprovalRules, approvalRulesCfg)
325323
_ <-
326-
approvalRulesToUpdate.map { mergeRequestApprovalConfig =>
327-
client
328-
.putWithBody[Unit, UpdateMergeRequestLevelApprovalRulePayload](
329-
url.updateMergeRequestLevelApprovalRule(
330-
repo,
331-
mrOut.iid,
332-
mergeRequestApprovalConfig.id
333-
),
334-
UpdateMergeRequestLevelApprovalRulePayload(mergeRequestApprovalConfig.id),
335-
modify(repo)
336-
)
337-
.recoverWith { case UnexpectedResponse(_, _, _, status, body) =>
338-
// ToDo better log
339-
logger
340-
.warn(s"Unexpected response setting required reviewers: $status: $body")
341-
.as(List.empty)
342-
}
324+
approvalRulesToUpdate.map { case (approvalRuleCfg, activeRule) =>
325+
logger.info(
326+
s"Setting required approval count to ${approvalRuleCfg.requiredApprovals} for merge request approval rule '${approvalRuleCfg.approvalRuleName}' on ${mrOut.webUrl}"
327+
) >>
328+
client
329+
.putWithBody[
330+
MergeRequestLevelApprovalRuleOut,
331+
UpdateMergeRequestLevelApprovalRulePayload
332+
](
333+
url.updateMergeRequestLevelApprovalRule(
334+
repo,
335+
mrOut.iid,
336+
activeRule.id
337+
),
338+
UpdateMergeRequestLevelApprovalRulePayload(approvalRuleCfg.requiredApprovals),
339+
modify(repo)
340+
)
341+
.as(())
342+
.recoverWith { case UnexpectedResponse(_, _, _, status, body) =>
343+
logger
344+
.warn(s"Unexpected response setting required approvals: $status: $body")
345+
}
343346
}.sequence
344347
} yield mrOut
345348

349+
private[gitlab] def calculateRulesToUpdate(
350+
activeApprovalRules: List[MergeRequestLevelApprovalRuleOut],
351+
approvalRulesCfg: Nel[MergeRequestApprovalRulesCfg]
352+
): List[(MergeRequestApprovalRulesCfg, MergeRequestLevelApprovalRuleOut)] =
353+
activeApprovalRules.flatMap { activeRule =>
354+
approvalRulesCfg
355+
.find(_.approvalRuleName == activeRule.name)
356+
.map(_ -> activeRule)
357+
}
358+
346359
private def getUsernameToUserIdsMapping(repo: Repo, usernames: Set[String]): F[Map[String, Int]] =
347360
usernames.toList
348361
.parTraverse { username =>

‎modules/core/src/main/scala/org/scalasteward/core/forge/gitlab/Url.scala

-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
package org.scalasteward.core.forge.gitlab
1818

1919
import org.http4s.Uri
20-
import org.scalasteward.core.application.Config.MergeRequestApprovalsConfig
2120
import org.scalasteward.core.data.Repo
2221
import org.scalasteward.core.forge.data.PullRequestNumber
2322
import org.scalasteward.core.git.Branch

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

+65-5
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,12 @@ import munit.FunSuite
66
import org.http4s.syntax.literals._
77
import org.scalasteward.core.application.Cli.EnvVar
88
import org.scalasteward.core.application.Cli.ParseResult._
9-
import org.scalasteward.core.application.Config.StewardUsage
9+
import org.scalasteward.core.application.Config.{MergeRequestApprovalRulesCfg, StewardUsage}
1010
import org.scalasteward.core.forge.ForgeType
1111
import org.scalasteward.core.forge.github.GitHubApp
12+
import org.scalasteward.core.util.Nel
13+
import cats.syntax.either._
14+
1215
import scala.concurrent.duration._
1316

1417
class CliTest extends FunSuite {
@@ -63,7 +66,7 @@ class CliTest extends FunSuite {
6366
assertEquals(obtained.githubApp, Some(GitHubApp(12345678L, File("example_app_key"))))
6467
assertEquals(obtained.refreshBackoffPeriod, 1.day)
6568
assert(!obtained.gitLabCfg.mergeWhenPipelineSucceeds)
66-
assertEquals(obtained.gitLabCfg.requiredReviewers, None)
69+
assertEquals(obtained.gitLabCfg.requiredApprovals, None)
6770
assert(obtained.bitbucketCfg.useDefaultReviewers)
6871
assert(!obtained.bitbucketServerCfg.useDefaultReviewers)
6972
}
@@ -151,27 +154,84 @@ class CliTest extends FunSuite {
151154
assert(clue(obtained).startsWith("Usage"))
152155
}
153156

154-
test("parseArgs: non-default GitLab arguments") {
157+
test("parseArgs: non-default GitLab arguments and required reviewers") {
155158
val params = minimumRequiredParams ++ List(
156159
List("--gitlab-merge-when-pipeline-succeeds"),
157160
List("--gitlab-required-reviewers", "5")
158161
)
159162
val Success(StewardUsage.Regular(obtained)) = Cli.parseArgs(params.flatten)
160163

161164
assert(obtained.gitLabCfg.mergeWhenPipelineSucceeds)
162-
assertEquals(obtained.gitLabCfg.requiredReviewers, Some(5))
165+
assertEquals(obtained.gitLabCfg.requiredApprovals, Some(5.asLeft))
163166
}
164167

165-
test("parseArgs: invalid GitLab required reviewers") {
168+
test("parseArgs: non-default GitLab arguments and merge request level approval rule") {
166169
val params = minimumRequiredParams ++ List(
167170
List("--gitlab-merge-when-pipeline-succeeds"),
171+
List("--merge-request-level-approval-rule", "All eligible users:0")
172+
)
173+
val Success(StewardUsage.Regular(obtained)) = Cli.parseArgs(params.flatten)
174+
175+
assert(obtained.gitLabCfg.mergeWhenPipelineSucceeds)
176+
assertEquals(
177+
obtained.gitLabCfg.requiredApprovals,
178+
Some(Nel.one(MergeRequestApprovalRulesCfg("All eligible users", 0)).asRight)
179+
)
180+
}
181+
182+
test("parseArgs: multiple Gitlab merge request level approval rule") {
183+
val params = minimumRequiredParams ++ List(
184+
List("--merge-request-level-approval-rule", "All eligible users:1"),
185+
List("--merge-request-level-approval-rule", "Only Main:2")
186+
)
187+
val Success(StewardUsage.Regular(obtained)) = Cli.parseArgs(params.flatten)
188+
189+
assertEquals(
190+
obtained.gitLabCfg.requiredApprovals,
191+
Some(
192+
Nel
193+
.of(
194+
MergeRequestApprovalRulesCfg("All eligible users", 1),
195+
MergeRequestApprovalRulesCfg("Only Main", 2)
196+
)
197+
.asRight
198+
)
199+
)
200+
}
201+
202+
test("parseArgs: only allow one way to define Gitlab required approvals arguments") {
203+
val params = minimumRequiredParams ++ List(
204+
List("--merge-request-level-approval-rule", "All eligible users:0"),
205+
List("--gitlab-required-reviewers", "5")
206+
)
207+
val Error(errorMsg) = Cli.parseArgs(params.flatten)
208+
209+
assert(
210+
clue(errorMsg).startsWith(
211+
"You can't use both --gitlabRequiredReviewers and --merge-request-level-approval-rule at the same time"
212+
)
213+
)
214+
215+
}
216+
217+
test("parseArgs: invalid GitLab required reviewers") {
218+
val params = minimumRequiredParams ++ List(
168219
List("--gitlab-required-reviewers", "-3")
169220
)
170221
val Error(errorMsg) = Cli.parseArgs(params.flatten)
171222

172223
assert(clue(errorMsg).startsWith("Required reviewers must be non-negative"))
173224
}
174225

226+
test("parseArgs: invalid GitLab merge request level approval rule") {
227+
val params = minimumRequiredParams ++ List(
228+
List("--merge-request-level-approval-rule", "All eligible users:-3")
229+
)
230+
val Error(errorMsg) = Cli.parseArgs(params.flatten)
231+
232+
assert(clue(errorMsg).startsWith("Merge request level required approvals must be non-negative"))
233+
}
234+
175235
test("parseArgs: validate-repo-config") {
176236
val Success(StewardUsage.ValidateRepoConfig(file)) = Cli.parseArgs(
177237
List(

‎modules/core/src/test/scala/org/scalasteward/core/forge/gitlab/GitLabAlgTest.scala

+69
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
package org.scalasteward.core.forge.gitlab
2+
3+
import munit.CatsEffectSuite
4+
import org.http4s.Request
5+
import org.scalasteward.core.TestInstances.ioLogger
6+
import org.scalasteward.core.application.Config.{GitLabCfg, MergeRequestApprovalRulesCfg}
7+
import org.scalasteward.core.data.Repo
8+
import org.scalasteward.core.forge.ForgeType
9+
import org.scalasteward.core.mock.MockConfig.config
10+
import org.scalasteward.core.mock.MockContext.context.httpJsonClient
11+
import org.scalasteward.core.mock.MockEff
12+
import org.scalasteward.core.util.Nel
13+
14+
class GitLabAlgTest extends CatsEffectSuite {
15+
16+
private val gitlabApiAlg = new GitLabApiAlg[MockEff](
17+
forgeCfg = config.forgeCfg.copy(tpe = ForgeType.GitLab),
18+
gitLabCfg = GitLabCfg(mergeWhenPipelineSucceeds = false, requiredApprovals = None),
19+
modify = (_: Repo) => (request: Request[MockEff]) => MockEff.pure(request)
20+
)
21+
22+
test(
23+
"calculateRulesToUpdate -- ignore active approval rule that doesn't have approval rule configuration"
24+
) {
25+
val activeApprovalRules =
26+
List(
27+
MergeRequestLevelApprovalRuleOut(name = "A", id = 101),
28+
MergeRequestLevelApprovalRuleOut(name = "B", id = 201)
29+
)
30+
val approvalRulesCfg =
31+
Nel.one(MergeRequestApprovalRulesCfg(approvalRuleName = "B", requiredApprovals = 1))
32+
33+
val result =
34+
gitlabApiAlg.calculateRulesToUpdate(activeApprovalRules, approvalRulesCfg)
35+
val expectedResult =
36+
List(
37+
(
38+
MergeRequestApprovalRulesCfg(approvalRuleName = "B", requiredApprovals = 1),
39+
MergeRequestLevelApprovalRuleOut(id = 201, name = "B")
40+
)
41+
)
42+
43+
assertEquals(result, expectedResult)
44+
}
45+
46+
test(
47+
"calculateRulesToUpdate -- ignore approval rule configuration that doesn't have active approval rule"
48+
) {
49+
val activeApprovalRules =
50+
List(MergeRequestLevelApprovalRuleOut(name = "A", id = 101))
51+
val approvalRulesCfg =
52+
Nel.of(
53+
MergeRequestApprovalRulesCfg(approvalRuleName = "A", requiredApprovals = 1),
54+
MergeRequestApprovalRulesCfg(approvalRuleName = "B", requiredApprovals = 2)
55+
)
56+
57+
val result =
58+
gitlabApiAlg.calculateRulesToUpdate(activeApprovalRules, approvalRulesCfg)
59+
val expectedResult =
60+
List(
61+
(
62+
MergeRequestApprovalRulesCfg(approvalRuleName = "A", requiredApprovals = 1),
63+
MergeRequestLevelApprovalRuleOut(name = "A", id = 101)
64+
)
65+
)
66+
67+
assertEquals(result, expectedResult)
68+
}
69+
}

0 commit comments

Comments
 (0)