-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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.
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 the check run filter by id #18650
fix the check run filter by id #18650
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve
Hey @saurabh-git-dev, thanks so much for taking the time to contribute to Desktop! I believe this change makes sense but I'd really like for @tidy-dev (the original author of this) to take a look as well. My concern is centered around the fact that we named this function getLatestCheckRunsByName so I want to make sure that id is a suitable replacement. If it is then we should update the name of the function as well. |
I did some digging to see if I could find reasoning for using the name versus the id. (I couldn't remember this method) @niik you are the original author of the function for the pr status work. I only added filter based on if it was a push or a pull. 1eed3bc#diff-e3f9f5058c055c6e578c6019115cad54a10a80455eeeb4c4afca17df1b86150cL352 Looking at the docs, I cannot see a reason not to use id here instead of name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saurabh-git-dev Could you please update the function name to getLatestCheckRunsById?
I actually had the same thought as to why the "id" was not used. In the shared picture there are 2 jobs with the same name. And also the same workflow name. That's why appear in a 'Test' group. I looked at the grouping method. But it is used in many places. So did not change that. desktop/app/src/lib/ci-checks/ci-checks.ts Line 598 in b80f540
Although there is not a big impact with the grouping issue. |
method name changed and formatted the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💖 Thanks for the fix.
I tested across GitHub Action and Non-GitHub Action checks and works as expected.
Sounds like it would be more accurate to use id for unique workflow names as well.
Closes #18648
Description
This issue is due to filtering by check-run "name" instead of filtering by "id". It is replaced with "id" as we get check-run "id" in the API response(Attached API ref.).
If we change the name with id. We can see all the jobs with the same name.
API Ref:
https://docs.github.com/en/rest/checks/runs?apiVersion=2022-11-28#list-check-runs-for-a-git-reference
Screenshots