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

[sflow] Fix traceback seen for show sflow interface #1282

Merged
merged 1 commit into from
Dec 9, 2020
Merged

[sflow] Fix traceback seen for show sflow interface #1282

merged 1 commit into from
Dec 9, 2020

Conversation

GarrickHe
Copy link
Contributor

  • Fix traceback seen for 'show sflow interface' command

Signed-off-by: Garrick He garrick_he@dell.com

- What I did
Fixed a traceback seen when show sflow interface is executed.

- How I did it
Updated the script to follow Python3 conventions.

- How to verify it
Retested the command to ensure no traceback is seen.

@GarrickHe
Copy link
Contributor Author

This PR fixes this issue:
sonic-net/sonic-buildimage#6078

show/main.py Outdated Show resolved Hide resolved
@prsunny
Copy link
Contributor

prsunny commented Dec 2, 2020

@padmanarayana to review

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2020

This pull request introduces 1 alert and fixes 4 when merging c6a7d87737fa43853e56f13bf9fdfd07b4ba7faf into 04dd841 - view on LGTM.com

new alerts:

  • 1 for Syntax error

fixed alerts:

  • 4 for Module-level cyclic import

padmanarayana
padmanarayana previously approved these changes Dec 2, 2020
@GarrickHe
Copy link
Contributor Author

@prsunny - Test failures doesn't appear to be sFlow related.

@prsunny
Copy link
Contributor

prsunny commented Dec 3, 2020

retest this please

prsunny
prsunny previously approved these changes Dec 3, 2020
@GarrickHe
Copy link
Contributor Author

retest this please

@lguohan
Copy link
Contributor

lguohan commented Dec 3, 2020

can you add unit test case?

@GarrickHe
Copy link
Contributor Author

@lguohan - No problem. Will add test-case tomorrow. Moving this PR to draft for now.

@GarrickHe GarrickHe marked this pull request as draft December 3, 2020 06:40
* Fix traceback seen for 'show sflow interface' command
* Update unit-test cases

Signed-off-by: Garrick He <garrick_he@dell.com>
Signed-off-by: ghe <ghe@contoso.com>
Signed-off-by: Garrick He <garrick_he@dell.com>
@GarrickHe GarrickHe dismissed stale reviews from prsunny and padmanarayana via fbbb07e December 4, 2020 01:53
@GarrickHe GarrickHe marked this pull request as ready for review December 4, 2020 01:53
@GarrickHe
Copy link
Contributor Author

@prsunny @lguohan @padmanarayana - I added the required test-cases. I ran it with and without the fix to ensure this issue is covered.

@@ -261,6 +261,35 @@ def test_config_sflow_intf_sample_rate(self):

return

def test_config_disable_all_intf(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont see the test covering the app db entries or the 'show' command. ??

Copy link
Contributor Author

@GarrickHe GarrickHe Dec 4, 2020

Choose a reason for hiding this comment

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

I added the faulty entries into appl_db.json and the test_show_intf() on line 55 will ensure we don't traceback due to the bad entries.

The two new test I added test_config_enable_all_intf and test_config_disable_all_intf merely ensures the proper configDB entries get added when the user runs those commands.

@prsunny prsunny requested a review from lguohan December 4, 2020 17:48
@GarrickHe
Copy link
Contributor Author

@prsunny @lguohan - This is ready. let me know if there is anything else you need on my end. Thanks.

@prsunny prsunny merged commit 2b4a58c into sonic-net:master Dec 9, 2020
anand-kumar-subramanian pushed a commit to anand-kumar-subramanian/sonic-utilities that referenced this pull request Mar 2, 2021
Fixed a traceback seen when show sflow interface is executed.
Updated the script to follow Python3 conventions.

Signed-off-by: Garrick He <garrick_he@dell.com>
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.

4 participants