ci: Use supported ansible-lint action; run ansible-lint against the collection #661

richm · 2023-12-14T16:15:17Z

ci: Use supported ansible-lint action; run ansible-lint against the collection

The old ansible-community ansible-lint is deprecated. There is a
new ansible-lint github action. The new ansible-lint has several checks
related to ansible-test and the ignore files. Many of our ignore settings
are not allowed any more and are required to be fixed or addressed in the
Ansible preferred way.

The python imports have to be wrapped in a try/except ImportError, and
where possible, an error must be returned from the module explaining
what was not able to be imported.

The module documentation must comply with the Ansible standards. One
aspect of this is the version_added must be a valid ansible-core
version in X.Y.Z format. Note that this version isn't really used
anywhere, so it doesn't matter for users of the role, it is purely
an ansible-test and import gating issue.

The result of this is that the .sanity files can be reduced to the
bare minimum which will greatly reduce the maintenance burden of
those files, make it easier to support newer versions of Ansible,
and make it easier to import the system roles collection into Galaxy
and Automation Hub.

The latest Ansible repo gating tests run ansible-lint against
the collection format instead of against individual roles.
We have to convert the role to collection format before running
ansible-test.

Role developers can run this locally using
tox -e collection,ansible-lint-collection
See linux-system-roles/tox-lsr#125

Add --- doc start to .markdownlint.yaml

The file examples/down_profile+delete_interface.yml
was not used and was causing ansible-lint errors.

ansible-lint enforces the order of keywords in plays - name,
then hosts, then vars, then tasks.

codecov · 2023-12-14T16:35:46Z

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (bbdc7f7) 43.31% compared to head (e034387) 45.03%.

Files	Patch %	Lines
library/network_state.py	0.00%	12 Missing ⚠️
module_utils/network_lsr/nm/client.py	20.00%	11 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #661      +/-   ##
==========================================
+ Coverage   43.31%   45.03%   +1.71%     
==========================================
  Files          13       12       -1     
  Lines        3068     3080      +12     
  Branches      394      428      +34     
==========================================
+ Hits         1329     1387      +58     
+ Misses       1739     1692      -47     
- Partials        0        1       +1

Flag	Coverage Δ
sanity	`20.61% <20.00%> (+0.78%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

richm · 2023-12-14T16:44:58Z

[citest]

liangwen12year · 2023-12-16T16:23:05Z

library/network_state.py

@@ -22,15 +22,38 @@
 author: "Wen Liang (@liangwen12year)"
 """

+EXAMPLES = r"""


To make the format consistent, should we also add the options: desired_state: for network_state ?

To make the format consistent, should we also add the options: desired_state: for network_state ?

It was already there -

network/library/network_state.py

Line 17 in 1a689f7

options:

One thing to note is that this internal module documentation is intended for end users to call these modules directly, without using the role. This use case does not work and is unsupported. For us, this internal role documentation and formatting is really only so that we can pass ansible-lint and ansible-test gating tests. For us, the main documentation is the README.md for the role, which describes the working and supported interface. Users are not supposed to know or care that there are modules used internally in the role to implement the functionality of the role.

liangwen12year · 2023-12-18T20:17:38Z

tests/integration/conftest.py

-
-def pytest_addoption(parser):
-    parser.addoption(
-        "--provider", action="store", default="nm", help="Network provider"


I have checked the commit c29a2bf, is not the hook used with tests/integration/test_ethernet.py ?

I have checked the commit c29a2bf, is not the hook used with tests/integration/test_ethernet.py ?

I see it in that commit - but where/how is it used? doing git grep conftest in the network role local git clone shows nothing

It seems to me that the tests_integration_pytest.yml failure is related to the changes, I think that the initialization hooks are the common usage of conftest.py.

https://docs.pytest.org/en/7.1.x/example/simple.html#pass-different-values-to-a-test-function-depending-on-command-line-options

https://docs.pytest.org/en/6.2.x/reference.html#initialization-hooks

ok - I see - conftest.py is used internally by pytest - so no explicit reference - I added it back

liangwen12year · 2023-12-19T14:51:10Z

[citest]

richm · 2023-12-20T03:15:31Z

[citest]

richm · 2023-12-20T14:44:15Z

[citest bad]

liangwen12year · 2023-12-20T16:56:09Z

module_utils/network_lsr/nm/__init__.py

-
-# pylint: enable=import-error, no-name-in-module
-
-provider.NetworkManagerProvider


What is the reason to remove this file ?

What is the reason to remove this file ?

https://docs.ansible.com/ansible/latest/dev_guide/testing/sanity/empty-init.html

It appears that this isn't being used directly - but perhaps it is needed to make the unit tests work - not sure - but for sure we need to make this an empty file, and figure out some other way to make the unit tests work if needed

I don't see any unit test failures though - so IMO it isn't needed

The only test failures I see are

tests_provider_nm and tests_regression_nm on CentOS 9 - I don't think we need to run these tests on platforms where initscripts is not supported

tests_bond_options_nm on CentOS 8 - maybe the test is disabling network access? cannot connect to the EPEL repo

For skipping initscripts tests, I have the PR for review, #663

for the tests_bond_options_nm on CentOS 8, the test is not fully started yet, it fails in "INIT Prepare setup" where dnsmasq needs to be installed, maybe the EPEL repo can not be accessed itself sometime ? Also I noticed that some other bond tests (e.g. tests_bond_nm.yml) may fail for the same reason, and tests_bond_options.yml can also pass sometimes. see:

https://dl.fedoraproject.org/pub/alt/linuxsystemroles/logs/lsr-citool_network-663-70e4146_CentOS-Stream-8_20231220-205206/artifacts/summary.html
https://dl.fedoraproject.org/pub/alt/linuxsystemroles/logs/lsr-citool_network-663-70e4146_CentOS-Stream-8_20231220-205218/artifacts/summary.html

richm · 2023-12-21T15:30:25Z

[citest]

richm · 2023-12-23T20:52:53Z

[citest]

richm · 2023-12-25T21:28:40Z

[citest]

richm · 2023-12-27T17:07:20Z

[citest bad]

…ollection The old ansible-community ansible-lint is deprecated. There is a new ansible-lint github action. The new ansible-lint has several checks related to ansible-test and the ignore files. Many of our ignore settings are not allowed any more and are required to be fixed or addressed in the Ansible preferred way. The python imports have to be wrapped in a try/except ImportError, and where possible, an error must be returned from the module explaining what was not able to be imported. The module documentation must comply with the Ansible standards. One aspect of this is the `version_added` must be a valid ansible-core version in X.Y.Z format. Note that this version isn't really used anywhere, so it doesn't matter for users of the role, it is purely an `ansible-test` and import gating issue. The result of this is that the .sanity files can be reduced to the bare minimum which will greatly reduce the maintenance burden of those files, make it easier to support newer versions of Ansible, and make it easier to import the system roles collection into Galaxy and Automation Hub. The latest Ansible repo gating tests run ansible-lint against the collection format instead of against individual roles. We have to convert the role to collection format before running ansible-test. Role developers can run this locally using `tox -e collection,ansible-lint-collection` See linux-system-roles/tox-lsr#125 Add `---` doc start to .markdownlint.yaml The file `examples/down_profile+delete_interface.yml` was not used and was causing ansible-lint errors. ansible-lint enforces the order of keywords in plays - `name`, then `hosts`, then `vars`, then `tasks`. Signed-off-by: Rich Megginson <rmeggins@redhat.com>

richm requested a review from liangwen12year as a code owner December 14, 2023 16:15

richm force-pushed the ansible-lint-test-2.16 branch from 33883c0 to 7b1a52e Compare December 14, 2023 16:18

richm changed the title ~~ansible lint test 2.16~~ ci: Use supported ansible-lint action; run ansible-lint against the collection Dec 14, 2023

richm force-pushed the ansible-lint-test-2.16 branch from 7b1a52e to 1a689f7 Compare December 14, 2023 16:33

liangwen12year reviewed Dec 16, 2023

View reviewed changes

liangwen12year reviewed Dec 18, 2023

View reviewed changes

richm force-pushed the ansible-lint-test-2.16 branch from 3a61e9b to 7156bb8 Compare December 20, 2023 01:29

liangwen12year reviewed Dec 20, 2023

View reviewed changes

richm force-pushed the ansible-lint-test-2.16 branch from 7156bb8 to 921be00 Compare December 21, 2023 15:26

richm force-pushed the ansible-lint-test-2.16 branch from 921be00 to e034387 Compare December 23, 2023 20:52

liangwen12year approved these changes Dec 29, 2023

View reviewed changes

richm force-pushed the ansible-lint-test-2.16 branch from e034387 to 661f72f Compare January 6, 2024 00:35

richm merged commit e4d4997 into linux-system-roles:main Jan 6, 2024
19 checks passed

richm deleted the ansible-lint-test-2.16 branch January 6, 2024 00:36

liangwen12year mentioned this pull request Jan 11, 2024

ci: Use new ansible-lint action, which requires collection format and 2.16 #668

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Use supported ansible-lint action; run ansible-lint against the collection #661

ci: Use supported ansible-lint action; run ansible-lint against the collection #661

richm commented Dec 14, 2023 •

edited

Loading

codecov bot commented Dec 14, 2023 •

edited

Loading

richm commented Dec 14, 2023

liangwen12year Dec 16, 2023

richm Dec 18, 2023

liangwen12year Dec 18, 2023

richm Dec 18, 2023

liangwen12year Dec 19, 2023

richm Dec 20, 2023

liangwen12year commented Dec 19, 2023

richm commented Dec 20, 2023

richm commented Dec 20, 2023

liangwen12year Dec 20, 2023

richm Dec 20, 2023

richm Dec 20, 2023

liangwen12year Dec 20, 2023

richm commented Dec 21, 2023

richm commented Dec 23, 2023

richm commented Dec 25, 2023

richm commented Dec 27, 2023


		# pylint: enable=import-error, no-name-in-module

		provider.NetworkManagerProvider

ci: Use supported ansible-lint action; run ansible-lint against the collection #661

ci: Use supported ansible-lint action; run ansible-lint against the collection #661

Conversation

richm commented Dec 14, 2023 • edited Loading

codecov bot commented Dec 14, 2023 • edited Loading

Codecov Report

richm commented Dec 14, 2023

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

liangwen12year commented Dec 19, 2023

richm commented Dec 20, 2023

richm commented Dec 20, 2023

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 Dec 21, 2023

richm commented Dec 23, 2023

richm commented Dec 25, 2023

richm commented Dec 27, 2023

richm commented Dec 14, 2023 •

edited

Loading

codecov bot commented Dec 14, 2023 •

edited

Loading