Repository navigation
Conversation
For commits that are not on the default branch, the GraphQL field `associatedPullRequests` also returns pull requests opened in other repositories, e.g. forks that sync from this one. Their numbers do not exist in the released repository, so looking them up fails with a 404 and no success comment is posted at all. Also pass the commit sha when loading more than 100 associated pull requests of a commit (`response` was undefined) and make the multipaged test exercise that path: its second mock replaced the first one. The PR fields shared by the three queries are now one fragment, so a field cannot be missing from one of them. Fixes semantic-release#1032 Fixes semantic-release#1092
Author
|
@babblebey @jedwards1211 please check this out, this has been long pending or tell me some other alternative to fix it |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a release is cut from a branch that is not the default branch (e.g. releases from
mainwhiledevelopis the default), thesuccessstep crashes and posts no comment at all:Root cause: the GraphQL field
associatedPullRequestsreturns every pull request that contains the commit, including pull requests opened in other repositories. For commits on the default branch GitHub only returns the merged PR, but for commits on other branches it also returns open PRs from forks (for example a fork'spull[bot]sync PRfork-owner/fork#66). The plugin then callsGET /repos/<owner>/<repo>/pulls/66/commitson the released repository, where#66is an unrelated issue, gets a 404, and thepFilterover all associated PRs rejects.Verified with the GraphQL API on frappe/helpdesk commit
c295efa8e:associatedPullRequestsreturns#66(repositoryjango-blockchained/desk, a fork) next to#3882,#3897and#3931(repositoryfrappe/helpdesk). This happens on every release while such a fork PR stays open.Same bug as reported in #1032 and #1092. #1292 proposed the same approach but was closed by its author before review; this PR adds the regression test and the pagination fix below.
Fix
repository { nameWithOwner }for each associated PR and drop PRs whose repository is not the released one. The comparison uses theowner/repothe plugin already resolves viaGET /repos/{owner}/{repo}, so renamed repositories keep working.uniqBy(…, "number"), so a foreign PR can no longer shadow a same-numbered PR of this repository.response.commit.oid) and threw aReferenceError. The per-commit pagination now lives in a smallloadAllAssociatedPRshelper (while (pageInfo.hasNextPage)) that passes the commit'soid, which also removes the manualcursor/hasNextPageflags and the duplicated push site. The existing "multipaged associatedPRs" test never reached that branch because its secondpostOnceon the same URL usedoverwriteRoutes: trueand replaced the first mock; it now matches on the query name and asserts theshaandcursorvariables.prFieldsfragment, sorepositorycannot be present on page 1 but missing on page 2. The twoflatten()calls on already-flat arrays are gone with it.Note: the pagination path is live for the first time. On a heavily forked repository a non-default-branch commit can carry hundreds of fork PRs, which now means a few sequential 100-node pages per commit instead of a crash.
Edge cases considered
repository= this repository (onlyheadRepositoryis the fork), so contributor PRs are still commented on. Filtering onheadRepositorywould be wrong.owner/reposlug.Tests
#66is returned next to PR#1of this repository; only#1is looked up and commented on (no mock exists for#66, so any request for it fails the test).