Skip to content

Commit 2dd07ba

Browse files
rtyleymzuehlke
authored andcommitted
Fix #3300: Use correct GitHub query to find existing Pull Requests
As described in #3300, Scala Steward has been using a now-invalid form of `head` query parameter when querying for existing GitHub Pull Requests. This has meant that it's been trying to raise new pull requests when existing ones with the same branch name were already present, leading to exceptions from the GitHub API. The `head` parameter being used would look like: ``` [organization]/[repo]:[ref-name] ``` ...however the documentation for the `List pull requests` endpoint says it should be just: ``` [organization]:[ref-name] ``` (see https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests) ...it actually _does_ make sense to _not_ have the repo name in there, as it _is_ redundant - the path in the REST API request already contains /repos/{owner}/{repo}/pulls, giving the original repo, and GitHub only allows one fork of a repo per organisation or user, so including the user or org in the prefix should be sufficient to uniquely identify a fork. Although the `[organization]/[repo]:[ref-name]` format must have worked in the past, it looks like GitHub must have made a change that means if you use it to search for PRs by branch name _now_, you'll get zero results - the PRs we're interested in won't come back. Consequently, this change updates Scala Steward to use the approved `head` parameter format, dropping the `/[repo]` part. The code in Scala Steward that constructed the parameter _was_ in the `forge.listingBranch()` method, but once it was fixed to remove `/[repo]`, it became identical to the `forge.createBranch()` method, and so it made sense to go from having 2 methods back to just 1 (there was originally just one `headFor` method back before PR #649).
1 parent 9a8f6d3 commit 2dd07ba

File tree

4 files changed

+14
-42
lines changed

4 files changed

+14
-42
lines changed

modules/core/src/main/scala/org/scalasteward/core/forge/github/GitHubApiAlg.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ final class GitHubApiAlg[F[_]](
9999
F.raiseWhen(repoOut.archived)(RepositoryArchived(repo))
100100
}
101101

102-
/** https://docs.github.com/en/rest/pulls?apiVersion=2022-11-28#list-pull-requests */
102+
/** https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests */
103103
override def listPullRequests(repo: Repo, head: String, base: Branch): F[List[PullRequestOut]] =
104104
client.get(url.listPullRequests(repo, head, base), modify)
105105

modules/core/src/main/scala/org/scalasteward/core/forge/package.scala

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,21 +23,9 @@ import org.scalasteward.core.git.Branch
2323
package object forge {
2424

2525
/** Determines the `head` (GitHub) / `source_branch` (GitLab, Bitbucket) parameter for searching
26-
* for already existing pull requests.
26+
* for already existing pull requests or creating new pull requests.
2727
*/
28-
def listingBranch(forgeType: ForgeType, fork: Repo, updateBranch: Branch): String =
29-
forgeType match {
30-
case GitHub =>
31-
s"${fork.owner}/${fork.repo}:${updateBranch.name}"
32-
33-
case GitLab | Bitbucket | BitbucketServer | AzureRepos | Gitea =>
34-
updateBranch.name
35-
}
36-
37-
/** Determines the `head` (GitHub) / `source_branch` (GitLab, Bitbucket) parameter for creating a
38-
* new pull requests.
39-
*/
40-
def createBranch(forgeType: ForgeType, fork: Repo, updateBranch: Branch): String =
28+
def headFor(forgeType: ForgeType, fork: Repo, updateBranch: Branch): String =
4129
forgeType match {
4230
case GitHub =>
4331
s"${fork.owner}:${updateBranch.name}"

modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ final class NurtureAlg[F[_]](config: ForgeCfg)(implicit
9292
private def processUpdate(data: UpdateData): F[ProcessResult] =
9393
for {
9494
_ <- logger.info(s"Process update ${data.update.show}")
95-
head = forge.listingBranch(config.tpe, data.fork, data.updateBranch)
95+
head = forge.headFor(config.tpe, data.fork, data.updateBranch)
9696
pullRequests <- forgeApiAlg.listPullRequests(data.repo, head, data.baseBranch)
9797
result <- pullRequests.headOption match {
9898
case Some(pr) if pr.state.isClosed && data.update.isInstanceOf[Update.Single] =>
@@ -230,7 +230,7 @@ final class NurtureAlg[F[_]](config: ForgeCfg)(implicit
230230
.on(u => List(u.currentVersion.value), _.updates.map(_.currentVersion.value))
231231
.flatTraverse(gitAlg.findFilesContaining(data.repo, _))
232232
.map(_.distinct)
233-
branchName = forge.createBranch(config.tpe, data.fork, data.updateBranch)
233+
branchName = forge.headFor(config.tpe, data.fork, data.updateBranch)
234234
allLabels = labelsFor(data.update, edits, filesWithOldVersion, artifactIdToVersionScheme)
235235
labels = filterLabels(allLabels, data.repoData.config.pullRequests.includeMatchedLabels)
236236
requestData = NewPullRequestData.from(

modules/core/src/test/scala/org/scalasteward/core/forge/ForgePackageTest.scala

Lines changed: 9 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,9 @@ class ForgePackageTest extends FunSuite {
1616
val update = ("ch.qos.logback".g % "logback-classic".a % "1.2.0" %> "1.2.3").single
1717
val updateBranch = git.branchFor(update, None)
1818

19-
test("listingBranch (single)") {
20-
assertEquals(listingBranch(GitHub, repo, updateBranch), s"foo/bar:${updateBranch.name}")
21-
assertEquals(listingBranch(GitLab, repo, updateBranch), updateBranch.name)
22-
}
23-
24-
test("createBranch (single)") {
25-
assertEquals(createBranch(GitHub, repo, updateBranch), s"foo:${updateBranch.name}")
26-
assertEquals(createBranch(GitLab, repo, updateBranch), updateBranch.name)
19+
test("headFor (single)") {
20+
assertEquals(headFor(GitHub, repo, updateBranch), s"foo:${updateBranch.name}")
21+
assertEquals(headFor(GitLab, repo, updateBranch), updateBranch.name)
2722
}
2823

2924
}
@@ -40,14 +35,9 @@ class ForgePackageTest extends FunSuite {
4035

4136
val updateBranch = git.branchFor(update, None)
4237

43-
test("listingBranch (grouped)") {
44-
assertEquals(listingBranch(GitHub, repo, updateBranch), s"foo/bar:update/my-group")
45-
assertEquals(listingBranch(GitLab, repo, updateBranch), updateBranch.name)
46-
}
47-
48-
test("createBranch (grouped)") {
49-
assertEquals(createBranch(GitHub, repo, updateBranch), s"foo:update/my-group")
50-
assertEquals(createBranch(GitLab, repo, updateBranch), updateBranch.name)
38+
test("headFor (grouped)") {
39+
assertEquals(headFor(GitHub, repo, updateBranch), s"foo:update/my-group")
40+
assertEquals(headFor(GitLab, repo, updateBranch), updateBranch.name)
5141
}
5242
}
5343

@@ -62,16 +52,10 @@ class ForgePackageTest extends FunSuite {
6252

6353
val updateBranch = git.branchFor(update, None)
6454

65-
test("listingBranch (grouped) with $hash") {
66-
assertEquals(listingBranch(GitHub, repo, updateBranch), s"foo/bar:update/my-group-1164623676")
67-
assertEquals(listingBranch(GitLab, repo, updateBranch), updateBranch.name)
55+
test("headFor (grouped) with $hash") {
56+
assertEquals(headFor(GitHub, repo, updateBranch), s"foo:update/my-group-1164623676")
57+
assertEquals(headFor(GitLab, repo, updateBranch), updateBranch.name)
6858
}
69-
70-
test("createBranch (grouped) with $hash") {
71-
assertEquals(createBranch(GitHub, repo, updateBranch), s"foo:update/my-group-1164623676")
72-
assertEquals(createBranch(GitLab, repo, updateBranch), updateBranch.name)
73-
}
74-
7559
}
7660

7761
}

0 commit comments

Comments
 (0)