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

[SONiC] Design and test plan document for refactoring port_config.ini parse code #613

Merged
merged 4 commits into from
Jun 3, 2020

Conversation

Junchao-Mellanox
Copy link
Contributor

Why I did this?

portconfig.py is designed to manage and parse port_config.ini file. But there are a few existing code implement its own logic to get port_config.ini path or parse the content of port_config.ini. We should try re-use portconfig.py as possible as we can.

What I did?

  1. Design document for refactoring port_config.ini parsing code
  2. Simple test plan to test the change

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

LGTM

[portconfig.py](https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/portconfig.py) is defined in [sonic-config-engine](https://github.com/Azure/sonic-buildimage/tree/master/src/sonic-config-engine). In order to reuse this module:

1. For those modules who have build-time unit test, they may declare sonic-config-engine as a build-time dependency in make rules.
2. For those modules who are installed in a docker, we must make sure sonic-config-engine is also installed in that docker, for example, pmon docker need reuse portconfig.py and it has sonic-config-engine installed.
Copy link
Contributor

@jleveque jleveque May 20, 2020

Choose a reason for hiding this comment

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

I have had the idea for a while now that portconfig.py, minigraph.py, etc. should be pulled out of the sonic-config-engine package and placed in their own package. Like we now have sonic-daemon-base, I believe we should have a package like, sonic-utils (however, we need a different name because this is too similar to sonic-utilities) which is basically a library of functionality that is used throughout SONiC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. Should we do it within this feature or a seperate feature?

@zhenggen-xu
Copy link
Collaborator

@Junchao-Mellanox Can you please take a look at these PRs to see if your points were already covered?

sonic-net/sonic-utilities#767
sonic-net/sonic-buildimage#3909
sonic-net/sonic-platform-common#72

@qiluo-msft
Copy link
Contributor

@Junchao-Mellanox could you response @zhenggen-xu 's comment #613 (comment) ?

Copy link
Contributor

@daall daall left a comment

Choose a reason for hiding this comment

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

LGTM, please address @zhenggen-xu's question before merging please

@qiluo-msft qiluo-msft self-assigned this Jun 2, 2020
@Junchao-Mellanox
Copy link
Contributor Author

@Junchao-Mellanox Can you please take a look at these PRs to see if your points were already covered?

Azure/sonic-utilities#767
Azure/sonic-buildimage#3909
Azure/sonic-platform-common#72

Yes, those PRs cover a lot of the current design except that:

  1. Change port_config.ini port index start from 0 to 1 for mellanox platform
  2. Reuse port_config.ini/platform.json parse logic in sonic-platform-commn
  3. And there are two files need change to handle the port_config.ini/platform.json file parth properly:

@qiluo-msft qiluo-msft merged commit d6239d4 into sonic-net:master Jun 3, 2020
kartik-arista pushed a commit to kartik-arista/SONiC that referenced this pull request Jul 16, 2020
… parse code (sonic-net#613)

* add design document to refactor port config logic
* Add a simple test plan for this enhancement
* Add description for changing port index start from 1
* Add more detail for the test plan
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.

5 participants