-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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-cfggen] Allow cfggen to work on system without ports #7999
Conversation
Signed-off-by: liora <liora@nvidia.com>
This reverts commit 75ff366.
237a6ec
to
38e39ac
Compare
Signed-off-by: liora <liora@nvidia.com>
@@ -317,9 +317,6 @@ def main(): | |||
if args.port_config is None: | |||
args.port_config = device_info.get_path_to_port_config_file(hwsku) | |||
(ports, _, _) = get_port_config(hwsku, platform, args.port_config, asic_id) | |||
if not ports: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still applicable. Could you help add a positive testcase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qiluo-msft
While I implemented the unittest I noticed a bug.
I handled the flow of reading the ports from port_config.ini but not the flow of reading the ports from platrfom.json+hwsku.json. Now it is fixed and I added unittest as well.
Appreciate your review.
@@ -317,9 +317,6 @@ def main(): | |||
if args.port_config is None: | |||
args.port_config = device_info.get_path_to_port_config_file(hwsku) | |||
(ports, _, _) = get_port_config(hwsku, platform, args.port_config, asic_id) | |||
if not ports: | |||
print('Failed to get port config', file=sys.stderr) | |||
sys.exit(1) | |||
deep_update(data, {'PORT': ports}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qiluo-msft
Thanks, I changed the code according to your comment, I will explain.
The code I deleted, verified both conditions:
- ports is not None.
- ports is not empty
Since now (in modular chassis without linecards), ports dictionary can be empty, we just need to verify it is not None.
Signed-off-by: liora <liora@nvidia.com>
Signed-off-by: liora <liora@nvidia.com>
Signed-off-by: liora <liora@nvidia.com>
The compilation is failing since test "test_buffers_multi_asic_template " is failing with the error below. Failed test log: Passing test log from my env: test_multinpu_cfggen.py::TestMultiNpuCfgGen::test_buffers_multi_asic_template PASSED [100%] ================================================================================================= 1 passed, 35 deselected in 0.18 seconds ================================================================================================= |
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
/AzurePipelines run |
Commenter does not have sufficient privileges for PR 7999 in repo Azure/sonic-buildimage |
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
@qiluo-msft can you please approve this commit? |
@@ -85,3 +91,10 @@ def test_platform_json_all_ethernet_interfaces(self): | |||
output_dict = ast.literal_eval(output.strip()) | |||
expected = ast.literal_eval(json.dumps(fh_data)) | |||
self.assertDictEqual(output_dict, expected) | |||
|
|||
@mock.patch('portconfig.readJson', mock.MagicMock(return_value={INTF_KEY:{}})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what this file is? Can you please elaborate? Additionally, can you confirm if the format readJson is correct?
Signed-off-by: liora liora@nvidia.com
Why I did it
Allow cfggen to work on system without ports in platform.json or in port_config.ini
How I did it
Add json write of PORT section only if the dictionary that contains the ports is not empty.
How to verify it
sonic-cfggen -k ACS-MSN3700 -H -j /etc/sonic/init_cfg.json --print-data
Which release branch to backport (provide reason below if selected)
Description for the changelog
A picture of a cute animal (not mandatory but encouraged)