-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
Set up GitHub actions to replace Travis #215
Conversation
e3201c2
to
15192f1
Compare
@jeffwidman @nickjj Please help to review & merge this PR to enable the CI. After the CI is enabled, we could merge the following PR to release 0.14 (the CI will be passed after the first two PRs are merged):
After that, we could work on the 1.0 version and drop support to Python 2.7 and 3.6. |
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 all looks fine to me. A couple of small questions, but I'm going to merge as-is to unblock the other PR's for upgrades, and let @greyli decide on handling my comments on this PR via a follow-up PR.
- '*.rst' | ||
pull_request: | ||
branches: | ||
- 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.
Would we not want to have tests fire on all pull requests? Ie, if someone does a stacked PR, be nice to have tests fire.
pip install -U wheel | ||
pip install -U setuptools |
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.
Curious why you ran this as two separate commands rather than pip install -U wheel setuptools
?
To be clear, this isn't a "please change" request, but simply curious to learn.
fail-fast: false | ||
matrix: | ||
include: | ||
- {name: Linux, python: '3.11', os: ubuntu-latest, tox: py311} |
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.
Was it a conscious decision to have 3.11
as the latest version rather than 3.12
?
LOL, looks like you went ahead with merge just as I left my review, 👍 from me, just please take a look at some point. |
Should we revert and modify this PR to pull in Jeff's suggestions or roll forward? At the very least I do think we'll want the test suite to run on every PR. |
IMO, we should roll forward. I was planning to merge this myself til @greyli beat me to it. Because having working CI, even if only on |
Thanks for the review! I think I enabled the auto-merge option... Anyway, I fixed the issues you pointed out and made some more improvements in #217. |
fixes #190
The tests can be triggered normally:
greyli#1