OpenAstronomy / azure-pipelines-templates

An opinionated set of Azure Pipelines templates for Python projects using pyproject.toml (PEP517) and tox
https://openastronomy-azure-pipelines.readthedocs.io/en/latest/
BSD 2-Clause "Simplified" License
11 stars 20 forks source link

Upgrade Codecov upload method #68

Closed ConorMacBride closed 3 years ago

ConorMacBride commented 3 years ago

Codecov has deprecated the Bash uploader (https://docs.codecov.com/docs/about-the-codecov-bash-uploader). This PR upgrades the template's upload method to the new method recommended by Codecov (https://docs.codecov.com/docs/codecov-uploader).

I haven't implemented the script integrity check, however can look into that if you like. This would need to be tested by opening a PR in a project which uses this template (SunPy for example) with the template set to this PR in the azure-pipelines.yml. I've tested on my own code and it seems to work. https://dev.azure.com/ConorMacBride/mcalf/_build/results?buildId=146&view=results (However due to an unrelated issue, which prompted me to open this PR, the coverage report does not actually get uploaded to Codecov. Not sure what the issue is though.)

Cadair commented 3 years ago

mmmm, tasty binary download and execution. :stuck_out_tongue:

It certainly would be nice to implement the integrity checking, it would prevent projects which use this uploader getting bitten by a repeat of the codecov bash uploader security issue. It might be a bit of a pain to do on all three OSes though.

Cadair commented 3 years ago

@ConorMacBride when you are happy with this, if you want to open a PR to sunpy which swaps the branch of the templates we are using to this one, I will happily merge it and give it a test out.

ConorMacBride commented 3 years ago

Thanks @Cadair 👍 I'm still trying to understand how the CODECOV_TOKEN variable is set. I'm almost certain I'm doing it correctly but it's not recognised using the latest version of the template. I would prefer to see #17 resolved before this is merged in case I introduce any changes that are not backwards compatible. I'll also implement the integrity checking on as many OSes as feasible.

ConorMacBride commented 3 years ago

I've added the integrity check for all OSes and it all appears to be working as expected on SunPy (https://github.com/sunpy/sunpy/pull/5597) so this PR is probably ready to be reviewed and merged.

~However, it's still not uploading to Codecov for my package. The codecov script returns a 500 error. I notice that it detects a CODECOV_TOKEN env variable on the SunPy pipeline, so maybe SunPy can upload because that is provided. I can't work out how to add a CODECOV_TOKEN to my package pipeline. The current/latest template works with my package (it didn't for a while recently — other packages had the same issue), so this PR actually breaks that. Maybe it'll not work with all packages that don't provide the token, which is optional for public repos.~

~Example of it not uploading: https://dev.azure.com/ConorMacBride/mcalf/_build/results?buildId=161&view=logs&j=eef8727b-df72-5d14-3454-70c3d8886df3&t=0d6839a3-fdfb-5986-7b05-2ea40aaf956b~

ConorMacBride commented 3 years ago

@Cadair the issue in the previous comment seems to have resolved itself without any changes on my part. So this PR should be ready to review.

Cadair commented 3 years ago

@ConorMacBride Am I right in thinking this is good to go now?

ConorMacBride commented 3 years ago

Yes, ready to be merged!

Cadair commented 3 years ago

I added the -n flag to the codecov command so we get nice names for each of the builds on codecov, which should help with debugging:

image

I am now just using your branch to fix the CI :rofl:

Cadair commented 3 years ago

Thanks a lot @ConorMacBride This is a really nice improvement.