Skip to content

Commit

Permalink
Fix scala-steward-org#3300: Use correct GitHub query to find existing…
Browse files Browse the repository at this point in the history
… Pull Requests

As described in scala-steward-org#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 scala-steward-org#649).
  • Loading branch information
rtyley committed Feb 29, 2024
1 parent 9a8f6d3 commit 9ace322
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 42 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ final class GitHubApiAlg[F[_]](
F.raiseWhen(repoOut.archived)(RepositoryArchived(repo))
}

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,9 @@ import org.scalasteward.core.git.Branch
package object forge {

/** Determines the `head` (GitHub) / `source_branch` (GitLab, Bitbucket) parameter for searching
* for already existing pull requests.
* for already existing pull requests or creating new pull requests.
*/
def listingBranch(forgeType: ForgeType, fork: Repo, updateBranch: Branch): String =
forgeType match {
case GitHub =>
s"${fork.owner}/${fork.repo}:${updateBranch.name}"

case GitLab | Bitbucket | BitbucketServer | AzureRepos | Gitea =>
updateBranch.name
}

/** Determines the `head` (GitHub) / `source_branch` (GitLab, Bitbucket) parameter for creating a
* new pull requests.
*/
def createBranch(forgeType: ForgeType, fork: Repo, updateBranch: Branch): String =
def headFor(forgeType: ForgeType, fork: Repo, updateBranch: Branch): String =
forgeType match {
case GitHub =>
s"${fork.owner}:${updateBranch.name}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ final class NurtureAlg[F[_]](config: ForgeCfg)(implicit
private def processUpdate(data: UpdateData): F[ProcessResult] =
for {
_ <- logger.info(s"Process update ${data.update.show}")
head = forge.listingBranch(config.tpe, data.fork, data.updateBranch)
head = forge.headFor(config.tpe, data.fork, data.updateBranch)
pullRequests <- forgeApiAlg.listPullRequests(data.repo, head, data.baseBranch)
result <- pullRequests.headOption match {
case Some(pr) if pr.state.isClosed && data.update.isInstanceOf[Update.Single] =>
Expand Down Expand Up @@ -230,7 +230,7 @@ final class NurtureAlg[F[_]](config: ForgeCfg)(implicit
.on(u => List(u.currentVersion.value), _.updates.map(_.currentVersion.value))
.flatTraverse(gitAlg.findFilesContaining(data.repo, _))
.map(_.distinct)
branchName = forge.createBranch(config.tpe, data.fork, data.updateBranch)
branchName = forge.headFor(config.tpe, data.fork, data.updateBranch)
allLabels = labelsFor(data.update, edits, filesWithOldVersion, artifactIdToVersionScheme)
labels = filterLabels(allLabels, data.repoData.config.pullRequests.includeMatchedLabels)
requestData = NewPullRequestData.from(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,9 @@ class ForgePackageTest extends FunSuite {
val update = ("ch.qos.logback".g % "logback-classic".a % "1.2.0" %> "1.2.3").single
val updateBranch = git.branchFor(update, None)

test("listingBranch (single)") {
assertEquals(listingBranch(GitHub, repo, updateBranch), s"foo/bar:${updateBranch.name}")
assertEquals(listingBranch(GitLab, repo, updateBranch), updateBranch.name)
}

test("createBranch (single)") {
assertEquals(createBranch(GitHub, repo, updateBranch), s"foo:${updateBranch.name}")
assertEquals(createBranch(GitLab, repo, updateBranch), updateBranch.name)
test("headFor (single)") {
assertEquals(headFor(GitHub, repo, updateBranch), s"foo:${updateBranch.name}")
assertEquals(headFor(GitLab, repo, updateBranch), updateBranch.name)
}

}
Expand All @@ -40,14 +35,9 @@ class ForgePackageTest extends FunSuite {

val updateBranch = git.branchFor(update, None)

test("listingBranch (grouped)") {
assertEquals(listingBranch(GitHub, repo, updateBranch), s"foo/bar:update/my-group")
assertEquals(listingBranch(GitLab, repo, updateBranch), updateBranch.name)
}

test("createBranch (grouped)") {
assertEquals(createBranch(GitHub, repo, updateBranch), s"foo:update/my-group")
assertEquals(createBranch(GitLab, repo, updateBranch), updateBranch.name)
test("headFor (grouped)") {
assertEquals(headFor(GitHub, repo, updateBranch), s"foo:update/my-group")
assertEquals(headFor(GitLab, repo, updateBranch), updateBranch.name)
}
}

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

val updateBranch = git.branchFor(update, None)

test("listingBranch (grouped) with $hash") {
assertEquals(listingBranch(GitHub, repo, updateBranch), s"foo/bar:update/my-group-1164623676")
assertEquals(listingBranch(GitLab, repo, updateBranch), updateBranch.name)
test("headFor (grouped) with $hash") {
assertEquals(headFor(GitHub, repo, updateBranch), s"foo:update/my-group-1164623676")
assertEquals(headFor(GitLab, repo, updateBranch), updateBranch.name)
}

test("createBranch (grouped) with $hash") {
assertEquals(createBranch(GitHub, repo, updateBranch), s"foo:update/my-group-1164623676")
assertEquals(createBranch(GitLab, repo, updateBranch), updateBranch.name)
}

}

}

0 comments on commit 9ace322

Please sign in to comment.