consolidate inventory attributes #224

dbarrosop · 2018-08-05T22:14:32Z

This PR:

consolidates inventory attributes as discussed in Consolidate inventory attributes #202
brings consistency in the way overrides are handled
adds upgrading notes
includes a tutorial in the form of a jupyter playbook that explains how to manage connections and their parameters
enables tests for jupyter playbooks

coveralls · 2018-08-05T22:19:06Z

Pull Request Test Coverage Report for Build 896

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 93.123%

Totals
Change from base Build 871:	0.0%
Covered Lines:	1178
Relevant Lines:	1265

💛 - Coveralls

dmfigol

Other than host everything looks good to me.

dmfigol · 2018-08-08T01:07:37Z

nornir/core/inventory.py

+    def hostname(self):
+        """String used to connect to the device. Either ``hostname`` or ``self.name``"""
+        return self.get("hostname", self.name)
+


I think we agreed on host, no?

I don't want to use host because host usually refers to the Host object so it's confusing, ie, task.host.host vs task.host.hostname. I am also fine naming it something else, I'd just avoid host.

I think host makes sense in the way that it can refer both to an IP address and a hostname. Though I agree that task.host.host looks a bit strange.

I am also fine with naming it something else, but what? :) I have no other ideas.
The reason I don't like hostname is because it can be also an IP address. IP address is not a hostname.
The reason I prefer host is not because I like it, but because it is used by majority of libraries:
netmiko uses host and ip
pyvmomi uses host
ncclient uses host
napalm uses hostname
django uses ALLOWED_HOSTS

While task.host.host does not look very beautiful, I would still prefer this.

I am fine with address, ip, connection_address, mgmt_ip, quantum_connection, others but I am going to veto host for consistency purposes :D

I vote for address

I vote host or hostname. I pretty strongly dislike address.

I am fine with hostname. I would say we just leave it as that and move on.

ktbyers · 2018-08-08T02:30:29Z

nornir/core/inventory.py

-        nos: Optional[str] = None,
-        connection_options: Optional[Dict[str, Any]] = None,
+        port: Optional[int] = None,
+        platform: Optional[int] = None,


Type hint should be Optional[str] not int

Correct, the problems of copy&paste.

Still an issue with the latest commit. Should be platform: Optional[str]

ktbyers · 2018-08-08T02:32:44Z

nornir/core/inventory.py

-        connection_options: Optional[Dict[str, Any]] = None,
+        port: Optional[int] = None,
+        platform: Optional[int] = None,
+        connection_options: Optional[int] = None,


Should be Optional[Dict[str, Dict]] ?

Correct again.

ktbyers · 2018-08-08T02:43:56Z

nornir/plugins/connections/napalm.py

-        network_api_port: int,
-        operating_system: str,
-        nos: str,
+        port: int,


Couldn't port also be passed by Nornir Core as None? This is just from a Python type checking perspective.

And again :D

ktbyers · 2018-08-08T04:05:58Z

nornir/plugins/connections/netmiko.py

-        network_api_port: int,
-        operating_system: str,
-        nos: str,
+        port: int,


Same item of whether port can be None.

There were a couple of other places with respect to Port where I had this same concern.

Will try to dig them out.

dmfigol · 2018-08-08T12:28:37Z

nornir/core/inventory.py

+    def get_connection_parameters(
+        self, connection: Optional[str] = None
+    ) -> Dict[str, Any]:
+        if not connection:


I was thinking about this and I have mixed feelings. My concern is that it is a little bit too much magic which can be not so intuitive, but I also understand the reasoning behind including this.
What I think could be less magic:
To form a dictionary with default Host attributes hostname, port, username, password, platform and then just update with connection_options. connection_options would always be provided in a format understandable by the backend library (for the plugins we are shipping with nornir, user-defined plugins can do anything). For example, if the backend library uses pass instead of password, if you want to override Host password, in connection_options you should define pass. Then to pass one resulting dictionary to the plugin and let it form its own args. In the connection plugin it would be something like:
conn_params['pass'] = options.get('pass', options.get('password'))
This looks a little ugly, but it feels a little more explicit.

I don't know what is best here, 50/50.

conn_params['pass'] = options.get('pass', options.get('password'))

The problems I see with that are:

It's going to bring inconsistent behaviors between drivers as everybody has to remember to implement exactly the same behavior.

It's going to confuse users as in some plugins "password" works and in others they have to use "pass".

That means I can't override the connection with code. I am talking about the possibility of calling Host.open_connection("netmiko", my_user, my_pass,...) directly. Even though I am being explicit the plugin might end up not using what I passed and revert to the inventory options.

To form a dictionary with default Host attributes hostname, port, username, password, platform and then just update with connection_options

Isn't that exactly what this is doing? :D

1.It's going to bring inconsistent behaviors between drivers as everybody has to remember to implement exactly the same behavior.

If you need to override connection options, you should always refer to the backend library docs. Users would need to manually install the library already.

2.It's going to confuse users as in some plugins "password" works and in others they have to use "pass".

Partially, override should be used only with the library docs.
If I use ncclient and currently my platform is set to ios, but I need to pass cisco_iosxe, I would want to put it in the ncclient format:

sw1: ... platform: ios ncclient_options: device_params: name: cisco_iosxe

If I override netmiko, I would put:

sw1: ... platform: ios netmiko_serial_options: device_type: terminal_server

Doesn't it make sense?

That means I can't override the connection with code. I am talking about the possibility of calling Host.open_connection("netmiko", my_user, my_pass,...) directly.

Override by providing a new connection_options dict formatted how the library expects it.

Isn't that exactly what this is doing? :D
Almost...

See my alternative proposals below

I think @dbarrosop behavior is the right behavior. I initially had reservations about his solution (last night), but I thought about it more and thought this was the right behavior.

The point that convinced me was his item #1 above (i.e. nornir-core needs to enforce consistent behavior that $connection_options take precedence over top-level arguments).

dmfigol · 2018-08-08T16:18:22Z

My alternative proposals:
1.

    def get_connection(self, connection: str) -> Any:
        if self.nornir:
            config = self.nornir.config
        else:
            config = None
        if connection not in self.connections:
            self.open_connection(
                connection,
                self.get_connection_parameters(connection),
                configuration=config,
            )
        return self.connections[connection].connection


    def open_connection(
        self,
        connection: str,
        connection_options: Optional[int] = None,
        configuration: Optional[Config] = None,
    ) -> None:
        if connection in self.connections:
            raise ConnectionAlreadyOpen(connection)

        self.connections[connection] = self.nornir.get_connection_type(connection)()
        conn_params = self.get_connection_parameters(connection)
        self.connections[connection].open(options=connection_options, configuration=configuration)
        return self.connections[connection]


    def get_connection_parameters(
        self, connection: Optional[str] = None
    ) -> Dict[str, Any]:
        params = {
            "hostname": self.hostname,
            "port": self.port,
            "username": self.username,
            "password": self.password,
            "platform": self.platform,
        }
        if connection and connection in self.get('connection_options', {}):
            params.update(self.get('connection_options')[connection])
        return params

Connection plugin would take the dictionary and extract relevant parts.

I think it is better than what I proposed above:

    def get_connection(self, connection: str) -> Any:
        if self.nornir:
            config = self.nornir.config
        else:
            config = None
        if connection not in self.connections:
            self.open_connection(
                connection,
                self.get('connection_options', {}).get(connection),
                configuration=config,
            )
        return self.connections[connection].connection


    def open_connection(
        self,
        connection: str,
        connection_options: Optional[Dict[str, Any]] = None,
        configuration: Optional[Config] = None,
    ) -> None:
        if connection in self.connections:
            raise ConnectionAlreadyOpen(connection)

        self.connections[connection] = self.nornir.get_connection_type(connection)()
        self.connections[connection].open(host=self, connection_options=connection_options, configuration=configuration)
        return self.connections[connection]

then plugin would do:

    class Netconf(ConnectionPlugin):
        def open(
            self,
            host: Host,
            connection_options: Optional[Dict[str, Any]] = None,
            configuration: Optional[Config] = None,
        ) -> None:
            params = {
                'host': host.hostname,
                'username': host.username,
                'pass': host.password,  # ncclient uses password, but I've put "pass" for example
                'device_params': {'name': host.platform}
            }
            if host.port:
                params["port"] = host.port
            if connection_options:
                params.update(connection_options)

            self.connection = manager.connect(**params)

… avoid confusions

dbarrosop · 2018-08-09T11:27:53Z

As discussed already, the plugins' only purpose is to connect to the devices, not to implement any logic to resolve the arguments correctly, that's the mission of nornir-core, hence, plugins should already receive correct parameters. Otherwise we end up duplicating code and bringing inconsistencies. I have also renamed connection_options to "advanced_options" as I think people were confused about the purpose of it and for good reasons. This commit should illustrate how this works:

4825e23

dummy_no_overrides gets all the host parameters
dummy gets the host parameters with the overrides specified under connection_options -> dummy in the inventory.

ktbyers · 2018-08-09T15:14:15Z

Yes, I disagree with this:

As discussed already, the plugins' only purpose is to connect to the devices, 
not to implement any logic to resolve the arguments correctly, that's the
mission of nornir-core, hence, plugins should already receive correct 
parameters.

I think correctly connecting to devices will imply plugins have logic.

For example, I think we will still need logic in the napalm-plugin to resolve the nxos_ssh issue. We will need logic to resolve naming differences like pass versus password in the NETCONF case.

I agree with your implementation, but I think Nornir-core's job is to provide a consistent set of arguments to plugins and also to ensure $connection_options have precedence. After that it is the plugins responsibility to have any logic needed to properly make the connection (from those arguments Nornir-core provides).

dmfigol · 2018-08-09T23:25:08Z

@ktbyers
I agree that it is actually plugin's job to format the provided arguments in the way underlying library understands them (_process_args this we discussed in your PR/issue)

Question for everyone. In case of discrepancies between the nornir naming and libraries, like this:

hostname vs host vs address
password vs pass vs pwd
platform vs device_type vs device_params

etc.

How would you specify the override in the connection options?

by using nornir naming
by using library naming

ktbyers · 2018-08-10T02:32:18Z

nornir/core/inventory.py

+                "username": conn_params.get("username", self.username),
+                "password": conn_params.get("password", self.password),
+                "platform": conn_params.get("platform", self.platform),
+                "advanced_options": conn_params.get("advanced_options", {}),


Okay, I think I misunderstood what you were doing here and the name change highlighted that? What is advanced_options, and how does it relate to what we had specified in #202?

ktbyers · 2018-08-10T03:19:11Z

I misunderstood a significant aspect of David's original PR which his renaming of connection_options to advanced_options has highlighted.

Now I think this is different, or more likely there was miscommunication, from what we discussed in #202.

I thought in #202 we agreed to the following (updated to reflect current inventory names)?

dev1.group_1:
    hostname: foo.whatever.com    # TBD on hostname versus host versus address
    username: my_user
    password: my_password
    platform: eos
    site: bma
    role: spine
    groups:
        - group_1
    connection_options:
        napalm:
            platform: eos
            port: 443
            optional_args:
                blah: bleh
        netmiko:
            device_type: arista_eos
            port: 443
        remote_console:
            plugin: netmiko
            hostname: my_terminal_server.acme
            username: blah
            password: whatevs
            port: 7787
            device_type: terminal_server

I was expecting the following arguments would be passed to napalm plugin:

hostname
username
password
port
platform
connection_options

Where connection options is the following dictionary unmodified (i.e. whatever was specified in connection_options['napalm'] would be passed as-is to the plugin.

platform: eos
port: 443
optional_args:
       blah: bleh

The same would apply for Netmiko:

hostname
username
password
port
platform
connection_options

where connection_options would be the following dictionary

device_type: arista_eos
port: 443

I then generally expected we would do the following in the plugin (using Netmiko as an example):

params = {
   'host': hostname,
   'username': username,
   'password': password,
   'device_type': platform,
   'port': port
}
params.update(connection_options)
ConnectionHandler(**Params)

I generally thought that was what you were doing in this PR except that you were doing an override of the top-level parameters to ensure they came from connection_options if they were specified there...but that is clearly not the case.

I need to test this PR with some real code on Netmiko and NAPALM, but I don't think I like the solution proposed in this PR.

ktbyers · 2018-08-10T05:52:14Z

I think the main ambiguity/confusion from #202 is that with Netmiko connection_options I was expecting this (i.e. generally arguments that could be passed directly to Netmiko's __init__ method

  connection_options:
        netmiko:
            device_type: arista_eos
            port: 443
            key_file: whatever.key
            use_keys: True

Whereas with NAPALM you already have a dictionary of 'additional_arguments' in optional_args so you already have a nesting of arguments:

    connection_options:
        napalm:
            platform: eos
            port: 443
            optional_args:
                blah: bleh

Looking at the specification we created in #202 I see how you could infer either of the two ways.

dbarrosop · 2018-08-10T07:20:32Z

For example, I think we will still need logic in the napalm-plugin to resolve the nxos_ssh issue. We will need logic to resolve naming differences like pass versus password in the NETCONF case.

Yes, I think my comment wasn't clear. What I meant is that nornir should handle the resolutions of the standard attributes, then it's up to the plugin to do with them what needs to be done. Same with advanced_options.

dbarrosop · 2018-08-10T07:30:12Z

@ktbyers, I think you are describing the same behavior I wanted to implemented. The way I see this working is like this:

my_host:
    # default standard attributes
    username: blah
    password: bleh
    port: 123
    connection_options:
          netmiko:
              # overriden standard attributes
              hostname: overriden_hostname
              port: 321
              advanced_options:
                     # whatever the plugin wants to implement
                     ssh_config_file:   ~/.ssh/config
                     other: bleh

Then netmiko's implementation could be (current one is buggy, just saw that, but I will fix it):

        parameters = {
            "host": hostname,
            "username": username,
            "password": password,
            "port": port,
        }

        if platform is not None:
            # Look platform up in corresponding map, if no entry return the host.nos unmodified
            platform = napalm_to_netmiko_map.get(platform, platform)
            parameters["device_type"] = platform

        advanced_options = advanced_options or {}
        parameters.update(advanced_options)
        self.connection = ConnectHandler(**parameters)

I just added an extra level of indentation to separate the standard attributes override from the free-form advanced options so we can do safely parameters.update(advanced_options) without having the standard attributes override interfere and cause problems.

The renaming of "connection_options" to "advanced_options" was just to avoid ambiguity as we kept talking about connection options but sometimes we were referring to the ${connection}_options and sometimes to the inner content that gets mapped into the underlying library.

Sorry if my initial implementation, which was bad, confused everybody. I will fix it once I am sure we are on the same page.

dbarrosop · 2018-08-10T07:35:07Z

napalm implementation is correct although, as you probably noticed, it has logic to extract the timeout value and then it maps directly into optional_args. I am fine doing instead:

parameters.update(advanced_options)
connection = network_driver(**parameters)

which would be consistent with the behavior described in my previous comment about netmiko.

dbarrosop · 2018-08-10T07:39:41Z

How would you specify the override in the connection options?

@dmfigol See my last two comments. I think we should support a standard way of overriding parameters while being completely flexible in the advanced_options. The reasoning is to avoid forcing users to dig out the correct incantation from the underlying library's documentation to do something as simple as overriding the password. If they need to do something fancier they will have to do it but for the simplest use case it should be straightforward.

ktbyers · 2018-08-10T15:16:56Z

Okay, that all makes sense.

Yes, there was a previous PR on the Netmiko plugin to change the order of applying the parameters versus advanced_options (whatever they were termed earlier). But I just put that on hold while we were resolving this...so we can just fix that issue as part of this.

ktbyers · 2018-08-10T16:00:21Z

The main thing I am concerned about is that we are pushing complexity onto the inventory structure:

my_host:
    # default standard attributes
    username: blah
    password: bleh
    port: 123
    connection_options:
          netmiko:
              # overriden standard attributes
              hostname: overriden_hostname
              port: 321
              advanced_options:
                     # whatever the plugin wants to implement
                     ssh_config_file:   ~/.ssh/config
                     other: bleh

I would rather have this:

my_host:
    # default standard attributes
    username: blah
    password: bleh
    port: 123
    connection_options:
          netmiko:
              hostname: overriden_hostname
              port: 321
              ssh_config_file:   ~/.ssh/config
              other: bleh

Or, I would like this even better (but this is worse from the nornir-core perspective i.e. there would have to be more magic in core to make this work)

my_host:
    # default standard attributes
    username: blah
    password: bleh
    port: 123
    netmiko_options:
          hostname: overriden_hostname
          port: 321
          ssh_config_file:   ~/.ssh/config
          other: bleh

I think you would probably have to do something like the following to make either of these work...if 'hostname', 'port', 'username', 'password', 'platform' exist as first-level keys inside the connection_options.netmiko, then they will override any argument of the same name that happens directly under the host (and they would have to get 'popped' off from 'connection_options.netmiko'). Otherwise, you generally would be forced to remove them in the plugin (i.e. 'platform' can't be fed into Netmiko ConnectHandler as **kwargs so it would have to be removed).

dmfigol · 2018-08-10T16:09:26Z

my_host:
    # default standard attributes
    username: blah
    password: bleh
    port: 123
    connection_options:
          netmiko:
              # overriden standard attributes
              hostname: overriden_hostname
              port: 321
              advanced_options:
                     # whatever the plugin wants to implement
                     ssh_config_file:   ~/.ssh/config
                     other: bleh

I like this idea! This makes sense. If you want to override standard parameters, you use nornir naming. If you want to do something specific, you would put that under advanced_options.
The downside, as Kirk mentioned, is that the inventory becomes a little too complicated (nested). Considering all arguments, I think I like this the most:

my_host:
    # default standard attributes
    username: blah
    password: bleh
    port: 123  # ? see my point below
    netmiko_options:  # getting rid of one level
        # overriden standard attributes
        hostname: overriden_hostname
        port: 321
        advanced_options:
                # whatever the plugin wants to implement
                ssh_config_file:   ~/.ssh/config
                other: bleh

I guess nornir will need to do standard attribute override while the connection plugin will need to take those standard attributes + advanced options and format them in the backend library format.

I don't really like mixing standard attributes and plugin attributes on the same level. What if there is a naming discrepancy: password vs pwd. What is the correct way to override it? With advanced_options you can clearly explain that standard attributes are overridden at the top-level, everything library specific goes under advanced_options (and takes precedence)

Another thing to think about: does port really deserve to be a standard attribute? Why I bring this up: since it is a standard attribute, it will always be passed to the connection plugin (either some value or None), it means that connection plugin will need to handle additionally if port exists or not. Not the biggest problem, but how often do you specify the port other than default? When you need that, you could easily do it under advanced_options.

ktbyers · 2018-08-10T16:33:58Z

FYI, this is what is currently implemented in this PR:

my_host:
    # default standard attributes
    username: blah
    password: bleh
    port: 123
    connection_options:
          netmiko:
              # overriden standard attributes
              hostname: overriden_hostname
              port: 321
              advanced_options:
                     # whatever the plugin wants to implement
                     ssh_config_file:   ~/.ssh/config
                     other: bleh

There was a commit a day or two ago that renamed an attribute toadvanced_options.

I don't really like mixing standard attributes and plugin attributes on the same level. 
What if there is a naming discrepancy: password vs pwd. What is the correct 
way to override it?

Yes, that is a downside...I think it would just end up being similar to this (in the plugin):

pass = connection_options.get('pass', password)

You would probably do it using dictionaries and update(), but the pattern would be the same (there would be some other magic you would have to do in core regarding the case if someone put 'password' or other first-level arguments inside the connection_options).

I am probably fine with either way (i.e. to leave it as-is i.e. what is implemented here) or to eliminate connection_options key. I mainly disliked 'advanced_options' so if those are staying then it doesn't matter too much to me. I probably have a slight preference for your second proposal (but David would have more insight and how much problems that causes in core, if any).

Port I would probably vote to move out of first-level arguments (i.e. not have port as a first-level argument).

dbarrosop · 2018-08-10T17:03:29Z

@dmfigol's second proposal is fine, I don't think it adds more complexity. I added the extra indentation actually because of the "defaults" grouping I originally described but that you didn't like. For clarity the proposal I am talking about is:

my_host:
    # default standard attributes
    username: blah
    password: bleh
    port: 123  # ? see my point below
    netmiko_options:  # getting rid of one level
        # overriden standard attributes
        hostname: overriden_hostname
        port: 321
        advanced_options:
                # whatever the plugin wants to implement
                ssh_config_file:   ~/.ssh/config
                other: bleh

I guess nornir will need to do standard attribute override while the connection plugin will need to take those standard attributes + advanced options and format them in the backend library format.

Yeah, that's correct. Standard attributes are resolved by nornir but then how to use those and the advanced_options is up to the library. I only wanted to avoid having to delegate the overrides to the plugins.

I mainly disliked 'advanced_options' so if those are staying then it doesn't matter too much to me. I probably have a slight preference for your second proposal

I will implement the second one and evaluate how problematic might be to remove altogether the advanced_options key. I like it for OCD reasons but I think it's worth at least checking if it's viable or not.

ktbyers · 2018-08-10T17:06:55Z

Sounds good to me.

dbarrosop · 2018-08-12T12:37:42Z

Ok, I think this one is ready. It's a lot but I think I am quite satisfied with the result. I looked at removing the "advanced_options" subblock and is not terrible. The get_connection_parameters would look like this:

    def get_connection_parameters(
        self, connection: Optional[str] = None
    ) -> Dict[str, Any]:
        if not connection:
           ...
        else:
            # we need a copy to avoid modifying the inventory
            conn_params = dict(self.get(f"{connection}_options", {})) 
            return {
                "hostname": conn_params.pop("hostname", self.hostname),
                "port": conn_params.pop("port", self.port),
                "username": conn_params.pop("username", self.username),
                "password": conn_params.pop("password", self.password),
                "platform": conn_params.pop("platform", self.platform),
                "advanced_options": conn_params,
            }

so from an implementation perspective is not bad. I just have mixed feelings about mixing standard and non-standard attributes. I understand how it's slightly easier if just skip that indentation but it feels ugly and slightly harder to document/explain. In summary, I am 50-50 here so if anyone has strong opinions let me know. Otherwise I will just leave it as it is now.

dmfigol

Just a couple of small things. I like implementation and public API.

dmfigol · 2018-08-12T13:31:26Z

nornir/core/inventory.py

@@ -395,7 +389,7 @@ def __init__(
            for group_name, group_details in groups.items():
                if group_details is None:
                    group = Group(name=group_name, nornir=nornir)
-                elif isinstance(group_details, dict):
+                elif isinstance(group_details, (dict, CommentedMap)):


I propose we use more generic collections.Mapping:
elif isinstance(group_details, collections.Mapping):

dmfigol · 2018-08-12T13:31:38Z

nornir/core/inventory.py

@@ -413,7 +407,7 @@ def __init__(

        self.hosts = {}
        for n, h in hosts.items():
-            if isinstance(h, dict):
+            if isinstance(h, (dict, CommentedMap)):


and here :)

dmfigol · 2018-08-12T13:53:49Z

nornir/core/inventory.py

-        nos: Optional[str] = None,
-        connection_options: Optional[Dict[str, Any]] = None,
+        port: Optional[int] = None,
+        platform: Optional[int] = None,


Still an issue with the latest commit. Should be platform: Optional[str]

dmfigol · 2018-08-12T13:54:34Z

nornir/core/inventory.py

-        connection_options: Optional[Dict[str, Any]] = None,
+        port: Optional[int] = None,
+        platform: Optional[int] = None,
+        advanced_options: Optional[int] = None,


Optional[Dict[str, Any]]

dmfigol · 2018-08-12T14:13:57Z

I don't have a strong opinion about advanced_options, but I slightly prefer having it, because there are no second thoughts where/how to put an override for the standard attribute which has a different name than Nornir.

ktbyers · 2018-08-12T20:05:46Z

I am fine with the advanced_options either way (either being there or not in inventory structure).

I wonder if we should use a different term for it though if it stays (since advanced_options could have basic arguments)

Maybe back to connection_options or driver_options.

ktbyers · 2018-08-12T20:31:14Z

I think for the plugins/connections/napalm.py you would want these changes;

$ git diff
diff --git a/nornir/plugins/connections/napalm.py b/nornir/plugins/connections/napalm.py
index a1e86ea..0814b46 100644
--- a/nornir/plugins/connections/napalm.py
+++ b/nornir/plugins/connections/napalm.py
@@ -36,12 +36,9 @@ class Napalm(ConnectionPlugin):
         }
         parameters.update(advanced_options)
 
-        if port and "port" not in advanced_options:
+        if port and "port" not in advanced_options['optional_args']:
             parameters["optional_args"]["port"] = port
 
-        if advanced_options.get("timeout"):
-            parameters["timeout"] = advanced_options["timeout"]
-
         network_driver = get_network_driver(platform)
         connection = network_driver(**parameters)
         connection.open()

timeout is already handled by the parameters.update and port would be inside of optional_args.

Here is the inventory I was testing with (this was groups.yaml):

---
defaults: {}
  # nornir_username: admin
  # nornir_password: <password>

nxos:
  port: 443
  napalm_options:
      platform: nxos
      advanced_options:
        timeout: 300
        optional_args:
          port: 8443

ktbyers · 2018-08-13T02:31:14Z

We should merge what we have into the 2.0 branch (fairly soon). So we have a new base to work from.

dbarrosop · 2018-08-13T08:32:17Z

timeout is already handled by the parameters.update and port would be inside of optional_args.

You are right, that was a leftover from the old behavior.

We should merge what we have into the 2.0 branch (fairly soon). So we have a new base to work from.

I will address the comments, rename "advanced_options" back to "connection_options" and merge. If there are other details we can fix later on.

consolidate inventory attributes

7974652

dbarrosop mentioned this pull request Aug 5, 2018

Consolidate inventory attributes #202

Closed

dbarrosop added 2 commits August 6, 2018 20:53

device_type is now platform

7d9945f

black

d1fc315

dmfigol reviewed Aug 8, 2018

View reviewed changes

This was referenced Aug 8, 2018

Allow setting of attribute for nornir_ attributes #200

Closed

Change order of netmiko_options #216

Closed

ktbyers reviewed Aug 8, 2018

View reviewed changes

dmfigol reviewed Aug 8, 2018

View reviewed changes

dbarrosop added 2 commits August 9, 2018 12:38

fix signature and rename configuration_options to advanced_options to…

d32b11e

… avoid confusions

add tests to validate parameters resolution

4825e23

fix breaking change from ruamel.yaml

46d7b3d

ktbyers reviewed Aug 10, 2018

View reviewed changes

fixes to documentation

b437401

dbarrosop added 5 commits August 12, 2018 13:13

pass _options without requiring indentation

0a4d26e

added tutorial on how to handle connections

32a8531

test jupyter notebooks

773113a

added upgrading notes

29ba624

enable nbval test in travis

e463e54

dbarrosop changed the title ~~[WIP] consolidate inventory attributes~~ consolidate inventory attributes Aug 12, 2018

dmfigol requested changes Aug 12, 2018

View reviewed changes

address comments

1c7a0af

dbarrosop merged commit 07c7113 into 2.0 Aug 13, 2018

dbarrosop deleted the attributes_refactor branch August 13, 2018 09:58

consolidate inventory attributes #224

consolidate inventory attributes #224

Conversation

dbarrosop commented Aug 5, 2018 • edited

coveralls commented Aug 5, 2018 • edited

Pull Request Test Coverage Report for Build 896

💛 - Coveralls

dmfigol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbarrosop Aug 8, 2018 • edited

Choose a reason for hiding this comment

dmfigol Aug 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmfigol Aug 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmfigol Aug 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmfigol commented Aug 8, 2018 • edited

dbarrosop commented Aug 9, 2018 • edited

ktbyers commented Aug 9, 2018 • edited

dmfigol commented Aug 9, 2018 • edited

Choose a reason for hiding this comment

ktbyers commented Aug 10, 2018

ktbyers commented Aug 10, 2018

dbarrosop commented Aug 10, 2018

dbarrosop commented Aug 10, 2018 • edited

dbarrosop commented Aug 10, 2018

dbarrosop commented Aug 10, 2018

ktbyers commented Aug 10, 2018

ktbyers commented Aug 10, 2018 • edited

dmfigol commented Aug 10, 2018 • edited

ktbyers commented Aug 10, 2018

dbarrosop commented Aug 10, 2018

ktbyers commented Aug 10, 2018

dbarrosop commented Aug 12, 2018

dmfigol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmfigol Aug 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmfigol commented Aug 12, 2018

ktbyers commented Aug 12, 2018

ktbyers commented Aug 12, 2018 • edited

ktbyers commented Aug 13, 2018

dbarrosop commented Aug 13, 2018

dbarrosop commented Aug 5, 2018 •

edited

coveralls commented Aug 5, 2018 •

edited

dbarrosop Aug 8, 2018 •

edited

dmfigol Aug 12, 2018 •

edited

dmfigol Aug 8, 2018 •

edited

dmfigol Aug 8, 2018 •

edited

dmfigol commented Aug 8, 2018 •

edited

dbarrosop commented Aug 9, 2018 •

edited

ktbyers commented Aug 9, 2018 •

edited

dmfigol commented Aug 9, 2018 •

edited

dbarrosop commented Aug 10, 2018 •

edited

ktbyers commented Aug 10, 2018 •

edited

dmfigol commented Aug 10, 2018 •

edited

dmfigol Aug 12, 2018 •

edited

ktbyers commented Aug 12, 2018 •

edited