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.

Sign up for GitHub

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

Jump to bottom

ripd/ripngd: use common header for display command #16229

Merged
merged 3 commits into from
Jul 9, 2024
Merged

ripd/ripngd: use common header for display command #16229

merged 3 commits into from
Jul 9, 2024

Conversation

anlancs
Copy link
Contributor

@anlancs anlancs commented Jun 16, 2024

Both rip and ripng can import routes from other protocols, e.g. ISIS. But their header doesn't list the description for these abbreviations.

Let show ip rip (and show ipv6 ripng) uses the common header.

Before:

Codes: R - RIP, C - connected, S - Static, O - OSPF, B - BGP
Sub-codes:
      (n) - normal, (s) - static, (d) - default, (r) - redistribute,
      (i) - interface

After:

Codes: K - kernel route, C - connected, L - local, S - static,
       R - RIP, O - OSPF, I - IS-IS, B - BGP, E - EIGRP, N - NHRP,
       T - Table, v - VNC, V - VNC-Direct, A - Babel, F - PBR,
       f - OpenFabric, t - Table-Direct,
       > - selected route, * - FIB route, q - queued, r - rejected, b - backup
       t - trapped, o - offload failure

Sub-codes:
      (n) - normal, (s) - static, (d) - default, (r) - redistribute,
      (i) - interface

@ton31337
Copy link
Member

It might be misleading output a bit, because for e.g. the output itself does not have selected route, or rejected...

@anlancs
Copy link
Contributor Author

anlancs commented Jun 17, 2024

@ton31337 Yes. But seems all daemons have already use this behavior.

@ton31337
Copy link
Member

@ton31337 Yes. But seems all daemons have already use this behavior.

E.g.?

@anlancs
Copy link
Contributor Author

anlancs commented Jun 17, 2024 •
edited
Loading

@ton31337
I misunderstood, you are right, the extra information is only for rip/ripng.
It's not clear why rip/ripng is an exception...

@ton31337
Copy link
Member

So... just adding missing protocols would be the simplest thing to do? Or splitting SHOW_ROUTE_V4_HEADER into two parts.

@anlancs anlancs force-pushed the ripd/fix-header-show branch from e97e6c2 to 414cd17 Compare June 20, 2024 07:51
@github-actions github-actions bot added size/S and removed size/XS labels Jun 20, 2024
@anlancs
Copy link
Contributor Author

anlancs commented Jun 20, 2024

So... just adding missing protocols would be the simplest thing to do? Or splitting SHOW_ROUTE_V4_HEADER into two parts.

Done, use the simple way.

@anlancs anlancs force-pushed the ripd/fix-header-show branch from 414cd17 to 524209e Compare June 24, 2024 13:17
@github-actions github-actions bot added size/XS and removed size/S labels Jun 24, 2024
@anlancs anlancs force-pushed the ripd/fix-header-show branch from 524209e to 7250413 Compare June 24, 2024 13:21
@github-actions github-actions bot added size/S and removed size/XS labels Jun 24, 2024
@ton31337
Copy link
Member

@frrbot rereview

@ton31337
Copy link
Member

Could you split it into separate commits?

@anlancs anlancs force-pushed the ripd/fix-header-show branch from 7250413 to 82e516b Compare June 25, 2024 06:28
@anlancs
Copy link
Contributor Author

anlancs commented Jun 25, 2024

@ton31337
Done, thanks!

@anlancs anlancs force-pushed the ripd/fix-header-show branch from 82e516b to 4a6ac1a Compare June 25, 2024 06:33
ton31337
ton31337 approved these changes Jul 3, 2024
View reviewed changes
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

LGTM

@anlancs anlancs force-pushed the ripd/fix-header-show branch from 4a6ac1a to 3884690 Compare July 4, 2024 10:23
@anlancs
Copy link
Contributor Author

anlancs commented Jul 4, 2024

Just rebased it.

@RodrigoMNardi
Copy link
Contributor

Couldn't it be two different show commands?
The current one would be more summarized and more detailed as proposed in the PR.
Example: show ip rip -> current format
show ip rip verbose -> new format

@ton31337
Copy link
Member

ton31337 commented Jul 4, 2024

@anlancs could you fix the tests?

@ton31337
Copy link
Member

ton31337 commented Jul 4, 2024

Couldn't it be two different show commands? The current one would be more summarized and more detailed as proposed in the PR. Example: show ip rip -> current format show ip rip verbose -> new format

This is a bug (missing other protocols), not like a new version of the output).

@RodrigoMNardi
Copy link
Contributor

Couldn't it be two different show commands? The current one would be more summarized and more detailed as proposed in the PR. Example: show ip rip -> current format show ip rip verbose -> new format

This is a bug (missing other protocols), not like a new version of the output).

Ok.

anlancs added 3 commits July 5, 2024 09:31
@anlancs
ripngd: adjust header for display command
2aa27ac 
Both rip and ripng can import routes from other protocols, e.g. ISIS.
But their header doesn't list the description for these abbreviations.

Adjust `show ipv6 ripng` 's header for display command.

Before:
```
Codes: R - RIPng, C - connected, S - Static, O - OSPF, B - BGP
Sub-codes:
```

After:
```
Codes: K - kernel route, C - connected, L - local, S - static,
       R - RIPng, O - OSPF, I - IS-IS, B - BGP, E - EIGRP, N - NHRP,
       T - Table, v - VNC, V - VNC-Direct, A - Babel, F - PBR,
       f - OpenFabric, t - Table-Direct
Sub-codes:
```

Signed-off-by: anlan_cs <[email protected]>
@anlancs
ripd: adjust header for display command
c0b6095 
Continue to adjust `show ip rip` 's header for display comand.

Signed-off-by: anlan_cs <[email protected]>
@anlancs
tests: update tests for ripd and ripngd
b707ed8 
Since the displayed header of "show ip rip" and "show ipv6 ripng" are changed,
we should update tests of ripd and ripngd.

Signed-off-by: anlan_cs <[email protected]>
@anlancs anlancs force-pushed the ripd/fix-header-show branch from 3884690 to b707ed8 Compare July 5, 2024 01:57
@frrbot frrbot bot added the tests Topotests, make check, etc label Jul 5, 2024
@github-actions github-actions bot added size/M and removed size/S labels Jul 5, 2024
@anlancs
Copy link
Contributor Author

anlancs commented Jul 5, 2024

Done.
Thanks for your great help! @ton31337 @RodrigoMNardi

@RodrigoMNardi
Copy link
Contributor

ci:retry

@riw777 riw777 self-requested a review July 9, 2024 15:48
riw777
riw777 approved these changes Jul 9, 2024
View reviewed changes
Copy link
Member

@riw777 riw777 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

@riw777 riw777 merged commit 52376aa into FRRouting:master Jul 9, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ton31337 ton31337 ton31337 approved these changes

@riw777 riw777 riw777 approved these changes

Assignees
No one assigned
Labels
master rip ripng size/M tests Topotests, make check, etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anlancs @ton31337 @RodrigoMNardi @riw777