Skip to content
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

Useless combo of docker-image and target-python-version #12

Open
klando opened this issue Nov 17, 2021 · 5 comments
Open

Useless combo of docker-image and target-python-version #12

klando opened this issue Nov 17, 2021 · 5 comments
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@klando
Copy link
Contributor

klando commented Nov 17, 2021

Felix Fontein clarified a point about docker imageS and (target) python versionS in ansible-test: it's either one or the other, but using a named docker image + a specific python target is not of interest (the default docker image is the one to use for testing with several python target).

I didn't understood that before...

As a consequence it might be of interest to outline that in this Action, and eventually pretends to prevent bad combination of options, something this PR does not address at all.

@webknjaz webknjaz added the enhancement New feature or request label Nov 17, 2021
@webknjaz
Copy link
Member

Feel free to send a PR updating the README and maybe adding a warning annotation via runner commands. Here's an example: https://github.com/pypa/gh-action-pypi-publish/blob/bea5cda/twine-upload.sh#L10-L18.

@webknjaz webknjaz added documentation Improvements or additions to documentation good first issue Good for newcomers help wanted Extra attention is needed labels Nov 17, 2021
@felixfontein
Copy link
Contributor

Images can contain multiple Python version (for example the default image does), for such images the explicit Python version is useful. For other images it usually is not.

@webknjaz
Copy link
Member

@felixfontein are you saying that a runtime warning would be disturbing the UX, and we just need to mention this in the docs?

@felixfontein
Copy link
Contributor

It depends.

For sanity tests, you should always use the default container and never specify target-python-version.

For unit tests, you should always use the default container, but specifying target-python-version can make sense (if you want to split up unit tests per Python version - useful if running them for one Python version takes a long time).

For integration tests, all containers can make sense, and whether target-python-version makes sense depends on the container. It should always be specified for the default container, it should never be specified for most OS containers coming with ansible-test, and it must always be specified for custom containers.

@webknjaz
Copy link
Member

Got it. Feel free to send a PR with your vision. I don't have strong opinions on this so won't prioritize implementing the solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants