-
Notifications
You must be signed in to change notification settings - Fork 667
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
Added CLI For Ext Media Lib #1156
base: master
Are you sure you want to change the base?
Conversation
This pull request introduces 1 alert when merging e7e6598 into 2e024de - view on LGTM.com new alerts:
|
please add unit tests and also fix lgtm alerts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add unit tests and also fix lgtm alerts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks ok, will approve once UT log is provided
@@ -357,6 +357,42 @@ class SFPShow(object): | |||
|
|||
def display_eeprom(self): | |||
click.echo(self.output) | |||
@multi_asic_util.run_on_multi_asic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add blank line above this
@@ -368,6 +404,21 @@ class SFPShow(object): | |||
def cli(): | |||
"""sfpshow - Command line utility for display SFP transceivers information""" | |||
pass | |||
# 'summary' subcommand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add blank line above this
Added spaces
This comment has been minimized.
This comment has been minimized.
Please update the command reference to include the new CLI. show interfaces transceiver summary |
sub = \ | ||
[interface, sfp_info_dict.get('display_name', 'N/A'), sfp_info_dict.get('power_rating_max', 'N/A'), | ||
sfp_info_dict.get('vendor_name', 'N/A'), sfp_info_dict.get('vendor_serial_number', 'N/A'), | ||
sfp_info_dict.get('vendor_part_number', 'N/A'), sfp_info_dict.get('qsa_adapter', 'N/A'), sfp_info_dict.get('qualified', 'N/A')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to make the "Qualified" field in the show command optional, via a platform.json parameter? Concerned that it may cause confusion among users and vendors if vendors don’t implement it.
- What I did
Added CLI which summarizes the info for media based on new parser
Parser PR is sonic-net/sonic-platform-common#129
- How I did it
Extended CLICK
- How to verify it
Visually verified it against expected values
- Previous command output (if the output of a command-line utility has changed)
Nil
- New command output (if the output of a command-line utility has changed)