-
Notifications
You must be signed in to change notification settings - Fork 45
Added integration with napalm-yang #264
base: develop
Are you sure you want to change the base?
Conversation
| 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. |
mirceaulinic
left a comment
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
| # 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
|
|
||
| @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_MODELStonapalm-yangwhich 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.