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

OSPF parser for Junos #146

Merged
merged 13 commits into from
Aug 13, 2018
Merged

OSPF parser for Junos #146

merged 13 commits into from
Aug 13, 2018

Conversation

ckishimo
Copy link
Contributor

@dbarrosop reopening as requested. Thanks

@coveralls
Copy link

coveralls commented Jul 18, 2018

Coverage Status

Coverage increased (+0.08%) to 84.147% when pulling 8d1a094 on ckishimo:ospfv2 into 75f22a5 on napalm-automation:develop.

@ckishimo
Copy link
Contributor Author

@dbarrosop do not merge! despite passing the tests, I made a mistake and lost the data in the tests... I need to review it. Sorry for that

@dbarrosop
Copy link
Member

ok ,ping me when this is ready for review :)

@ckishimo
Copy link
Contributor Author

now I understand... I only implemented the parser. However for the tests the translator is needed as well 😅 I'll try but it may take a while...

@ckishimo
Copy link
Contributor Author

ckishimo commented Aug 9, 2018

@dbarrosop Please have a look... Note I had to remove from the tests the OSPF authentication as the OC model only supports authentication-type and not the key. Thanks

post: "simple-password"
- path: authentication
present: false
post: "none"
Copy link
Member

Choose a reason for hiding this comment

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

should this be null instead? I'd expect this leaf to be empty if there is no authentication. The model is unclear though here, it just says "string".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbarrosop The translation from Junos -> OC with OSPF authentication is working with the current code, so I would not remove anything 😃

What I meant is I did not include any OSPF authentication in the tests because they were failing due to the OC not supporting the key. Let me know if not clear. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

ok, merging :)

@dbarrosop
Copy link
Member

Awesome work, just had a tiny comment :)

@dbarrosop dbarrosop merged commit 2d63504 into napalm-automation:develop Aug 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants