Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Netconf custom verifier #102

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

KacperSzmitko
Copy link
Contributor

Added custom netconf verifier example that i accidentally deleted here

Copy link
Collaborator

@ThomasJRyan ThomasJRyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Just needs some housekeeping

@@ -11,10 +11,8 @@
__contact__ = '[email protected]'
__copyright__ = 'Cisco Systems, Inc.'

from .count_verifier import CountVerifier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this removed intentionally? Seems strange to just get rid of it

from dataclasses import field, dataclass
from google.protobuf import json_format

# Import base classes. For non pyats installation you can use class provided within this module
try:
from genie.libs.sdk.triggers.blitz.verifiers import GnmiDefaultVerifier
from genie.libs.sdk.triggers.blitz.verifiers import GnmiDefaultVerifier, NetconfDefaultVerifier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this or GnmiDefaultVerifier failed to import, then the entire file will break since you won't be able to subclass NetconfDefaultVerifier. Is there something in the exception below you should be doing?

from genie.libs.sdk.triggers.blitz.rpcverify import OptFields

@dataclass
class MyCustomReturns(OptFields):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer something a bit more descriptive that MyCustomReturns. Honestly, I'd even just prefer CustomReturns over the current name

@miott
Copy link
Contributor

miott commented Nov 8, 2023

@KacperSzmitko would you please address Thomas's concerns?

@KacperSzmitko
Copy link
Contributor Author

I did it in last commit

@miott miott closed this Nov 8, 2023
@miott miott reopened this Nov 8, 2023
@miott
Copy link
Contributor

miott commented Nov 8, 2023

@ThomasJRyan all the "housekeeping" has been addressed. Can you approve this?
@yuekyang , @lsheikal , can one of you review this?

@miott
Copy link
Contributor

miott commented Nov 13, 2023

Thanks @ThomasJRyan . @omehrabi , @lsheikal , @yuekyang , need one more approver.

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

Successfully merging this pull request may close these issues.

3 participants