SCT-3141 Add support for config repo GitHub release workflow #408
Conversation
dauglyon
changed the title
|
The changes to, and the presence of, |
|
The first thing I notice is that there's no coverage information... |
Co-authored-by: MrCreosote <[email protected]>
Co-authored-by: MrCreosote <[email protected]>
| raise ValueError(f'No release with tag "{tag}" found in validator config repo "{repo_path}"') | ||
| else: | ||
| # Use the latest release | ||
| target_release = releases[0] |
There was a problem hiding this comment.
I don't seem to see a test for the no provided tag case
There was a problem hiding this comment.
test_config_get_validators_github_bad_tag covers a bad tag. Multiple tests don't provide an explicit tag
There was a problem hiding this comment.
Hmm, I'm not seeing those. I only see one happy path test and it uses a tag
There was a problem hiding this comment.
bad tag: https://github.com/kbase/sample_service/pull/408/files#diff-daeaaa8b25848f3758165ca3a64ab5d0a9d57ace8942f4d66c6894665153da56R190
provides no tag and finds no tagged release:
https://github.com/kbase/sample_service/pull/408/files#diff-daeaaa8b25848f3758165ca3a64ab5d0a9d57ace8942f4d66c6894665153da56R218
| target_release = releases[0] | ||
|
|
||
| assets = target_release.get_assets() | ||
| if not assets: |
There was a problem hiding this comment.
There doesn't seem to be a test for this case
There was a problem hiding this comment.
If you mean the no-assets case. It's not currently replicable with GH, but the API spec doesnt specifically disallow an empty array here so, this is a "just in case" error that would require mocking GH reponses to test, which i think would be wild overkill
There was a problem hiding this comment.
If it's practically impossible for this case to occur, I would just take code path out and add a comment that currently google will always have > 0 assets. If things change it'll throw a TypeError which I think is fine
| raise ValueError( | ||
| f'Failed to open validator configuration file at {url}: {str(e.reason)}') from e | ||
| if config_asset: | ||
| raise ValueError(f'Error downloading config asset from repo "{repo_path}" '+ |
There was a problem hiding this comment.
There doesn't seem to be a test for this case
There was a problem hiding this comment.
Again, its just differentiated error text for an already tested error
There was a problem hiding this comment.
Same comment as below re test writing
| f'Failed to open validator configuration file at {url}: {str(e)}') from e | ||
| if config_asset: | ||
| raise ValueError( | ||
| f'Failed to open validator configuration file from repo "{repo_path}" '+ |
There was a problem hiding this comment.
There doesn't seem to be a test for this case
There was a problem hiding this comment.
Mentioned I think in a previous comment, I'm not of the opinion one is needed, as this is the same (already tested) bad-yaml case with different err text. Covered by test_config_get_validators_fail_bad_yaml
There was a problem hiding this comment.
I guess I don't understand why you wouldn't add a test? It's an uncovered code path and so should get a test for that code path IMO. The tests look pretty simple to write so naively it seems like a 5 minutes thing. I would certainly write one if it was my PR
There was a problem hiding this comment.
Mostly because it's a weird amount of overhead to use a third party service in tests to test a code path that is only differentiated by earlier (tested) behavior. It's also not a functionally different code path, it just changes what string is thrown...
Co-authored-by: MrCreosote <[email protected]>
Co-authored-by: MrCreosote <[email protected]>
…_service into config_repo_release_support
Adds support for downloading the validation config from a github release. Addresses step two of the Implementation Plan for Sample Service Validator Workflow. This PR uses the github API to grab the assets. This is because the rate-limit (it seems only for some IPs) can be ~10 downloads per hour (instructions to check your own rate limit). Unfortunately this seems undocumented, but I have hit it a few times. This goes up to the documented 1000 requets per hour per repo when using the API with a token.