fix: replace github search api with graphql in success lifecycle method #857
Conversation
There was a problem hiding this comment.
I'm pretty sure this works, but can't exactly test, hence I've got to figure how to update the test for a mock .... This is where i'm kinda BLOCKED..
See the comments associated to this review first.... then hear my BLOCKER 😉
I notice the convention with the usage of the fetch-mock's sandbox().getOnce() method which mocks a request and provides a response to the immediate request provided a mock endpoint like so...
fetchMock
.sandbox()
.getOnce(`https://api.github.local/repos/${owner}/${repo}`, {
full_name: `${redirectedOwner}/${redirectedRepo}`,
}); // getOnce("<mockRequestEnpoint>", "<mockResponse>")but my question is...
HOW EXACTLY DO I MOCK THE GRAPHQL ENDPOINT??? using similar convention 🤔
- I'll need the exact appropriate mock GraphQL endpoint????
I'm not sleeping on it too 😉
UPDATE:
I found https://github.com/octokit/graphql.js?tab=readme-ov-file#writing-tests 🤔, requires some new dependencies 😞 I think now... If there's a way to do this without adding any new dependency, I'll take that
you don't need the |
Co-authored-by: Gregor Martynus <[email protected]>
Co-authored-by: Gregor Martynus <[email protected]>
Oh wow, I'll give that a shot... I already done too much if this is the case in my initial attempt 🫣 |
|
Holler if you need help with this PR |
… release commits and issues solved by PR/commits comments`
…release commits and issues closed by PR/commits comments with custom URL`
… returned by search (compare sha and merge_commit_sha)`
…ciated with release commits`
…ks at the bottom`
…ks with no additional releases (top)`
babblebey
changed the title
gr2m
changed the title
|
I ran it locally and got the following error In order to run it locally, checkout the The way I ran it locally is as follows
Not sure if this could be streamlined ... but we should document it somewhere for future reference and improve if we can 😁 |
|
It's possible that this error is an edge case that only happens because I ran it locally. The commit only exist on my machine due to the local testing. When your query is sent, the commit does not exist and probably returns null or even an error. If that happens, I'd simply ignore the commit and move on to the next one |
@gr2m, I figured it was a GraphQL error... turned out I had a some part of the query returned string (from the I have recorded and added a screencast to description above. |
|
Great work! I'd like to see that the success step actually posts a comment, we can pair on it during our checkin today |
|
Hi @gr2m, turns out the reason why the comment wasn't posting to only merged PRs and not the associated closed/fixed issue is because I failed to retrieve the Kindly checkout description for updated Demo Video, also PRs and Issue below...
🤦 |
| 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("")} | ||
| } | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
unfortunately this is happening on an internal repo but ill do my best! @babblebey
There was a problem hiding this comment.
@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!
There was a problem hiding this comment.
I'm currently working on a fix in regard the edge case here #892
There was a problem hiding this comment.
just capturing the relation to #867 (comment) as well
|
How fitting that the release of this fix also ran into the secondary rate limit issue: |
I sigh in relief**** 😮💨 |
This Pull Request refactors the
successlifecycle function replacing the usage of Github "Search API" with "GraphQL API".Changes Made
Added
buildAssociatedPRsQueryhelper function to which builds GraphQL query for fetching associated PRs to a list of commit hash (sha). It does this by collecting a list ofshaswhich maps down to a list of strings that leverages the GraphQLfragmentenabling ability to request data of multiple commits in one request usingGitObject.Replaced the instance of the search api consumption with the
getSearchQueriesmethod integration with the graphql api integration, which was used to retrieve all associated pull requests.Modified test suites in acknowledgement of the changes..
Related Issue
Fixes #644
Screencast/Screenshot
screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.06.24-14_54_49.webm