fix most ansible-test issues, suppress the rest #362

richm · 2021-03-03T18:53:21Z

Automation Hub, and possibly Galaxy in the future, require the
collection to be screened with ansible-test sanity among other
checks. The role had a number of issues:

Use AssertionError instead of assert
Use of logging module not in accordance with standards, but these
are ok and the errors were suppressed
Several import errors which are ok because they are checked
elsewhere
init.py in the module_utils directories must be empty, so a
new file myerror.py was added to move the code from init.py
- NOTE: network_lsr/nm/init.py is not empty
The documentation block in the module was not properly constructed
or formatted.
shellcheck issues, including removing unused files
use unused instead of _ (underscore) for variables that are
unused

library/network_connections.py

nhosoi

lgtm

tyll

Please avoid at least introducing words that do not align with our conscious language initiative and it also seems that logging should be improved instead of ignoring the warning.

library/network_connections.py

tyll · 2021-03-04T09:42:04Z

library/network_connections.py

            c = []
+            # pylint: disable=blacklisted-name


Suggested change

# pylint: disable=blacklisted-name

# disable check for bad names:

# pylint: disable=C0102

The name for the C0102 check does not match our conscious language initiative, therefore using the code with an explanation instead of the name seems to be better here. What is the bad variable name here? _? Is there a better pattern to do something x times than a for _ in range()? Maybe we can get rid of the warning completelely.

I rewrote the code to avoid this issue - check it out

tyll · 2021-03-04T09:42:37Z

library/network_connections.py

@@ -1988,6 +2015,7 @@ def _check_ethtool_setting_support(self, idx, connection):
                    idx, "ethtool.%s specified but not supported by NM", specified
                )

+            # pylint: disable=blacklisted-name


see the other comment about this change

I rewrote the code to avoid this issue e.g. use for option in specified.keys()

module_utils/network_lsr/ethtool.py

module_utils/network_lsr/nm/active_connection.py

tests/ensure_provider_tests.py

tyll · 2021-03-04T09:47:03Z

tests/get_coverage.sh

@@ -32,10 +31,12 @@ call_ansible() {
 }

 remote_coverage_dir="$(mktemp -d /tmp/remote_coverage-XXXXXX)"
-trap "rm -rf '${remote_coverage_dir}'" EXIT
+trap 'rm -rf "${remote_coverage_dir}"' EXIT


Is this change correct? AFAIU, this will make $remote_coverage_dir to be evaluated when the script exits but it should use the value it has at the time the trap is configured.

shellcheck assumes that the correct thing to do is to use single quotes around the entire trap command, and only expand it, including any variables used by the trap command, at trap evaulation time. https://github.com/koalaman/shellcheck/wiki/SC2064 - but we know what we're doing here, and we want the variable expanded at the point where the trap is declared. So we follow the directions:

Exceptions If you don't care that the trap code is expanded early because the commands/variables won't change during execution of the script, or because you want to use the current and not the future values, then you can ignore this message.

so we will tell shellcheck to be quiet

tests/merge_coverage.sh

tyll

Thank you, I only have two comments.

module_utils/network_lsr/argument_validator.py

module_utils/network_lsr/ethtool.py

richm · 2021-03-09T01:06:11Z

one thing to note - @cathay4t @liangwen12year @ffmancera - if this merges, all of your PRs will need to be rebased, and possibly there will be conflicts that will have to be fixed

richm · 2021-03-12T18:08:04Z

Rebase report - what effect will merging this PR have on other PRs:

Specify PCI address to configure profile #363 - rebases cleanly
Enable Enhanced Open (OWE) support in network role #356 - rebases cleanly
Add support for ETHTOOL Ring option in Network Role #355 - needs rebase to latest main branch - then minor changes
Enable SAE (WPA3-Personal) support in Network role #344 - needs rebase to latest main branch - then minor changes
Fix initscripts parameter names for declaring static ipv6 connections #311 - needs rebase to latest main branch
Add 'defroute' option #309 - needs rebase to latest main branch

richm · 2021-03-13T00:42:32Z

so wdyt? Can we merge this PR, and I help PR authors rebase and fixup their PRs?

richm · 2021-03-17T16:49:51Z

ok to merge?

liangwen12year · 2021-03-17T18:12:02Z

library/network_connections.py

@@ -1408,7 +1416,8 @@ def check_mode_set(self, check_mode, connections=None):
            or (c == CheckMode.PRE_RUN and check_mode in [CheckMode.REAL_RUN])
            or (c == CheckMode.REAL_RUN and check_mode in [CheckMode.DONE])
            or (c == CheckMode.DRY_RUN and check_mode in [CheckMode.DONE])
-        )
+        ):
+            raise AssertionError("check_mode value is incorrect {0}".format(c))


should this be raise AssertionError("updating check_mode value from {0} into {1} is incorrect".format(c, check_mode)) ?

fixed 2bfe053#diff-6b3f5f6149ac1330c5e2c832286b63b3597158a998f2244ddbec2045ace1f794R1420

liangwen12year · 2021-03-17T19:08:12Z

module_utils/network_lsr/nm_provider.py

+from __future__ import absolute_import, division, print_function
+
+__metaclass__ = type
+


Is 'type' the default 'metaclass' in python ?

Is 'type' the default 'metaclass' in python ?

https://docs.ansible.com/ansible/2.9/dev_guide/testing/sanity/metaclass-boilerplate.html

liangwen12year

LGTM!

ffmancera · 2021-03-18T14:07:04Z

@richm There are 2 black formatting errors. Other than that, it looks good to me. :-)

richm · 2021-03-18T15:21:50Z

@richm There are 2 black formatting errors. Other than that, it looks good to me. :-)

fixed

richm · 2021-03-18T16:36:11Z

@tyll have all of your requested changes been addressed?

requested changes were addressed

tyll · 2021-03-21T16:02:06Z

@tyll have all of your requested changes been addressed?

yes, thank you.

tyll · 2021-03-24T08:58:56Z

module_utils/network_lsr/ethtool.py

@@ -46,7 +50,7 @@ def get_perm_addr(ifname):
            res = ecmd.tobytes()
        except AttributeError:  # tobytes() is not available in python2
            res = ecmd.tostring()
-        _, size, perm_addr = struct.unpack("II%is" % MAX_ADDR_LEN, res)
+        dummy, size, perm_addr = struct.unpack("II%is" % MAX_ADDR_LEN, res)


The inclusive open source community orientation training at

https://training.linuxfoundation.org/training/inclusive-open-source-community-orientation-lfc102/

recommends in

https://trainingportal.linuxfoundation.org/learn/course/inclusive-open-source-community-orientation-lfc102/making-change/making-change

to avoid the term "Dummy Value" and use "Placeholder Value" instead. I do not see an explanation for this.

I guess I have to register to see the contents of that link? At any rate - what is the suggestion?

placeholder, size, perm_addr = struct.unpack("II%is" % MAX_ADDR_LEN, res)

?

why do you want to replace _ in the first place? I though that is pretty common and pythonic.

An alternative name might be unused. While I am surprised to hear "dummy" is discouraged, unused anyway seems a better name.

why do you want to replace _ in the first place? I though that is pretty common and pythonic.

#362 (comment)
tl;dr - _ is also used for i18n, and Ansible wants to reserve _ for future use.

An alternative name might be unused. While I am surprised to hear "dummy" is discouraged, unused anyway seems a better name.

"dummy" is a derogatory word which can be seen as non-inclusive

unused works for me

I'll note that "dummy" is used in many places in the network role:

> git grep -c dummy examples/dummy_simple.yml:4 library/network_connections.py:1 module_utils/network_lsr/argument_validator.py:1 pylintrc:2 tests/ensure_provider_tests.py:1 tests/playbooks/tests_dummy.yml:5 tests/tasks/create_dummy_profile.yml:1 tests/tasks/manage_test_interface.yml:9 tests/tests_dummy_nm.yml:2 tests/tests_helpers_and_asserts.yml:2

I guess I have to register to see the contents of that link? At any rate - what is the suggestion?

You can easily login with your Red Hat Google account if you like. The course was recommended in the past and you can get a Fedora badge if you complete it.

I'll note that "dummy" is used in many places in the network role:

It is also an official network interface type, see https://developers.redhat.com/blog/2018/10/22/introduction-to-linux-interfaces-for-virtual-networking/#dummy so we cannot get it completely removed.

> git grep -c dummy examples/dummy_simple.yml:4 library/network_connections.py:1 module_utils/network_lsr/argument_validator.py:1 pylintrc:2

This is a good find since it shwos that unused is configured as an expectedly not used variable name, so we can use that.

tests/ensure_provider_tests.py:1
tests/playbooks/tests_dummy.yml:5
tests/tasks/create_dummy_profile.yml:1
tests/tasks/manage_test_interface.yml:9
tests/tests_dummy_nm.yml:2
tests/tests_helpers_and_asserts.yml:2

richm · 2021-03-25T15:07:44Z

[citest]

jharuda · 2021-03-25T15:20:59Z

[citest]

richm · 2021-03-25T18:33:09Z

[citest]

.sanity-ansible-ignore-2.9.txt

tyll · 2021-04-07T19:01:44Z

Not sure if this is related to other changes, but there are now CI errors (Tox Python 3.6):

https://github.com/linux-system-roles/network/pull/362/checks?check_run_id=2290367054

Running sanity test 'import' with Python 2.6
541
ERROR: Found 4 import issue(s) on python 2.6 which need to be resolved:
542
ERROR: plugins/module_utils/network_lsr/nm/active_connection.py:13:0: traceback: ImportError: No module named gi (at plugins/module_utils/network_lsr/nm/client.py:13:0)
543
ERROR: plugins/module_utils/network_lsr/nm/client.py:13:0: traceback: ImportError: No module named gi
544
ERROR: plugins/module_utils/network_lsr/nm/connection.py:13:0: traceback: ImportError: No module named gi (at plugins/module_utils/network_lsr/nm/client.py:13:0)
545
ERROR: plugins/module_utils/network_lsr/nm/provider.py:11:0: traceback: ImportError: No module named gi (at plugins/module_utils/network_lsr/nm/client.py:13:0)

gi does not exist on Python 2.6 and is also not needed there. This is a module needed to access the NetworkManager API. Not sure where this is coming from, seems to be related to the conversion to collections.

Also, there are some warnings, not sure what causes them, maybe it is related to broken symlinks?
https://github.com/linux-system-roles/network/pull/362/checks?check_run_id=2290367054#step:5:257

tyll

Most shebang exceptions could be cleaned up.

.sanity-ansible-ignore-2.9.txt

tyll · 2021-04-07T19:16:45Z

.sanity-ansible-ignore-2.9.txt

+plugins/modules/network_connections.py validate-modules:no-default-for-required-parameter
+plugins/modules/network_connections.py validate-modules:parameter-type-not-in-doc
+plugins/modules/network_connections.py validate-modules:undocumented-parameter
+tests/network/covstats shebang!skip


Seems this just needs the whitespace to be removed from the script to fix this.

apparently not, if you are talking about the shebang

This exception is about what is described in https://docs.ansible.com/ansible/latest/dev_guide/testing/sanity/shebang.html, isn't it? It states #!/bin/bash as a valid Shebang but the file uses #! /bin/bash. Not sure what I am missing here.

#! /bin/bash is valid for the Linux kernel, though.

This is the error, for example:

ERROR: tests/network/covstats:1:1: unexpected non-module shebang: b'#!/bin/bash' See documentation for help: https://docs.ansible.com/ansible/2.9/dev_guide/testing/sanity/shebang.html

I think the problem is that ansible-test doesn't understand executable files outside of the expected module/plugin directories. So it thinks that tests/covstats is a module since it is executable and has a shebang, but it isn't in the right directory. This is because we need to have these helper scripts but there isn't a good place to keep them. There isn't a general Ansible role/collection convention for helper scripts used during build/test time. The ssh role uses .dev-tools for this - https://github.com/linux-system-roles/ssh/tree/master/.dev-tools - I think a dotfile directory is a good idea - we used to use .travis/ for these sorts of scripts.

The scripts are currently meant for a human to be used so hiding them in a dotdir seems to be wrong. I am fine with skipping them for now.

.sanity-ansible-ignore-2.9.txt

tyll · 2021-04-07T19:18:03Z

.sanity-ansible-ignore-2.9.txt

+plugins/module_utils/network_lsr/__init__.py shebang!skip
+plugins/module_utils/network_lsr/argument_validator.py shebang!skip
+plugins/module_utils/network_lsr/myerror.py shebang!skip


The shebangs can be removed from these files and then they don't need to be skipped.

.sanity-ansible-ignore-2.9.txt

tyll · 2021-04-07T19:28:07Z

The commit message is not accurate anymore since a few things were done differently, now. If you like, you can move some of the changes like the python code in a separate PR that we can merge to reduce the amount of changes piling up in this PR.

richm · 2021-04-07T20:13:06Z

The commit message is not accurate anymore since a few things were done differently, now. If you like, you can move some of the changes like the python code in a separate PR that we can merge to reduce the amount of changes piling up in this PR.

Based on the recent changes, including updates to the PR description and commit message, do we still need to split this?

tyll · 2021-04-07T20:51:01Z

The commit message is not accurate anymore since a few things were done differently, now. If you like, you can move some of the changes like the python code in a separate PR that we can merge to reduce the amount of changes piling up in this PR.

Based on the recent changes, including updates to the PR description and commit message, do we still need to split this?

I assumed the CI failures would cause more pain, I am fine with merging this as-is once the CI is finished.

tyll

👍

tyll · 2021-04-08T14:51:51Z

[citest bad]

Automation Hub, and possibly Galaxy in the future, require the collection to be screened with `ansible-test sanity` among other checks. The role had a number of issues: * Use `AssertionError` instead of `assert` * Use of `logging` module not in accordance with standards, but these are ok and the errors were suppressed * Several import errors which are ok because they are checked elsewhere * __init__.py in the module_utils directories must be empty, so a new file myerror.py was added to move the code from __init__.py * NOTE: network_lsr/nm/__init__.py is not empty * The documentation block in the module was not properly constructed or formatted. * shellcheck issues, including removing unused files * use `unused` instead of `_` (underscore) for variables that are unused add WARNING to module docs - collection users should not use directly Signed-off-by: Rich Megginson <[email protected]>

coveralls · 2021-04-09T17:41:41Z

Pull Request Test Coverage Report for Build 86f0bcd6bbd4a47245861e4100d2741dd79db24a-PR-362

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.04%) to 39.43%

Totals
Change from base Build 730032963:	-0.04%
Covered Lines:	996
Relevant Lines:	2526

💛 - Coveralls

richm force-pushed the ansible-test branch from a4ea77f to 0acd80e Compare March 3, 2021 18:53

richm requested review from cathay4t and nhosoi March 3, 2021 19:09

nhosoi reviewed Mar 3, 2021

View reviewed changes

library/network_connections.py Show resolved Hide resolved

nhosoi approved these changes Mar 3, 2021

View reviewed changes

tyll previously requested changes Mar 4, 2021

View reviewed changes

richm force-pushed the ansible-test branch from 46a2868 to ae28c0c Compare March 4, 2021 20:49

tyll reviewed Mar 5, 2021

View reviewed changes

module_utils/network_lsr/argument_validator.py Outdated Show resolved Hide resolved

module_utils/network_lsr/ethtool.py Outdated Show resolved Hide resolved

tyll requested review from tyll and ffmancera March 5, 2021 09:21

richm force-pushed the ansible-test branch 2 times, most recently from 2618ee1 to e70dd1b Compare March 8, 2021 19:08

richm mentioned this pull request Mar 9, 2021

use tox-lsr 2.2.1 #360

Closed

richm force-pushed the ansible-test branch from e70dd1b to ec6c03f Compare March 12, 2021 17:49

richm force-pushed the ansible-test branch 3 times, most recently from b779577 to 1b15341 Compare March 12, 2021 22:17

richm mentioned this pull request Mar 12, 2021

RHELPLAN-69928 Add a note to each module Doc to indicate it is private richm/linux-system-roles-network#1

Closed

tyll requested a review from liangwen12year March 15, 2021 16:19

richm force-pushed the ansible-test branch from 1b15341 to 7459a29 Compare March 15, 2021 19:07

liangwen12year reviewed Mar 17, 2021

View reviewed changes

richm force-pushed the ansible-test branch from 7459a29 to 2bfe053 Compare March 17, 2021 18:38

liangwen12year reviewed Mar 17, 2021

View reviewed changes

liangwen12year approved these changes Mar 17, 2021

View reviewed changes

richm force-pushed the ansible-test branch from 2bfe053 to 8fe9b59 Compare March 18, 2021 15:21

tyll reviewed Mar 24, 2021

View reviewed changes

richm force-pushed the ansible-test branch from 8fe9b59 to f3e4970 Compare March 24, 2021 21:04

nhosoi mentioned this pull request Apr 1, 2021

Cleaning up ansible-lint errors except '106', '303' and '403' #374

Merged

richm force-pushed the ansible-test branch from f3e4970 to 6f92848 Compare April 7, 2021 18:46

tyll reviewed Apr 7, 2021

View reviewed changes

.sanity-ansible-ignore-2.9.txt Outdated Show resolved Hide resolved

richm force-pushed the ansible-test branch from 6f92848 to e3fbbc3 Compare April 7, 2021 19:00

tyll reviewed Apr 7, 2021

View reviewed changes

richm force-pushed the ansible-test branch 3 times, most recently from bfd7c3e to 1f4a0c9 Compare April 7, 2021 19:57

tyll approved these changes Apr 7, 2021

View reviewed changes

tyll force-pushed the ansible-test branch from 1f4a0c9 to fa269b1 Compare April 9, 2021 17:39

tyll merged commit f5ff30a into linux-system-roles:main Apr 9, 2021

	# pylint: disable=blacklisted-name
	# disable check for bad names:
	# pylint: disable=C0102

		from __future__ import absolute_import, division, print_function

		__metaclass__ = type

fix most ansible-test issues, suppress the rest #362

fix most ansible-test issues, suppress the rest #362

Conversation

richm commented Mar 3, 2021 • edited Loading

nhosoi left a comment

Choose a reason for hiding this comment

tyll left a 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

tyll left a comment

Choose a reason for hiding this comment

richm commented Mar 9, 2021

richm commented Mar 12, 2021 • edited Loading

richm commented Mar 13, 2021

richm commented Mar 17, 2021

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

liangwen12year left a comment

Choose a reason for hiding this comment

ffmancera commented Mar 18, 2021

richm commented Mar 18, 2021

richm commented Mar 18, 2021

tyll commented Mar 21, 2021

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

richm commented Mar 25, 2021

jharuda commented Mar 25, 2021

richm commented Mar 25, 2021

tyll commented Apr 7, 2021

tyll left a 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

tyll commented Apr 7, 2021

richm commented Apr 7, 2021

tyll commented Apr 7, 2021

tyll left a comment

Choose a reason for hiding this comment

tyll commented Apr 8, 2021

coveralls commented Apr 9, 2021 • edited Loading

Pull Request Test Coverage Report for Build 86f0bcd6bbd4a47245861e4100d2741dd79db24a-PR-362

💛 - Coveralls

richm commented Mar 3, 2021 •

edited

Loading

richm commented Mar 12, 2021 •

edited

Loading

coveralls commented Apr 9, 2021 •

edited

Loading