aci_aaa_user: Manage AAA users on ACI fabrics

[dagwieers]

SUMMARY

Manage AAA users on ACI fabrics.

ISSUE TYPE

• New Module Pull Request

COMPONENT NAME

aci_aaa_user

ANSIBLE VERSION

v2.5

[ansibot]

cc @brunocalogero @jmcgill298 @schunduri
click here for bot help

[mattclay]

The import sanity test is checking for imports from outside the python standard library which are not in a try/except ImportError block. Take a look at how existing modules handle that:

ansible/lib/ansible/modules/cloud/amazon/s3_sync.py

Lines 219 to 223 in 20174f1

	try:
	from dateutil import tz
	HAS_DATEUTIL = True
	except ImportError:
	HAS_DATEUTIL = False

[mattclay]

Sorry about the close/reopen. I hit the wrong button.

[dagwieers]

@mattclay I knew I still had to do HAS_DATEUTIL, but I wasn't expecting that to be related to the output you get when you don't. How would anyone else know they need a try-except block to avoid the missing libraries ? Maybe we should catch this and report this better ?

[mattclay]

@dagwieers When ansibot posts comments for sanity test failures, those messages include a help link "[?]" which provides documentation on the related test. Here's an example: #35573 (comment)

Now some of the referenced docs are a bit sparse, so PRs to improve them are welcome, as are any suggestions on how to better link to those docs. Here are the docs for the import sanity test:

https://docs.ansible.com/ansible/devel/dev_guide/testing/sanity/import.html

[dagwieers]

@mattclay Hmmm, I never expected the link [?] to provided detailed information related to the failing task, most likely because the link name doesn't really differentiate for different test-failures.

I would suggest to name the link related to the performed test so that it is more clear. But for all we know, I am the only one who didn't realize this...

[mattclay]

@dagwieers The "[?]" is specific to each sanity test. For cases where the test has a small number (possibly only one) type of message, the help page can be quite specific to the failure. For others, like pylint, there are a large number of possible messages, so the help page is more generic.

Could you provide an example of what you would find to be a more intuitive way to link to the help? I had a few goals in mind when I created the original message formatting:

1. Keep the test description to a single line, so most of the focus could be on the messages.
2. Show the command required to execute the test locally.

Of course this didn't leave much room for a help link, which is why we ended up with just "[?]". I considered making the name of the test in the example command a link, but that seemed to have formatting problems, and didn't work as-is if someone tried to copy and paste it into a terminal.

Perhaps it would be helpful to add a brief explanation at the bottom of the comment, just before the bot help link, that states additional help is available for each test by clicking on the "[?]" link?

[dagwieers]

IIRC the sanity tests have a unique number, so why not simply use that for the name [E211] Obviously the number does not really have a lot of meaning, but it does give a hint that the link is unique to the error shown. I am confident this gives a bit more clue that it's just not a generic help, like the ansibot help page.

Bonus points if the error number is also reflected in the link/title of the page, to demonstrate this uniqueness and 1:1 relationship with the error message.

[mattclay]

The help pages are specific to each sanity test, not the individual messages generated by a test. A sanity test like import doesn't really have specific messages, it just reports any exceptions it encounters. Of course the cause is always some variation of not handling import failures, so the help page for the import sanity test explains that.

When it comes to help pages like pep8, pylint, etc., we can explain about the tool being used on the help page, but there's not much benefit to repeating documentation for that tool on the page, nor can we easily link to help specific to each code/message, but instead just a general link for the test.

For example, here's a typical pep8 error block -- it has one help link, but two different errors:

The test ansible-test sanity --test pep8 [?] failed with the following errors:

contrib/inventory/foreman.py:56:1: E302 expected 2 blank lines, found 1
contrib/inventory/foreman.py:74:39: E261 at least two spaces before inline comment

That link goes to the page on the pep8 sanity test, but isn't specific to either E302 or E261.

[dagwieers]

Maybe then use [explain] or [import] or [pep8] something more descriptive.

[mattclay]

I like "[explain]" -- so how about something like this?

The test ansible-test sanity --test pep8 [explain] failed:

contrib/inventory/foreman.py:56:1: E302 expected 2 blank lines, found 1
contrib/inventory/foreman.py:74:39: E261 at least two spaces before inline comment

[dagwieers]

@mattclay I think that's better. It would have prevented me to ask you, so let's hope it works for others too.

But granted in this case I would never have expected that a try-except block was needed to shut the tests up, it didn't seem related to tests failing, but rather the module not performing correctly in the normal test-environment. In most cases the error message gives sufficient hints to what the solution needs to be.

[mattclay]

I suppose specifically for the import sanity test we could add something like " -- missing try/except ImportError?" to the end of each message... but that isn't always correct. For example, it could be due to a syntax error or something else.

[mattclay]

Or maybe some tests, such as import, could use an extra hint after the errors, so something like:

The test ansible-test sanity --test import [explain] failed:

import errors shown here

Are you missing a try/except ImportError block for imports not in the python standard library?