-
Notifications
You must be signed in to change notification settings - Fork 48
Added integration with napalm-yang #264
base: develop
Are you sure you want to change the base?
Conversation
napalm_base/base.py
Outdated
self.config = napalm_base.yang.Yang("config", self) | ||
self.state = napalm_base.yang.Yang("state", self) | ||
return self | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks strange...you are accessing an attribute (i.e. using it as a getter), but then causing it to behave as a setter? So you are basically creating a method that looks like an attribute and returns the object.
Can you provide some more details on why here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, so that's certainly a hack. Ideally we would have something like:
class Foo(object):
def __init__(self):
self.config = napalm_base.yang.Yang("config", self)
self.state = napalm_base.yang.Yang("state", self)
class NetworkDriver(object):
def __init__(self):
self.yang = Foo()
But right now we can't do that because __init__
throws a NotImplementedException
and no driver calls super for that reason. So to make that happen we would have to change all drivers. With this hack I managed to provide same behavior without having to change any driver.
If you have a better solution let me know, this is the best I could come up with so far. We could probably use a wrapper or a metaclass but figured this was simpler/easier to read (if we go with this hack I will add details to the docstrings so people reading this knows why the hack).
I have added a mechanism to print the models as well, I guess it's useful for reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor suggestions, in case we want to avoid having napalm-yang
as a requirement.
napalm_base/base.py
Outdated
@@ -22,6 +22,7 @@ | |||
# local modules | |||
import napalm_base.exceptions | |||
import napalm_base.helpers | |||
import napalm_base.yang |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to avoid napalm-yang
as a dependency for napalm-base
, we can check if it is installed, i.e.:
try:
import napalm_yang
HAS_NY = True
except ImportError:
HAS_NY = False
napalm_base/base.py
Outdated
@@ -93,6 +94,13 @@ def __raise_clean_exception(exc_type, exc_value, exc_traceback): | |||
# Traceback should already be attached to exception; no need to re-attach | |||
raise exc_value | |||
|
|||
@property | |||
def yang(self): | |||
if not hasattr(self, "config"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just before this, we can:
if not HAS_NY:
raise SomeException('Please install napalm-yang for this fancy stuff')
Okay, Python 2 tests passed here. I think this only waits for python 3 support in napalm-yang @dbarrosop? |
I won't wait until napalm-yang has support for py3 because that will just delay things and |
DO NOT MERGE
Depends on napalm-automation/napalm-yang#61
This PR integrates napalm-yang into the drivers. See:
https://github.com/napalm-automation/napalm-base/compare/dbarrosop/napalm-yang?expand=1#diff-b284a28710cce90d9d9be3a7f4cabc8e
(gist with the execution https://gist.github.com/dbarrosop/8fae309ecbfe7d1207044aca5732beba)
"getters" are dynamic which means there is no dependency loop between napalm-yang and the drivers themselves. I just have to move the
SUPPORTED_MODELS
tonapalm-yang
which I will do once we are happy with the PR.@mirceaulinic @ktbyers thoughts?
Note: test.py will go away before merging. That will be part of the documentation.