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.

Sign up for GitHub

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

fix: replace github search api with graphql in success lifecycle method #857

Merged
merged 37 commits into from
Jul 1, 2024
Merged

fix: replace github search api with graphql in success lifecycle method #857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
8929f7e
chore: add `loadAssociatedPRsQuery` helper to generate gql query
babblebey Jun 13, 2024
ce1c3e4
feat: implement graphql in associatedPRs fetch
babblebey Jun 13, 2024
d24079c
Merge branch 'master' into refactor/replace-search-api
babblebey Jun 13, 2024
76a2cd1
build: add `assert` to devDeps
babblebey Jun 13, 2024
b9bbd1f
do: extracted `loadAssociatedPRsQuery` to separate function
babblebey Jun 13, 2024
ca0bc4a
do (test): current attemption situation
babblebey Jun 13, 2024
f3be681
nits: prepend string `commit` to gal query aliases
babblebey Jun 14, 2024
1516ea3
nits: rename associatedPRs gql query builder helper
babblebey Jun 14, 2024
1eaffe0
Merge branch 'master' into refactor/replace-search-api
babblebey Jun 14, 2024
c58c3e2
fix: typo in `success.js`
babblebey Jun 14, 2024
6714074
fix: address test case `Add comment and labels to PRs associated with…
babblebey Jun 14, 2024
0299e79
do: address test case `Add comment and labels to PRs associated with …
babblebey Jun 14, 2024
e229ad3
do: address test case `Do not add comment and labels for unrelated PR…
babblebey Jun 14, 2024
813969b
do: address test case `Do not add comment and labels if no PR is asso…
babblebey Jun 14, 2024
b7963d7
do: address test case `Do not add comment and labels to PR/issues fro…
babblebey Jun 14, 2024
0d1f44f
do: address test case `Ignore missing and forbidden issues/PRs`
babblebey Jun 14, 2024
d8b5396
do: address test case `Add custom comment and labels`
babblebey Jun 14, 2024
703f7bb
do: address test case `Add custom label`
babblebey Jun 14, 2024
c4f5df3
do: address test case `Comment on issue/PR without ading a label`
babblebey Jun 14, 2024
5585c3b
do: address test case `Editing the release to include all release lin…
babblebey Jun 14, 2024
d336aa8
do: address test case `Editing the release to include all release lin…
babblebey Jun 14, 2024
4b7c9a9
do: address test case `Editing the release to include all release lin…
babblebey Jun 14, 2024
69e67cf
do: address other test case except "Make multiple search queries if n…
babblebey Jun 14, 2024
c909ed7
Merge branch 'refactor/replace-search-api' of https://github.com/sema…
babblebey Jun 14, 2024
96f5e3a
fix: address test case `Make multiple search queries if necessary`
babblebey Jun 14, 2024
db42a89
nits: make tiny changes in `success.js`
babblebey Jun 14, 2024
d97754d
fix: address `integration` tests
babblebey Jun 14, 2024
d268e51
Revert "build: add `assert` to devDeps"
babblebey Jun 14, 2024
6ca26fc
refactor: replace `isArray` method with `Object.values`
babblebey Jun 14, 2024
791dc93
fix(build): fix lint issues
babblebey Jun 14, 2024
5ebd038
chore: drop `getSearchQueries` integration from `success.js` and depr…
babblebey Jun 14, 2024
61eea50
Merge branch 'master' into refactor/replace-search-api
babblebey Jun 19, 2024
9b2a923
chore: remove deprecated `getSearchQueries` helper
babblebey Jun 19, 2024
9c58e02
fix: modify `buildAssociatedPRsQuery` returned string
babblebey Jun 21, 2024
737fd05
Merge branch 'master' into refactor/replace-search-api
babblebey Jun 24, 2024
807c400
feat: retrieve `body` property for `associatedPRs`
babblebey Jun 24, 2024
53c8024
Merge branch 'master' into refactor/replace-search-api
gr2m Jul 1, 2024
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
16 changes: 0 additions & 16 deletions lib/get-search-queries.js
View file
Open in desktop

This file was deleted.

94 changes: 58 additions & 36 deletions lib/success.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import debugFactory from "debug";
import parseGithubUrl from "./parse-github-url.js";
import resolveConfig from "./resolve-config.js";
import { toOctokitOptions } from "./octokit.js";
import getSearchQueries from "./get-search-queries.js";
import getSuccessComment from "./get-success-comment.js";
import findSRIssues from "./find-sr-issues.js";
import { RELEASE_NAME } from "./definitions/constants.js";
Expand Down Expand Up @@ -65,44 +64,38 @@ export default async function success(pluginConfig, context, { Octokit }) {
const releaseInfos = releases.filter((release) => Boolean(release.name));
const shas = commits.map(({ hash }) => hash);

const searchQueries = getSearchQueries(
`repo:${owner}/${repo}+type:pr+is:merged`,
shas,
).map(
async (q) =>
(await octokit.request("GET /search/issues", { q })).data.items,
const { repository } = await octokit.graphql(
buildAssociatedPRsQuery(shas),
{ owner, repo },
);

const searchQueriesResults = await Promise.all(searchQueries);
const uniqueSearchQueriesResults = uniqBy(
flatten(searchQueriesResults),
"number",
const associatedPRs = Object.values(repository).map(
(item) => item.associatedPullRequests.nodes,
);
const prs = await pFilter(
uniqueSearchQueriesResults,
async ({ number }) => {
const commits = await octokit.paginate(
"GET /repos/{owner}/{repo}/pulls/{pull_number}/commits",
{
owner,
repo,
pull_number: number,
},
);
const matchingCommit = commits.find(({ sha }) => shas.includes(sha));
if (matchingCommit) return matchingCommit;

const { data: pullRequest } = await octokit.request(
"GET /repos/{owner}/{repo}/pulls/{pull_number}",
{
owner,
repo,
pull_number: number,
},
);
return shas.includes(pullRequest.merge_commit_sha);
},
);
const uniqueAssociatedPRs = uniqBy(flatten(associatedPRs), "number");

const prs = await pFilter(uniqueAssociatedPRs, async ({ number }) => {
const commits = await octokit.paginate(
"GET /repos/{owner}/{repo}/pulls/{pull_number}/commits",
{
owner,
repo,
pull_number: number,
},
);
const matchingCommit = commits.find(({ sha }) => shas.includes(sha));
if (matchingCommit) return matchingCommit;

const { data: pullRequest } = await octokit.request(
"GET /repos/{owner}/{repo}/pulls/{pull_number}",
{
owner,
repo,
pull_number: number,
},
);
return shas.includes(pullRequest.merge_commit_sha);
});

debug(
"found pull requests: %O",
Expand Down Expand Up @@ -250,3 +243,32 @@ export default async function success(pluginConfig, context, { Octokit }) {
throw new AggregateError(errors);
}
}

/**
* Builds GraphQL query for fetching associated PRs to a list of commit hash (sha)
* @param {Array<string>} shas
* @returns {string}
*/
export function buildAssociatedPRsQuery(shas) {
return `#graphql
query getAssociatedPRs($owner: String!, $repo: String!) {
repository(owner: $owner, name: $repo) {
${shas
.map((sha) => {
return `commit${sha.slice(0, 6)}: object(oid: "${sha}") {
...on Commit {
associatedPullRequests(first: 100) {
nodes {
url
number
body
}
}
}
}`;
})
.join("")}
}
}
Comment on lines +254 to +272
Copy link
Member

Choose a reason for hiding this comment

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

We will need to address the possibility that there might be more than 100 commits. If that occurs, we need to send multiple requests as only 100 nodes can be requested with a single GraphQL request

Copy link

Choose a reason for hiding this comment

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

was this addressed in this PR? im working with a repo that has more than 100 commits and am experiencing this secondary rate error on v24.0.0

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @brandon-kyle-bailey

The Edge Case isn't addressed in this PR nor yet.

But, could you kindly open an issue with details of this error you're getting in regard this 100+ commit, thanks.

Copy link

Choose a reason for hiding this comment

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

unfortunately this is happening on an internal repo but ill do my best! @babblebey

Copy link

Choose a reason for hiding this comment

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

@babblebey im not able to reliable reproduce it at this point since re-running so will hold off on logging an issue. will revisit if it reappears!

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm currently working on a fix in regard the edge case here #892

Copy link
Member

Choose a reason for hiding this comment

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

just capturing the relation to #867 (comment) as well

`;
}
41 changes: 0 additions & 41 deletions test/get-search-queries.test.js
View file
Open in desktop

This file was deleted.

58 changes: 33 additions & 25 deletions test/integration.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,14 +432,17 @@ test("Comment and add labels on PR included in the releases", async (t) => {
repeat: 2,
},
)
.getOnce(
`https://api.github.local/search/issues?q=${encodeURIComponent(
`repo:${owner}/${repo}`,
)}+${encodeURIComponent("type:pr")}+${encodeURIComponent(
"is:merged",
)}+${commits.map((commit) => commit.hash).join("+")}`,
{ items: prs },
)
.postOnce("https://api.github.local/graphql", {
data: {
repository: {
commit123: {
associatedPullRequests: {
nodes: [prs[0]],
},
},
},
},
})
.getOnce(
`https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`,
[{ sha: commits[0].hash }],
Expand Down Expand Up @@ -649,14 +652,17 @@ test("Verify, release and notify success", async (t) => {
{ html_url: releaseUrl },
{ body: { draft: false } },
)
.getOnce(
`https://api.github.local/search/issues?q=${encodeURIComponent(
`repo:${owner}/${repo}`,
)}+${encodeURIComponent("type:pr")}+${encodeURIComponent(
"is:merged",
)}+${commits.map((commit) => commit.hash).join("+")}`,
{ items: prs },
)
.postOnce("https://api.github.local/graphql", {
data: {
repository: {
commit123: {
associatedPullRequests: {
nodes: [prs[0]],
},
},
},
},
})
.getOnce(
`https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`,
[{ sha: commits[0].hash }],
Expand Down Expand Up @@ -686,7 +692,6 @@ test("Verify, release and notify success", async (t) => {
browser_download_url: otherAssetUrl,
},
)

.postOnce(
`https://api.github.local/repos/${owner}/${repo}/issues/1/comments`,
{
Expand Down Expand Up @@ -800,14 +805,17 @@ test("Verify, update release and notify success", async (t) => {
},
},
)
.getOnce(
`https://api.github.local/search/issues?q=${encodeURIComponent(
`repo:${owner}/${repo}`,
)}+${encodeURIComponent("type:pr")}+${encodeURIComponent(
"is:merged",
)}+${commits.map((commit) => commit.hash).join("+")}`,
{ items: prs },
)
.postOnce("https://api.github.local/graphql", {
data: {
repository: {
commit123: {
associatedPullRequests: {
nodes: [prs[0]],
},
},
},
},
})
.getOnce(
`https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`,
[{ sha: commits[0].hash }],
Expand Down
Loading