Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade Codecov upload method #68

Merged
merged 9 commits into from
Oct 28, 2021

Conversation

ConorMacBride
Copy link
Member

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
Copy link
Member

Cadair commented Oct 5, 2021

mmmm, tasty binary download and execution. 😛

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.

@ConorMacBride ConorMacBride force-pushed the upgrade-codecov branch 3 times, most recently from 1905bd1 to 47e8d8c Compare October 5, 2021 16:17
@Cadair
Copy link
Member

Cadair commented Oct 6, 2021

@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
Copy link
Member Author

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.

Signed-off-by: Conor MacBride <[email protected]>

Use Powershell for Windows

Signed-off-by: Conor MacBride <[email protected]>

Update CODECOV_TOKEN variable

Signed-off-by: Conor MacBride <[email protected]>

Access CODECOV_TOKEN as env var

Signed-off-by: Conor MacBride <[email protected]>

Remove check for no token

Signed-off-by: Conor MacBride <[email protected]>

Add original token check template exp

Signed-off-by: Conor MacBride <[email protected]>
Signed-off-by: Conor MacBride <[email protected]>
@ConorMacBride
Copy link
Member Author

ConorMacBride commented Oct 14, 2021

I've added the integrity check for all OSes and it all appears to be working as expected on SunPy (sunpy/sunpy#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
Copy link
Member Author

@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.

run-tox-env.yml Outdated
${{ if ne(variables.CODECOV_TOKEN, '') }}:
env:
CODECOV_TOKEN: $(CODECOV_TOKEN)
- ${{ if or(eq(env_pair.key, 'macos'), eq(env_pair.key, 'linux')) }}:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage is only uploaded if eq(env_pair.key, 'linux'). As for linux env_pair.key can be either linux, manylinux or linux32. This means that reports currently won't be uploaded for Docker containers. I'll update the template.

Signed-off-by: Conor MacBride <[email protected]>
@Cadair
Copy link
Member

Cadair commented Oct 27, 2021

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

@ConorMacBride
Copy link
Member Author

Yes, ready to be merged!

run-tox-env.yml Show resolved Hide resolved
@Cadair
Copy link
Member

Cadair commented Oct 28, 2021

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 🤣

@Cadair Cadair merged commit 9d1e97f into OpenAstronomy:master Oct 28, 2021
@Cadair
Copy link
Member

Cadair commented Oct 28, 2021

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

@ConorMacBride ConorMacBride deleted the upgrade-codecov branch November 26, 2021 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants