[docstrCoverage] Add docstrCoverage badge service

[fabiosangregorio]

This PR adds the badge for docstr-coverage (#5339), a documentation coverage package for python.

[shields-ci]

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

Generated by 🚫 dangerJS against 99e0f49

[chris48s]

Hello. Thanks for working on this. I'm looking forward to reviewing this as I think it may be a useful pattern for other badges.

That said, I guess the first very high-level thing we need to raise here is that our badges usually fetch an API response (JSON/XML/YAML) and extract a value from it. We do restrict the maximum response size we will accept, but usually if the job is "fetch some JSON and extract a number from it" we can reasonably expect that this will be of an acceptable size and this operation can be completed in some sensible amount of time. Once the problem becomes "fetch a zip file, unzip it and extract a number from one file in that zip", it feels like we could easily get into dealing with larger files, longer execution times, more disk space/memory used, etc. I've done no investigations on this myself yet, but...

• Is there any restriction on the maximum size of a GH artifact? Could they be arbitrarily large?
• What sort of execution times are you seeing with small examples?
• Do we need to be mindful of any kind of cleanup of disk space after extracting the zip?
• Beyond our existing blanket restriction on max response size, do you think there are any ways we can ensure we place some restrictions on the artifacts we'll accept to ensure sensible performance?
• Is size the only thing we need to worry about i.e: is the overhead of extracting a zip broadly linear with the size, or is it possible to have a "small but complex" zip?

I'd also welcome feedback from one of the other @badges/shields-maintainers on these issues. I think its important not to sleepwalk into a big performance problem here.

Aside from that, I haven't looked at the service code in any real detail yet, but just doing a really quick first pass, there are several basic things to sort out:

• There are several build failures. Can you have a look at the failing tests/lint checks. You can run npm run prettier and npm run lint to address the formatting issues.
• Although the thing you're measuring here is about docstr_coverage, this is fundamentally a github badge. Can we move the code to the /github dir and the url into the /github namespace. Just thinking forwards slightly: If we can ensure reasonable performance constraints, I can see this pattern (calculate a thing based on a github actions artifact) possibly being quite useful and I can see us ending up with a bunch of "X (from github actions artifact) badges. Abstracting the code for that can come later, but in terms of the URLs, how about we put this under something like /github/artifacts/python-docstr-coverage and give ourselves room to grow here.
• This PR adds an additional dependency yauzl but doesn't add it to package.json. This is working because it is a transitive dependency of other libraries we depend on but this should be added to package.json as a first-order dependency if we explicitly use it.
• Conversely, there are other changes to package.json and package-lock.json which are unrelated to this PR. Can you back these out please.
• There are also some changes to .gitignore and .vscode/launch.json which are unrelated to adding a service badge. Can we back those out too. If you want to propose those changes, lets move that to another PR please.

Lets also get those issues sorted then we can dive into the code review properly.

[calebcartwright]

I haven't had a chance to look at this in detail yet either, but share the same concerns. For some reason based on the messages I saw in Discord I thought this would just need a json/txt type of file via the GH APIs, with some preceding work to identify the correct run/build.

I'd have significant concerns about downloading archive files and extracting them

[chris48s]

Since posting last night, I've done a little bit more thinking/reading about this and there's a couple of other points to throw in:

1. In its current form, this badge makes 3 HTTP requests (2 JSON and 1 zip), so beyond the performance concerns associated with extracting the data from a zip, there is also that additional overhead. Usually we try to avoid adding integrations that make more than a single request.
2. Github only keeps action artefacts for 90 days, so badges on projects that don't build frequently will expire after a while. This isn't necessarily a showstopper on its own (I think this is probably true of some other badges we host. For example I think some of the CI badges start erroring if your project has not had a recent build), but the shelf-life is definitely something to consider.

[fabiosangregorio]

Thank you for taking a look at this PR. I know it's borderline but I hope we can sort things out.

Following your concerns:

this badge makes 3 HTTP requests (2 JSON and 1 zip), so [...] there is also that additional overhead

Sadly Github Actions API are supported only using the JSON API.. If GraphQL were supported we could skip the second JSON request and directly download the zip file.

I also thought of inserting the percentage directly in the artifact name so we could avoid downloading it, but it feels really hacky.

Is there any restriction on the maximum size of a GH artifact? Could they be arbitrarily large? Beyond our existing blanket restriction on max response size, do you think there are any ways we can ensure we place some restrictions on the artifacts we'll accept to ensure sensible performance?

I think they can be arbitrarily large, but the second JSON response contains the size of the artifact we're about to download, so we could manually limit the size of it (since it's going to contain only a 0-100 number, we could limit this to a small amount of bytes)

What sort of execution times are you seeing with small examples?

Between 3-6 seconds of execution time. This is actually the reason some tests are currently failing.
Again, sadly, it seems like these precise JSON APIs I'm using do not support pagination, so I cannot request the latest build run only, but I'm bound to fetch the latest 20 or so.
This should not scale with project size though, it should remain constant.

Do we need to be mindful of any kind of cleanup of disk space after extracting the zip?

I'm not saving files to disk, I'm using a Buffer, so there should be no problems regarding this.

Is size the only thing we need to worry about i.e: is the overhead of extracting a zip broadly linear with the size, or is it possible to have a "small but complex" zip?

I'm not sure about this, I'll take a deeper look at it.

Regarding the basic things to sort out, I'll update the PR as soon as possible.

Surfacing an artifact through the CI build and reading from it was something we discussed about on Discord, and to me it seems the most sensible/secure way to achieve a dynamic badge without a real external service. What do you think about this?

[chris48s]

Between 3-6 seconds of execution time. This is actually the reason some tests are currently failing

So this in itself is a big issue. The most common place where our badges are viewed is GitHub. GitHub proxies images via its own cache and there's a hard limit of 4 seconds it will wait for a response. If the cache calls our endpoint and it takes more than 4 seconds, GitHub won't render the image, even if it returns a valid image after 4.1 seconds so if the best-case scenario is 3 seconds this badge would probably mostly render fail to render in one of the places its most likely to be viewed.
While its definitely the case that we spend most of our render time waiting on network I/O, our production servers are processing thousands of requests per minute, so it will be a little slower in production anyway.
We would definitely need to get this execution time down for this to be workable.

I think they can be arbitrarily large, but the second JSON response contains the size of the artifact we're about to download, so we could manually limit the size of it (since it's going to contain only a 0-100 number, we could limit this to a small amount of bytes)

If GraphQL were supported we could skip the second JSON request and directly download the zip file.

I think it would be fine to remove the second call and infer the zip URL from the first response
i.e: If I call
https://api.github.com/repos/fabiosangregorio/telereddit/actions/workflows/docs.yml/runs
I don't actually need to make a call
https://api.github.com/repos/fabiosangregorio/telereddit/actions/runs/177402254/artifacts
to work out that the direct link to the artifact is
https://api.github.com/repos/fabiosangregorio/telereddit/actions/runs/177402254/artifacts/zip
Its not "hacky" - its a supported endpoint: https://developer.github.com/v3/actions/artifacts/#download-an-artifact
We would lose knowing the size up-front though, which is useful.
On that first call, we can actually make that return faster by calling
https://api.github.com/repos/fabiosangregorio/telereddit/actions/workflows/docs.yml/runs?per_page=1
given we only need the first record. We'd probably have to do something more sophisticated to restrict the size of the zip though :/

The other option would be to call https://api.github.com/repos/fabiosangregorio/telereddit/actions/artifacts That does give us the size up-front. Then we could key off the artifact name instead of the workflow name. That would get us down to 2 API calls and we'd know the size up-front (allowing us to 'fail fast' easily if we think the zip is too big). We don't get to do the per_page=1 trick because the artifact we want isn't necessarily the first if there are multiple workflows, but we could probably reduce the page size from the default. What do you think of keying off artifact name instead of workflow? I think this would be the easiest way.

As you say, it would be really nice if this was available via the v4 API.

Regarding the basic things to sort out, I'll update the PR as soon as possible.

Lets make sure the core idea is sound first. I don't want to give you a bunch of "busy work" if there just isn't a way forward here.

Surfacing an artifact through the CI build and reading from it was something we discussed about on Discord, and to me it seems the most sensible/secure way to achieve a dynamic badge without a real external service. What do you think about this?

I do think this could be a useful pattern which is why I'm trying to suggest ways we can find a solution rather than just saying "no", but we do need to find a performant way to do it if we're going to merge this.

[fabiosangregorio]

Thank you again for the celerity and for the research. Sorry for the late replies, but I'm on holiday :) 😎

On that first call, we can actually make that return faster by calling https://api.github.com/repos/fabiosangregorio/telereddit/actions/workflows/docs.yml/runs?per_page=1 given we only need the first record.

This is not entirely true. We only want to fetch the runs for a specific branch that were successful. From the docs this can be achieved using Parameters such as branch=master&status=success. However, I'm not able to use these parameters in combination with per_page parameter. (e.g. https://api.github.com/repos/fabiosangregorio/telereddit/actions/workflows/docs.yml/runs?per_page=1 returns one result while https://api.github.com/repos/fabiosangregorio/telereddit/actions/workflows/docs.yml/runs?branch=master&status=success&per_page=1 returns 20 results), although I'm not sure why. As I said, I'm fairly new to Github APIs, so I might be using it wrong (?).

We'd probably have to do something more sophisticated to restrict the size of the zip though :/

I think the response headers for the download request would contain the size of the file.

What do you think of keying off artifact name instead of workflow? I think this would be the easiest way.

This would mean we could have the user name the artifact docstrcoverage-<branch>.zip and download the artifact based on that, it sure looks like the easiest way. The only thing we have to keep into account is that if a repo has a lot of artifacts we might not find it in the response, so we'd have to search through the previous pages.

Its not "hacky" - its a supported endpoint

I think I wasn't clear on this. I meant that I also thought of inserting the percentage directly in the artifact name, so we wouldn't have to download it in order to get the percentage, something like docstrcoverage-<branch>-<percentage>.zip. But this feels hacky, because on the CI side the user would have to somehow name the artifact based on the output of the docstr-coverage package, possibly through a shady-looking bash script. But this would mean we'd have only ONE JSON request.
This would also make the service less generic.

I think leveraging the name will make the performance acceptable. If you agree, I'll go ahead and make the modifications when I get back.

[paulmelnikow]

Why is the artifact zipped?

[fabiosangregorio]

@paulmelnikow from the docs:

Gets a redirect URL to download an archive for a repository. This URL expires after 1 minute. Look for Location: in the response header to find the URL for the download. The :archive_format must be zip

[chris48s]

I think I wasn't clear on this. I meant that I also thought of inserting the percentage directly in the artifact name, so we wouldn't have to download it in order to get the percentage, something like docstrcoverage--.zip. But this feels hacky, because on the CI side the user would have to somehow name the artifact based on the output of the docstr-coverage package, possibly through a shady-looking bash script. But this would mean we'd have only ONE JSON request.
This would also make the service less generic.

I see what you mean now. Yes - agreed. As far as possible, the procedure should be that the user dumps the output of a thing to an artifact (in this case the output of docstr-coverage but in principle any build/coverage/docs tool that outputs a file) in the most obvious/default way possible and we parse it.

I think leveraging the name will make the performance acceptable. If you agree, I'll go ahead and make the modifications when I get back.

Yeah. Lets have a go at:

• The badge takes artifact name as a param
• We call /actions/artifacts, grab a record based on artifact name and check the size
• If the reported size is over some very small threshold, throw an error (this is probably something we need to work out, but for the moment lets say 50Kb as a starting point). We routinely download JSON responses much larger than this.
• If the reported size is under that, grab the zip and process it

..and see what the performance is like. My instinct is that should be in a sensible ballpark but see how it goes.

[calebcartwright]

There's clearly been a lot of great discussion in this thread, but I confess I'm having trouble following the current/latest plan, particularly as it relates to retrieving the information from GitHub.

Here's what I think I've gathered (please let me know if I'm wrong).

1. Call the List Repo Artifacts
2. Use the provided artifact name to locate the record for the target artifact in the response from step 1
3. Follow the steps Chris outlined above to then grab the archive (check size, error if too big, otherwise use the Download Artifact endpoint to grab the zip
4. Extract the archive from step 3
5. Parse the file from extracted archive in step 4
6. build and return badge

If I do indeed have that correct, then one question I'd have is whether we'd still be able to support branch filtering so that users could have a coverage badge based on their default branch.

[chris48s]

Gah. That's a good point that I hadn't thought of - actions/artifacts doesn't give us the branch :(

[fabiosangregorio]

If I'm correct, we agreed to have the branch name in the artifact name, in a format like docstrcoverage-<branch>.zip, so that we can still filter artifacts based on the name. This will take care of branch filtering.

[calebcartwright]

If I'm correct, we agreed to have the branch name in the artifact name, in a format like docstrcoverage-<branch>.zip, so that we can still filter artifacts based on the name. This will take care of branch filtering.

I suppose that could functionally get the job done, but I do have some concerns about a solution that's not really deterministic and necessitates education and communication outside of the Shields ecosystem. We can try to document that requirement for being able to use this badge as best we can (in the modal badge builder, somewhere within our repo, etc.) but a large portion of our user base will miss those and I suspect we'd end up fielding a lot of "why isn't this working right" questions, especially if this pattern was extended to support other report-related information published from a Workflow/Job run.

I feel like one of the biggest gaps still in place today with GitHub Actions is the absence of basic test results/code coverage reporting that most other CI/pipeline platforms seem to support these days. Perhaps in parallel it'd be worth seeing if GitHub have any plans to support this more natively.

[chris48s]

I'm not massively keen on "this works, but you have to adopt a specific naming convention" either.

I feel like one of the biggest gaps still in place today with GitHub Actions is the absence of basic test results/code coverage reporting that most other CI/pipeline platforms seem to support these days

Hmm. I'm not convinced this would be any easier with Travis/CircleCI/etc. They're all fairly reliant on reporting to external services like coveralls/codecov/etc. I think we could arrive at a reasonable solution here, but the REST endpoints that exist just spread what we need across a few different API calls.

Just for the lolz, I opened a thread on the GH community forum asking about support for action artifacts on the the Graphql API https://github.community/t/artifacts-with-the-v4-graphql-api/124640/2 as that would probably allow us to solve that.

[calebcartwright]

Hmm. I'm not convinced this would be any easier with Travis/CircleCI/etc. They're all fairly reliant on reporting to external services like coveralls/codecov/etc.

Travis is fairly in the same boat, though Circle provides native support for test results as does AppVeyor

https://circleci.com/docs/2.0/collect-test-data/#api
https://www.appveyor.com/docs/running-tests/#uploading-xml-test-results

Other platforms we integrate with like Jenkins and Azure DevOps make both test results and coverage natively available, and GitLab Pipelines make the data available as well. I'm not saying that GHA is alone in not really having support here, just that they're definitely on the lower end of the spectrum on feature/support for test artifacts.

[PyvesB]

I must admit that I've not followed the discussions here. My understanding is that the badge would require multiple API calls which would take too long to render, and cutting down those API calls would force us to infer information such as branch filtering using error-prone conventions. Do we see an acceptable way forward on this one or should we drop it?

[fabiosangregorio]

@PyvesB you are correct. Unless GitHub introduces support for GraphQL APIs on Github Actions (no evidence of it so far) the render time of this badge would be untenable.

Feel free to decide whether to leave this PR open in hope of this feature becoming available, or to close it.

[PyvesB]

Thanks for confirming @fabiosangregorio ! I'll go ahead and close it, it can always be reopened with a single button click if/when the time comes. Regardless, thanks for your interest in the Shields.io project. 😉