Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite nxos_file_copy as an action plugin #60643

Merged
merged 16 commits into from Aug 29, 2019

Conversation

@mikewiebe
Copy link
Contributor

commented Aug 15, 2019

SUMMARY

This PR rewrites the nxos_file_copy module as an action plugin. This was suggested by @Qalthos to resolve the following deprecation warnings generated by the module.

[DEPRECATION WARNING]: Param 'username' is deprecated. See the module docs for more information. This feature will be removed in version 2.9. Deprecation warnings can be disabled by setting 
deprecation_warnings=False in ansible.cfg.
[DEPRECATION WARNING]: Param 'host' is deprecated. See the module docs for more information. This feature will be removed in version 2.9. Deprecation warnings can be disabled by setting 
deprecation_warnings=False in ansible.cfg.
[DEPRECATION WARNING]: Param 'password' is deprecated. See the module docs for more information. This feature will be removed in version 2.9. Deprecation warnings can be disabled by setting 
deprecation_warnings=False in ansible.cfg.

In addition to rewriting this as a plugin, the following is also included in this PR

  • Better input validation testing.
  • Refactoring of the code to improve reliability for various nxos file copy scenarios when initiating the copy from the nxos device.
  • Addition of a module option to compact nxos image files when they are copied to the device.
  • Addition of a module option to copy using kstack when file copies are initiated from the device increase copy speeds if the image supports it.
ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request
COMPONENT NAME

nxos_file_copy

ADDITIONAL INFORMATION

Here is background information from my chat with @Qalthos on this topic.

12:27] <mikewiebe> That is already being done here: https://github.com/ansible/ansible/blob/e433feaecc3792a74854eedb88e117f0cfa4be49/lib/ansible/plugins/action/nxos.py#L49
[12:27] <mikewiebe> The problem is that the _task_args are host, password and username which are being deprecated
[12:28] <mikewiebe> What I show here is the change to use nxos_host, nxos_password, nxos_username instead
[12:28] <mikewiebe> But, I don't want to expose these as user parameters in the module task.  
[12:34] <@Qalthos> You're setting task args in the action plugin yes, but that's all you're doing
[12:34] <@Qalthos> Consider the net_put module https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/files/net_put.py
[12:35] <@Qalthos> The only contents of that file are documentation strings, because everything it does is in the action plugin
[12:37] <@Qalthos> This isn't really a recommended path for a lot of modules, but as you're not really using the connection plugin anyway, you might as well do that sort of thing in its own custom action plugin
[12:37] <@Qalthos> (like https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/action/net_put.py)
[12:39] <@Qalthos> net_put has the benefit of only running on network_cli, and can therefore get away with connection.copy_file(), but you should be able to do the same sort of thing you're doing now in your module, I think
[12:43] <@Qalthos> That said, this is not going to make 2.8, so we don't have to do something literally today, and I know trishnag is aware of the issue
[12:46] <@Qalthos> mikewiebe: Does that make sense? I don't know if maybe you discarded this as an option before or if I'm overlooking something
[12:48] == made2591 [~made2591@dslb-002-205-196-164.002.205.pools.vodafone-ip.de] has joined #ansible-network
[12:48] <mikewiebe> Sorry!  Got pulled away for a few minutes
[12:49] <mikewiebe> Thanks for the pointers.  I will look at those more closely and see if I can leverage that for this case and will add more questions in the PR if needed

@mikewiebe mikewiebe changed the title Rewrite nxos_file_copy as an action plugin [WIP] Rewrite nxos_file_copy as an action plugin Aug 15, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/plugins/action/nxos_file_copy.py:88:79: undefined-variable Undefined variable 'unicode'

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

test/sanity/ignore.txt:4610:1: Ignoring 'lib/ansible/modules/network/nxos/nxos_file_copy.py' is unnecessary

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

test/sanity/ignore.txt:4611:1: Ignoring 'lib/ansible/modules/network/nxos/nxos_file_copy.py' is unnecessary

The test ansible-test sanity --test shebang [explain] failed with 1 error:

lib/ansible/plugins/action/nxos_file_copy.py:0:0: should not have a shebang

The test ansible-test sanity --test validate-modules [explain] failed with 4 errors:

test/sanity/ignore.txt:4612:1: A100 Ignoring 'E324' on 'lib/ansible/modules/network/nxos/nxos_file_copy.py' is unnecessary
test/sanity/ignore.txt:4613:1: A100 Ignoring 'E327' on 'lib/ansible/modules/network/nxos/nxos_file_copy.py' is unnecessary
test/sanity/ignore.txt:4614:1: A100 Ignoring 'E337' on 'lib/ansible/modules/network/nxos/nxos_file_copy.py' is unnecessary
test/sanity/ignore.txt:4615:1: A100 Ignoring 'E338' on 'lib/ansible/modules/network/nxos/nxos_file_copy.py' is unnecessary

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/plugins/action/nxos_file_copy.py:88:79: undefined-variable Undefined variable 'unicode'
lib/ansible/plugins/action/nxos_file_copy.py:266:0: anomalous-backslash-in-string Anomalous backslash in string: '\s'. String constant might be missing an r prefix.

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

test/sanity/ignore.txt:4610:1: Ignoring 'lib/ansible/modules/network/nxos/nxos_file_copy.py' is unnecessary

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

test/sanity/ignore.txt:4611:1: Ignoring 'lib/ansible/modules/network/nxos/nxos_file_copy.py' is unnecessary

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/plugins/action/nxos_file_copy.py:266:59: W605 invalid escape sequence '\s'

The test ansible-test sanity --test shebang [explain] failed with 1 error:

lib/ansible/plugins/action/nxos_file_copy.py:0:0: should not have a shebang

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/plugins/action/nxos_file_copy.py:90:79: undefined-variable Undefined variable 'unicode'
lib/ansible/plugins/action/nxos_file_copy.py:293:0: anomalous-backslash-in-string Anomalous backslash in string: '\s'. String constant might be missing an r prefix.

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

test/sanity/ignore.txt:4575:1: Ignoring 'lib/ansible/modules/network/nxos/nxos_file_copy.py' is unnecessary

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

test/sanity/ignore.txt:4576:1: Ignoring 'lib/ansible/modules/network/nxos/nxos_file_copy.py' is unnecessary

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/plugins/action/nxos_file_copy.py:293:36: W605 invalid escape sequence '\s'

The test ansible-test sanity --test shebang [explain] failed with 1 error:

lib/ansible/plugins/action/nxos_file_copy.py:0:0: should not have a shebang

The test ansible-test sanity --test validate-modules [explain] failed with 8 errors:

lib/ansible/modules/network/nxos/nxos_file_copy.py:0:0: E319 RETURN.changed": expected a dictionary for dictionary value @ data['changed"']. Got True
lib/ansible/modules/network/nxos/nxos_file_copy.py:0:0: E319 RETURN.changed: expected a dictionary for dictionary value @ data['changed']. Got True
lib/ansible/modules/network/nxos/nxos_file_copy.py:0:0: E319 RETURN.copy_cmd: expected a dictionary for dictionary value @ data['copy_cmd']. Got 'copy scp://username@fileserver.example.com/images/nxos.9.2.1.bin bootflash:/nxos.9.2.1.bin vrf management'
lib/ansible/modules/network/nxos/nxos_file_copy.py:0:0: E319 RETURN.file_system: expected a dictionary for dictionary value @ data['file_system']. Got 'bootflash:'
lib/ansible/modules/network/nxos/nxos_file_copy.py:0:0: E319 RETURN.local_file: expected a dictionary for dictionary value @ data['local_file']. Got './network-integration.cfg'
lib/ansible/modules/network/nxos/nxos_file_copy.py:0:0: E319 RETURN.remote_file: expected a dictionary for dictionary value @ data['remote_file']. Got 'network-integration.cfg'
lib/ansible/modules/network/nxos/nxos_file_copy.py:0:0: E319 RETURN.remote_scp_server: expected a dictionary for dictionary value @ data['remote_scp_server']. Got 'fileserver.example.com'
lib/ansible/modules/network/nxos/nxos_file_copy.py:0:0: E319 RETURN.transfer_status: expected a dictionary for dictionary value @ data['transfer_status']. Got 'Sent: File copied to remote device.'

The test ansible-test sanity --test yamllint [explain] failed with 5 errors:

lib/ansible/modules/network/nxos/nxos_file_copy.py:97:3: key-duplicates DOCUMENTATION: duplication of key "file_pull_compact" in mapping
lib/ansible/modules/network/nxos/nxos_file_copy.py:185:3: key-duplicates RETURN: duplication of key "file_system" in mapping
lib/ansible/modules/network/nxos/nxos_file_copy.py:186:3: key-duplicates RETURN: duplication of key "local_file" in mapping
lib/ansible/modules/network/nxos/nxos_file_copy.py:187:3: key-duplicates RETURN: duplication of key "remote_file" in mapping
lib/ansible/modules/network/nxos/nxos_file_copy.py:188:3: key-duplicates RETURN: duplication of key "transfer_status" in mapping

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/plugins/action/nxos_file_copy.py:89:45: undefined-variable Undefined variable 'basestring'

The test ansible-test sanity --test no-basestring [explain] failed with 1 error:

lib/ansible/plugins/action/nxos_file_copy.py:89:20: do not use `isinstance(s, basestring)`

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/network/nxos/nxos_file_copy.py:169:29: E313 RETURN is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/network/nxos/nxos_file_copy.py:169:29: error RETURN: syntax error: mapping values are not allowed here

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

The test ansible-test sanity --test use-compat-six [explain] failed with 1 error:

lib/ansible/plugins/action/nxos_file_copy.py:28:1: use `ansible.module_utils.six` instead of `six`

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/plugins/action/nxos_file_copy.py:310:0: anomalous-backslash-in-string Anomalous backslash in string: '\/'. String constant might be missing an r prefix.

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/plugins/action/nxos_file_copy.py:310:49: W605 invalid escape sequence '\/'

click here for bot help

@mikewiebe mikewiebe changed the title [WIP] Rewrite nxos_file_copy as an action plugin Rewrite nxos_file_copy as an action plugin Aug 20, 2019

@mikewiebe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

@trishnaguha and @chrisvanheuveln Ready for review.

@mikewiebe mikewiebe force-pushed the mikewiebe-ansible:rel290/nxos_file_copy_action branch from 8d29036 to 6e2447b Aug 29, 2019

@abadger

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

rebuild_merge

@ansibot ansibot removed the needs_rebase label Aug 29, 2019

mikewiebe added 2 commits Aug 29, 2019
@trishnaguha

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

@abadger LGTM from my side. Would you mind to merge this?
Thanks!

@abadger

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

@trishnaguha it hasn't passed shippable yet. There's currently a shippable outage. i'm waiting for that to resolve so I can requeue this to make sure it doesn't cause any failures. (Failures would block today's release).

@ansibot ansibot added shipit and removed core_review labels Aug 29, 2019

@abadger abadger closed this Aug 29, 2019

@abadger abadger reopened this Aug 29, 2019

@abadger

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

I've retriggered shippable now that their outage is over. Let's see if this is passing now.

@mikewiebe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

@abadger After I rebased to the latest devel I discovered that a commit impacted the net_put action plugin and also this plugin. Likely others as well that are using network_cli. Details can be found here but this needs to be fixed for release 2.9

#61568

@abadger

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Okay, talked with bthornto and he cleared this for merge now. I'll see if the fix for #61568 passes in shippable if so, I'll merge it for beta1. If not, it will be a candidate for merging before rc1.

@abadger abadger merged commit 02572cb into ansible:devel Aug 29, 2019

1 check passed

Shippable Run 141057 status is SUCCESS.
Details
@abadger

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Merged for 2.9.0beta1

adharshsrivatsr added a commit to adharshsrivatsr/ansible that referenced this pull request Sep 3, 2019
Rewrite nxos_file_copy as an action plugin (ansible#60643)
* Initial nxos_file_copy action plugin work
* Remove code from nxos_file_copy module
* Add file_push and file_pull support
* Additional refactoring and shipable updates
* Simplify outcomes and update doc header
* Add more error data information for easier debugging
* Reorder outcomes and add additional tests
* Capture more data for permission denied outcome
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.