sync with latest template #178

richm · 2020-03-30T00:45:53Z

Just use the python command in the virtual environment, instead
of using envpython. This solves some problems where the
attempt to resolve the envpython to the syspython by following
symlinks causes python not to be able find some packages installed
in the venv.

Add support for python 3.8-dev in travis.

Do not install ansible by default in all tox venvs for pytest
tests. If the user needs to install ansible for pytests, the
user will add ansible to the new pytest_extra_requirements.txt
For python 2.6, which requires ansible 2.6, there is a special
pytest26_extra_requirements.txt, and an example
ansible26_requirements.txt which the user will use to specify
the extra dependencies for installing ansible 2.6 on python 2.6.

There are two extra dependencies added to the list of dependencies
installed on the travis machines when using python 2.6 - libffi-dev
and libssl-dev - these are required in order to install ansible 2.6
and its extra dependencies.

For python 2.6, an older version of tox must be used, which does not
have support for commands_pre, so allow tests/setup_module_utils.sh
to be run directly from .travis/runpytest.sh on those platforms.
the user will select to install ansible

Use the file .yamllint_defaults.yml for the base yamllint
configuration. User customizations go into .yamllint.yml, which
extends .yamllint_defaults.yml. This allows the use of yamllint
with IDEs that do not allow specifying command line arguments
for the yamllint command.

Only run coveralls in travis in the py35 environment.

tyll · 2020-03-30T07:22:07Z

.yamllint_custom.yml

@@ -0,0 +1,17 @@
+# SPDX-License-Identifier: MIT
+---
+extends: .yamllint.yml


IMHO it makes more sense to have a file like .yamllint_defaults.yml that contains the defaults and a .yamllint.yml that extends it. This makes sure that yamllint without special configuration picks up the custom configuration instead of only the defaults. This would be useful for running yamllint inside of IDEs/editors.

This is a good point.

So the presence of _custom in the name of the file doesn't lead you to believe that this is the file that should have local customizations?

It makes me understand that it is meant for local customomizations but the yamllint tool does not know this. It will only load .yamllint.yml by default. Therefore it would be better to have the customizations in .yamllint.yml so that yamllint can be called by IDEs without arguments specific to system roles.

ah - so it is only for IDE support where the IDE cannot pass command line arguments to the yamllint command

yes, or just to avoid having to configure the IDE specifically for each project, for example for one-time contributors.

.travis.yml

tyll · 2020-03-30T12:16:33Z

tox.ini

 deps =
+    py{27,36,37,38}: ansible


This seems to be a new dependency since it is not removed from the py26 env (it does not seem to be specified, there).

ansible does not work with python 2.6
ansible is needed for pytest

Are you sure? https://docs.ansible.com/ansible/latest/dev_guide/developing_python_3.html#minimum-version-of-python-3-x-and-python-2-x "Module-side, we support Python 3.5 or greater and Python 2.6 or greater." And this seems to be used for module code testing, not running Ansible itself.

(maybe I don't understand why Ansible is needed for pytest.)

Are you sure? https://docs.ansible.com/ansible/latest/dev_guide/developing_python_3.html#minimum-version-of-python-3-x-and-python-2-x "Module-side, we support Python 3.5 or greater and Python 2.6 or greater." And this seems to be used for module code testing, not running Ansible itself.

I tried it - it doesn't work - I tried installing the latest ansible 2.7.x in a py26 tox venv and it complained about the version of python. Please try it. NOTE that I understand what the ansible documentation says, I'm suggesting that it may be incorrect.

in kernel_settings and storage and possibly other roles, ansible is used directly in the unit tests. I'm not sure if ansible is needed by pylint or flake tests.
If it is going to be a problem to have ansible always installed for every python environment, then I'll figure out a way to refactor this, to make the ansible dependency per-tox environment, or even a per-repo dependency installed via some sort of custom requirements file.

Do the kernel_settings and storage role target CentOS 6, too, or only newer versions? Then it would not matter for them. But the reasoning in the comment does not make sense for other roles that also target CentOS6/Python 2.6.

I'm coming up with a much more comprehensive way to specify that ansible is a dependency for pytests for different platforms, which also takes into consideration that installing ansible in a tox venv on 2.6 has additional requirements, both on the ubuntu platform side in the form of additional C library dependencies, and on the pip side in the form of older versions of some dependencies. This will give the role developer the flexibility to decide if ansible is a dependency for the pytest tests, and which version of ansible to use.

The tests mock the ansible module: https://github.com/linux-system-roles/network/blob/master/tests/unit/test_nm_provider.py#L17

Hmm, is this approach going to be scalable and maintainable if used for all the roles that have embedded modules?

Hmm, is this approach going to be scalable and maintainable if used for all the roles that have embedded modules?

My approach is to give unit tests for modules an actual run-time environment that is as close to a production environment as possible, with a real ansible with the role module_utils/ code in the real ansible.module_utils.*module namespace, so that no sys.path munging, or module Mock-ing, is necessary.

That being said, I think the approach that network takes might be useful for certain unit test cases where you want to test a single method or small group of methods, and nothing else, and find that dragging in all of ansible is an impediment.

But I still think that unit testing with a real ansible is a good thing that we should do by default.

tyll · 2020-03-30T14:01:40Z

tox.ini

-  2.7: py27,flake8,pylint,custom
-  3.5: coveralls,custom
-  3.6: py36,black,yamllint,custom
+  2.7: py27,coveralls,flake8,pylint,custom


Is this change correct? I believe this change was part of another sync attempt and it does not make sense in the current configuration since coveralls needs to run only once.

I'll change coveralls to be executed only in the 3.6 environment.

@tyll Also - there is a new .travis/runcoveralls.sh in the template - can this be used to replace the network

bash -c 'cd ..; bash .travis/fix-coverage.sh; cd -' coveralls

?
It looks as though there is some creation and fixup of .coveragerc in template .travis/runcoveralls.sh but I don't know if this is the same as what network is doing, or if we need some additional work to template .travis/runcoveralls.sh in order to be suitable for network.

tyll · 2020-03-30T14:19:37Z

As with the last sync attempt, it would be nicer to sync the commits for the actual changes to be able to review and approve them together with their justification from the commit message. There are changes that are good like changing the Ansible versions, some changes that need discussion like changing the yamllint config file.

pcahyna · 2020-03-30T14:22:02Z

As with the last sync attempt, it would be nicer to sync the commits for the actual changes to be able to review and approve them together with their justification from the commit message. There are changes that are good like changing the Ansible versions, some changes that need discussion like changing the yamllint config file.

I think we discussed with @i386x the possibility to do this (sync commit-by-commit) and determined that it was not that important because (IIRC) after the initial sync, new changes would be small. Maybe this was a wrong assumption.

richm · 2020-03-30T14:27:21Z

As with the last sync attempt, it would be nicer to sync the commits for the actual changes to be able to review and approve them together with their justification from the commit message. There are changes that are good like changing the Ansible versions, some changes that need discussion like changing the yamllint config file.

In the future, for template changes, I'll submit the PR to the template repo and the network repo at the same time so we can get a wider audience for reviews, and make sure the changes work for network before merging them for template.

richm · 2020-03-30T20:16:07Z

@tyll I think I have addressed all of your review feedback. This PR now contains the changes from the latest template as well as the new changes from PR linux-system-roles/template#18 which address your review feedback.

richm · 2020-03-30T20:22:17Z

updated linux-system-roles/auto-maintenance#8 also

.travis/utils.sh

.yamllint_defaults.yml

tyll

Please not my inline comments

tyll · 2020-03-30T20:32:31Z

pytest26_extra_requirements.txt

+
+# Write extra requirements for running pytest with python2.6 here:
+# NOTE: If your pytests need ansible, use the requirements from
+# ansible26_requirements.txt


Just wondering, does this mean to copy the contents from ansible26_requirements here or does adding -r ansible26_requirements.txt work here?

See if the comment in the pytest26_extra_requirements.txt in the latest commit answers your question

tyll · 2020-03-30T20:35:06Z

tox.ini


 [testenv:yamllint]
 deps = yamllint
+whitelist_externals =


This env does not use bash, so the whitelist_externals does not seem to be needed here.

it is needed because of the command_pre in [testenv] which applies to every section . . . so I suppose the right answer here is to remove the command_pre. This means setup_module_utils.sh will not be done automatically by default for every test env. I suppose I could add a command_pre for setup_module_utils.sh to every env, but that seems fragile and redundant, because in the case of network, it isn't needed for pytest. So I guess the right answer is to add a hook in the scripts that might need to set up module_utils for testing, such as .travis/runpytest.sh, .travis/runpylint.sh, .travis/runflake8.sh, that is controlled by .travis/config.sh and allows the user to specify that module_utils needs to be setup for testing.

done - see latest commit

tyll · 2020-03-30T20:39:39Z

.travis/runcoveralls.sh

+
+# Environment variables:
+#
+#   LSR_PUBLISH_COVERAGE


It seems that the change to set LSR_PUBLISH_COVERAGE to strict is missing. Should it be added to config.sh?

What change would that be? The initial commit of runcoveralls.sh to the template is here - linux-system-roles/template@57513da#diff-9c9e55de5f8cf641950dc28851dbd328 - I don't see where the default value of LSR_PUBLISH_COVERAGE would have been strict

I believe the default value in the template should be strict.

The default value of LSR_PUBLISH_COVERAGE should be left empty, since afaik at now only network role is using coverage & coveralls. Setting the LSR_PUBLISH_COVERAGE is done in .travis/config.sh, so to enable coverage reporting just put LSR_PUBLISH_COVERAGE=strict or LSR_PUBLISH_COVERAGE=yes there. Just a note, I am not sure what are @tyll expectations on coverage reporting, there is a discussion in #120 about it. Similar PR with .travis/runcoveralls.sh is also #152.

Ok, I see. The coveralls stuff is still WIP. I suggest then that this PR is ok, and most of the work being done for #120 and #152 should really go into template (except the .travis/config.sh changes), then sync'd back to network (and the other roles).

The default value of LSR_PUBLISH_COVERAGE should be left empty, since afaik at now only network role is using coverage & coveralls. Setting the LSR_PUBLISH_COVERAGE is done in .travis/config.sh, so to enable coverage reporting just put LSR_PUBLISH_COVERAGE=strict or LSR_PUBLISH_COVERAGE=yes there. Just a note, I am not sure what are @tyll expectations on coverage reporting, there is a discussion in #120 about it. Similar PR with .travis/runcoveralls.sh is also #152.

I guess the value for the network role should be yes then, because it should be enabled and (even if it is broken) and not accidentally be disabled IMHO. Please note, the value yes is not mentioned in the documentation for the variable, it only mentions emtpy, strict and debug. This is why I proposed strict.

richm · 2020-03-30T22:21:43Z

grr - coveralls test in 3.5 - I guess I need to become quite familiar with coveralls and figure out what to do here :-P

richm · 2020-03-30T22:31:42Z

grr - coveralls test in 3.5 - I guess I need to become quite familiar with coveralls and figure out what to do here :-P

@tyll - perhaps you can tell me what this is supposed to do, and how I'm supposed to fix this . . .

richm · 2020-03-30T23:05:06Z

grr - coveralls test in 3.5 - I guess I need to become quite familiar with coveralls and figure out what to do here :-P

@tyll - perhaps you can tell me what this is supposed to do, and how I'm supposed to fix this . . .

Maybe I shouldn't use strict by default? This is what network is doing currently: https://travis-ci.com/github/linux-system-roles/network/jobs/306713129

coveralls run-test: commands[0] | bash -c 'cd ..; bash .travis/fix-coverage.sh; cd -'
+ cat
+ mv .coverage .coverage.merge
mv: cannot stat '.coverage': No such file or directory
/home/travis/build/linux-system-roles/network/tests
coveralls run-test: commands[1] | coveralls
Submitting coverage to coveralls.io...
Coverage submitted!
Job #558.3
https://coveralls.io/jobs/60690839
___________________________________ summary ____________________________________
  custom: commands succeeded
  coveralls: commands succeeded

Note that there is no .coverage file but the current network role tox task ignores that and successfully submits coverage to coveralls.io.

It seems that coveralls should be used to collect the coverage information generated by pytest on various platforms, and coverage information generated by running the molecule tests with a special

    ansible_python_interpreter:
      "{{ coverage }} run -p --include *ansible_module_{{ coverage_module }}.py"

then collect all of the coverage information from the various hosts/environments and merge them together. But that doesn't appear to be how tox + travis is working. For one, in travis, all of the pytest tests are run on possibly separate VMs. For another, the coveralls test isn't run after all of the other tests have run and generated coverage information to merge and publish.

So I guess I don't know what the tox coveralls test is supposed to do, both inside and outside of travis.

Just use the `python` command in the virtual environment, instead of using `envpython`. This solves some problems where the attempt to resolve the `envpython` to the `syspython` by following symlinks causes python not to be able find some packages installed in the venv. Add support for python 3.8-dev in travis. Do not install ansible by default in all tox venvs for pytest tests. If the user needs to install ansible for pytests, the user will add `ansible` to the new `pytest_extra_requirements.txt` For python 2.6, which requires ansible 2.6, there is a special `pytest26_extra_requirements.txt`, and an example `ansible26_requirements.txt` which the user will use to specify the extra dependencies for installing ansible 2.6 on python 2.6. There are two extra dependencies added to the list of dependencies installed on the travis machines when using python 2.6 - libffi-dev and libssl-dev - these are required in order to install ansible 2.6 and its extra dependencies. For python 2.6, an older version of tox must be used, which does not have support for `commands_pre`, so allow `tests/setup_module_utils.sh` to be run directly from `.travis/runpytest.sh` on those platforms. the user will select to install ansible Use the file `.yamllint_defaults.yml` for the base `yamllint` configuration. User customizations go into `.yamllint.yml`, which extends `.yamllint_defaults.yml`. This allows the use of `yamllint` with IDEs that do not allow specifying command line arguments for the `yamllint` command. Only run coveralls in travis in the py35 environment.

richm · 2020-03-30T23:23:37Z

. . . so until I figure out what coveralls is supposed to do in the context of tox or travis, I have disabled LSR_PUBLISH_COVERAGE=strict

richm · 2020-03-31T01:40:44Z

[citest bad]

i386x · 2020-03-31T14:45:30Z

grr - coveralls test in 3.5 - I guess I need to become quite familiar with coveralls and figure out what to do here :-P

The purpose of fail in 3.5 (151cefc) is missing pytest run before coverage, which ends up in missing .coverage file and hence the error. The correct coveralls placement should be imho

2.7: py27,coveralls,flake8,pylint,custom
3.5: custom
3.6: py36,coveralls,black,yamllint,custom

i386x · 2020-03-31T15:05:18Z

Coverage submitted!
Job #558.3
https://coveralls.io/jobs/60690839

Note that there is no .coverage file but the current network role tox task ignores that and successfully submits coverage to coveralls.io.

Despite on the fact that coveralls wrote that coverage result were submitted, there are no data displayed in https://coveralls.io/jobs/60690839 (compare with https://coveralls.io/builds/28545429).

richm · 2020-03-31T16:23:25Z

I am removing this PR in favor of #181 and #182, and more to come once these are merged

richm requested review from tyll and i386x March 30, 2020 00:46