-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
ci: some minor fixes #675
ci: some minor fixes #675
Conversation
Signed-off-by: Henry Schreiner <[email protected]>
547aad2
to
b46cd67
Compare
on: [push, pull_request] | ||
on: | ||
push: | ||
branches: [main] |
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.
Without this, CI is currently running twice on things like dependabot PRs.
env: | ||
FORCE_COLOR: "1" | ||
PRE_COMMIT_COLOR: "always" |
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 believe this was fixed in git / pre-commit.
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.
Hmm, maybe not. Just a sec.
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.
No, I take that back.
|
||
concurrency: | ||
group: ${{ github.workflow }}-${{ github.ref }} | ||
cancel-in-progress: 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.
Makes repeated pushes to the same PR faster. Generates a few "cancelled job" notifications, though.
jobs: | ||
build: | ||
runs-on: ${{ matrix.os }} | ||
strategy: | ||
matrix: | ||
os: [ubuntu-latest, windows-latest, macos-latest] | ||
os: [ubuntu-20.04, windows-latest, macos-latest] |
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.
Temporary until the coverage issue on 22.04 is resolved.
miniforge-variant: Mambaforge | ||
use-mamba: 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.
A lot faster & supports 3.11.
The coverage issue: Lines 373 to 394 in cc710bd
I think this is never None on Ubuntu 22.04 & Python 3.11. Shouldn't the 100% requirement be on the combined coverage, rather than partial? Anyway, should this be marked as not being required to be covered, etc? |
Hmmm wonder why it would never be Agree on combined coverage being used for the If we can't do that then yeah I agree we should exclude it from coverage measurement and revert back to ubuntu-latest This all looks good to me 👍🏻 |
Fixes to get CI working again, and some speed improvements.