Adjust GitHub Actions CI/CD linters #7611
Conversation
webknjaz
commented
Jan 18, 2020
webknjaz
commented
- workflow vs job name
- TOX_PARALLEL_NO_SPINNER
- vendoring
- env state logging
- Pip cache
webknjaz
force-pushed
the
testing/gh-actions-ci-cd-linters
branch
2 times, most recently
from
5563141
to
c6be21d
Compare
| - name: Log Python version | ||
| run: >- | ||
| python --version | ||
| - name: Log Python location | ||
| run: >- | ||
| which python | ||
| - name: Log Python env | ||
| run: >- | ||
| python -m sysconfig |
There was a problem hiding this comment.
nit: I'd prefer to merge these into one. Reasons detailed in a later comment.
There was a problem hiding this comment.
(explained @ https://github.com/pypa/pip/pull/7611/files#r368251546) It's easier to inspect/separate outputs of different commands if each has a separate label. I think that for me it's coming from Ansible's best practices of always naming steps...
| - name: Pre-fetch pre-commit hooks | ||
| # This is to separate test step from deps install | ||
| if: matrix.env.TOXENV == 'lint' | ||
| run: >- | ||
| .tox/lint/${{ runner.os == 'Windows' && 'Scripts' || 'bin' }}/python -m | ||
| pre_commit | ||
| install-hooks |
There was a problem hiding this comment.
Is this needed? pre-commit would do this on it's own on first run.
If it's not necessary, I'd rather that we drop it.
There was a problem hiding this comment.
It's good to separate install/init step from the actual test run because then it's clear what fails. It's the same as tox --notest + tox in a separate step.
| - name: Update setuptools and tox dependencies | ||
| run: | | ||
| - name: Update setuptools | ||
| run: >- | ||
| python -m pip install --upgrade setuptools | ||
| - name: Install tox | ||
| run: >- | ||
| python -m pip install --upgrade tox tox-venv | ||
| - name: Log the list of packages | ||
| run: >- | ||
| python -m pip freeze --all |
There was a problem hiding this comment.
nit: These as a single bundle was probably nicer -- it's not really a cause for concern, but I don't think this information has ever been useful for most tasks and even when it is, the existing single bundle is named well enough to know where to look.
There was a problem hiding this comment.
I think it's mostly personal preference. But I've found out if there's separate distinct steps, it's easier to inspect logs.
.github/workflows/python-linters.yml
Outdated
| name: >- | ||
| ${{ matrix.env.TOXENV }} | ||
| / | ||
| ${{ matrix.python-version }} | ||
| / | ||
| ${{ matrix.os }} |
There was a problem hiding this comment.
nit:
name: >-
${{ matrix.env.TOXENV }}-${{ matrix.python-version }} on ${{ matrix.os }}There was a problem hiding this comment.
Not sure if that's shorter. My goal was to make it fit better in the sidebar while still communicating the description better.
We could even do something like this (spaceless):
name: >-
${{ matrix.env.TOXENV }}/${{ matrix.python-version }}@${{ matrix.os }}
|
re vendoring failing on Windows: Looks a lot like #7600. :) |
* workflow vs job name * TOX_PARALLEL_NO_SPINNER * vendoring * env state logging * Pip cache
webknjaz
force-pushed
the
testing/gh-actions-ci-cd-linters
branch
from
1a593d1
to
476bcb8
Compare
|
@pradyunsg it's green |
lock
bot
added
the
auto-locked
