Conversation
|
I didn't find anything else, however I would be glad if someone could test the telegram bot. But looking at the code I am rather confident with the current state of the PR. |
| BGP.local_pref: 100 | ||
| BGP.community: (64511,4) (64511,34) (64511,24) | ||
| BGP.large_community: (4242420604, 2, 50) (4242420604, 501, 4242423914) (4242420604, 502, 44) (4242420604, 504, 4) (4242421080, 104, 1) (4242421080, 101, 44) (4242421080, 103, 126) | ||
| via 172.22.96.65 on dn42-bird3 |
There was a problem hiding this comment.
Just a thought, it might be better to move the Bird3 test case into a separate file (maybe with 2-3 routes in total?)
This would make it more clear where the output is coming from, as I don't think you would ever see mixed Bird 2 / 3 output formats in one command.
There was a problem hiding this comment.
I thought about that, but that would require either duplicating all appropriate tests or rework them to work with multiple inputs.
There was a problem hiding this comment.
In my POV : The code is straightforward, the test coverage is adequate, and the changes are minimal and focused.
That said, if the codebase doesn't already have a pattern for this, the current approach is pragmatic
There was a problem hiding this comment.
IMO this is fine, since bird-lg-go doesn't require an additional flag to switch mode between bird2/bird3, so it should simultaneously work with both of them.
|
Looks good to me. I don't use bird3 so cannot test the Telegram bot, but we can fix this in a separate PR if it doesn't work. |
4 participants
BGP route info is written as
bgp_xinstead ofBGP.xFor now the only meaningful difference I found is the AS path used for 3 features:
pathcommand (untested)I'll try to check for other necessary changes