napalm-automation / napalm-ansible

Apache License 2.0
245 stars 103 forks source link

Targuan feature/action plugins #113

Closed ktbyers closed 6 years ago

ktbyers commented 6 years ago

Testing

ktbyers commented 6 years ago

Tests are failing on:

sshpass -p vagrant ansible-playbook -i napalm_connection/hosts napalm_connection/connection_info_in_args.yaml -u vagrant -k

with sshpass command not found in the Travis-CI environment.

ktbyers commented 6 years ago

@ogenstad Did you have some comments on this? @dbarrosop mentioned that you did, but I couldn't see where they were?

ogenstad commented 6 years ago

I setup an example of what I was thinking in #114. I haven't really done anything there aside from change locations and rename the files which were named .yml instead of .py. I think I dropped a comment about this on an issue, but probably most of it is lost in some Slack archive.

The downside of this change is that it would require that users change their "library" setting in ansible.cfg as a one off change during this upgrade.

Also I don't think we should ship a package called action_plugins as it was implemented, i.e. everything should be under napalm_ansible.

If the package would be changed in this way a next step could be to shave off common code between the modules and store that elsewhere within the package. This would force people to actually install the package with pip as opposed to just cloning the git repo. I think it makes much more sense to do it this way anyway, and I don't see a good way to include the action plugins in the package without this change.

The issue with sshpass looks like something old, could perhaps be fixed with a rebase.

itdependsnetworks commented 6 years ago

@ktbyers mentioned that it may be possible to just install these in the ansible default locations.

which would mean no change from a user's ansible.cfg file. Which to me would be a huge benefit.

ktbyers commented 6 years ago

Yes, one possibility would be to install things here (would need to check versions of Ansible this is supported on).

filter_plugins = ~/.ansible/plugins/filter_plugins/
action_plugins = ~/.ansible/plugins/action_plugins/       
library = ~/.ansible/plugins/modules

Potential Issues:

  1. Doesn't work with virtual environments (current solution doesn't really work with virtual environments either since we have to update .ansible.cfg, but this is probably worse with virtual env).
  2. Issue if someone overwrites the library in .ansible.cfg (and potentially migration issues from what we currently have).

Advantages:

  1. We don't have to update the .ansible.cfg twice.
  2. One standard location where we install things.
ogenstad commented 6 years ago

An alternative to ansible.cfg would be to use environment variables. The napalm-ansible cli tool could also describe how to set those.

dbarrosop commented 6 years ago

filter_plugins = ~/.ansible/plugins/filter_plugins/

I don't like it. Has someone actually talked with ansible about this issue. Python supports already discovering plugins so I am not sure why they don't want to implement it:

https://packaging.python.org/guides/creating-and-discovering-plugins/

ktbyers commented 6 years ago

@dbarrosop @ogenstad @itdependsnetworks @targuan I think we should move forward with what @ogenstad implemented (i.e. install into site-packages but just updated to include the action plugin) and control this via .ansible.cfg.

I have made some review comments in what he submitted.

I don't think it is worthwhile in trying to circle back to Ansible to try to get them to change .ansible. It would take a long-time and would just bog down getting napalm-ansible updated to support this.

I don't have a big preference on using ~/.ansible versus using site-packages for the virtual environment and updating .ansible.cfg. There are trade-offs with either.

itdependsnetworks commented 6 years ago

@ktbyers I tend to agree with you, as much as I don't like having to update 3 places in ansible.cfg file, it's probably the best solution for now.

I'll make a proposal within ansible for better distribution of non-core modules, plugins, etc...

dbarrosop commented 6 years ago

Fine by me.

ktbyers commented 6 years ago

Closing so that we work on the newer #114 PR