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

[Celestica] Add new platform SilverstoneX #8797

Open
wants to merge 6 commits into
base: 201911
Choose a base branch
from

Conversation

nicwu-cel
Copy link
Contributor

Why I did it

Added Support for Celestica SilverstoneX platform

How I did it

Implemented the support for Celestica SilverstoneX platform

Platform: x86_64-cel_silverstone-x-r0
HwSKU: Silverstone-x
ASIC: innovium
ASIC Count: 1

How to verify it

Verified the show command outputs

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

[Celestica] Add new platform SilverstoneX

A picture of a cute animal (not mandatory but encouraged)

@ghost
Copy link

ghost commented Sep 19, 2021

CLA assistant check
All CLA requirements met.

@lgtm-com
Copy link

lgtm-com bot commented Sep 19, 2021

This pull request introduces 37 alerts when merging e8c0113 into d1f6596 - view on LGTM.com

new alerts:

  • 16 for Unused import
  • 12 for Except block handles 'BaseException'
  • 4 for 'import *' may pollute namespace
  • 2 for Should use a 'with' statement
  • 1 for Module imports itself
  • 1 for Unused local variable
  • 1 for Wrong name for an argument in a class instantiation

@nicwu-cel
Copy link
Contributor Author

Seems these 2 compiling failures are not related to platform code that we want to push. Could you check for this, thanks.

@Blueve
Copy link
Contributor

Blueve commented Sep 22, 2021

Triggered re-run failed checks for this PR.
@nicwu-cel could you also address LGTM alerts?

@nicwu-cel
Copy link
Contributor Author

nicwu-cel commented Sep 23, 2021

Triggered re-run failed checks for this PR.
@nicwu-cel could you also address LGTM alerts?

OK, seems most of them are about the syntax of Python. Could I ignore them and merge this to 201911 branch? By the way, if I indeed need to fix these warnings, should I submit a new PR after I fix these alerts? Thanks

@Blueve
Copy link
Contributor

Blueve commented Sep 24, 2021

OK, seems most of them are about the syntax of Python. Could I ignore them and merge this to 201911 branch? By the way, if I indeed need to fix these warnings, should I submit a new PR after I fix these alerts? Thanks

Why not fix them in current PR ; )
We must ensure PR will not introduce new alerts in release branches.

One more question, why this PR is not target to master branch? Are we plan to support this platform only for 201911?
Usually, we always put new feature to master branch first and then back port to release branch if need.

@nicwu-cel
Copy link
Contributor Author

OK, seems most of them are about the syntax of Python. Could I ignore them and merge this to 201911 branch? By the way, if I indeed need to fix these warnings, should I submit a new PR after I fix these alerts? Thanks

Why not fix them in current PR ; )
We must ensure PR will not introduce new alerts in release branches.

One more question, why this PR is not target to master branch? Are we plan to support this platform only for 201911?
Usually, we always put new feature to master branch first and then back port to release branch if need.

OK, I'll fix these alerts in current PR.
We use 201911 branch to develop sonic_platform APIs with Python2 and we are not sure if it works in master branch or not. If it works in master branch, it's OK for us to put new feature to master branch first and then back port to 201911 branch.

@Blueve
Copy link
Contributor

Blueve commented Sep 25, 2021

OK, I'll fix these alerts in current PR.
We use 201911 branch to develop sonic_platform APIs with Python2 and we are not sure if it works in master branch or not. If it works in master branch, it's OK for us to put new feature to master branch first and then back port to 201911 branch.

Thanks, please try master first. Usually we need forward compatibility and supported platform should always work in newest branch unless we have specific reason. @lguohan I would like to hear your suggestion.

@lgtm-com
Copy link

lgtm-com bot commented Sep 27, 2021

This pull request introduces 8 alerts when merging fec8ee3 into 1b168c3 - view on LGTM.com

new alerts:

  • 4 for Unused import
  • 1 for Unused local variable
  • 1 for Except block handles 'BaseException'
  • 1 for 'import *' may pollute namespace
  • 1 for Wrong number of arguments in a class instantiation

@lgtm-com
Copy link

lgtm-com bot commented Sep 27, 2021

This pull request introduces 1 alert when merging 1006398 into 1b168c3 - view on LGTM.com

new alerts:

  • 1 for Wrong name for an argument in a class instantiation

@nicwu-cel
Copy link
Contributor Author

Seems the only alert is not a real alert because it meets Python syntax. Could we only push this code to 201911? Is there a comment from @lguohan? Thanks.

@sujinmkang
Copy link
Collaborator

@nicwu-cel Can we have show platform cli command output with this platform support? And sonic-mgmt/tests/platform_tests/api test result?

@lgtm-com
Copy link

lgtm-com bot commented Nov 4, 2021

This pull request introduces 1 alert when merging 583bf10 into 5b88d9e - view on LGTM.com

new alerts:

  • 1 for Wrong name for an argument in a class instantiation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants