search

napalm-automation / napalm-ios

Apache License 2.0
31 stars 40 forks source link

Convert file system handling to @property; update is_alive for no device. #176

Closed ktbyers closed 7 years ago

ktbyers commented 7 years ago
coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.09%) to 70.807% when pulling 453ca4ba0613908ad4cceb51729d05ae597359a2 on ktbyers:itdependsnetworks-auto_detect into 69db3eb504c74b2c7973e6cd5e2d3183759ff70e on napalm-automation:develop.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.09%) to 70.807% when pulling 453ca4ba0613908ad4cceb51729d05ae597359a2 on ktbyers:itdependsnetworks-auto_detect into 69db3eb504c74b2c7973e6cd5e2d3183759ff70e on napalm-automation:develop.

ktbyers commented 7 years ago

@mirceaulinic FYI, I made a minor change to is_alive here. Basically check that self.device is not set to None (i.e. there is an active connection). I was some seeing some testing exceptions otherwise.

i.e. things like self.device.send_command(null) would be called when self.device was None.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.09%) to 70.807% when pulling 38d527b7cbe4b438abfe2d8194db659fd72d255c on ktbyers:itdependsnetworks-auto_detect into 69db3eb504c74b2c7973e6cd5e2d3183759ff70e on napalm-automation:develop.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.09%) to 70.807% when pulling 38d527b7cbe4b438abfe2d8194db659fd72d255c on ktbyers:itdependsnetworks-auto_detect into 69db3eb504c74b2c7973e6cd5e2d3183759ff70e on napalm-automation:develop.

ktbyers commented 7 years ago

@itdependsnetworks FYI, can you review this.

I did manually test the exception handling to make sure it worked properly on PY2 and PY3. I also tested a merge against a real device both using autodetect and with passing in the file system as an optional argument.

itdependsnetworks commented 7 years ago

Pulled your branch and looks good to me.

ktbyers commented 7 years ago

Okay, I will let @mirceaulinic have a chance to comment on the is_alive change and then should be good to go.

mirceaulinic commented 7 years ago

Thanks @ktbyers, looks good. Just as an aside, I would have caught AttibuteError and return the same {'is_alive': False} (apparently try-catch tends to be faster than if-else, in particular when the try block already exists). But that's just an insignificant detail, don't know actually why I put it here... but I will submit this anyway :-)