move auto detetec to merge and replace only

[itdependsnetworks]

Addresses #148 I believe the only two places the file system is actually needed is on merge and replace.

[coveralls]

Coverage decreased (-0.2%) to 70.52% when pulling 6ad5fed on itdependsnetworks:auto_detect into 42f1a7f on napalm-automation:develop.

[mirceaulinic]

It looks on the surface, makes sense to me.

[ktbyers]

I think we should just convert this to a Python setter/getter format which @dbarrosop mentioned previously. Basically, whenever anyone accesses this attribute, it will go and do the autodetect if it is not currently sent.

This will prevent the current solution from breaking if we expand the use of this attribute.

[coveralls]

Coverage increased (+0.6%) to 71.263% when pulling d523fca on itdependsnetworks:auto_detect into 42f1a7f on napalm-automation:develop.

[coveralls]

Coverage increased (+0.6%) to 71.263% when pulling d523fca on itdependsnetworks:auto_detect into 42f1a7f on napalm-automation:develop.

[coveralls]

Coverage increased (+0.6%) to 71.263% when pulling d523fca on itdependsnetworks:auto_detect into 42f1a7f on napalm-automation:develop.

[ktbyers]

I know you are copying the previous pattern here, but we probably should do:

except Exception:

If it fails at this point catch it and re-raise the exception with our message.

I would put our message as:

"Netmiko _autodetect_fs failed to workaround specify dest_file_system in optional_args"

In other words, what I put above was for the case when _autodetect_fs didn't exist in Netmiko (which won't happen now). I think we should convert this to something more general.

[ktbyers]

Shouldn't be necessary (see comment below)

[ktbyers]

Can't we just do:

# In __init__, line 86:
# Note extra underscore.
self._dest_file_system = optional_args.get('dest_file_system', None)

@property
def dest_file_system(self):
    if self._dest_file_system is None:
        self._dest_file_system = self._discover_file_system()
    return self._dest_file_system

Then anytime object.dest_file_system gets accessed it will use the current value (if there is one) or will call the _discover_file_system() method.

Then we don't have to do the artificial calls in replace and merge.

[ktbyers]

I think this will fix the testing error:

diff --git a/napalm_ios/ios.py b/napalm_ios/ios.py
index c72ef66..e184c11 100644
--- a/napalm_ios/ios.py
+++ b/napalm_ios/ios.py
@@ -139,9 +139,10 @@ class IOSDriver(NetworkDriver):
     def _discover_file_system(self):
         try:
             return self.device._autodetect_fs()
-        except Exception:
-            raise Exception("Netmiko _autodetect_fs failed to workaround specify "
-                                 "dest_file_system in optional_args.")
+        except Exception as e:
+            e.message = "Netmiko _autodetect_fs failed to workaround specify " \
+                        "dest_file_system in optional_args."
+            raise
 
     def is_alive(self):
@@ -168,13 +169,14 @@ class IOSDriver(NetworkDriver):
         """Returns a flag with the state of the SSH connection."""
         null = chr(0)
         try:
-            # Try sending ASCII null byte to maintain
-            #   the connection alive
-            self.device.send_command(null)
+            if self.device is None:
+                return {'is_alive': False}
+            else:
+                # Try sending ASCII null byte to maintain the connection alive
+                self.device.send_command(null)
         except (socket.error, EOFError):
-            # If unable to send, we can tell for sure
-            #   that the connection is unusable,
-            #   hence return False.
+            # If unable to send, we can tell for sure that the connection is unusable,
+            # hence return False.
             return {
                 'is_alive': False
             }

[ktbyers]

The is_alive fix was because I saw this when I ran the tests:

test/unit/test_getters.py::TestGetter::test_method_signatures <- ../napalm-base/napalm_base/test/getters.py Exception ignored in: <bound method IOSDriver.__del__ of <napalm_ios.ios.IOSDriver object at 0xb5ff8dcc>>
Traceback (most recent call last):
  File "/home/gituser/NAPALM/napalm/napalm-base/napalm_base/base.py", line 69, in __del__
    if self.is_alive()["is_alive"]:
  File "/home/gituser/NAPALM/napalm/napalm-ios/napalm_ios/ios.py", line 184, in is_alive
    'is_alive': self.device.remote_conn.transport.is_active()
AttributeError: 'NoneType' object has no attribute 'remote_conn'

and a similar error for send_command

[ktbyers]

The is_alive method should look like this:

    def is_alive(self):
        """Returns a flag with the state of the SSH connection."""
        null = chr(0)
        try: 
            if self.device is None:
                return {'is_alive': False}
            else:
                # Try sending ASCII null byte to maintain the connection alive
                self.device.send_command(null)
        except (socket.error, EOFError):
            # If unable to send, we can tell for sure that the connection is unusable,
            # hence return False.
            return {
                'is_alive': False
            }    
        return {
            'is_alive': self.device.remote_conn.transport.is_active()
        } 

If you want me to submit this into a separate PR that is fine also.

Basically make sure it doesn't run this if self.device hasn't even been initialized.

[ktbyers]

What I posted above doesn't work. I will correct.

[ktbyers]

Made an updated PR (built upon @itdependsnetworks work here):

#176

Closing this one.