-
Notifications
You must be signed in to change notification settings - Fork 5
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
Revert test matrix back to manual #111
Conversation
Annoyingly, I had to add a line allowing coverage failures due to the current issues -- perhaps this can be taken out if/when those get resolved. |
@IAlibay this is ready for review if you have a chance to have a look! I verified that tests failed before pushing the fix, and tests seem to be passing now that the coverage step is allowed to fail. Apologies it took longer than expected. |
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.
Apologies for the briefness, sending this on the go from a phone.
A few initial review comments / questions.
@@ -120,6 +130,10 @@ jobs: | |||
if: github.repository == '{{ cookiecutter.github_host_account }}/{{ cookiecutter.repo_name }}' && github.event_name != 'schedule' | |||
uses: codecov/codecov-action@v3 | |||
with: | |||
# codecov can be flaky and result in spurious failures |
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.
tbd - api token entry
The main failure here is a lack of API token, not having it often cause failures even when not on forks. How do we best ensure that folks do this?
{{cookiecutter.repo_name}}/.github/workflows/{{cookiecutter._ci_name}}.yaml
Outdated
Show resolved
Hide resolved
{{cookiecutter.repo_name}}/.github/workflows/{{cookiecutter._ci_name}}.yaml
Outdated
Show resolved
Hide resolved
{{cookiecutter.repo_name}}/.github/workflows/{{cookiecutter._ci_name}}.yaml
Show resolved
Hide resolved
{{cookiecutter.repo_name}}/.github/workflows/{{cookiecutter._ci_name}}.yaml
Show resolved
Hide resolved
Ok, I've set up a CODECOV token for the https://github.com/MDAnalysis/mdakit-cookie repository and upgrading the Edit: nevermind now CI isn't running for some reason. -_- |
Unless I'm missing something, the codecov upload action may be forever doomed to failure as it's not a valid GitHub run. The CI now gets around it by on-the-fly editing the CI to accept a failed codecov action. |
This reverts commit 2601768.
This reverts commit 51bdcff.
* tmp let CI run without codecov * check directory exists * make package and repo name markedly different * tmp add assert False * fix env value? * fix path? * fix var name * add more listing * even more listing * am doofus * rm some temporary changes * add list back in * tmp try fixes to check ci * tmp simplify matrix * allow os * allow mdanalysis versions * use manual python matrix * Revert "tmp try fixes to check ci" This reverts commit 613debe. * Revert "Revert "tmp try fixes to check ci"" This reverts commit 5133e40. * Revert "Revert "Revert "tmp try fixes to check ci""" This reverts commit 38cf157.
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.
Looks good, just a couple of really quick things - thanks so much for all your work on this @lilyminium !
I'm going to have a brief look at what's been happening in codecov land re: tokens, if there's anything interesting I'll add a separate review, if not then the current solution is imho as good as it'll get.
@@ -123,11 +113,22 @@ jobs: | |||
file: coverage.xml | |||
name: codecov-{{ '${{ matrix.os }}' }}-py{{ '${{ matrix.python-version }}' }} | |||
verbose: True | |||
# to upload coverage reports, set a secret called CODECOV_TOKEN |
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.
Do we have a set of setup instruction docs somewhere where we can put this? (not in this PR but let's raise an issue to get that done at some point :) )
# to upload coverage reports, set a secret called CODECOV_TOKEN | ||
# in the repository settings | ||
# (Obtain this from the Codecov website after setting up the repository there) | ||
token: {{ '${{ secrets.CODECOV_TOKEN }}' }} |
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 there's a thing with codecov v4 about this - not sure if we should/want to bump up the version (it might make things break even more!)
fail_ci_if_error: true | ||
# use OpenID Connect (OIDC) if you have this enabled -- this will ignore | ||
# the `token` argument above | ||
use_oidc: true |
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.
Are the cookiecutter tests currently failing because this is set to true?
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.
Follow up: should this be true by default?
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.
Probably related; TIL use_oidc
is v4 only.
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.
They're currently failing because of the issues solved in #115! I did test that the fix in #115 resolves this. The codecov action currently gets edited on the fly so failure is ignored.
Probably related; TIL use_oidc is v4 only.
Interesting, the logs seemed to indicate that codecov was picking up the OIDC token and using it to connect.
Follow up: should this be true by default?
I would assume that OIDC is a better means of validation than the plain CODECOV token, if users can do so -- that being said I'm not quite sure how it well it works for forks. I gave it a try just in case it magically helped the upload. Happy to remove and look at it more in detail later for a simpler PR.
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.
If you can be bothered, there's a few action versions that could be bumped up here (upload, download, and checkout can all go to v4, setup-miniconda I think would be good to bump to v3 [I know it doesn't play nice with macos-latest osx-arm64 runners])
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.
Something I had to do with the broken kits was bump up the conda-incubator/setup-miniconda version.
I found a lot of times that v2 would fail with osx-arm64 for some reason or another.
@lilyminium - just had a brief look at the codecov action history, this came up: codecov/codecov-action#1404 I've not had time to test it out (or read up on all the history related to it 😅), but it looks like the latest version of v4 might fix all the problems? Is it worth trying here? |
dc547e2
to
e879faf
Compare
Upgrading to codecov v4 kicked off a whole bunch of things (needing to upgrade act, leading to a bunch of issues where the most recent is libmamba failing to install MDAnalysis into the image due to lack of disk space). I think I might do that in a separate PR so we can firstly get Lawson's fix in and not overly complicate this one. |
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.
Sounds good, probably lots of smaller PRs for each bump is better anyways, sorry for the wild goose chase!
Thanks @IAlibay for all the work you put into reviewing! |
Relates to #110
Changes made in this Pull Request:
PR Checklist