Add check_coverage script
#839
Conversation
addie9800
left a comment
addie9800
left a comment
There was a problem hiding this comment.
This is a really useful addition; it will save us a lot of headaches when addressing layout changes!
| return None | ||
|
|
||
| # Use cached file if it exists and caching is enabled | ||
| if use_cache and os.path.exists(cache_path): |
There was a problem hiding this comment.
I wonder if we should display a message indicating whether the cached files are being used.
There was a problem hiding this comment.
I can add something, but it should be easy to notice anyways in terms of execution time. I also think the usual workflow would rely on using the cached files.
| if a.name == __ARTIFACT_NAME__: | ||
| zip_url = a.archive_download_url | ||
| r = requests.get(zip_url, headers={"Authorization": f"token {__TOKEN__}"}) | ||
| if r.status_code != 200: |
There was a problem hiding this comment.
I would also add some user feedback in this case to make debugging easier
scripts/check_coverage.py
Outdated
| __REPO__ = "flairNLP/fundus" | ||
| __WORKFLOW_NAME__ = "Publisher Coverage" | ||
| __ARTIFACT_NAME__ = "Publisher Coverage" | ||
| __TOKEN__ = os.getenv("GITHUB_TOKEN") # needs to be a fine-grained token |
There was a problem hiding this comment.
I have multiple questions here. A fine-grained token would be a personal access token linked to a specific personal account. Since it can be defined to be only valid for a single repo (or so I thought, I couldn't choose fundus explicitly though), that might not be too much of an issue, but did you also look into the option of a GitHub App Access token? Also, if we stick to the version with the fine-grained token, I would also add the necessary permissions this token must have in order to keep the setup as simple as possible.
There was a problem hiding this comment.
did you also look into the option of a GitHub App Access token?
No, but I also wouldn't know what that is or differs that from the rest. Maybe you can take a look into this?
Also, if we stick to the version with the fine-grained token, I would also add the necessary permissions this token must have in order to keep the setup as simple as possible.
I will add something about the permission, but as far as i understand, we can only download artifacts from GitHub using a fine grained token, nut it doesn't need to have any permissions.
There was a problem hiding this comment.
nut it doesn't need to have any permissions
Ah yes, my bad. It seems like I misunderstood the docs there.
Maybe you can take a look into this?
It seems like there is a bit of an overhead in setting this up, but from what I understand GitHub Apps cover the functionality we would ideally like to have, by creating an option for authentication that is not linked to a specific user and can be tied to a specific repository. By default, only the repo owner can install a GitHub App, though, so if we want to try it, we could get the permissions for that. I would be happy to look into it if you think we should go ahead. The alternative would be for one of us to create a token that gives read access to all public repositories we are a part of, which I guess is not that bad, but probably something that should be avoided if possible.
| pbar.update() | ||
|
|
||
| if not txt: | ||
| continue |
There was a problem hiding this comment.
Maybe we should add a warning here
There was a problem hiding this comment.
Added some messages and a verbosity flag
| continue | ||
|
|
||
| if not (parsed := parse_coverage_file(txt)): | ||
| continue |
Add a script to parse the latest time a publisher succeeded from the
Publisher Coverageaction:Example Output: