RHELPLAN-42920 - Collections - script - add tox/travis tests for coll… #45

nhosoi · 2020-09-26T20:45:20Z

…ection conversion, verification

Adding collection to envlist in tox.ini and script .travis/collections.sh
that is invoked by "tox -e collection".

Notes:

collection.sh clone's https://github.com/linux-system-roles/auto-maintenance
and https://github.com/linux-system-roles/template. Once auto-maintenance pr/13
is merged, the line cloning the TEMPORARY branch should be replaced.
collection.sh runs tox with the envlist "yamllint,py38,shellcheck,custom", by
default. If any other tests to be run, it could added in tox.ini in each role.
For instance, this command line adds "black,flake8" to the default envlist and
they pass.
commands =
bash {toxinidir}/.travis/collection.sh {toxworkdir} "black,flake8,yamllint,py38,shellcheck,custom"
I could not find a way to run pylint successfully in the collections format yet.

richm · 2020-09-28T19:14:37Z

.travis/collection.sh

+# git clone --branch some_stable_tag https://github.com/linux-system-roles/auto-maintenance
+
+# TEMPORARY
+git clone --single-branch --branch role2collection https://github.com/nhosoi/auto-maintenance


I think it would be better to add ansible_role_parser.py to lsr_role2collection.py - that way we could just download a single file - we wouldn't have to use git clone we could just do something like
curl -L -o lsr_role2collection.py https://raw.githubusercontent.com/linux-system-roles/auto-maintenance/master/lsr_role2collection.py or curl -L -o lsr_role2collection.py https://raw.githubusercontent.com/linux-system-roles/auto-maintenance/$STABLE_TAG/lsr_role2collection.py

but we can worry about that later

I think it would be better to add ansible_role_parser.py to lsr_role2collection.py - that way we could just download a single file - we wouldn't have to use git clone we could just do something like
curl -L -o lsr_role2collection.py https://raw.githubusercontent.com/linux-system-roles/auto-maintenance/master/lsr_role2collection.py or curl -L -o lsr_role2collection.py https://raw.githubusercontent.com/linux-system-roles/auto-maintenance/$STABLE_TAG/lsr_role2collection.py

but we can worry about that later

@richm, I've pushed another commit. Do you think it's ok to keep ansible_role_parser.py independent? Or do you think it's better be in lsr_role2collection.py?

Replacing git clone with curl following the suggestion by @richm. https://github.com/linux-system-roles/template/pull/45#pullrequestreview-497822660 But ansible_role_parser.py is not added to lsr_role2collection.py. I'm wondering ansible_role_parser.py is more useful to be a library to share?

Do you think it's ok to keep ansible_role_parser.py independent? Or do you think it's better be in lsr_role2collection.py?

I think it would be better to have 1 file, but let's get this PR merged first.

I'm wondering ansible_role_parser.py is more useful to be a library to share?

We should have 1 file that can be imported into another python script, or run from the command line, by putting the non-reusable code in a main() method and changing the script like this:

if __name__ == "__main__": sys.exit(main())

but we can do that in a separate PR

Thank you, @richm. Do you think it's ok to merge this PR? Or do we need some more discussions?

Thank you, @richm. Do you think it's ok to merge this PR? Or do we need some more discussions?

I think it's ok to merge.

richm · 2020-09-28T19:16:31Z

.travis/collection.sh

+
+COLLECTION_SRC_PATH=${TOPDIR}/.. COLLECTION_DEST_PATH=${toxworkdir} COLLECTION_ROLE=${role} python auto-maintenance/lsr_role2collection.py > "${toxworkdir}"/collection.out 2>&1
+
+COLLECTION_SRC_PATH=${toxworkdir} COLLECTION_DEST_PATH=${toxworkdir} python auto-maintenance/lsr_role2collection.py --tox >> "${toxworkdir}"/collection.out 2>&1


Is there some reason to use env. vars. here instead of e.g. --src-path, --dest-path, etc.?

No, no reasons. We can use the options.

richm · 2020-09-28T19:17:46Z

.travis/collection.sh

+toxworkdir=${1:-${TOPDIR}/.tox}
+cd "${toxworkdir}"
+toxworkdir=$(pwd)
+envlist=${2:- "yamllint,py38,shellcheck,custom"}


I don't think we need custom in here

i386x

I could not find a way to run pylint successfully in the collections format yet.

We use runpylint.sh and custom_pylint.py combo for this. If not done yet, these files should be updated to match also the collections format.

tox.ini

nhosoi · 2020-09-30T21:55:13Z

"tox -e collection" results

PASSED

certificate
kdump
kernel_settings
logging
metrics
nbde_client
nbde_server
selinux
storage
timesync
tlog

FAILED

network

___________________________________ summary ____________________________________
  black: commands succeeded
ERROR:   pylint: commands failed
ERROR:   flake8: commands failed
ERROR:   yamllint: commands failed
  py38: commands succeeded
ERROR:   shellcheck: commands failed
runcollection.sh: tox in the converted collection format failed.
ERROR: InvocationError for command /usr/bin/bash .travis/runcollection.sh .tox (exited with code 1)
___________________________________ summary ____________________________________
ERROR:   collection: commands failed

EXCEPTION

tuned - there's no .travis, tox.ini, etc.

richm · 2020-10-01T00:10:47Z

Don't worry about tuned

network does not use setup_module_utils.sh - it actually rewrites the python import paths in python code https://github.com/linux-system-roles/network/blob/main/tests/unit/test_network_connections.py#L13-L25 - I think it would be better if network used setup_module_utils.sh or something like it - at any rate, that network unit test code will need to be edited to make it work with collections

nhosoi · 2020-10-01T04:22:08Z

Don't worry about tuned

All right.

network does not use setup_module_utils.sh - it actually rewrites the python import paths in python code https://github.com/linux-system-roles/network/blob/main/tests/unit/test_network_connections.py#L13-L25 - I think it would be better if network used setup_module_utils.sh or something like it - at any rate, that network unit test code will need to be edited to make it work with collections

I see. I manually copied setup_module_utils.sh and .travis/utils.sh, which does not fit in the network design... These are the errors from pylint in the collection converted network after running setup_module_utils.sh. The first one is the code handling the basestring which is not available in python3. Since I had to use python 3.6 instead of 2.7 [0], it's newly reported. Probably, it has to be somehow ignored. The second one is in a test python script under the tests directory, which the conversion script does not process for now.

************* Module network_lsr.utils
plugins/module_utils/network_lsr/utils.py:17:34: E0602: Undefined variable 'basestring' (undefined-variable)
************* Module test_ethernet
tests/network/integration/test_ethernet.py:29:8: E0401: Unable to import 'library.network_connections' (import-error)

[0] The reason why python was upgraded to 3.6 for pylint is I ran into this problem pylint-dev/pylint#1470 and to workaround it. It seems pylint newer than 2.4.x has the fix. First, I tried just upgrading pylint to pylint>=2.5 with python-2.7, but it was rejected. If we keep using python-2.7 for pylint, I'd think we may need to copy the custom module_utils instead of create symlinks.

richm · 2020-10-02T21:11:08Z

.travis/utils.sh

+    bash "$TOPDIR/tests/setup_module_utils.sh" "$srcdir" "$destdir"
+  else
+    setup_module_utils=$( ls "$TOPDIR"/tests/*/setup_module_utils.sh )
+    setup_module_util=$( echo "$setup_module_utils" | awk '{print $1}' )


setup_module_util=$( ls "$TOPDIR"/tests/*/setup_module_utils.sh | head -1 )

richm · 2020-10-02T21:13:01Z

.yamllint_defaults.yml

@@ -13,4 +13,6 @@ rules:
  truthy:
    allowed-values: ["yes", "no", "true", "false"]
    level: error
+  line-length:
+    level: warning


What is the reason for this change? We want line-length to be an error for regular roles, and we want to override it to be ignored for collections

Yes, true... I'm going to revert this change and rethink about it.

richm · 2020-10-02T21:15:09Z

tox.ini

-envdir = {toxworkdir}/env-{env:TRAVIS_PYTHON_VERSION:2.7}
-basepython = python2.7
+envdir = {toxworkdir}/env-{env:TRAVIS_PYTHON_VERSION:3.6}
+basepython = python3.6


can't use python2.7?

If we can install pylint>=2.5 with python2.7, we can... Or instead of creating symlinks, copying the custom module_utils, it might work with python2.7...

If just switching back to python2.7 in tox.ini, it fails like this:

************* Module certificate_request E:279, 0: Unable to import 'ansible_collections.fedora.system_roles.plugins.module_utils.certificate.providers.__init__' (import-error) ************* Module certificate.providers.certmonger E: 5, 0: Unable to import 'ansible_collections.fedora.system_roles.plugins.module_utils.certificate.providers.base' (import-error) ************* Module certificate.providers E: 1, 0: Unable to import 'ansible_collections.fedora.system_roles.plugins.module_utils.certificate.providers.certmonger' (import-error) ERROR: InvocationError for command /usr/bin/bash .travis/runpylint.sh --errors-only (exited with code 2)

instead of creating symlinks, copying the custom module_utils

It doesn't like symlinks?

instead of creating symlinks, copying the custom module_utils

It doesn't like symlinks?

That's what i'm suspecting... Let me test copying with python2.7 if we prefer 2.7. :)

It's not that I prefer python2.7, but 1) I don't want to change this arbitrarily 2) we need coverage on python2.7. It would be great if we could do pylint (and pytest) on python 2 and python 3 (but we can do that later).

One problem is we cannot use this pylint change (using python3 + pylint>=2.5) for network due to its referring basestring (See https://github.com/linux-system-roles/network/blob/main/module_utils/network_lsr/utils.py#L17). Even if its in the else of PY3, pylint fails by the undefined variable error. :(

Why does it fail if there is noqa:F821 ?
EDIT: F821 is for flake8, not for pylint.
And how is it possible that lint passes for the original code in the repository?

pcahyna · 2020-10-06T22:03:26Z

@nhosoi , if it helps, we agreed (with @richm) to reduce the requirements on Travis tests for the converted collection:

we don't need to execute Python unit tests on the converted collection
we don't need to statically check (lint) Python unit tests in the converted collection, it is enough to lint Python modules/other Ansible plugins/module_utils (as those will be user visible).

nhosoi · 2020-10-06T23:08:18Z

@nhosoi , if it helps, we agreed (with @richm) to reduce the requirements on Travis tests for the converted collection:
* we don't need to execute Python unit tests on the converted collection

* we don't need to statically check (lint) Python unit tests in the converted collection, it is enough to lint Python modules/other Ansible plugins/module_utils (as those will be user visible).

Thank you, @pcahyna.

I've been investigating why pylint with python 2.7 + pylint-1.9.5 does not successfully pass against the collections format.

************* Module certificate_request
E:282, 0: Unable to import 'ansible_collections.fedora.system_roles.plugins.module_utils.certificate.providers.__init__' (import-error)
************* Module certificate.providers.certmonger
E:  8, 0: Unable to import 'ansible_collections.fedora.system_roles.plugins.module_utils.certificate.providers.base' (import-error)
************* Module certificate.providers
E:  4, 0: Unable to import 'ansible_collections.fedora.system_roles.plugins.module_utils.certificate.providers.certmonger' (import-error)

I replaced symlinks with actual files by modifying setup_module_utils.sh to workaround this issue pylint-dev/astroid#583.
Then, I ran into another issue: ansible_collections is not found as a module, which is happening in the function _find_spec_with_path(in env-2.7/lib/python2.7/site-packages/astroid/interpreter/_import/spec.py). The sys.path contains the path to the parent directory of ansible_collections, but it is not found, although it is found with python 3.6 + pylint-2.6.0. It turned out, 2 new finders are added to astroid 2.0, and one of them ExplicitNamespacePackageFinder finds ansible_collections and it makes the pylint pass with python 3.6.

_SPEC_FINDERS = (
    ImpFinder,
    ZipFinder,
)
if _HAS_MACHINERY and sys.version_info[:2] > (3, 3):
    _SPEC_FINDERS += (PathSpecFinder, )
_SPEC_FINDERS += (ExplicitNamespacePackageFinder, )

That's said I was proposing to upgrade from python 2.7 to 3.6 for pylint/pytest. But if there's no need to run them in the collections format, we could drop the part.

In my investigation, one question has come up... Looking into the other collections source codes, I noticed the __future__ module is imported. I slightly hoped if it could solve my pylint import problem, but it did not help to find ansible_collections. Aside from my issue, if we support system roles in rhel7, is it safer to add these lines to the custom modules and module_utils or it does not matter any more?

diff --git a/module_utils/certificate/providers/base.py b/module_utils/certificate/providers/base.py
index 3fcf457..22d94d8 100644
--- a/module_utils/certificate/providers/base.py
+++ b/module_utils/certificate/providers/base.py
@@ -1,3 +1,6 @@
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
 import hashlib
 import os
 import ipaddress

richm · 2020-10-07T00:31:27Z

but it did not help to find ansible_collections
...
is it safer to add these lines to the custom modules and module_utils or it does not matter any more?

If it doesn't help, why would we add __future__?

nhosoi · 2020-10-07T01:09:13Z

but it did not help to find ansible_collections
...
is it safer to add these lines to the custom modules and module_utils or it does not matter any more?

If it doesn't help, why would we add __future__?

Just because I saw lots of the modules in the roles have it... I thought it might be quite common and encouraged to import it. :)

<<snip>>
./ansible/netcommon/plugins/modules/net_lldp_interface.py:from __future__ import absolute_import, division, print_function
./ansible/posix/plugins/modules/at.py:from __future__ import absolute_import, division, print_function
<<snip>>

pcahyna · 2020-10-07T08:24:48Z

@nhosoi

is it safer to add these lines to the custom modules and module_utils or it does not matter any more?

https://docs.ansible.com/ansible/latest/dev_guide/developing_python_3.html#use-forward-compatibility-boilerplate

richm · 2020-10-07T15:08:42Z

https://docs.ansible.com/ansible/latest/dev_guide/developing_python_3.html#use-forward-compatibility-boilerplate

Yes, we should add that __future__ boilerplate to all of our modules code - but in separate PRs, not in this one.

nhosoi · 2020-10-07T15:37:35Z

https://docs.ansible.com/ansible/latest/dev_guide/developing_python_3.html#use-forward-compatibility-boilerplate

Yes, we should add that __future__ boilerplate to all of our modules code - but in separate PRs, not in this one.

I'm opening a task. Thanks!

nhosoi · 2020-10-07T15:54:49Z

@nhosoi , if it helps, we agreed (with @richm) to reduce the requirements on Travis tests for the converted collection:
* we don't need to execute Python unit tests on the converted collection

* we don't need to statically check (lint) Python unit tests in the converted collection, it is enough to lint Python modules/other Ansible plugins/module_utils (as those will be user visible).

@richm, @pcahyna, let me double-check on it...
This is the current test list to run in tox -e collection.
envlist="black,pylint,flake8,yamllint,py38,shellcheck"
Are we dropping pylint from it like this? Or even less?
envlist="black,flake8,yamllint,py38,shellcheck"
Or shall we leave pylint with python version 3.x?

richm · 2020-10-07T16:27:23Z

Are we dropping pylint from it like this? Or even less?

Let's drop pylint for now. Both black and flake8 give us most of that coverage

pcahyna · 2020-10-07T17:49:48Z

https://docs.ansible.com/ansible/latest/dev_guide/developing_python_3.html#use-forward-compatibility-boilerplate

Yes, we should add that __future__ boilerplate to all of our modules code - but in separate PRs, not in this one.

Indeed - I did not mean to imply that it should be added by the conversion script unless it is really needed in the resulting collection, which it apparently is not.

pcahyna · 2020-10-07T17:54:37Z

we don't need to statically check (lint) Python unit tests in the converted collection, it is enough to lint Python modules/other Ansible plugins/module_utils (as those will be user visible).

This is the current test list to run in tox -e collection.
envlist="black,pylint,flake8,yamllint,py38,shellcheck"
Are we dropping pylint from it like this? Or even less?
envlist="black,flake8,yamllint,py38,shellcheck"

This is not what I meant - my intent was to use pylint only on modules/plugins and module_utils. But if it does not help with your issue (looks like it does not) and/or is too difficult to implement and flake8 provides the same service, then dropping pylint is fine too.
(I don't think black will provide the same service - it is just a formatter, isn't it?)

richm · 2020-10-07T17:57:24Z

(I don't think black will provide the same service - it is just a formatter, isn't it?)

We use it like this: black --check --diff which tells black to check for formatting issues and report them. There is some overlap in the issues that black, flake8, and pylint report.

nhosoi · 2020-10-07T18:18:35Z

(I don't think black will provide the same service - it is just a formatter, isn't it?)

We use it like this: black --check --diff which tells black to check for formatting issues and report them. There is some overlap in the issues that black, flake8, and pylint report.

Regarding black and flake8, both have no problem running against the collection converted formats. Only an issue I ran into so far was pylint running with Python 2. I haven't tried pytest yet, but probably it'll reveal some more issues, I'd guess.

In terms of the testing areas among black, flake8 and pylint, I like pylint's ability to check import paths. Since collection converter modifies the path, e.g., ansible.module_utils.some.thing to ansible_collections.NS.COLLECTION.plugins.module_utils.some.thing, it'll give us a good chance to capture the broken path/conversion quickly.

pcahyna · 2020-10-07T18:23:52Z

I would not bother too much with pytest, unless it is really easy.

nhosoi · 2020-10-07T22:20:37Z

https://docs.ansible.com/ansible/latest/dev_guide/developing_python_3.html#use-forward-compatibility-boilerplate

Yes, we should add that __future__ boilerplate to all of our modules code - but in separate PRs, not in this one.

I'm opening a task. Thanks!

FYI: RHELPLAN-56179 - Add the future boilerplate to all of the modules and module_utils code

linux-system-roles#45 (review) But ansible_role_parser.py is not added to lsr_role2collection.py. I'm wondering ansible_role_parser.py is more useful to be a library to share?

richm

lgtm
please rebase and squash

…ection conversion, verification Adding collection to envlist and travis in tox.ini. Adding script .travis/runcollection.sh that is invoked by "tox -e collection". Default test list specified in runcollection.sh: "black, flake8, yamllint, py38, shellcheck". yamllint: since collection conversion removes some line breaks, yamllint tends to fail with line too long. Setting the level to warning in .yamllint_defaults.yml when yamllint is executed in the collection format. utils.sh: supporting collections format path, e.g., - "$TOPDIR"/tests/ROLENAME/setup_module_utils.sh - SRC_COLL_MODULE_UTILS_DIR and DEST_COLL_MODULE_UTILS_DIR

nhosoi · 2020-10-08T20:03:28Z

Thank you, @richm. Merged.

nhosoi requested review from i386x, pcahyna and richm as code owners September 26, 2020 20:45

nhosoi force-pushed the collections branch from 83f8c30 to 4de0d1c Compare September 26, 2020 21:47

richm reviewed Sep 28, 2020

View reviewed changes

nhosoi force-pushed the collections branch from 2129a8c to 8bb3c59 Compare September 29, 2020 23:32

i386x reviewed Sep 30, 2020

View reviewed changes

tox.ini Outdated Show resolved Hide resolved

tox.ini Show resolved Hide resolved

nhosoi force-pushed the collections branch from 8bb3c59 to e66c8c6 Compare September 30, 2020 20:51

nhosoi changed the title ~~[WIP] - RHELPLAN-42920 - Collections - script - add tox/travis tests for coll…~~ RHELPLAN-42920 - Collections - script - add tox/travis tests for coll… Sep 30, 2020

nhosoi force-pushed the collections branch from e66c8c6 to 6c5d96b Compare October 2, 2020 00:11

richm reviewed Oct 2, 2020

View reviewed changes

richm approved these changes Oct 8, 2020

View reviewed changes

nhosoi force-pushed the collections branch from a0ac750 to f2715f4 Compare October 8, 2020 19:05

nhosoi merged commit 3615d99 into linux-system-roles:master Oct 8, 2020


		COLLECTION_SRC_PATH=${TOPDIR}/.. COLLECTION_DEST_PATH=${toxworkdir} COLLECTION_ROLE=${role} python auto-maintenance/lsr_role2collection.py > "${toxworkdir}"/collection.out 2>&1

		COLLECTION_SRC_PATH=${toxworkdir} COLLECTION_DEST_PATH=${toxworkdir} python auto-maintenance/lsr_role2collection.py --tox >> "${toxworkdir}"/collection.out 2>&1

RHELPLAN-42920 - Collections - script - add tox/travis tests for coll… #45

RHELPLAN-42920 - Collections - script - add tox/travis tests for coll… #45

Conversation

nhosoi commented Sep 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richm Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i386x left a comment

Choose a reason for hiding this comment

nhosoi commented Sep 30, 2020 • edited Loading

"tox -e collection" results

richm commented Oct 1, 2020

nhosoi commented Oct 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcahyna Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

pcahyna commented Oct 6, 2020

nhosoi commented Oct 6, 2020

richm commented Oct 7, 2020

nhosoi commented Oct 7, 2020

pcahyna commented Oct 7, 2020

richm commented Oct 7, 2020

nhosoi commented Oct 7, 2020

nhosoi commented Oct 7, 2020

richm commented Oct 7, 2020

pcahyna commented Oct 7, 2020

pcahyna commented Oct 7, 2020

richm commented Oct 7, 2020

nhosoi commented Oct 7, 2020 • edited Loading

pcahyna commented Oct 7, 2020

nhosoi commented Oct 7, 2020

richm left a comment

Choose a reason for hiding this comment

nhosoi commented Oct 8, 2020

richm Oct 8, 2020 •

edited

Loading

nhosoi commented Sep 30, 2020 •

edited

Loading

pcahyna Oct 7, 2020 •

edited

Loading

nhosoi commented Oct 7, 2020 •

edited

Loading