feat: update GitHub issues API to support fetching issues by repository host#8135
feat: update GitHub issues API to support fetching issues by repository host#8135sonikro wants to merge 1 commit into
Conversation
Changed Packages
|
|
Thanks for the contribution! |
268569c to
743dad7
Compare
…sitory host Signed-off-by: Jonathan Nagayoshi <jonathan@nagayoshi.com.br>
|
This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution! |
|
Hi @sonikro, sorry this closed on you, I was starting a review on this two weeks ago but seems I didn't hit submit but when I did just now I seem to have lost my comments. I'll try and review this again in the next few days. |
|
Hi @hopehadfield and @Fortune-Ndlovu, this PR existed first, any chance we can consolidate the work done in #9105 here? I have not looked at the newer PR myself I just could remember reviewing a PR that did the same thing already when I found I didn't submit my review here. |
Fortune-Ndlovu
left a comment
There was a problem hiding this comment.
Hey @sonikro, nice work on the multi-host support! I took a close look at this PR and I think the architectural change (grouping repos by host at the API layer) is the right approach.
One thing I noticed though, this PR removes the getHostnameFromEntity call from useGetIssuesByRepoFromGithub.ts, but there's still a call to getHostnameFromEntity in useEntityGithubRepositories.ts where the repository list is built. That's actually the call site that crashes for file: type source-locations it throws TypeError: Invalid URL before the repos ever reach the API layer.
I opened #9105 which fixes getHostnameFromEntity itself to handle file: type locations. Happy to consolidate that fix into this PR if that's easier for the maintainers. The two changes are complementary:
- This PR (#8135): Refactors the API to support multi-host and removes the hostname param from
fetchIssuesByRepoFromGithub - #9105: Fixes
getHostnameFromEntityso it doesn't throw onfile:type source-locations (which is what actually causes the crash)
Without the getHostnameFromEntity fix, entities with file: source-locations will still crash in useEntityGithubRepositories before the repos reach your new multi-host grouping logic.
Let me know how you'd like to proceed. I can push the getHostnameFromEntity fix to this branch, or we can merge them separately.
Hey, I just made a Pull Request!
Problem
By moving the hostname discovery from the Group to the Repo (and splitting the query into one per github host) we solve both problems.
This allows groups that are not synced from Github to still use the plugin + adds a new feature that allows you to see all of your issues across multiple github instances.
✔️ Checklist
Signed-off-byline in the message. (more info)