Refactor [Codecov]

[calebcartwright]

This is ready for review now

Couple notes (more detailed versions below):

• Token (for private repos) moved from route param to query param via new redirector functionality
• The coverage value is now rounded to the nearest whole number to be consistent with our other coverage badges (the legacy service always rounded down before, as does the native Codecov badge)

Still a work in progress, but opening this as a draft PR to start a dialog on a few items

• the token parameter
  The legacy service supported coverage badges for private repos via a user-supplied token in a route param. Typical/public repo pattern was codecov/c/:vcs/:user/:repo, for private repos the user adds /token/:token before the vcs param: codecov/c/token/:token/:vcs/:user/:repo. I was thinking it would be easier if the token was supplied via a query param (codecov/c/:vcs/:user/:repo?token=) vs. trying to keep the optional token in the route and having to useformat/capture. I figured I could then create a redirect service for the token route to maintain backwards compatibility. Thoughts?
• codecov api
  The legacy service was using Codecov's native badge (https://codecov.io/gh/codecov/example-python/graphs/badge.svg) with plain text (https://codecov.io/gh/codecov/example-python/graphs/badge.txt). Codecov has a documented REST API for the data we need (https://docs.codecov.io/reference#section-get-a-single-repository) that returns a friendly JSON object, so I've switched to that. Any concerns there?
• coverage rounding
  The Codecov UI displays the exact coverage value, but their badges display coverage rounded down to the integer value (90.98% coverage shows as 90%), and that integer value is what our legacy service implementation saw and rendered. The Codecov API returns the precise coverage value (i.e. the 90.98%) so we've now got the option to either continue the Math.floor approach and be consistent with the native Codecov badges, or we could apply the same rounding that we do on other Shields coverage badges (90.98% would show as 91%) I went with the latter in the refactored service, but let me know if anyone thinks we should switch back to rounding down

Note: I've intentionally left the legacy service content in place (renamed) while this PR is in draft mode for easier side-by-side comparisons, but of course will remove it

[shields-ci]

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

Generated by 🚫 dangerJS against 7e35cf7

[calebcartwright]

I'm having a little difficulty with the redirect around inserting the token query param after the extension.

I.e. we want to redirect the old path:
/codecov/c/token/abc123def456/gh/codecov/private-example.svg

to:
/codecov/c/gh/codecov/private-example.svg?token=abc123def456

but right now my redirect is currently being sent to:
/codecov/c/gh/codecov/private-example?token=abc123def456.svg

Is there a way in the redirect service definition target to specify new/additional query params? I'm not seeing a way to do that in the current redirector definition, so I'm going to look into tweaking that.

If anyone knows of a way to specify new/additional query params on the target please let me know!

[paulmelnikow]

It definitely doesn't do that yet! 😁

I'd probably suggest adding a second function like targetQueryParams. Or maybe target should be renamed to transformPath, and transformQueryParams added.

I have a couple changes to redirector.js in flight right now for some work I'm doing related to #2068. Let me get to a pausing point and put a small bit of that into PR.

[calebcartwright]

I'd probably suggest adding a second function like targetQueryParams. Or maybe target should be renamed to transformPath, and transformQueryParams added.

That's exactly what I was thinking too! I had started with renaming target to targetUrl and added addTargetQueryParams but I like what you've described here much better. There's nothing urgent about the Codecov refactor here whatsoever so happy to hold off on this one.

Let me know if I can be of any help with the redirector stuff, otherwise I'll just keep an eye out and come back to this one whenever we're ready 👍

[paulmelnikow]

So… that PR got big (#3093). 😝

If you want I could pick out the redirector validation bits, which probably could be reviewed separately, much quicker.

[calebcartwright]

If you want I could pick out the redirector validation bits, which probably could be reviewed separately, much quicker

I'd say don't worry about pulling that out. Happy to wait til #3093 gets merged

[calebcartwright]

I'm going to take a stab at updating the redirector function to support transforming query params (assuming you aren't already working on it/finished it @paulmelnikow)

[paulmelnikow]

Nice one!

[paulmelnikow]

@calebcartwright Let me know if you want anything else from me before merging!

[calebcartwright]

@paulmelnikow sorry I should've left a comment. I want to spend a bit more time challenging the schema in relation to the API response as I thought of a couple other scenarios. Will also add a couple of the code comments we've discussed in this thread

Will give you a shout when I'm done

[calebcartwright]

Alrighty, I created some sample apps and ran through some various scenarios and concluded the schema is best as-is 👍

@paulmelnikow - I'm all set here now

[shields-deployment]

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch: