Conversation
|
@robertodauria |
robertodauria
left a comment
robertodauria
left a comment
There was a problem hiding this comment.
I've added a few inline comments. Additionally:
- I see this doesn't remove .travis.yml, even if the issue mentions doing it
- The issue also says to use poetry to run the scripts, which does not seem the case anywhere in this workflow (?)
A few of the issues above (installing Travis-specific tools in GitHub Actions, carrying over the codecov pip package unused, replicating EOL Python versions instead of following the modernization plan in the issue) suggest the workflow was generated by translating the existing .travis.yml rather than being designed from an understanding of what each piece does. Please make sure you understand the purpose of every line in the workflow before submitting.
| fail-fast: false | ||
| matrix: | ||
| include: | ||
| - python-version: "3.6" |
There was a problem hiding this comment.
Python 3.6/3.7 are ancient and EOL. You mentioned in the issue to start using 3.10 (which was the right call), why not following through with it? :)
| include: | ||
| - python-version: "3.6" | ||
| tox-env: py36 | ||
| container-image: python:3.6-buster |
There was a problem hiding this comment.
I assume this is using container images because setup-python refuses to install 3.6/3.7. You're fighting the tooling to preserve something that should be dropped.
Also, debian buster is EOL since 2024.
| - name: Install CI tools (Travis before_script parity) | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -U poetry tox-travis codecov tox |
There was a problem hiding this comment.
What's the point of tox-travis in a Github Actions setup?
| pip install -U poetry tox-travis codecov tox | ||
|
|
||
| - name: Run tests via tox | ||
| run: tox -e "${{ matrix.tox-env }}" |
There was a problem hiding this comment.
There's no tox.ini and the tox config still has a Travis-specific passenv. Perhaps we want CI GITHUB_* there?
| - name: Cache pip downloads | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: /root/.cache/pip |
There was a problem hiding this comment.
I suspect this is less useful than it could be. tox creates virtualenvs for testing, but the .tox/ directory isn't cached, causing every run to reinstall every dependency from scratch.
| name: CI | ||
|
|
||
| on: | ||
| push: |
There was a problem hiding this comment.
I think on: push + on: pull_request without branch filters means every PR triggers two different workflow runs. What about filtering with push: branches: [main]?
2 participants
Refs #120
This PR adds GitHub Actions CI for Murakami
What’s included
.github/workflows/ci.ymltoxtests on Python 3.6 and 3.7 (matching Travis CI test matrix)poetry,tox-travis,codecov)Notes
release.yml).This change is