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

[Mellanox] Enhancement for fan led management #4437

Merged
merged 13 commits into from
May 13, 2020
Merged

[Mellanox] Enhancement for fan led management #4437

merged 13 commits into from
May 13, 2020

Conversation

Junchao-Mellanox
Copy link
Collaborator

- What I did

  1. Add fan drawer class to Mellanox platform API
  2. Refactor led code for Fan class and Psu class, put all led logic in led.py
  3. Add device data to avoid sku specific code

- How I did it

- How to verify it

Manually test, regression is on going

- Description for the changelog

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

@Junchao-Mellanox Junchao-Mellanox mentioned this pull request Apr 17, 2020
Closed
@lgtm-com
Copy link

lgtm-com bot commented Apr 17, 2020

This pull request fixes 4 alerts when merging ae23ba9 into 6df0e4b - view on LGTM.com

fixed alerts:

  • 2 for Unused local variable
  • 2 for Wrong number of arguments in a class instantiation

@jleveque
Copy link
Contributor

@Junchao-Mellanox: It looks like the breaks the Mellanox build. Can you please check?

@Junchao-Mellanox
Copy link
Collaborator Author

@Junchao-Mellanox: It looks like the breaks the Mellanox build. Can you please check?

Depends on sonic-net/sonic-platform-common#83

nazariig
nazariig reviewed Apr 21, 2020
View reviewed changes
platform/mellanox/mlnx-platform-api/sonic_platform/chassis.py Outdated
@@ -29,6 +28,7 @@
MLNX_NUM_PSU = 2

GET_HWSKU_CMD = "sonic-cfggen -d -v DEVICE_METADATA.localhost.hwsku"
GET_PLATFORM_CMD = "sonic-cfggen -d -v DEVICE_METADATA.localhost.platform"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Junchao-Mellanox why not to use from sonic_device_util import get_machine_info?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! I didn't notice that there is such a convinient utilitiy class. Will change soon.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@Junchao-Mellanox
Merge branch 'master' into fan-led
a580e50 
Conflicts:
	platform/mellanox/mlnx-platform-api/sonic_platform/chassis.py
	platform/mellanox/mlnx-platform-api/sonic_platform/device_data.py
	platform/mellanox/mlnx-platform-api/sonic_platform/fan.py
	platform/mellanox/mlnx-platform-api/sonic_platform/psu.py
	platform/mellanox/mlnx-platform-api/tests/mock_platform.py
@Junchao-Mellanox
Get platform name from machine_info
b3dca21
@keboliu
Copy link
Collaborator

keboliu commented Apr 24, 2020

retest this please

@jleveque
Copy link
Contributor

Please fix conflicts

@lgtm-com
Copy link

lgtm-com bot commented May 6, 2020

This pull request fixes 4 alerts when merging 81a83c3 into fd953a4 - view on LGTM.com

fixed alerts:

  • 2 for Unused local variable
  • 2 for Wrong number of arguments in a class instantiation

@lgtm-com
Copy link

lgtm-com bot commented May 10, 2020

This pull request fixes 4 alerts when merging 75a1d08 into 286aa35 - view on LGTM.com

fixed alerts:

  • 2 for Unused local variable
  • 2 for Wrong number of arguments in a class instantiation

@Junchao-Mellanox
Copy link
Collaborator Author

Retest vsimage please

@Junchao-Mellanox
Copy link
Collaborator Author

Retest vsimage please

1 similar comment
@Junchao-Mellanox
Copy link
Collaborator Author

Retest vsimage please

@jleveque jleveque merged commit 5e6c204 into sonic-net:master May 13, 2020
@henry-ma henry-ma mentioned this pull request Sep 23, 2020
Merged
@Junchao-Mellanox Junchao-Mellanox deleted the fan-led branch December 15, 2020 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jleveque jleveque jleveque approved these changes

@nazariig nazariig Awaiting requested review from nazariig

Assignees
No one assigned
Labels
Enhancement ➕ Platform: Mellanox Submodule Update ⏫
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Junchao-Mellanox @jleveque @keboliu @nazariig