Add [GithubCheckRuns] service

[mbtools]

Description

This service is for displaying the results of check runs in GitHub.

Data

• Is there a public API? Yes
• Does the API require an API key? Uses typical GitHub auth approach
• Link to the API documentation: https://docs.github.com/en/rest/checks/runs#list-check-runs-for-a-git-reference

API path would be: /{owner}/{repo}/commits/{ref}/check-runs

For example: https://api.github.com/repos/badges/shields/commits/master/check-runs

Motivation

Check Runs (and Check Suites) reflect the status of various GitHub Apps/Integrations/Services/etc. and are most often seen in the Checks tab for a PR or visible as icons next to the commit descriptions.

Notes

This service returns the aggregated status of a ref. It currently does not allow to select a specific check run (job) as envisioned in #4364 but this can certainly be added later (as query parameter).

Preview

[shields-ci]

	Messages
📖	✨ Thanks for your contribution to Shields, @mbtools!

Generated by 🚫 dangerJS against 9eaa065

[calebcartwright]

It currently does not allow to select a specific check run (job) as envisioned in #4364 but this can certainly be added later (as query parameter).

Interesting, thanks for making this note! Since refs can be branches and branches can contain / that does mean we have to account for that in the route parameter (should be something like :ref+) though if we do keep the ref as a route parameter that prevents us from being able to incorporate any additional route params to follow

[mbtools]

It didn't occur to me that branches with / could be a problem but you are right. I use GH check status as a starting point which uses :ref but it seems like this is not correct either. Other places referring to branches use :branch* as the final part.

Should it be :ref+ or :ref*?

[calebcartwright]

Should it be :ref+ or :ref*?

That determination is driven off the requirements of the upstream endpoint. Most of the time the upstream endpoint supports an optional parameter that can be a branch, but in a smaller set of cases the branch/ref is actually required.

I assumed that for check runs a ref would be required, is that correct?

[mbtools]

yes, ref is required

[calebcartwright]

yes, ref is required

Then you'll want to use + to reflect that it's required (* would be optional/zero or more). I think the Bitbucket Pipelines badge is similar and may serve as a helpful reference

[calebcartwright]

I've done a quick pass through this and have opted to leave a few higher level comments/questions vs. inline comments.

As far as the code, seems to generally be in good order, though I'm curious if the various other helpers, e.g. isBuildStatus could be used more directly. If, not it'd be helpful if you could elaborate on the what and why of some of the custom mappings you're doing, e.g. a count of zero mapping to no tests considering that checks are much more than tests.

Additionally, I think more could be extracted within the transform, at least from the call flow perspective of the handle function; in cases where it makes sense to have a transform function (which indeed seems to be the case here) we'd typically expect whatever transform returns to be the thing that handle can pass along directly to whatever function is doing the rendering

[mbtools]

There are several status/conclusion values returned by the API that are not included in build-status.js. I didn't want to mess with the existing list there.

The individual conclusions have to be aggregated in a way first and then mapped to a status anyway. I tried to map them to existing states that work with renderBuildStatusBadge. That's why it's no tests. no check runs would make more sense but requires changing build-status.js.

JS is really a hobby of mine so I'm sure there are more efficient ways to transform/map things. Happy to learn from someone who's more proficient than me.

[calebcartwright]

There are several status/conclusion values returned by the API that are not included in build-status.js. I didn't want to mess with the existing list there.

Understood. There's always a balance between trying to determine whether to incorporate something from a service into the shared enumerations.

I still believe there's a bit more to think through here though relative to our desired behavior. For example, if the runs have an "in progress" status, does the conclusion field strictly always exist? Given the list of values defined in the schema for the conclusion, I'd be surprised if an actively running case has a conclusion field already set with one of those values. We should probably also look for opportunities to be consistent with our other similar types of badges (e.g. would a for the "in progress" state would message of pending be more consistent with other badges, like the check state, as opposed to using processing for the message)

On the implementation side, if we anticipate that we'll need a more service/endpoint-specific mapping beyond the general purpose build status (which I'm certainly open to, especially since it seems like we'll indeed need a two-fold mapping), then we should still refactor the code a bit to make it more easily testable. The handle function really should just be an orchestrator, and except in cases where the data extraction/transform is quite trivial (e.g. a simple destructure), we strongly prefer moving that logic outside the handle function so that it can be covered via more standard unit testing approaches.

[mbtools]

Prettier

[mbtools]

I finally found some time for this...

Meanwhile GH has updated their API docs which made defining the joi schema a piece of cake.

I combined the state mapping into a separate method and added a spec test for it as well.

I ran npm run prettier:check and everything is fine. I can't figure out why it fails here. Can you have a look, please?

[mbtools]

One file had CR instead of LF... Anyway, all checks have passed! Yay!

Ready for final review 😄

[mbtools]

bump

[mbtools]

Ladies and Gentlemen, Dear Maintainers,

it's been 2 years since I opened this PR. Do I qualify as the most patient contributor on GitHub yet?

I will give it one more shot. Please review and let me know what should be changed. If I do not get any reaction soon, I will close this PR.

Best,
Marc

[bourgeoisor]

I've been watching this PR for over a year now, hoping it gets merged. A little bit of communication from the maintainers would be great to get this going. If there's changes that needs to happen before this is merged in, then let's spell out those changes so they can be fixed.

[PyvesB]

As a reminder, this is an open-source project, a small team of maintainers dedicate their spare time for free, some of us having diligently done so since 2017. If folks want this so badly, why don't they participate in a constructive way, for example by doing a first review pass, testing the new badges, and reporting on their findings here? Implying that there's been a five year wait, for an issue that saw outside activity for the first time in 2022, feels all the more out of place.

@mbtools we do value your contribution and the time you put into this, and I'm sorry we've kept you on the hook for so long. I am a little stretched at the moment, but I do want to pick this up at some point. If you could extend your patience a little further, that would be great! 😃

[mbtools]

No worries, I will leave it open. Thanks for putting it back on the radar 👍

[PyvesB]

The deployment to a review app to test this out has failed, hopefully #10134 should fix it.

[github-actions]

🚀 Updated review app: https://pr-7759-badges-shields.fly.dev

[github-actions]

🚀 Updated review app: https://pr-7759-badges-shields.fly.dev

[mbtools]

Looks to be working well in the review app 😀

[github-actions]

🚀 Updated review app: https://pr-7759-badges-shields.fly.dev

[mbtools]

I added an optional name filter so you can get the status of one particular check run

[github-actions]

🚀 Updated review app: https://pr-7759-badges-shields.fly.dev

[mbtools]

[PyvesB]

Thanks for the good work, one small change and I'd say we can merge this! :)

[PyvesB]

Thanks for your patience on this one @mbtools! Could you please prepare the follow up PR we've been discussing in #4364 (comment)? 🙏🏻

[mbtools]

Woohoo! I will have some 🍰 and pop a bottle of 🍾 🥳 🎈 🎉

PS: I will get back to the comment later today