-
Notifications
You must be signed in to change notification settings - Fork 134
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
Add Code Coverage Capability #313
Conversation
Supersedes #264. |
This is ready to merge if review is approved. Might be nice to try to merge before weekend testing but not required. I will then test the codecov capability off master as well. We may still need another PR to fix any issue that arises, but we won't know for sure until it's on master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ready to go.
setenv CODECOV_TOKEN "df12b574-8dce-439d-8d3b-ed7428d7598a" # consortium icepack | ||
#setenv CODECOV_TOKEN "0b3feb13-0110-4618-821e-248c2f1f7cf3" # apcraig icepack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not have time to review before, but I think maybe these should have been kept secret ? i.e. using a file https://docs.codecov.io/docs/about-the-codecov-bash-uploader#section-file
but now that it's already merged it's out there...
That token allows upload access to these repos on codecov...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
codecov does not make any mention that these should be secret. I recognize it's probably not ideal to put these in the public domain. With a file, are you suggesting that file NOT be in the repository? That means multiple people would need a copy of that file on multiple platforms. If the file is in the repository, isn't it exposed anyway? How did we do this with travis?
If it's clear we can better hide this token, I think it's worth doing. Even if it's out there already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I meant a file not in the repo, like a .codecov-token
in the $HOME
of the user running the test suites.
For Travis this is what encrypted variables are for : https://docs.travis-ci.com/user/encryption-keys/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we would use an environment variable in the script, without defining it, and users uploading the coverage report would just have to do
export CODECOV_TOKEN=$(cat $HOME/.codecov-token)
before running the script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What vulnerabilities do you think we have with the token being exposed? Is it just the codecov site or is our github repo at risk too? We can easily delete the current code cov sites and create new ones. We lose whatever data is there, but it's easy to regenerate. Hopefully if we create new codecov web sites, we get new tokens.
If this is a concern, It's disappointing that the codecov site doesn't address it in some way, even if just as a warning in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "vulnerability" would be that anybody can upload coverage reports to these codecov repos. Our GitHub repo is not at risk.
But as I said, I'm not 100% sure - but as I understand the from the codecov Bash uploader page, it's the only authentication used so I think with this token you can upload to the codecov projects.
Also, rereading the page, https://docs.codecov.io/docs/about-the-codecov-bash-uploader, it appears the token is not required for Travis CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, good to know. I'm not convinced we should be too worried. It would be unfortunate if someone came in and trashed our codecov site with bogus reports, but I'm not sure why anyone would. Also, we cannot do the codecov testing with travis, we don't have enough resources to run full test suites on travis so that's why we need the token.
I think we have a few options.
- shift the token to an unreadable file (or other hidden approach) that we all will need to create manually on all platforms where we codecov test.
- consider removing current codecov web sites and generating new sites and tokens before we do that. (NOTE: I just tested that and we do NOT get a new token if we do that, so it's not an option).
- remove the codecov documentation and from our user guides. in many ways, that information is there for internal folks and not the community. we could try to hide the feature as much as possible. But the token might still be exposed in the report_codecov.csh script if we don't address hiding it.
- we could try to undo the codecov PRs before we hide the tokens. can we undo the PRs so they are no longer visible in the repository at all? I guess that would require a git push --force and we'd lose history and anyone with a recent clone might run into problems. Given this was just pushed a couple days ago, it's probably OK to do if we do it soon?
- proceed but recognize we might eventually run into a problem with the exposed token. I think the risk is low both with respect to chance and cost.
I guess I propose the following.
- We update the scripts so we hide the token in a file as suggested by @phil-blain. I propose that the "cat $HOME/.codecov-token" be part of our scripts and that the .codecov-token file be set to unreadable except to user. I prefer it to be in the script versus having to remember to manually set the CODECOV_TOKEN env variable before running the script. It's easy to forget to do that.
- We just create a new PR for both Icepack and CICE that hides the token, and we leave the history alone. The token is exposed in the current hash, but will not be moving forward and the risk is low anyway.
What do others think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to remove the documentation. It's important for us to know how it works (bus factor, etc.)
I agree the risk is low, the easiest way would be to create new PRs hiding the tokens.
If we undo the PRs (git reset --hard
, git push --force
) you are right that recent clones will need special handling (as well as any old clone that pulled the latests changes). As to whether that would completely hide the tokens : I don't think so, because this PR will stay here and the tokens are present in these commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the implementation in CICE and Icepack in #314 and CICE-Consortium/CICE#436 to hide the token. The implementation allows the user to set the token manually as an env variable, CODECOV_TOKEN. It will also look for ~/.codecov_cice_token and ~/.codecov_icepack_token and "source" that file if it exists and the env varaible is not set already. CICE and Icepack have different token ids so we need two files.
PR checklist
Add code coverage capability
apcraig
Full test suite on gordon successful.
https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#5b68a9cf3082923dd006e26102c0d564b2b602ac
For sample output, see https://codecov.io/gh/apcraig/Icepack
Will need to verify it works from the Consortium master and saves to https://codecov.io/gh/CICE-Consortium/Icepack after merged.