run github actions also on python 3.8 and 3.10 #107
Conversation
tox.ini
Outdated
| @@ -1,7 +1,7 @@ | |||
| [tox] | |||
| minversion = 3.23.0 | |||
| # py is before lint because it can alter tracked files | |||
| envlist = py,lint | |||
There was a problem hiding this comment.
that is not needed. you can call py310 even without mentioning it in default envlist. That makes is easier for people as it will not force them to install multiple versions of python
There was a problem hiding this comment.
I changed it as suggested.
91d4ef1 to
662eb15
Compare
|
@ziegenberg When you rebase this it should now pass on py310 too. Root cause was outdated constraints from ruamel.yaml library. My recent updates sorted that. As I seen that you made multiple pull-requests to this repository I imagine that you will like my invitation to join https://github.com/ansible-community/schemas/invitations ;) |
662eb15 to
fc676ab
Compare
|
I rebased on to the current |
|
I will rebase once #118 is merged. |
|
@ssbarnea I guess, with this PR the automated checks need to be adapted... |
.github/workflows/test.yml
Outdated
| @@ -35,17 +35,21 @@ jobs: | |||
| - run: npm test | |||
| tox: | |||
| name: >- | |||
| ${{ matrix.env.TOXENV }} | |||
| ${{ matrix.tox_env }} | |||
There was a problem hiding this comment.
Let's use
| ${{ matrix.tox_env }} | |
| ${{ matrix.toxenv }} |
to match the typical tox's terminology and env var naming.
.github/workflows/test.yml
Outdated
| include: | ||
| - python-version: 3.8 | ||
| os: ubuntu-20.04 | ||
| tox_env: py38,lint |
There was a problem hiding this comment.
Does lint need to be run under every Python runtime possible?
There was a problem hiding this comment.
running lint on the latest should be sufficient. I'll change it.
There was a problem hiding this comment.
Maybe put it into a separate job?
There was a problem hiding this comment.
done as suggested
.github/workflows/test.yml
Outdated
| env: ${{ matrix.env }} | ||
| run: | | ||
| set -ex | ||
| ${{ matrix.PREFIX }} tox -e ${{ matrix.tox_env }} --notest --skip-missing-interpreters false -vv |
There was a problem hiding this comment.
Drop the explicit env as suggested above (by the way, it'll cause a potential bug if you add a space after the comma in the matrix factors, this is why you should always watch out for quoting when injecting such things like this) + drop non-existing PREFIX.
There was a problem hiding this comment.
done as suggested
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
2abed09 to
f40a4da
Compare
|
I applied the suggested changes, rebased and pushed. |
.github/workflows/test.yml
Outdated
| env: ${{ matrix.env }} | ||
| - name: Initialize tox envs | ||
| run: | | ||
| set -ex |
There was a problem hiding this comment.
This is unnecessary because
- it's just one command that we're running below
- it is already visible in the log, no need to print it out twice
.github/workflows/test.yml
Outdated
|
|
||
| - name: Test with tox | ||
| - name: >- | ||
| Test with tox -e ${{ matrix.toxenv }} |
.github/workflows/test.yml
Outdated
| env: | ||
| TOX_PARALLEL_NO_SPINNER: 1 | ||
| TOXENV: ${{ matrix.toxenv }} |
There was a problem hiding this comment.
It is also possible to do
| TOXENV: ${{ matrix.toxenv }} | |
| TOXENV: ${{ matrix.noxenv && matrix.toxenv || 'py' }} |
And drop the explicit toxenv entries from the job definitions except for lint.
| env: | ||
| - TOXENV: py,lint | ||
| include: | ||
| - python-version: 3.8 |
There was a problem hiding this comment.
This could be even kept in the matrix with only one job in "include".
There was a problem hiding this comment.
Would you move the Python-only entries into the matrix too?
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
|
@webknjaz is this what you had in mind? |
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
| python3 -m tox --notest --skip-missing-interpreters false -vv | ||
| env: ${{ matrix.env }} | ||
| - name: Initialize tox envs | ||
| run: tox --notest --skip-missing-interpreters false -vv |
There was a problem hiding this comment.
What was the reason for dropping the explicit runpy python -m syntax?
| run: | | ||
| python3 -m tox | ||
| env: ${{ matrix.env }} | ||
| set -ex |
There was a problem hiding this comment.
This one is unnecessary here two.
|
I wish a merry Xmas |
Same to you and thanks for the work. We will continue after the winter break. |
extends:
This will succeed once #106 is merged.