Fix #98 #99
Fix #98 #99
Conversation
bewing
commented
Dec 2, 2016
•
bewing
commented
|
Looking at the test case I broke. |
|
It's the mocked data what it broke. As you changed what the method does internally you have to adapt it. But wait for now as we might decide to leave this one as it is or deprecate it and write a new method for checking the FIB. |
Yes, I think we should leave it as is + start discussion about a new method for checking the FIB, if possible aligned with OC format. |
With this doesn't mean we can leave the bug :) But let's not change the behaviour. |
|
Need to expand testing on this method. |
| @@ -985,16 +985,20 @@ def get_mac_address_table(self): | |||
| def get_route_to(self, destination='', protocol=''): | |||
| routes = dict() | |||
|
|
|||
| if protocol.lower() not in ['', 'bgp', 'connected', 'isis', 'ospf', 'rip', 'static']: | |||
There was a problem hiding this comment.
More than happy to, was just trying to match up closer to the junos reference spec. Do we want to try to throw a special UnknownProtocol exception or let the command parser raise an exception when an unknown protocol is passed in, instead?
There was a problem hiding this comment.
That would be the most optimal, but for the moment let's just raise TypeError.
And do check only if the protocol is specified, otherwise will return all possible route.s
There was a problem hiding this comment.
Can you add in that list also direct and when user requests direct to rewrite the value of the protocol var to connected?
There was a problem hiding this comment.
Eeerm... I know I'll contradict ourselves (did we discuss that before?) but I think we should remove this line completely. I don't see any reason why we'd limit to some certain protocols only. I will make the necessary adjustment on the other drivers in order to avoid this limitation!
There was a problem hiding this comment.
My approach was to enumerate ALL protocols supported by the driver, and raise TypeError for a bad one, instead of having the underlying driver pyeapi raise a CommandError. Either way works for me
| try: | ||
| ipv = '' | ||
| if IPNetwork(destination).version == 6: | ||
| ipv = 'v6' | ||
| except AddrFormatError: | ||
| return 'Please specify a valid destination!' | ||
|
|
||
| command = 'show ip{ipv} route {destination} detail'.format( | ||
| command = 'show ip{ipv} route {destination} {protocol} detail'.format( |
There was a problem hiding this comment.
Here should be specify the protocol only if requested by the user. Otherwise return all possible routes.
There was a problem hiding this comment.
If protocol arg is blank, all routes are returned, so this is already occurring
| @@ -985,16 +985,20 @@ def get_mac_address_table(self): | |||
| def get_route_to(self, destination='', protocol=''): | |||
| routes = dict() | |||
|
|
|||
| if protocol.lower() not in ['', 'bgp', 'connected', 'isis', 'ospf', 'rip', 'static']: | |||
There was a problem hiding this comment.
That would be the most optimal, but for the moment let's just raise TypeError.
And do check only if the protocol is specified, otherwise will return all possible route.s
|
Looks good - I'll test tomorrow! |
|
Tested and looks good: just please allow also the equivalence direct <-> connected and ready to merge. However I have noticed few other weird stuff, but they are not related to #98 so they will be fixed later. |
|
Thanks @bewing! |
