-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix docker image building + CI improvements #641
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #641 +/- ##
==========================================
- Coverage 92.21% 90.97% -1.24%
==========================================
Files 124 127 +3
Lines 8594 8679 +85
==========================================
- Hits 7925 7896 -29
- Misses 669 783 +114
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
If I understand https://docs.codecov.com/docs/flags correctly, using flags should help with codcov comparing coverage from when we run slow tests (in the past) to PRs |
@IAlibay (and anyone else reviewing this) This run tests the docker pipeline: https://github.com/OpenFreeEnergy/openfe/actions/runs/6897862437 So we want to make sure that passes before we merge this in. |
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.
overall lgtm, couple of comments
.github/workflows/ci.yaml
Outdated
|
||
- name: codecov-schedule | ||
if: ${{ github.repository == 'OpenFreeEnergy/openfe' | ||
&& github.event_name == 'schedule' }} |
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.
schedule is always a bit of a pain because codecov has that annoying thing where it fails if it gets the same commit too many times, would it make sense to limit this to merge instead?
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.
Ah, so its the merges! I couldn't figure out how (as the yaml existed) codcov got a bunch of the coverage for the slow tests since I noticed we didn't run the slow ones on a schedule, but we run the slow tests whenever it isn't a pull_request. Should I do not pr & not schedule
to match the logic of when we run slow tests? Like if someone make a commit to main directly, I don't think that would be a merge event but the slow tests would still run.
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.
@IAlibay Let me know if the updated logic is better now, I went with "not schedule && not pull request" so we can cover any case where a slow test is ran (not pull) but only upload if it isn't a schedule run.
.github/workflows/ci.yaml
Outdated
|
||
- name: codecov-schedule | ||
if: ${{ github.repository == 'OpenFreeEnergy/openfe' # we only want to upload a slow report if | ||
&& github.event_name != 'schedule' # 1) it isn't a schedule run |
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.
there's a syntax error on this line apparently
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.
can't weve comments into the expression it seems, fixed!
Co-authored-by: Irfan Alibay <[email protected]>
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!
Developers certificate of origin
Fixes #610