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

feat: allow defining a python version list for GHA action #601

Closed
wants to merge 1 commit into from

Conversation

mayeut
Copy link
Contributor

@mayeut mayeut commented Apr 24, 2022

This PR extends #594 by allowing to define a list of python versions to setup.
The default list is the same as in #594
The list is defined as a string because GitHub Actions does not support lists as inputs.

@FollowTheProcess
Copy link
Collaborator

Hi @mayeut, first of all thanks for spending the time to do this! We appreciate the effort! I have some thoughts though:

IMO this is best left to https://github.com/actions/setup-python, the wntrblm/nox action will currently install into whatever python is the default (via pipx, normally a version of 3.8) but combined with a setup-python step to install any desired version, Nox can then launch a session using any version requested. I've tried this myself on some personal projects since we added the action and it works perfectly this way.

In addition to the above, I don't like specifying the python version list as a long string in the yaml file, to me this is a smell and a sign that we maybe shouldn't do it that way.

Is there any way of specifying this as a true yaml list? If so I think I'd be happier with this but still a bit of me thinks it should be handled by the setup-python action as this is a standard across many languages in GHA, the "accepted" way to get language set up is a "setup-language" action, and this would introduce another way specifically for Nox projects.

Having said that, this is all based on my understanding of your proposal which may obviously be wrong, if I am please feel free to challenge! Ditto any other maintainers input on this.

@DiddiLeija
Copy link
Collaborator

I also think setup-python can achieve this. Also, in case we decide to not add this feature, we should explain the way to set up a Python version in the Nox's documentation.

@mayeut
Copy link
Contributor Author

mayeut commented Apr 30, 2022

@FollowTheProcess, @DiddiLeija,

Thanks for taking the time to review this.
Since the PR combines multiple things, I'm happy to split/rework it in different ones to ease the reviews (mostly "This PR also adds some basic tests for the action and it allows ubuntu-18.04 VM by installing nox with python 3.10" and the documentation bit).

the wntrblm/nox action will currently install into whatever python is the default (via pipx, normally a version of 3.8)

Adding tests to the action shows the following (https://github.com/mayeut/nox/runs/6239157159?check_suite_focus=true):

  • ubuntu 18.04: the action fails to install nox, pipx uses python 3.6.9 (from system python3)
  • ubuntu 20.04: nox is installed using Python 3.8.10 (from system python3)
  • windows-2019: nox is installed using Python 3.7 (from tool cache)
  • windows-2022: nox is installed using Python 3.9 (from tool cache)
  • macos-10.15: nox is installed using Python 3.10 (not from tool cache)
  • macos-11: nox is installed using Python 3.10 (not from tool cache)
  • macos-12: nox is installed using Python 3.10 (not from tool cache)

follow-up PR for this: #606

Also, in case we decide to not add this feature, we should explain the way to set up a Python version in the Nox's documentation.

follow-up PR for documentation: #607

Now that those bits are taken care of, let's go back to the python version list:

In addition to the above, I don't like specifying the python version list as a long string in the yaml file, to me this is a smell and a sign that we maybe shouldn't do it that way. Is there any way of specifying this as a true yaml list?

I agree with this not being perfect but, unfortunately, GHA inputs only allows string...

but still a bit of me thinks it should be handled by the setup-python action as this is a standard across many languages in GHA, the "accepted" way to get language set up is a "setup-language" action, and this would introduce another way specifically for Nox projects..

I also think setup-python can achieve this.

Obviously, setup-python can be used to achieve this and in fact, this is what the action is already using as a convenience for the user but, only for a subset of python versions (more or less the non EOL ones pre-installed on runners).
If I am to setup the wntrblm/nox action as it is today, I'm expecting to see it break all workflows using it about once a year (when the hard-coded python list is updated). It might be convenient to see that it's time to drop a Python version but, If I have my own deprecation policy, then it will just lead to all my workflows being broken and requiring some steps in order to setup the proper python to fix it. That's not very convenient so I'll probably just end up not using the action and hard-code the python list myself using setup-pyton (which on the other hand is overly verbose and will reduce workflow readability).

If I could specify the python list I want to support, then yes, I do want to use the action because it helps in every way possible. The intent will be clear (I do not want to setup 1 python multiple times, I want to setup nox with multiple python versions), the workflows will be more readable and it will never break on the nox action being updated (well, at least not because a python version was dropped in the hard-coded list).

I do agree that it's unfortunate that we can't pass a true yaml list, that there's probably still too many hard-coded things in this PR (that, I can probably do something about it) but I really do think the feature is worth the addition, maybe not in this form.

@mayeut
Copy link
Contributor Author

mayeut commented Jun 15, 2022

closing in favor of #609

@mayeut mayeut closed this Jun 15, 2022
@mayeut mayeut deleted the gha-select-python branch June 16, 2022 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants