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

Adding a pair of proxy and execution modules for managing Arista switches via the pyeapi #48356

Merged
merged 6 commits into from Jul 20, 2018

Conversation

Projects
None yet
4 participants
@mirceaulinic
Copy link
Member

commented Jun 28, 2018

What does this PR do?

Adds the initial version of two new modules: one Proxy and one Execution module for managing Arista switches via pyeapi which is the official library from Arista, using the eAPI.

The execution module is flexible enough to execute the commands to the remote device without necessarily requiring to be running inside the pyeapi Proxy Minion. I named the modules arista_pyeapi, as I will provide a set of modules for managing Arista switches without having the pyeapi external dependency without necessarily having feature parity with arista_pyeapi (as that would imply rewriting a good part of the pyeapi library into Salt).

@cachedout cachedout added the Awesome label Jul 1, 2018

fun_kwargs = {}
pyeapi_kwargs = __salt__['config.get']('pyeapi', {})
pyeapi_kwargs.update(kwargs) # merge the CLI args with the opts/pillar
for karg, warg in six.iteritems(pyeapi_kwargs):

This comment has been minimized.

Copy link
@cachedout

cachedout Jul 1, 2018

Collaborator

We have nearly identical code over in netmiko_mod. Refactor for DRY?

This comment has been minimized.

Copy link
@mirceaulinic

mirceaulinic Jul 6, 2018

Author Member

Apologies for late reply @cachedout. Yes, there's a little overlap between them. We could have the following function in the utils:

def prepare_kwargs(all_kwargs, class_init_kwargs):
    for karg, warg in six.iteritems(all_kwargs):
        if karg not in class_init_kwargs:
            if warg is not None:
                fun_kwargs[karg] = warg
            continue
        if warg is not None:
            init_args[karg] = warg
    return init_args, fun_kwargs

Then the code in this execution module would become:

def _prepare_connection(**kwargs):
    '''
    Prepare the connection with the remote network device, and clean up the key
    value pairs, removing the args used for the connection init.
    '''
    init_args = {}
    fun_kwargs = {}
    pyeapi_kwargs = __salt__['config.get']('pyeapi', {})
    pyeapi_kwargs.update(kwargs)  # merge the CLI args with the opts/pillar
    init_args, fun_kwargs = salt.utils.args.prepare_kwargs(pyeapi_kwargs, PYEAPI_INIT_KWARGS)
    if 'transport' not in init_args:
        init_args['transport'] = 'https'
    conn = pyeapi.client.connect(**init_args)
    node = pyeapi.client.Node(conn, enablepwd=init_args.get('enablepwd'))
    return node, fun_kwargs

I'm not disagreeing, just that I'm not sure if it's really worth it, particularly as it's harder to port a module in an older Salt version due to the utils dependency.
On the other hand, I have a couple of more modules pending in the queued to be submitted, that would have the same code as well.

What do you think would be the best? Thanks!

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Jul 9, 2018

Contributor

If you use __utils__['args.prepare_kwargs'] they should just have to drop the newer args module into salt://_utils/ and sync them and it should be easily portable.

This comment has been minimized.

Copy link
@mirceaulinic

mirceaulinic Jul 9, 2018

Author Member

Fantastic, I will do it like that then - will refactor and push tomorrow morning. Thanks guys!

# -----------------------------------------------------------------------------


def get_connection(**kwargs):

This comment has been minimized.

Copy link
@cachedout

cachedout Jul 1, 2018

Collaborator

Same here. Code is in the netmiko module. Refactor for DRY?

@mirceaulinic mirceaulinic force-pushed the mirceaulinic:pyeapi-modules branch from 3d73ef6 to dddef49 Jul 11, 2018

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2018

Hi @cachedout - I have pushed dddef49 to remove the redundant code. I will follow up with another PR for Netmiko to use the new args.prepare_kwargs util function.

CC. @gtmanfred @rallytime

@mirceaulinic mirceaulinic force-pushed the mirceaulinic:pyeapi-modules branch from dddef49 to 8d81709 Jul 11, 2018

@rallytime
Copy link
Contributor

left a comment

I have two small changes that are needed.

Minion. If you want to use the :mod:`pyeapi Proxy <salt.proxy.arista_pyeapi>`,
please follow the documentation notes for a proper setup.
'''
from __future__ import absolute_import

This comment has been minimized.

Copy link
@rallytime

rallytime Jul 16, 2018

Contributor

We also need to import print_function and unicode_literals here.

username: example
password: example
'''
from __future__ import absolute_import

This comment has been minimized.

Copy link
@rallytime

rallytime Jul 16, 2018

Contributor

Same comment here.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2018

I'm good here now. Thanks, @mirceaulinic. If you can get the changes suggested by @rallytime in then I think we're good to go here.

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

Okay, added 2b61f5d to solve the imports.

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2018

Bump @cachedout @rallytime - apologies if I'm pushy, but I'd love to have this in so I can follow up with the next PR which depends on this (of course, if you are happy with the code), so I can finish before the 6th of Aug. Thanks!

@rallytime rallytime merged commit 3baa318 into saltstack:develop Jul 20, 2018

4 of 9 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
codeclimate 9 issues to fix
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

@mirceaulinic It's merged now. I was off yesterday so I hadn't see then notification yet. :)

Thanks for working on all of this for the feature release! We appreciate it, as always. :)

One thing - I know you've been adding a bunch of new modules - can you make sure you get those listed in the Fluorine release notes?

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2018

Yes, thanks for reminder @rallytime. I will surely add them to the release notes too!

@mirceaulinic mirceaulinic deleted the mirceaulinic:pyeapi-modules branch Jul 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.