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

Add CLI command for ECMP calculator #2538

Closed
wants to merge 4 commits into from

Conversation

liorghub
Copy link
Contributor

@liorghub liorghub commented Dec 5, 2022

What I did

I have added a new CLI command for calling ECMP calculator.

How I did it

Add new CLI command. Command handler calls script /usr/bin/ecmp_calc.py

How to verify it

Call command and verify its output.

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

root@sonic:/home/admin# show ip ecmp-egress-port --ingress-port Ethernet64 --packet /tmp/packet.json
Egress port: Ethernet0
root@sonic:/home/admin# 

@liat-grozovik
Copy link
Collaborator

@prsunny could you please review or suggest someone who can?

Copy link
Collaborator

@liat-grozovik liat-grozovik left a comment

Choose a reason for hiding this comment

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

missing tests for new code. please add
missing changes in command reference guide. please add

I dont think we should have -i before the interface name. just use the interface name
check for reference 'show interfaces transceiver presence Ethernet0'
same for the -p.

@prsunny
Copy link
Contributor

prsunny commented Dec 12, 2022

This is a vendor specific utility. I thought we can directly use the utility to get the hash result. I don't think we need to expose it via a generic CLI command.

@liat-grozovik
Copy link
Collaborator

In this case, the utility is already available and readme file explain directly how it can be used.
I was under the impression you would like to have the CLI and if the utility available it will be running.
But if this is not required, we can close this PR and leave with the utility itself.

@liorghub liorghub closed this Dec 14, 2022
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