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

[show] Add 'show' CLI for system-health feature #971

Merged
merged 25 commits into from
Oct 12, 2020
Merged

[show] Add 'show' CLI for system-health feature #971

merged 25 commits into from
Oct 12, 2020

Conversation

shlomibitton
Copy link
Contributor

- What I did
Add 'show' CLI for system-health feature.
Add Unit test for the new CLI.

Commands:
- summary Show system-health summary information
- detail Show system-health detail information
- monitor-list Show system-health monitored services and devices name list

- How I did it
Added support to 'show' script.
Added new unit test.

- How to verify it
Run commands:
- show system-health summary
- show system-health summary
- show system-health summary

- 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)

Add 'show' CLI for system-health feature
d58195e 
Signed-off-by: Shlomi Bitton <shlomibi@mellanox.com>
@lgtm-com
Copy link

lgtm-com bot commented Jun 30, 2020

This pull request introduces 4 alerts when merging 8a634ab into aae89b7 - view on LGTM.com

new alerts:

  • 3 for Except block handles 'BaseException'
  • 1 for Unused import

@shlomibitton shlomibitton changed the title Add 'show' CLI for system-health feature [Mellanox] Add 'show' CLI for system-health feature Jun 30, 2020
@keboliu keboliu requested a review from jleveque June 30, 2020 14:38
@shlomibitton shlomibitton changed the title [Mellanox] Add 'show' CLI for system-health feature [system health] Add 'show' CLI for system-health feature Jun 30, 2020
Add unit test for 'system-health' feature, add support for testing in…
61d90ac 
… 'show' script, Fix comments

Signed-off-by: Shlomi Bitton <shlomibi@mellanox.com>
@lgtm-com
Copy link

lgtm-com bot commented Jul 6, 2020

This pull request introduces 4 alerts when merging 61d90ac into 60e5410 - view on LGTM.com

new alerts:

  • 3 for Except block handles 'BaseException'
  • 1 for Unused import

@keboliu
Copy link
Collaborator

keboliu commented Jul 7, 2020

retest this please

@shlomibitton
Copy link
Contributor Author

retest this please

@lgtm-com
Copy link

lgtm-com bot commented Jul 13, 2020

This pull request introduces 4 alerts when merging f63c8b0924dd790662051ae8d6ce431332720e9b into b4b5fd3 - view on LGTM.com

new alerts:

  • 3 for Except block handles 'BaseException'
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jul 13, 2020

This pull request introduces 4 alerts when merging 04d0672 into b4b5fd3 - view on LGTM.com

new alerts:

  • 3 for Except block handles 'BaseException'
  • 1 for Unused import

@jleveque jleveque changed the title [system health] Add 'show' CLI for system-health feature [show] Add 'show' CLI for system-health feature Jul 13, 2020
jleveque
jleveque suggested changes Jul 13, 2020
View reviewed changes
show/main.py Outdated Show resolved Hide resolved
sonic-utilities-tests/system_health_test.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Jul 14, 2020

This pull request introduces 4 alerts when merging c04dfed into a23479e - view on LGTM.com

new alerts:

  • 3 for Except block handles 'BaseException'
  • 1 for Unused import

liat-grozovik
liat-grozovik requested changes Jul 14, 2020
View reviewed changes
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.

@shlomibitton this PR is missing command line reference update. Please update it as well

jleveque
jleveque suggested changes Jul 14, 2020
View reviewed changes
Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

Please fix LGTM alerts.

@shlomibitton
Update Command-Reference.md
d33454c 
Add a CLI reference for system-health feature.
@lgtm-com
Copy link

lgtm-com bot commented Jul 15, 2020

This pull request introduces 4 alerts when merging d33454c into a23479e - view on LGTM.com

new alerts:

  • 3 for Except block handles 'BaseException'
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jul 15, 2020

This pull request introduces 1 alert when merging 28f9625 into a23479e - view on LGTM.com

new alerts:

  • 1 for Unused import

@shlomibitton
Copy link
Contributor Author

@liat-grozovik the CLI reference added, please review.
@jleveque 3 LGTM alerts fixed, last alert is because of an import needed for unit testing.

jleveque
jleveque reviewed Jul 15, 2020
View reviewed changes
doc/Command-Reference.md Outdated Show resolved Hide resolved
jleveque
jleveque suggested changes Jul 15, 2020
View reviewed changes
show/main.py Outdated Show resolved Hide resolved
@shlomibitton shlomibitton mentioned this pull request Jul 16, 2020
Merged
@shlomibitton
Fix comment
393c3de 
Change 'Ignore' to 'Ignored'
Junchao-Mellanox
Junchao-Mellanox reviewed Sep 9, 2020
View reviewed changes
show/main.py Outdated Show resolved Hide resolved
jleveque
jleveque suggested changes Sep 9, 2020
View reviewed changes
show/main.py Outdated Show resolved Hide resolved
jleveque
jleveque suggested changes Sep 10, 2020
View reviewed changes
show/system_health.py Outdated Show resolved Hide resolved
tests/system_health_test.py Outdated Show resolved Hide resolved
jleveque
jleveque suggested changes Sep 14, 2020
View reviewed changes
tests/system_health_test.py Outdated Show resolved Hide resolved
jleveque
jleveque suggested changes Sep 14, 2020
View reviewed changes
tests/system_health_test.py Outdated Show resolved Hide resolved
tests/system_health_test.py Outdated Show resolved Hide resolved
@liat-grozovik
Copy link
Collaborator

@jleveque can you please confirm your comments are resolved so we can merge?

@liat-grozovik liat-grozovik merged commit a71c72b into sonic-net:master Oct 12, 2020
This was referenced Oct 13, 2020
Closed
Closed
@shlomibitton shlomibitton deleted the shlomi_system_health_cli branch March 24, 2021 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sujinmkang sujinmkang sujinmkang approved these changes

@liat-grozovik liat-grozovik liat-grozovik approved these changes

@jleveque jleveque jleveque approved these changes

@Junchao-Mellanox Junchao-Mellanox Junchao-Mellanox left review comments

@keboliu keboliu keboliu left review comments

@lguohan lguohan Awaiting requested review from lguohan

Assignees
No one assigned
Labels
Enhancement show
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@shlomibitton @keboliu @jleveque @lguohan @liat-grozovik @sujinmkang @Junchao-Mellanox