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

[Python] Migrate applications/scripts to import sonic-py-common package #5043

Merged
merged 23 commits into from
Aug 3, 2020
Merged

[Python] Migrate applications/scripts to import sonic-py-common package #5043

merged 23 commits into from
Aug 3, 2020

Conversation

jleveque
Copy link
Contributor

@jleveque jleveque commented Jul 27, 2020

- Why I did it

As part of consolidating all common Python-based functionality into the new sonic-py-common package, this pull request:

  1. Redirects all Python applications/scripts in sonic-buildimage repo which previously imported sonic_device_util or sonic_daemon_base to instead import sonic-py-common, which was added in Introduce sonic-py-common package #5003
  2. Replaces all calls to sonic_device_util.get_platform_info() to instead call sonic_py_common.get_platform() and removes any calls to sonic_device_util.get_machine_info() which are no longer necessary (i.e., those which were only used to pass the results to sonic_device_util.get_platform_info().
  3. Removes unused imports to the now-deprecated sonic-daemon-base package and sonic_device_util.py module

This is the next step toward resolving #4999

Also reverted my previous change in which device_info.get_platform() would first try obtaining the platform ID string from Config DB and fall back to gathering it from machine.conf upon failure because this function is called by sonic-cfggen before the data is in the DB, in which case, the db_connect() call will hang indefinitely, which was not the behavior I expected. As of now, the function will always reference machine.conf.

- How to verify it

Test that all affected Python applications/scripts continue to function properly

- Which release branch to backport (provide reason below if seleted)

  • 201811
  • 201911
  • 202006

@sonic-net sonic-net deleted a comment from lgtm-com bot Jul 27, 2020
@sonic-net sonic-net deleted a comment from lgtm-com bot Jul 27, 2020
@sonic-net sonic-net deleted a comment from lgtm-com bot Jul 27, 2020
@sonic-net sonic-net deleted a comment from lgtm-com bot Jul 27, 2020
@sonic-net sonic-net deleted a comment from lgtm-com bot Jul 27, 2020
@sonic-net sonic-net deleted a comment from lgtm-com bot Jul 27, 2020
@lgtm-com

This comment has been minimized.

@sonic-net sonic-net deleted a comment from lgtm-com bot Jul 28, 2020
@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

@svc-acs
Copy link
Collaborator

svc-acs commented Jul 30, 2020

Build finished. No test results found.

@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

yxieca
yxieca previously approved these changes Jul 30, 2020
@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

@jleveque jleveque requested a review from yxieca August 2, 2020 23:38
@lgtm-com
Copy link

lgtm-com bot commented Aug 3, 2020

This pull request fixes 6 alerts when merging afd7e78 into 8dfc824 - view on LGTM.com

fixed alerts:

  • 6 for Unused import

@lguohan
Copy link
Collaborator

lguohan commented Aug 3, 2020

lgtm, one thing you might want to check is the arista platform, they have their platform code in their own directory, I do not know if you grep their directory or not.

@jleveque
Copy link
Contributor Author

jleveque commented Aug 3, 2020

lgtm, one thing you might want to check is the arista platform, they have their platform code in their own directory, I do not know if you grep their directory or not.

Yes. I grepped the entire repo as well as the submodules. Arista is not importing either sonic_device_util or sonic_daemon_base.

@jleveque jleveque merged commit 3b89e5d into sonic-net:master Aug 3, 2020
@jleveque jleveque deleted the use_sonic-py-common branch August 3, 2020 18:43
jleveque added a commit that referenced this pull request Aug 3, 2020
Consolidate common SONiC Python-language functionality into one shared package (sonic-py-common) and eliminate duplicate code.

The package currently includes four modules:
- daemon_base
- device_info
- logger
- task_base

NOTE: This is a combination of all changes from #5003, #5049 and some changes from #5043 backported to align with the 201911 branch. As part of the 201911 port, I am not installing the Python 3 package in the base image or in the VS container, because we do not have pip3 installed, and we do not intend to migrate to Python 3 in 201911.
santhosh-kt pushed a commit to santhosh-kt/sonic-buildimage that referenced this pull request Feb 25, 2021
…ge (sonic-net#5043)

As part of consolidating all common Python-based functionality into the new sonic-py-common package, this pull request:
1. Redirects all Python applications/scripts in sonic-buildimage repo which previously imported sonic_device_util or sonic_daemon_base to instead import sonic-py-common, which was added in sonic-net#5003
2. Replaces all calls to `sonic_device_util.get_platform_info()` to instead call `sonic_py_common.get_platform()` and removes any calls to `sonic_device_util.get_machine_info()` which are no longer necessary (i.e., those which were only used to pass the results to `sonic_device_util.get_platform_info()`.
3. Removes unused imports to the now-deprecated sonic-daemon-base package and sonic_device_util.py module

This is the next step toward resolving sonic-net#4999

Also reverted my previous change in which device_info.get_platform() would first try obtaining the platform ID string from Config DB and fall back to gathering it from machine.conf upon failure because this function is called by sonic-cfggen before the data is in the DB, in which case, the db_connect() call will hang indefinitely, which was not the behavior I expected. As of now, the function will always reference machine.conf.
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