napalm-automation / napalm-junos

Apache License 2.0
22 stars 42 forks source link

WIP: Implement `get_network_instances` #81

Closed mirceaulinic closed 7 years ago

mirceaulinic commented 7 years ago
dbarrosop commented 7 years ago

pylama is not happy with you : )

mirceaulinic commented 7 years ago

@dbarrosop did you have a look at the code - I was thinking to move some static hashmaps/globals in a separate class JunOSGlobals. Does this make sense to you?

dbarrosop commented 7 years ago

Yeah, I saw it, I was thinking about it. I think it makes more sense to have a constants.py file. We could have one on napalm-base for constants that might be good for all the drivers, ie, timeout. And then have a constants file on each driver. Something like:

napalm-junos/constants.py
---------------------------
from napalm_base.constants import *

THIS_IS_A_JUNOS_CONSTANT = 'blah'

And then:

napalm_junos.py
----------------

import constants as c

c.MY_GLOBAL_CONSTANT # (comes from the from napalm_base.constants import *)
c.THIS_IS_A_JUNOS_CONSTANT

It also has the benefit that if we need it, we could override MY_GLOBAL_CONSTANT on a particular driver by just adding it to the driver-specific constant files.

mirceaulinic commented 7 years ago

Yes, that looks good @dbarrosop!

mirceaulinic commented 7 years ago

For the record - submitted this PR according to the discussion above: https://github.com/napalm-automation/napalm-base/pull/149

mirceaulinic commented 7 years ago

I've rescheduled this one for 0.6.0 - can't make it this week when I plan to release 0.5.0.