Skip to content
This repository has been archived by the owner on Sep 17, 2019. It is now read-only.

Added integration with napalm-yang #264

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

dbarrosop
Copy link
Member

@dbarrosop dbarrosop commented Jun 10, 2017

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 to napalm-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.

@dbarrosop
Copy link
Member Author

Pinging @matejv and @bewing as I know they have been playing with napalm-yang as well.

self.config = napalm_base.yang.Yang("config", self)
self.state = napalm_base.yang.Yang("state", self)
return self

Copy link
Contributor

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?

Copy link
Member Author

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).

@dbarrosop
Copy link
Member Author

I have added a mechanism to print the models as well, I guess it's useful for reference.

Copy link
Member

@mirceaulinic mirceaulinic left a 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.

@@ -22,6 +22,7 @@
# local modules
import napalm_base.exceptions
import napalm_base.helpers
import napalm_base.yang
Copy link
Member

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

@@ -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"):
Copy link
Member

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')

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.6%) to 58.46% when pulling f0aa889 on dbarrosop/napalm-yang into e21f10f on develop.

@mirceaulinic
Copy link
Member

Okay, Python 2 tests passed here. I think this only waits for python 3 support in napalm-yang @dbarrosop?

@dbarrosop
Copy link
Member Author

I won't wait until napalm-yang has support for py3 because that will just delay things and napalm-yang is an optional module anyway. However, as I write some documentation on how to use this I am making minor adjustments so hold on this for a few more days.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants