iosxr_user.py - Fix provider and network_cli compatibility, Fix strin

[avisom]

…g and byte string comparsion (python3 compat), Fix "No such configuration item" handling when getting users from configuration

SUMMARY

Fixed issue when ssh key copying doesn't work with network_cli or netconf plugin, because provider argument always deleted in iosxr.py if using this plugins
Fixed python3 compatibility issue when string comparing with byte string
Fixed proper handling of "No such configuration item" when getting data for username section, without this module always try to delete user "No" when purging
Added some usefull info to warning message and improve its usage in PublicKeyManager class

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

iosxr_user

ADDITIONAL INFORMATION

Please backport this to 2.8 and 2.9 if it possible, because at this branches module functions related to ssh key copying actually doesn't work whether you setup provider or not, and also code doesn't work in python3 because of string and byte string comparsion

[avisom]

recheck

[pabelanger]

you need to run

tox -eblack

to properly format the code

[avisom]

you need to run

tox -eblack

to properly format the code

Thank you, did it

[avisom]

@pabelanger seems like all checks is ok, what i need to do next?

[pabelanger]

For iosxr, our jobs are non-voting so we need to manually check results. In this case, there looks to be an issue with your PR. Here is the following key error:

https://object-storage-ca-ymq-1.vexxhost.net/v1/a0b4156a37f9453eb4ec7db5422272df/ansible_15/15/202f65c40718d8372e1b138b6f7ad6d68740d9cb/check/ansible-test-network-integration-iosxr-python36/4162754/job-output.html#l99329

[avisom]

For iosxr, our jobs are non-voting so we need to manually check results. In this case, there looks to be an issue with your PR. Here is the following key error:

https://object-storage-ca-ymq-1.vexxhost.net/v1/a0b4156a37f9453eb4ec7db5422272df/ansible_15/15/202f65c40718d8372e1b138b6f7ad6d68740d9cb/check/ansible-test-network-integration-iosxr-python36/4162754/job-output.html#l99329

Unfortunately yes... It caused by deleting "iosxr_top_spec " in this commit

I can handle this situation in code, but the test
task "create user with private key (contents input)" still will not work because of "provider" argument deleting and this test, i think, couldn't work ever because of this

I think there is two options how we can fix this:
First - return iosxr_top_spec deleted by this commit and fix test for using directly provided arguments
Second - remove deleting of provider argument here

For me second option looks better, but i don't know for what reason provider argument need to be deleted

Just say me what to do, and I will do it as fast as i can :)

Thank you

[avisom]

@pabelanger seems like all tests is ok now, can we merge this request in its current state? If yes, i need to fix a little the warning message.

I excluded "provider" deletion for iosxr_user module (i think it is the only way to make it work in part of rsa key copying), and now it works as expected.

[NilashishC]

@avisom Thank you for this PR. You are absolutely correct, this is a bug that needs to be addressed.

However, we should not be relying on provider dict or top level spec to get the connection details. Top level spec has already been removed in 2.9 - https://github.com/ansible/ansible/pull/63680/files#diff-7f6de315f36397bf951157ea5328a090L93 and provider spec is deprecated and marked for removal in 2.14 - https://github.com/ansible/ansible/pull/66302/files#diff-7f6de315f36397bf951157ea5328a090R84.

You can instead import get_connection() from module_utils and use that to get connection parameters. For example -

conn = get_connection(module)
conn.get_option("remote_user")
conn.get_option("password")
conn.get_option("host")

For all the options you can refer to https://github.com/ansible-collections/ansible.netcommon/blob/master/plugins/connection/network_cli.py#L16.

Also, if you could add some integration tests to cover this scenario, that'd be great!

[avisom]

recheck

[Qalthos]

Using the existing copy_file path to do the heavy lifting gets a 👍 from me, but I would also like to see a new test that shows what is failing in 2.9 that is fixed here as I am not entirely following what is being reported

[avisom]

I think all issues described in PR are correctly handled at this moment.
All tests related to this functions seems to be working properly.

Two tests are failed constantly, one of it about iosxr_lag_interfaces, caused by fresh PR's,
and second one is test login, and strange thing about this play, that it is done twice during the tests, and only second try fails.

This commit was reverted and merged to master branch
There are still one question about useless "warning_message"...

@NilashishC @Qalthos waiting for your reply, thank you.

[avisom]

fixed

[avisom]

fixed

[avisom]

fixed

[avisom]

Can we merge this please or there are still some issues needs to be resolved?

[NilashishC]

@avisom can you please add an integration test for this module that covers this scenario?

[avisom]

fixed

[avisom]

fixed

[avisom]

@avisom can you please add an integration test for this module that covers this scenario?

Sorry, but i think i can't. It is hard to write tests without access to the test environment and hardware, at least for me.
And as i can see this module already have some tests covers some of this scenarios.

Can i do something else for this pull request to be merged? As i see there is some changes requested and i did that changes, but looks like i can't do anything with that changes request.

[rohitthakur2590]

@avisom can you please add an integration test for this module that covers this scenario?

Sorry, but i think i can't. It is hard to write tests without access to the test environment and hardware, at least for me.
And as i can see this module already have some tests covers some of this scenarios.

Can i do something else for this pull request to be merged? As i see there is some changes requested and i did that changes, but looks like i can't do anything with that changes request.

@avisom Could you please add the changelog fragment as that is mandatory to merge the PR.

[avisom]

recheck

[avisom]

@avisom can you please add an integration test for this module that covers this scenario?

Sorry, but i think i can't. It is hard to write tests without access to the test environment and hardware, at least for me.
And as i can see this module already have some tests covers some of this scenarios.
Can i do something else for this pull request to be merged? As i see there is some changes requested and i did that changes, but looks like i can't do anything with that changes request.

@avisom Could you please add the changelog fragment as that is mandatory to merge the PR.

@rohitthakur2590 Sorry for delay, added.

I hope we can finally merge this PR then, thank you!

[Andersson007]

@NilashishC @Qalthos @rohitthakur2590 can we merge this?

[trishnaguha]

@Andersson007 The conflict needs to be resolved.

[ansible-zuul]

LGTM!

[Andersson007]

@avisom thanks for the contribution!
@trishnaguha @NilashishC @Qalthos @pabelanger @rohitthakur2590 ansible-zuul thanks for reviewing and merging!:)