Skip to content
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

Branch selection #1469

Closed
wants to merge 29 commits into from
Closed
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
614ae93
adding selected branch to model
pk044 May 31, 2020
bf146e5
regex fix
pk044 May 31, 2020
2b491e4
adding more asserts for decoding repos
pk044 Jun 1, 2020
54d9c6a
adding branch to Repo, modifying regex
pk044 Jun 1, 2020
40af7af
scala regexes require passing nulls when pattern matching on optional…
pk044 Jun 1, 2020
77ec802
check out from base branch before creating a PR + prepend base branch…
pk044 Jun 4, 2020
ce01940
including branch name in Repo key encoding
pk044 Jun 4, 2020
2dbb90f
adjusting tests for checking branch names
pk044 Jun 4, 2020
30fe4d8
adjust tests
pk044 Jun 4, 2020
5a8ec70
adjusting tests
pk044 Jun 4, 2020
def6ec9
adjust tests
pk044 Jun 4, 2020
1b055b0
add selected branch tests for bitbucket
pk044 Jun 4, 2020
702cc5a
adding github tests for selected branch
pk044 Jun 4, 2020
2d9588e
adjust tests
pk044 Jun 4, 2020
45e6636
revert repos.md
pk044 Jun 4, 2020
b99e114
run scalafmt
pk044 Jun 4, 2020
2ad6983
run scalafmt
pk044 Jun 4, 2020
4c35076
run scalafmt
pk044 Jun 5, 2020
e9f5d59
use default parameters
pk044 Jun 7, 2020
b12912a
mention base branch in PR title
pk044 Jun 8, 2020
4d14b59
add Iterators to adopters :)
pk044 Jun 8, 2020
48684da
formatting
pk044 Jun 8, 2020
de5400a
checkout to default branch if selected branch was not specified
pk044 Jun 8, 2020
ee1b9a5
Merge remote-tracking branch 'fork/branch-selection' into branch-sele…
pk044 Jun 8, 2020
6983396
modify branch naming convention
pk044 Jun 18, 2020
a01709b
pass baseBranch to branchFor only if it's not default
pk044 Aug 11, 2020
ccc3772
Merge branch 'master' of https://github.com/scala-steward-org/scala-s…
pk044 Aug 11, 2020
4a3c6d3
condition fix
pk044 Aug 11, 2020
84247d3
formatting
pk044 Aug 11, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ Consider creating PR to add your company to the list and join the community.
* [Hellosoda](https://hellosoda.com/)
* [HolidayCheck](https://github.com/holidaycheck)
* [iAdvize](https://www.iadvize.com/en/)
* [Iterators](https://www.iteratorshq.com/)
* [LeadIQ](https://leadiq.com/)
* [Lightbend](https://www.lightbend.com/)
* [Ocado Technology](https://ocadotechnology.com/)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import cats.implicits._
import fs2.Stream
import io.chrisdavenport.log4cats.Logger
import org.scalasteward.core.buildtool.sbt.SbtAlg
import org.scalasteward.core.git.GitAlg
import org.scalasteward.core.git.{Branch, GitAlg}
import org.scalasteward.core.io.{FileAlg, WorkspaceAlg}
import org.scalasteward.core.nurture.NurtureAlg
import org.scalasteward.core.repocache.RepoCacheAlg
Expand Down Expand Up @@ -62,10 +62,11 @@ final class StewardAlg[F[_]](implicit

private def readRepos(reposFile: File): F[List[Repo]] =
fileAlg.readFile(reposFile).map { maybeContent =>
val regex = """-\s+(.+)/([^/]+)""".r
val regex = """-\s+(.+)/([^#\n]+)(?:#(.+))?""".r
val content = maybeContent.getOrElse("")
content.linesIterator.collect {
case regex(owner, repo) => Repo(owner.trim, repo.trim)
case regex(owner, repo, null) => Repo(owner.trim, repo.trim, branch = None)
case regex(owner, repo, branch) => Repo(owner.trim, repo.trim, Some(Branch(branch)))
}.toList
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class Http4sBitbucketApiAlg[F[_]: Sync](
val payload = CreatePullRequestRequest(
data.title,
Branch(data.head),
Repo(sourceBranchOwner, repo.repo),
Repo(sourceBranchOwner, repo.repo, repo.branch),
data.base,
data.body
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ private[http4s] object RepositoryResponse {
implicit private val repoDecoder: Decoder[Repo] = Decoder.instance { c =>
c.as[String].map(_.split('/')).flatMap { parts =>
parts match {
case Array(owner, name) => Repo(owner, name).asRight
case _ => DecodingFailure("Repo", c.history).asLeft
case Array(owner, name) => Repo(owner, name, None).asRight
case Array(owner, name, branch) => Repo(owner, name, Some(Branch(branch))).asRight
case _ => DecodingFailure("Repo", c.history).asLeft
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,18 @@ import org.scalasteward.core.repoconfig.CommitsConfig
import org.scalasteward.core.update.show

package object git {
def branchFor(update: Update): Branch =
Branch(s"update/${update.name}-${update.nextVersion}")
def branchFor(update: Update, baseBranch: Branch): Branch =
Branch(s"update/${baseBranch.name}/${update.name}-${update.nextVersion}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

branchFor is used in vcs.listingBranch to create the head parameter for VCSApiAlg#listPullRequests which is used to find already existing Scala Steward PRs. If we change this for PRs that target the default branch, Scala Steward will not find older PRs it already created and will create new ones. I guess this could result in thousands new but duplicated PRs by @scala-steward.

Can we make this optional and only pass it if baseBranch is not the default branch?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure! i'll push a fix soon:)


def commitMsgFor(update: Update, commitsConfig: CommitsConfig): String = {
def commitMsgFor(update: Update, commitsConfig: CommitsConfig, branch: Branch): String = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only stylistic, but can we mention the branch only if it is not the default?

val artifact = show.oneLiner(update)
val defaultMessage = s"Update $artifact to ${update.nextVersion}"
val defaultMessage = s"Update $artifact to ${update.nextVersion} (${branch.name})"
commitsConfig.messageOrDefault
.replace("${default}", defaultMessage)
.replace("${artifactName}", artifact)
.replace("${currentVersion}", update.currentVersion)
.replace("${nextVersion}", update.nextVersion)
.replace("${branchName}", branch.name)
}

// man 7 gitrevisions:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,16 @@ final class NurtureAlg[F[_]](implicit
for {
_ <- logger.info(s"Clone and synchronize ${repo.show}")
repoOut <- vcsApiAlg.createForkOrGetRepo(config, repo)
branch <- repo.branch.map(F.pure).getOrElse(vcsRepoAlg.defaultBranch(repoOut))
_ <-
gitAlg
.cloneExists(repo)
.ifM(F.unit, vcsRepoAlg.clone(repo, repoOut) >> vcsRepoAlg.syncFork(repo, repoOut))
defaultBranch <- vcsRepoAlg.defaultBranch(repoOut)
} yield (repoOut.repo, defaultBranch)
.ifM(
F.unit,
vcsRepoAlg.clone(repo, repoOut) >> vcsRepoAlg.syncFork(repo, repoOut) >> gitAlg
.checkoutBranch(repo, branch)
)
} yield (repoOut.repo, branch)

def updateDependencies(
repo: Repo,
Expand All @@ -85,12 +89,21 @@ final class NurtureAlg[F[_]](implicit
grouped = Update.groupByGroupId(updates)
sorted = grouped.sortBy(migrationAlg.findMigrations(_).size)
_ <- logger.info(util.logger.showUpdates(sorted))
_ <- gitAlg.checkoutBranch(repo, baseBranch)
baseSha1 <- gitAlg.latestSha1(repo, baseBranch)
_ <- NurtureAlg.processUpdates(
sorted,
update =>
processUpdate(
UpdateData(repo, fork, repoConfig, update, baseBranch, baseSha1, git.branchFor(update))
UpdateData(
repo,
fork,
repoConfig,
update,
baseBranch,
baseSha1,
git.branchFor(update, baseBranch)
)
),
repoConfig.updates.limit
)
Expand All @@ -99,7 +112,7 @@ final class NurtureAlg[F[_]](implicit
def processUpdate(data: UpdateData): F[ProcessResult] =
for {
_ <- logger.info(s"Process update ${data.update.show}")
head = vcs.listingBranch(config.vcsType, data.fork, data.update)
head = vcs.listingBranch(config.vcsType, data.fork, data.update, data.baseBranch)
pullRequests <- vcsApiAlg.listPullRequests(data.repo, head, data.baseBranch)
result <- pullRequests.headOption match {
case Some(pr) if pr.isClosed =>
Expand Down Expand Up @@ -142,7 +155,8 @@ final class NurtureAlg[F[_]](implicit
for {
_ <- logger.info("Commit and push changes")
commitMsgConfig = data.repoConfig.commits
_ <- gitAlg.commitAll(data.repo, git.commitMsgFor(data.update, commitMsgConfig))
_ <-
gitAlg.commitAll(data.repo, git.commitMsgFor(data.update, commitMsgConfig, data.baseBranch))
_ <- gitAlg.push(data.repo, data.updateBranch)
} yield Updated

Expand All @@ -160,7 +174,7 @@ final class NurtureAlg[F[_]](implicit
existingArtifactUrlsMap
.get(data.update.mainArtifactId)
.traverse(vcsExtraAlg.getReleaseRelatedUrls(_, data.update))
branchName = vcs.createBranch(config.vcsType, data.fork, data.update)
branchName = vcs.createBranch(config.vcsType, data.fork, data.update, data.baseBranch)
migrations = migrationAlg.findMigrations(data.update)
requestData = NewPullRequestData.from(
data,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import io.chrisdavenport.log4cats.Logger
import org.scalasteward.core.application.Config
import org.scalasteward.core.buildtool.BuildToolDispatcher
import org.scalasteward.core.data.{Dependency, DependencyInfo}
import org.scalasteward.core.git.GitAlg
import org.scalasteward.core.git.{Branch, GitAlg}
import org.scalasteward.core.repoconfig.RepoConfigAlg
import org.scalasteward.core.util.MonadThrowable
import org.scalasteward.core.util.logger.LoggerOps
Expand All @@ -48,7 +48,7 @@ final class RepoCacheAlg[F[_]](implicit
logger.info(s"Skipping due to previous error"),
for {
((repoOut, branchOut), cachedSha1) <- (
vcsApiAlg.createForkOrGetRepoWithDefaultBranch(config, repo),
vcsApiAlg.createForkOrGetRepoWithBranch(config, repo),
repoCacheRepository.findSha1(repo)
).parTupled
latestSha1 = branchOut.commit.sha
Expand All @@ -61,21 +61,23 @@ final class RepoCacheAlg[F[_]](implicit
private def cloneAndRefreshCache(repo: Repo, repoOut: RepoOut): F[Unit] =
for {
_ <- logger.info(s"Refresh cache of ${repo.show}")
defaultBranch <- vcsRepoAlg.defaultBranch(repoOut)
_ <- vcsRepoAlg.clone(repo, repoOut)
_ <- vcsRepoAlg.syncFork(repo, repoOut)
_ <- refreshCache(repo)
_ <- refreshCache(repo, defaultBranch)
} yield ()

private def refreshCache(repo: Repo): F[Unit] =
computeCache(repo).attempt.flatMap {
private def refreshCache(repo: Repo, defaultBranch: Branch): F[Unit] =
computeCache(repo, defaultBranch).attempt.flatMap {
case Right(cache) =>
repoCacheRepository.updateCache(repo, cache)
case Left(throwable) =>
refreshErrorAlg.persistError(repo, throwable) >> F.raiseError(throwable)
}

private def computeCache(repo: Repo): F[RepoCache] =
private def computeCache(repo: Repo, defaultBranch: Branch): F[RepoCache] =
for {
_ <- gitAlg.checkoutBranch(repo, repo.branch.getOrElse(defaultBranch))
branch <- gitAlg.currentBranch(repo)
latestSha1 <- gitAlg.latestSha1(repo, branch)
dependencies <- buildToolDispatcher.getDependencies(repo)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,29 +38,29 @@ trait VCSApiAlg[F[_]] {
if (config.doNotFork) getRepo(repo)
else createFork(repo)

final def createForkOrGetRepoWithDefaultBranch(config: Config, repo: Repo)(implicit
final def createForkOrGetRepoWithBranch(config: Config, repo: Repo)(implicit
F: MonadThrowable[F]
): F[(RepoOut, BranchOut)] =
if (config.doNotFork) getRepoWithDefaultBranch(repo)
else createForkWithDefaultBranch(repo)
if (config.doNotFork) getRepoWithBranch(repo)
else createForkWithBranch(repo)

final def createForkWithDefaultBranch(repo: Repo)(implicit
final def createForkWithBranch(repo: Repo)(implicit
F: MonadThrowable[F]
): F[(RepoOut, BranchOut)] =
for {
fork <- createFork(repo)
parent <- fork.parentOrRaise[F]
branchOut <- getDefaultBranch(parent)
branchOut <- getSelectedBranchOrDefault(parent, repo)
} yield (fork, branchOut)

final def getRepoWithDefaultBranch(repo: Repo)(implicit
final def getRepoWithBranch(repo: Repo)(implicit
F: Monad[F]
): F[(RepoOut, BranchOut)] =
for {
repoOut <- getRepo(repo)
branchOut <- getDefaultBranch(repoOut)
branchOut <- getSelectedBranchOrDefault(repoOut, repo)
} yield (repoOut, branchOut)

final def getDefaultBranch(repoOut: RepoOut): F[BranchOut] =
getBranch(repoOut.repo, repoOut.default_branch)
final def getSelectedBranchOrDefault(repoOut: RepoOut, repo: Repo): F[BranchOut] =
getBranch(repoOut.repo, repo.branch.getOrElse(repoOut.default_branch))
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@ object VCSRepoAlg {
else
for {
parent <- repoOut.parentOrRaise[F]
_ <- gitAlg.syncFork(repo, withLogin(parent.clone_url), parent.default_branch)
_ <- gitAlg.syncFork(
repo,
withLogin(parent.clone_url),
repo.branch.getOrElse(parent.default_branch)
)
} yield ()

val withLogin: Uri => Uri =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ object NewPullRequestData {
migrations: List[Migration] = List.empty
): NewPullRequestData =
NewPullRequestData(
title = git.commitMsgFor(data.update, data.repoConfig.commits),
title = git.commitMsgFor(data.update, data.repoConfig.commits, data.baseBranch),
body = bodyFor(
data.update,
artifactIdToUrl,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,30 @@
package org.scalasteward.core.vcs.data

import io.circe.{KeyDecoder, KeyEncoder}
import org.scalasteward.core.git.Branch

final case class Repo(
owner: String,
repo: String
repo: String,
branch: Option[Branch] = None
) {
def show: String = s"$owner/$repo"
def show: String = branch.map(branch => s"$owner/$repo/${branch.name}").getOrElse(s"$owner/$repo")
}

object Repo {
implicit val repoKeyDecoder: KeyDecoder[Repo] = {
val / = s"(.+)/([^/]+)".r
val regex = s"(.+)/([^#/\n]+)(?:#(.+))?".r
KeyDecoder.instance {
case owner / repo => Some(Repo(owner, repo))
case _ => None
case regex(owner, repo, null) => Some(Repo(owner, repo, None))
case regex(owner, repo, branch) => Some(Repo(owner, repo, Some(Branch(branch))))
case _ => None
}
}

implicit val repoKeyEncoder: KeyEncoder[Repo] =
KeyEncoder.instance(repo => repo.owner + "/" + repo.repo)
KeyEncoder.instance(repo =>
repo.branch
.map(branch => s"${repo.owner}/${repo.repo}/${branch.name}")
.getOrElse(s"${repo.owner}/${repo.repo}")
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,32 +22,33 @@ import org.scalasteward.core.application.SupportedVCS
import org.scalasteward.core.application.SupportedVCS.{Bitbucket, BitbucketServer, GitHub, Gitlab}
import org.scalasteward.core.data.ReleaseRelatedUrl.VersionDiff
import org.scalasteward.core.data.{ReleaseRelatedUrl, Update}
import org.scalasteward.core.git.Branch
import org.scalasteward.core.vcs.data.Repo

package object vcs {

/** Determines the `head` (GitHub) / `source_branch` (GitLab, Bitbucket) parameter for searching
* for already existing pull requests.
*/
def listingBranch(vcsType: SupportedVCS, fork: Repo, update: Update): String =
def listingBranch(vcsType: SupportedVCS, fork: Repo, update: Update, baseBranch: Branch): String =
vcsType match {
case GitHub =>
s"${fork.show}:${git.branchFor(update).name}"
s"${fork.show}:${git.branchFor(update, baseBranch).name}"

case Gitlab | Bitbucket | BitbucketServer =>
git.branchFor(update).name
git.branchFor(update, baseBranch).name
}

/** Determines the `head` (GitHub) / `source_branch` (GitLab, Bitbucket) parameter for creating
* a new pull requests.
*/
def createBranch(vcsType: SupportedVCS, fork: Repo, update: Update): String =
def createBranch(vcsType: SupportedVCS, fork: Repo, update: Update, baseBranch: Branch): String =
vcsType match {
case GitHub =>
s"${fork.owner}:${git.branchFor(update).name}"
s"${fork.owner}:${git.branchFor(update, baseBranch).name}"

case Gitlab | Bitbucket | BitbucketServer =>
git.branchFor(update).name
git.branchFor(update, baseBranch).name
}

def possibleTags(version: String): List[String] =
Expand Down
Loading