-
Notifications
You must be signed in to change notification settings - Fork 663
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
Cleanup of CI actions & cron job #3490
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3490 +/- ##
===========================================
- Coverage 93.86% 93.85% -0.01%
===========================================
Files 176 176
Lines 23208 23208
Branches 3300 3300
===========================================
- Hits 21784 21782 -2
- Misses 1372 1373 +1
- Partials 52 53 +1
Continue to review full report at Codecov.
|
@MDAnalysis/coredevs should be finished with this one. It's a pretty big overhaul of CI - but for the most part the "big changes" are just moving things over to composite actions to make things easier to re-use. All that's left to do is amend the trigger on gh-ci-cron.yaml to just be a cron job (kept as it currently is so we can make sure CI actually works). |
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 gave it a cursory glance and I like the modularization.
If no-one else with sufficient GitHub-foo has time to review, I'd say let's try it...
Thanks @orbeckst, I was hoping maybe @fiona-naughton or @lilyminium might also want to have a look through. I think we might want to publish (in the near future) some of these composite actions to streamline downstream develop builds in MDAKits. |
More eyes would be good, especially with a view towards using it for the MDAkit cookie cutter.
When I had opened the PR in the browser I hadn’t seen your comment and I then came back to the tab at the end of the day.
… Am 1/5/22 um 17:22 schrieb Irfan Alibay ***@***.***>:
If no-one else with sufficient GitHub-foo has time to review, I'd say let's try it...
Thanks @orbeckst, I was hoping maybe @fiona-naughton or @lilyminium might also want to have a look through. I think we might want to publish some of these actions to streamline downstream develop builds in MDAKits.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you were mentioned.
|
Modularising GH actions makes the main CI file much tidier, thank you. The default definitions of dependencies is long but allows for much easier mixing and matching. I did just make a PR making the dependency environment variables more list-like and therefore easier to diff; they're also printed in the log as part of the env.
That also sounds like a good idea. The cookiecutter probably won't need all of them, and this makes it way easier to split out. (One model that many projects follow successfully is installing test dependencies from an Actually, a GH Action installing |
.github/workflows/gh-ci-cron.yaml
Outdated
|
||
|
||
# Issue 3216 | ||
old-chemfiles: |
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'm not sure we should be maintaining support for older chemfiles versions. The package is pre 1.0 and will continually be unstable, so we're signing up for a lot of work.
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.
Currently our code paths enable support for both pre and post chemfiles 0.10. I agree that we probably shouldn't waste time trying to support old pre-1.0 here, in which case we should just drop support for it (in a separate PR).
I've raised a separate PR. Let's keep this in for now and remove it in the follow-up 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.
LGTM, might be worth cherry-picking @lilyminium 's patch onto this to avoid a merge snafu
* reorganize dependencies
Just going to let @fiona-naughton review this before switching things to cron-only and merging. |
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.
sorry for the delay - lgtm!
Fixes #3216 #1727
Towards #3052 #3442
Changes made in this Pull Request:
To do:
PR Checklist