Allow more lax env selection from computed factors

[terencehonles]

The previous env selection was rather strict when there were more than 2 factors supplied and this change relaxes it.

Given the following factors:

[{"py38", "lint"}, {"reqV1", "reqV2"}, {"opReqV1", "opReqV2"}]

The existing env selection would only match:

• py38-reqV1-opReqV1
• py38-reqV2-opReqV1
• py38-reqV1-opReqV2
• py38-reqV2-opReqV2 ...
• lint-reqV1-opReqV1 ...

It would fail to match the single factor "lint". Although, this may be correct for required factors, but for something like "lint" it may not need the additional factors that are required with the previous env selection.

This change allows selecting the following envs:

• py38
• py38-reqV1
• py38-reqV2
• py38-opReqV1
• py38-opReqV2
• py38-reqV1-opReqV1
• py38-reqV2-opReqV1
• py38-reqV2-opReqV2
• lint
• ...

In addition, this change makes the order of the factors no longer important:

• py38-opReqV1-reqV1
• reqV1-opReqV1-py38

All of these permutations are bound by the envlist that the user defines in their tox configuration so it is up to the user to keep their configuration organized and not go crazy with their factor ordering.

[ymyzk]

Thanks for opening the PR but the current behavior is actually by design. If we simply lax the env selection algorithm, it can break builds of the existing users so I cannot simply merge this change.

This is not very clean solution but if you need to run both environments like py38-reqV1-opReqV1 and lint. Configuring tox-gh-actions to cover the former type of environments and call tox again with the expected environment explicitly to cover the later type of environments.

$ tox
$ tox -e lint

If there's a good idea which can cover more use cases while not breaking the existing builds, that would be great.

[terencehonles]

If there's a good idea which can cover more use cases while not breaking the existing builds, that would be great.

What if we add a config option which defaults to the old behavior and prints a warning that it will change in a future version? That should be pretty easy to add.

This is not very clean solution but if you need to run both environments like py38-reqV1-opReqV1 and lint. Configuring tox-gh-actions to cover the former type of environments and call tox again with the expected environment explicitly to cover the later type of environments.

$ tox
$ tox -e lint

Unfortunately this gets pretty messy if you want to specify a single version of Python to run your lint tests. You can see my PR here https://github.com/encode/django-rest-framework/pull/7910, but I guess part of the problem is I was trying to have the envs run in their own runners. One issue with the suggestion you made is you would also need to track the exit status of both toxes.

[terencehonles]

I used your suggestion to clean up https://github.com/encode/django-rest-framework/pull/7910 since I doubted they wanted the PR that I had initially proposed, and having the different runners run the envs is more than tox-gh-actions would do anyways so what is currently there is closest to what my PR would do, but it would be nice to simplify the logic.

[ymyzk]

@terencehonles, thanks for updating the PR.

I understand the problem your project is facing but I'm still not sure whether adding another algorithm to find environments and/or making it as a new default is a good idea or not. For example, there're projects which prefers to use yet another approach to find environments like #44. I don't feel adding different types of algorithms to tox-gh-actions is scalable and it can easily make this plugin more difficult to understand. Considering these issues, I'd like to keep this PR on hold at this point.

In the future, I may be able to make the part to find environments more modular or even pluggable so that we can support various requirements while making tox-gh-actions simple enough.

I also had a look at encode/django-rest-framework#7910.

One issue with the suggestion you made is you would also need to track the exit status of both toxes.

I'm not sure this is actually a complicated problem. If we split the step to run tox into two steps, I don't think we need to worry much about the exit code and let GH Actions to handle that. How do you think?

- name: Run tox
  run: tox
- name: Run tox for extra environments
  if: ...
  run: tox -e ...

[terencehonles]

@ymyzk I appreciate you taking the time to look at the PR and respond. I do disagree with your point that these are different algorithms. I am under the impression both #44 or this PR are supersets of the original algorithm and most likely both #44 and this PR make sense for the defaults in the future.

Double checking #44, as I had seen it before, I realize that I'm not referring to the initial example proposed in the description, but instead the example proposed https://github.com/ymyzk/tox-gh-actions/issues/44#issuecomment-731645786

I do think the default algorithm should probably start with envconfigs.keys() | set(envlist) instead of just envlist as it does. I agree with you, that the example in the description of #44 is problematic because although you can determine that ci is an additional factor for the default env it's not clear what envs should use it. I would argue the proper way to define what was asked for in #44's description is https://github.com/ymyzk/tox-gh-actions/issues/44#issuecomment-731645786 however this would still run locally if:

[tox]
envlist = py3{,-ci}

so that is why the default algorithm should start with envconfigs.keys() | set(envlist) so that envlist does not have to specify py3-ci, but since it's defined explicitly in the config it will get used on the CI. I would also argue that you'd probably want to add:

[gh-actions:env]
GITHUB_ACTIONS = true : ci

so that ci is automatically added to all factors rather than explicitly stating them as in https://github.com/ymyzk/tox-gh-actions/issues/44#issuecomment-731645786

This is where I would argue that both #44 and this PR are still very compatible since if you had the following config:

[tox]
envlist = py3, flake8

[testenv]
deps = pytest
...

[testenv:py3-ci]
deps =
    {[testenv]deps}
    pytest-github-actions-annotate-failures
...

[testenv:flake8]
deps = flake8
...

[gh-actions]
python = 3.8: py3, flake8

[gh-actions:env]
GITHUB_ACTIONS=true: ci

Currently, this would match nothing. With what I argue is the fix that should be made from #44 ( envconfigs.keys() | set(envlist)) this would now match py3-ci, and with this PR it would properly match all expected envs py3-ci, flake8.

As is, the gh-actions:env selection is painfully strict and any "complex" selection would requiring listing envs by names you wouldn't expect without understanding how selection is actually happening.

I initially opened this PR for a change I was making to django-redis, but what you looked at in https://github.com/encode/django-rest-framework/pull/7910 is mostly what you suggested and not as complex as when I initially commented. In django-redis our ini looks more like this:

[tox:tox]                                                                               
envlist =                                                                      
    lint                                                                       
    py{36,37,38,39}-dj{22,31,32,main}-redis{latest,master}

[gh-actions]                                                                   
python =                                                                       
    3.6: py36                                                                  
    3.7: py37                                                                  
    3.8: py38, lint       
    3.9: py39  

[gh-actions:env]                                                               
DJANGO =                                                                       
    2.2: dj22                                                                  
    3.1: dj31                                                                  
    3.2: dj32                                                                  
    main: djmain                                                               
REDIS =                                                                        
    latest: redislatest                                                        
    master: redismaster   

Because we have two different ENV factors performing the lint would now have to be declared: lint-dj31-redislatest instead of lint as it is in the file. Obviously we could run a separate tox -e lint but it is non obvious that the second factor to Python 3.8 lint is unused if there are ENV factors. I am simply suggesting that the algorithm be adjusted to be less surprising and more likely what a user would expect (at least what I had expected).

[terencehonles]

One issue with the suggestion you made is you would also need to track the exit status of both toxes.

I'm not sure this is actually a complicated problem. If we split the step to run tox into two steps, I don't think we need to worry much about the exit code and let GH Actions to handle that. How do you think?

- name: Run tox
  run: tox
- name: Run tox for extra environments
  if: ...
  run: tox -e ...

Following up on the separate comment, this doesn't run the second step if the first fails, so you need to track the exit status manually if you want to make sure both run, but failures are still reported (which is the behavior if you had run tox against all the expected targets)

[ymyzk]

Following up on the separate comment, this doesn't run the second step if the first fails, so you need to track the exit status manually if you want to make sure both run, but failures are still reported (which is the behavior if you had run tox against all the expected targets)

Understood. That's true.