Adding support for netmiko send_command task #17
Conversation
|
Closing...will re-submit. |
| } | ||
|
|
||
| if host is None and ip is None: | ||
| parameters["ip"] = task.host["brigade_ip"] |
There was a problem hiding this comment.
I am introducing properties in another branch to simplify dealing with the arguments. Hopefully this will make it easier and more consistent:
https://github.com/napalm-automation/brigade/pull/13/files#diff-3809cfaf6a7bf2bf113debc68df5541cR133
There was a problem hiding this comment.
Sounds good....will review.
| return parameters | ||
|
|
||
|
|
||
| def netmiko_run(task, method, ip=None, host=None, username=None, password=None, |
There was a problem hiding this comment.
I think we should simplify tasks and remove ip=None, host=None, username=None, password=None, device_type=None as those can come easily from the inventory. Or is there any reason why you think you'd want to override this information because it's easier than getting it from the inventory?
There was a problem hiding this comment.
My preference is to have both as it gives flexibility on how you run the functions...i.e. can be run potentially independently of inventory.
I don't care that much though.
There was a problem hiding this comment.
I am fine with having a mechanism to override parameters but then I'd suggest having a mechanism we can reuse everywhere instead of solving it on each task in an adhoc manner. For instance, we could accept a "connection_parameters" in all the tasks and have a helper function in the host itself that resolves the parameters.
There was a problem hiding this comment.
Yes, definitely open to figuring out some common pattern.
| def netmiko_send_command(task, ip=None, host=None, username=None, password=None, | ||
| device_type=None, netmiko_dict=None, use_textfsm=False, | ||
| cmd_args=None, cmd_kwargs=None): | ||
| parameters = netmiko_args(task=task, ip=ip, host=host, username=username, |
There was a problem hiding this comment.
If this is a second task, please, use different files for different tasks. Mostly because I haven't really come up with a proper solution for connection: remote tasks and for those it might be convenient to just scp the file and execute remotely.
There was a problem hiding this comment.
Why? I think brigade shouldn't care...i.e. don't impose artificial constraints at the Python level.
I probably don't care about remote tasks (and don't want to recreate Ansible and its associated remote task problems)...so I am viewing these as entirely local tasks.
There was a problem hiding this comment.
Yeah, I agree with you but some tasks might make sense to run them remotely. For instance, a task to manage rpms/deb packages. So basically enforcing one task per file would mostly be for consistency. It also helps identifying which tasks are untested by checking the coverage report but this is a minor thing.
But as you hinted already, this is an artificial constraint, doing it or not doesn't really change anything and we might still change our minds a few times in the next few weeks so what I have been doing is adding the tasks here:
That way we can keep changing our mind as users should be getting the tasks as:
from brigade.plugins.tasks import networking
networking.netmiko_run
In summary, it doesn't really matter, keep them in the same file for now if you want. Just make sure you add the tasks to the __init__ file so they can be imported at the package level in case we decide to move them around at some point.
|
No problem, just adding a few comments nonetheless so you have them in mind if you reopen the PR. |
No description provided.