feat: adapting tests and code-style actions to avoid using venv when called by poetry #423
Conversation
|
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
|
@philipjusher - could you give it a try to this branch? |
|
You can try it out by testing the following change on your action (for example): https://github.com/ansys/openapi-common/pull/528/files |
There was a problem hiding this comment.
Should we keep ${{ env.ACTIVATE_VENV }} when running without poetry ?
I'm asking that because I've got the feeling that something might go wrong when the action to setup python is leveraging an already downloaded python release. Using virtual environments had me thinking that we were protected from such problems.
|
Ideally we should be running the code-style action on GitHub runners and not on self-hosted runners. So you won't be having this issue. |
| run: | | ||
| if grep -q 'build-backend = "poetry\.core\.masonry\.api"' "pyproject.toml"; then | ||
| python -m pip install poetry | ||
| python -m poetry install |
There was a problem hiding this comment.
not sure you can call poetry from python. I think it installs as command line tool
There was a problem hiding this comment.
You are completely right - my bad
| @@ -135,8 +152,7 @@ runs: | |||
| if: env.EXISTS_TESTS_REQUIREMENTS == 'false' | |||
| run: | | |||
| if grep -q 'build-backend = "poetry\.core\.masonry\.api"' "pyproject.toml"; then | |||
| python -m pip install poetry | |||
| poetry install --with tests | |||
There was a problem hiding this comment.
you see in the previous version it doesnt call with python.
|
hi sorry for the delay but i couldnt get this to work. https://github.com/ansys-internal/pyconceptev-core/actions/runs/8295360420/job/22702433906#step:2:257 I added a review as to what I think the issue is but I think you need to run some tests on the scripts to make sure they work as expected for the different set ups. |
|
No problem - I'll look into your issues shortly :) |
We will work on this as well shortly - we need to add more testing, I agree |
|
Issue fixed on main - see #426 @philipjusher |
Closes #348