Fix #3300: Use correct GitHub query to find existing Pull Requests #3303

rtyley · 2024-02-29T05:34:22Z

This PR has 2 commits, it may be helpful to review them separately:

✅ Fix the head parameter when querying GitHub for existing PRs
➡️ Refactor pull request head: replace conditionals with polymorphism - this deletes the forge package object, in favour of a new ForgeType.pullRequestHeadFor() method.

As described in #3300, Scala Steward is using a now-invalid form of head query parameter when querying for existing GitHub Pull Requests. This means that it's not been finding existing PRs, and so trying to raise new PRs with the same branch name as existing PRs, leading to exceptions from the GitHub API - and also, I think, #3301.

Scala Steward was using a head parameter of the form:

[organization]/[repo]:[ref-name]

...however the documentation for the List pull requests API endpoint says it should be just:

[organization]:[ref-name]

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 so that 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).

CodeCov

Note that Codecov is asserting that line #L95 is not covered by tests, but pullRequestHeadFor() certainly is covered by tests (in ForgeTypeTest), and line 95 itself is not added, just changed - I feel this is a warning that can be ignored.

rtyley · 2024-02-29T05:36:25Z

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

-  /** 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 */


This new url is the current one for the API documentation on the endpoint: https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests

It says:

head string
Filter pulls by head user or head organization and branch name in the format of user:ref-name or organization:ref-name. For example: github:new-script-format or octocat:test-branch.

...which is quite important for this PR, because it doesn't say you should include the repo name in a organization/repo:ref-name format.

codecov · 2024-02-29T05:38:58Z

Codecov Report

Attention: Patch coverage is 85.71429% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 91.18%. Comparing base (9a8f6d3) to head (51b76c2).

Files	Patch %	Lines
...ala/org/scalasteward/core/nurture/NurtureAlg.scala	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3303      +/-   ##
==========================================
- Coverage   91.19%   91.18%   -0.02%     
==========================================
  Files         167      166       -1     
  Lines        3408     3404       -4     
  Branches      304      317      +13     
==========================================
- Hits         3108     3104       -4     
  Misses        300      300

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rtyley · 2024-02-29T05:47:10Z

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

+  /** Determines the `head` (GitHub) / `source_branch` (GitLab, Bitbucket) parameter for searching
+    * for already existing pull requests or creating new pull requests.
+    */
+  def pullRequestHeadFor(@nowarn fork: Repo, updateBranch: Branch): String = updateBranch.name


This is the new home for the listingBranch()/createBranch() code in the now-deleted modules/core/src/main/scala/org/scalasteward/core/forge/package.scala file. After correcting listingBranch() to output the right string, it was identical to createBranch, so there was no point in having two methods- just pullRequestHeadFor() will do. The method is also moved to ForgeType, as it does vary by forge type (specifically, it's different for GitHub to everyone else).

The method was originally called headFor, back before #649.

rtyley · 2024-02-29T05:52:27Z

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

-      branchName = forge.createBranch(config.tpe, data.fork, data.updateBranch)
      allLabels = labelsFor(data.update, edits, filesWithOldVersion, artifactIdToVersionScheme)
      labels = filterLabels(allLabels, data.repoData.config.pullRequests.includeMatchedLabels)
-      requestData = NewPullRequestData.from(
-        data = data,
-        branchName = branchName,
-        edits = edits,
-        artifactIdToUrl = artifactIdToUrl,
-        artifactIdToUpdateInfoUrls = artifactIdToUpdateInfoUrls.toMap,
-        filesWithOldVersion = filesWithOldVersion,
-        addLabels = config.addLabels,
-        labels = data.repoData.config.pullRequests.customLabels ++ labels
-      )
-    } yield requestData
+    } yield NewPullRequestData.from(
+      data = data,
+      branchName = config.tpe.pullRequestHeadFor(data.fork, data.updateBranch),
+      edits = edits,
+      artifactIdToUrl = artifactIdToUrl,
+      artifactIdToUpdateInfoUrls = artifactIdToUpdateInfoUrls.toMap,
+      filesWithOldVersion = filesWithOldVersion,
+      addLabels = config.addLabels,
+      labels = data.repoData.config.pullRequests.customLabels ++ labels
+    )


This is a small clean-up I couldn't resist- there's no point in defining branchName & requestData back on old lines 233 & 236 - they're just used again immediately just once, and in both cases there's already sufficient context in the code for us to well understand what they are (NewPullRequestData's named parameter branchName, and then the name of the NewPullRequestData type itself).

rtyley · 2024-02-29T05:54:34Z

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

+import org.scalasteward.core.forge.ForgeType.{GitHub, GitLab}
+import org.scalasteward.core.git
+
+class ForgeTypeTest extends FunSuite {


Now that the implementation code is in ForgeType, this is mainly just a rename of ForgePackageTest to ForgeTypeTest - but additionally, with correction to all assertions so that they assert the correct head values, without the /repo part).

rtyley · 2024-02-29T05:57:37Z

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

+    override def pullRequestHeadFor(fork: Repo, updateBranch: Branch): String =
+      s"${fork.owner}:${updateBranch.name}"


This is the line of code crucial to the fix - using:

s"${fork.owner}:${updateBranch.name}"

...which removes the /${fork.repo} that was previously there:

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

Line 31 in 9a8f6d3

s"${fork.owner}/${fork.repo}:${updateBranch.name}"

rtyley · 2024-02-29T06:02:22Z

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

@@ -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 = config.tpe.pullRequestHeadFor(data.fork, data.updateBranch)


Note that Codecov is asserting that line #L95 is not covered by tests, but pullRequestHeadFor() certainly is covered by tests (in ForgeTypeTest), and line 95 itself is not added, just changed - I feel this is a warning that can be ignored.

… 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).

This is another example of replacing conditionals with polymorphism on `ForgeType`. This was a theme in commit b42866b, merged in this PR back in August 2023: scala-steward-org#3145 A benefit of this pattern is as new types of forge are implemented, the responsibilities for supporting that new `ForgeType` are all in one place, rather than spread across several methods elsewhere in the codebase. To confess, I think there's probably a further refactor that could be done, to introduce a better parameter object denoting a 'Pull Request Head' (that conceivably could better model the way that some `ForgeType`s don't even support forking) - but that can wait for another day, fixing scala-steward-org#3300 is more urgent!

mzuehlke

Thanks @rtyley for the investigation.

The only explanation is that Github after a long time stopped supporting the "alternative" syntax. If there was ever a warning, nobody paid attention to it, because it worked.

And thanks for the super easy to review PR 👍

rtyley · 2024-02-29T09:27:25Z

Thanks for approving the PR @mzuehlke ! I think this issue probably warrants a quick 0.29.1 release, as you've probably seen from #3301 and comments by other users it's a widespread issue for a lot of users!

I don't have merge rights on the Scala Steward repo - would you be able merge/release this fix?

mzuehlke · 2024-02-29T10:01:56Z

I will push a fix version in a minute

mzuehlke · 2024-02-29T10:26:05Z

Version v0.29.1 has been released. Please give it a try 🙏

rtyley · 2024-02-29T11:33:40Z

Version v0.29.1 has been released. Please give it a try 🙏

I've re-enabled our Scala Steward workflows to try out the new release, but unfortunately it looks like GitHub Actions are currently experiencing issues!...

Probably related, when I try to run the Scala Steward workflow, I get this error about not being able to decode a JWT:

Run scala-steward-org/scala-steward-action@v2.63.0
✓ Default Scala Steward configuration set to: common-config/scala-steward.conf
✓ Using multiple repos file: REPOSITORIES.md
Error:  ✕ A JSON web token could not be decoded

...this occurs during scala-steward-action, before the scala-steward JVM process is even started, so I think we can be sure that the v0.29.1 release is not the problem...!

Dichotomia · 2024-02-29T11:39:31Z

I tried v0.29.1 and it worked much better.

rtyley · 2024-02-29T15:06:38Z

unfortunately it looks like GitHub Actions are currently experiencing issues!...

These issues are (according to www.githubstatus.com) fully resolved, but surprisingly, I'm still not able to get a Scala Steward run to pass initialisation - they all fail, almost immediately, with:

Run scala-steward-org/scala-steward-action@v2.63.0
✓ Default Scala Steward configuration set to: common-config/scala-steward.conf
✓ Using multiple repos file: REPOSITORIES.md
Error:  ✕ A JSON web token could not be decoded

Curiously, I see someone from 4 days ago reporting a similar "JSON web token could not be decoded" error:

octokit/octokit.net#2833 (comment)

...this made it sound as though possibly new Node runtimes could be the problem, and I raised scala-steward-org/scala-steward-action#565 thinking that a change there might be necessary - but no, in the end it was my own mistake, I had deleted the steward GitHub App key! Once that was refreshed, everything worked perfectly!

rtyley commented Feb 29, 2024

View reviewed changes

rtyley added 2 commits February 29, 2024 06:21

rtyley force-pushed the fix-issue-3300-use-correct-github-query-to-find-existing-pull-requests branch from 0e513d9 to 51b76c2 Compare February 29, 2024 06:21

rtyley marked this pull request as ready for review February 29, 2024 06:42

rtyley mentioned this pull request Feb 29, 2024

Force-pushing over modified branches #3301

Closed

mzuehlke approved these changes Feb 29, 2024

View reviewed changes

mzuehlke added cat:forge bug Something isn't working labels Feb 29, 2024

mzuehlke merged commit 7ec418c into scala-steward-org:main Feb 29, 2024
4 of 6 checks passed

exoego approved these changes Feb 29, 2024

View reviewed changes

mzuehlke added this to the 0.30.0 milestone Feb 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3300: Use correct GitHub query to find existing Pull Requests #3303

Fix #3300: Use correct GitHub query to find existing Pull Requests #3303

rtyley commented Feb 29, 2024 •

edited

Loading

rtyley Feb 29, 2024 •

edited

Loading

codecov bot commented Feb 29, 2024 •

edited

Loading

rtyley Feb 29, 2024 •

edited

Loading

rtyley Feb 29, 2024

rtyley Feb 29, 2024 •

edited

Loading

rtyley Feb 29, 2024 •

edited

Loading

rtyley Feb 29, 2024

mzuehlke left a comment

rtyley commented Feb 29, 2024

mzuehlke commented Feb 29, 2024

mzuehlke commented Feb 29, 2024

rtyley commented Feb 29, 2024

Dichotomia commented Feb 29, 2024

rtyley commented Feb 29, 2024 •

edited

Loading

		/** 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 pullRequestHeadFor(fork: Repo, updateBranch: Branch): String =
		s"${fork.owner}:${updateBranch.name}"

Fix #3300: Use correct GitHub query to find existing Pull Requests #3303

Fix #3300: Use correct GitHub query to find existing Pull Requests #3303

Conversation

rtyley commented Feb 29, 2024 • edited Loading

CodeCov

rtyley Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 29, 2024 • edited Loading

Codecov Report

rtyley Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

rtyley Feb 29, 2024

Choose a reason for hiding this comment

rtyley Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

rtyley Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

rtyley Feb 29, 2024

Choose a reason for hiding this comment

mzuehlke left a comment

Choose a reason for hiding this comment

rtyley commented Feb 29, 2024

mzuehlke commented Feb 29, 2024

mzuehlke commented Feb 29, 2024

rtyley commented Feb 29, 2024

Dichotomia commented Feb 29, 2024

rtyley commented Feb 29, 2024 • edited Loading

rtyley commented Feb 29, 2024 •

edited

Loading

rtyley Feb 29, 2024 •

edited

Loading

codecov bot commented Feb 29, 2024 •

edited

Loading

rtyley Feb 29, 2024 •

edited

Loading

rtyley Feb 29, 2024 •

edited

Loading

rtyley Feb 29, 2024 •

edited

Loading

rtyley commented Feb 29, 2024 •

edited

Loading