-
Notifications
You must be signed in to change notification settings - Fork 502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support merge request level approval rules to set required approvals #2794 #3014
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -156,9 +156,11 @@ object Config { | |
final case class GitHubCfg( | ||
) extends ForgeSpecificCfg | ||
|
||
final case class MergeRequestApprovalRulesCfg(approvalRuleName: String, requiredApprovals: Int) | ||
|
||
final case class GitLabCfg( | ||
mergeWhenPipelineSucceeds: Boolean, | ||
requiredReviewers: Option[Int], | ||
requiredApprovals: Option[Either[Int, Nel[MergeRequestApprovalRulesCfg]]], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is how to read this; 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @endertunc I'd suggest a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sense I will add the prefix 👍 |
||
removeSourceBranch: Boolean | ||
) extends ForgeSpecificCfg | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,13 +23,18 @@ import io.circe._ | |
import io.circe.generic.semiauto._ | ||
import io.circe.syntax._ | ||
import org.http4s.{Request, Status, Uri} | ||
import org.scalasteward.core.application.Config.{ForgeCfg, GitLabCfg} | ||
import org.scalasteward.core.application.Config.{ForgeCfg, GitLabCfg, MergeRequestApprovalRulesCfg} | ||
import org.scalasteward.core.data.Repo | ||
import org.scalasteward.core.forge.ForgeApiAlg | ||
import org.scalasteward.core.forge.data._ | ||
import org.scalasteward.core.git.{Branch, Sha1} | ||
import org.scalasteward.core.util.uri.uriDecoder | ||
import org.scalasteward.core.util.{intellijThisImportIsUsed, HttpJsonClient, UnexpectedResponse} | ||
import org.scalasteward.core.util.{ | ||
intellijThisImportIsUsed, | ||
HttpJsonClient, | ||
Nel, | ||
UnexpectedResponse | ||
} | ||
import org.typelevel.log4cats.Logger | ||
|
||
import scala.concurrent.duration.{Duration, DurationInt} | ||
|
@@ -48,6 +53,8 @@ final private[gitlab] case class MergeRequestPayload( | |
target_branch: Branch | ||
) | ||
|
||
final private[gitlab] case class UpdateMergeRequestLevelApprovalRulePayload(approvals_required: Int) | ||
|
||
private[gitlab] object MergeRequestPayload { | ||
def apply( | ||
id: String, | ||
|
@@ -87,6 +94,11 @@ final private[gitlab] case class MergeRequestApprovalsOut( | |
approvalsRequired: Int | ||
) | ||
|
||
final private[gitlab] case class MergeRequestLevelApprovalRuleOut( | ||
id: Int, | ||
name: String | ||
) | ||
|
||
final private[gitlab] case class CommitId(id: Sha1) { | ||
val commitOut: CommitOut = CommitOut(id) | ||
} | ||
|
@@ -102,6 +114,8 @@ private[gitlab] object GitLabJsonCodec { | |
intellijThisImportIsUsed(uriDecoder) | ||
|
||
implicit val forkPayloadEncoder: Encoder[ForkPayload] = deriveEncoder | ||
implicit val updateMergeRequestLevelApprovalRulePayloadEncoder | ||
: Encoder[UpdateMergeRequestLevelApprovalRulePayload] = deriveEncoder | ||
implicit val userOutDecoder: Decoder[UserOut] = Decoder.instance { | ||
_.downField("username").as[String].map(UserOut(_)) | ||
} | ||
|
@@ -140,6 +154,14 @@ private[gitlab] object GitLabJsonCodec { | |
} yield MergeRequestApprovalsOut(requiredReviewers) | ||
} | ||
|
||
implicit val mergeRequestLevelApprovalRuleOutDecoder: Decoder[MergeRequestLevelApprovalRuleOut] = | ||
Decoder.instance { c => | ||
for { | ||
id <- c.downField("id").as[Int] | ||
name <- c.downField("name").as[String] | ||
} yield MergeRequestLevelApprovalRuleOut(id, name) | ||
} | ||
|
||
implicit val projectIdDecoder: Decoder[ProjectId] = deriveDecoder | ||
implicit val mergeRequestPayloadEncoder: Encoder[MergeRequestPayload] = | ||
deriveEncoder[MergeRequestPayload].mapJson(_.dropNullValues) | ||
|
@@ -240,7 +262,13 @@ final class GitLabApiAlg[F[_]: Parallel]( | |
for { | ||
mr <- mergeRequest | ||
mrWithStatus <- waitForMergeRequestStatus(mr.iid) | ||
_ <- maybeSetReviewers(repo, mrWithStatus) | ||
_ <- gitLabCfg.requiredApprovals match { | ||
case Some(Right(approvalRules)) => | ||
setApprovalRules(repo, mrWithStatus, approvalRules) | ||
case Some(Left(requiredReviewers)) => | ||
setReviewers(repo, mrWithStatus, requiredReviewers) | ||
case None => F.unit | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we call one flow or the other depending on the configuration provided. |
||
} | ||
mergedUponSuccess <- mergePipelineUponSuccess(repo, mrWithStatus) | ||
} yield mergedUponSuccess | ||
} | ||
|
@@ -270,28 +298,86 @@ final class GitLabApiAlg[F[_]: Parallel]( | |
case mr => | ||
logger.info(s"Unable to automatically merge ${mr.webUrl}").map(_ => mr) | ||
} | ||
import cats.implicits._ | ||
|
||
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" | ||
private def setReviewers( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth adding a comment here reminding readers that this is retained only for Gitlab versions older than... 12.3 I think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
repo: Repo, | ||
mrOut: MergeRequestOut, | ||
requiredReviewers: Int | ||
): F[MergeRequestOut] = | ||
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 | ||
|
||
private def setApprovalRules( | ||
repo: Repo, | ||
mrOut: MergeRequestOut, | ||
approvalRulesCfg: Nel[MergeRequestApprovalRulesCfg] | ||
): F[MergeRequestOut] = | ||
for { | ||
_ <- logger.info( | ||
s"Adjusting merge request approvals rules on ${mrOut.webUrl} with following config: $approvalRulesCfg" | ||
) | ||
activeApprovalRules <- | ||
client | ||
.get[List[MergeRequestLevelApprovalRuleOut]]( | ||
url.listMergeRequestLevelApprovalRules(repo, mrOut.iid), | ||
modify(repo) | ||
) | ||
.recoverWith { case UnexpectedResponse(_, _, _, status, body) => | ||
logger | ||
.warn(s"Unexpected response getting merge request approval rules: $status: $body") | ||
.as(List.empty) | ||
} | ||
approvalRulesToUpdate = calculateRulesToUpdate(activeApprovalRules, approvalRulesCfg) | ||
_ <- | ||
approvalRulesToUpdate.map { case (approvalRuleCfg, activeRule) => | ||
logger.info( | ||
s"Setting required approval count to ${approvalRuleCfg.requiredApprovals} for merge request approval rule '${approvalRuleCfg.approvalRuleName}' on ${mrOut.webUrl}" | ||
) >> | ||
client | ||
.put[MergeRequestApprovalsOut]( | ||
url.requiredApprovals(repo, mrOut.iid, requiredReviewers), | ||
.putWithBody[ | ||
MergeRequestLevelApprovalRuleOut, | ||
UpdateMergeRequestLevelApprovalRulePayload | ||
]( | ||
url.updateMergeRequestLevelApprovalRule( | ||
repo, | ||
mrOut.iid, | ||
activeRule.id | ||
), | ||
UpdateMergeRequestLevelApprovalRulePayload(approvalRuleCfg.requiredApprovals), | ||
modify(repo) | ||
) | ||
.map(_ => ()) | ||
.as(()) | ||
.recoverWith { case UnexpectedResponse(_, _, _, status, body) => | ||
logger | ||
.warn(s"Unexpected response setting required reviewers: $status: $body") | ||
.as(()) | ||
.warn(s"Unexpected response setting required approvals: $status: $body") | ||
} | ||
} yield mrOut | ||
case None => F.pure(mrOut) | ||
}.sequence | ||
} yield mrOut | ||
|
||
private[gitlab] def calculateRulesToUpdate( | ||
activeApprovalRules: List[MergeRequestLevelApprovalRuleOut], | ||
approvalRulesCfg: Nel[MergeRequestApprovalRulesCfg] | ||
): List[(MergeRequestApprovalRulesCfg, MergeRequestLevelApprovalRuleOut)] = | ||
activeApprovalRules.flatMap { activeRule => | ||
approvalRulesCfg | ||
.find(_.approvalRuleName == activeRule.name) | ||
.map(_ -> activeRule) | ||
} | ||
|
||
private def getUsernameToUserIdsMapping(repo: Repo, usernames: Set[String]): F[Map[String, Int]] = | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this description of the flag accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's not :) I will update it 👍