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

[techsupport] [202106] Removed -i option for docker commands and Improved Error Reporting #1843

Merged
merged 4 commits into from
Oct 25, 2021

Conversation

vivekrnv
Copy link
Contributor

Signed-off-by: Vivek Reddy Karri vkarri@nvidia.com

What I did

PR #1723 cannot be cherry-picked directly to 202106. Thus raised a separate PR

How I did it

How to verify it

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)

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
@yxieca
Copy link
Contributor

yxieca commented Sep 28, 2021

Please don't merge until ready to cherry-pick #1844 right after merging this PR.

@yxieca
Copy link
Contributor

yxieca commented Sep 28, 2021

@vivekreddynv it would be great if you could test your change with https://github.com/Azure/sonic-mgmt/tree/master/tests/show_techsupport.

yxieca added a commit that referenced this pull request Sep 28, 2021
What I did
Fix: sonic-net/sonic-buildimage#8850

Issue was introduced by #1723, #1833, and #1843 (pending merge)

The error_handler is a great idea but the bash script needs to be error free first.

How I did it
Fix bash script errors.

How to verify it
run show techsupport test..

Signed-off-by: Ying Xie <ying.xie@microsoft.com>
qiluo-msft pushed a commit that referenced this pull request Sep 29, 2021
What I did
Fix: sonic-net/sonic-buildimage#8850

Issue was introduced by #1723, #1833, and #1843 (pending merge)

The error_handler is a great idea but the bash script needs to be error free first.

How I did it
Fix bash script errors.

How to verify it
run show techsupport test..

Signed-off-by: Ying Xie <ying.xie@microsoft.com>
Copy link
Contributor

@yxieca yxieca left a comment

Choose a reason for hiding this comment

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

Please take #1844, #1846 as example, fixing the code before check in. Make sure your code passes sonic-mgmt show_techsupport tests.

@vivekrnv
Copy link
Contributor Author

Please take #1844, #1846 as example, fixing the code before check in. Make sure your code passes sonic-mgmt show_techsupport tests.

Hi @yxieca,

Thanks for pointing out these false positive cases.

Changes, for this PR were merged in master and 202012. pending in 202106.
The fix1 you've raised, is in master and 202012, pending in 202106.
The fix2 is pending in all 3.

I would suggest, merging this PR in 202106, followed by fix1 and fix2.

I will see what other sections of the code has the false positives issue and will verify the fixes by running the techsupport test. Will raise separate PR's after this?

What do you think?

@yxieca
Copy link
Contributor

yxieca commented Sep 29, 2021

@vivekredynv, what I noticed from making change for 202012 branch is that that branch is different from master branch, therefore more changes are needed. If you could make these changes and making sure they are complete, that would be better than missing a few and having to come back change again.

yxieca and others added 2 commits September 29, 2021 21:27
…#1844)

What I did
Fix: sonic-net/sonic-buildimage#8850

Issue was introduced by sonic-net#1723, sonic-net#1833, and sonic-net#1843 (pending merge)

The error_handler is a great idea but the bash script needs to be error free first.

How I did it
Fix bash script errors.

How to verify it
run show techsupport test..

Signed-off-by: Ying Xie <ying.xie@microsoft.com>
Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
@vivekrnv
Copy link
Contributor Author

vivekrnv commented Sep 29, 2021

Okay, it seems #1846 can't be cleanly cherry-picked to 202012. I'll manually port #1846 (any other fixes required) for 202012.

But both the #1844 & #1846 can be cleanly cherry-picked for 202106. Thus i would suggest to merge this PR, followed by 1844 & 1846. That'll put master & 202106 in a consistent state.

And if i have to make any changes to these two branches, it'll be easy to cherry-pick to both of them

@liat-grozovik
Copy link
Collaborator

@vivekreddynv is it ok to take it to 202106 or something else is missing?

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
volodymyrsamotiy pushed a commit to volodymyrsamotiy/sonic-utilities that referenced this pull request Oct 20, 2021
…ocker commands and Improved Error Reporting (sonic-net#1843)

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
@judyjoseph
Copy link
Contributor

Please don't merge until ready to cherry-pick #1844 right after merging this PR.

@vivekreddynv I see the above comment from Ying, is this PR ready to be merged ?

volodymyrsamotiy pushed a commit to volodymyrsamotiy/sonic-utilities that referenced this pull request Oct 21, 2021
…ocker commands and Improved Error Reporting (sonic-net#1843)

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
@yxieca yxieca merged commit b684149 into sonic-net:202106 Oct 25, 2021
praveen-li pushed a commit to praveen-li/sonic-utilities that referenced this pull request Feb 8, 2022
…#1844)

What I did
Fix: sonic-net/sonic-buildimage#8850

Issue was introduced by sonic-net#1723, sonic-net#1833, and sonic-net#1843 (pending merge)

The error_handler is a great idea but the bash script needs to be error free first.

How I did it
Fix bash script errors.

How to verify it
run show techsupport test..

Signed-off-by: Ying Xie <ying.xie@microsoft.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