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

[sonic-cfggen] Allow cfggen to work on system without ports #7999

Merged
merged 6 commits into from
Aug 23, 2021
Merged

[sonic-cfggen] Allow cfggen to work on system without ports #7999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dd95237
[sonic-buildimage] Update submodules for zero ports support
liorghub Jun 13, 2021
e662e65
Revert "[sonic-buildimage] Update submodules for zero ports support"
liorghub Jun 28, 2021
db76a6f
[sonic-cfggen] Allow cfggen to work on system without ports
liorghub Jun 13, 2021
fd1f5ce
[sonic-cfggen] Allow cfggen to work on system without ports
liorghub Jun 13, 2021
668db75
[sonic-cfggen] Allow cfggen to work on system without ports
liorghub Jun 13, 2021
95abadd
[sonic-cfggen] Allow cfggen to work on system without ports
liorghub Jun 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/sonic-config-engine/portconfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,9 @@ def parse_platform_json_file(hwsku_json_file, platform_json_file):
port_dict = readJson(platform_json_file)
hwsku_dict = readJson(hwsku_json_file)

if not port_dict:
if port_dict is None:
raise Exception("port_dict is none")
if not hwsku_dict:
if hwsku_dict is None:
raise Exception("hwsku_dict is none")

if INTF_KEY not in port_dict or INTF_KEY not in hwsku_dict:
Expand All @@ -285,8 +285,8 @@ def parse_platform_json_file(hwsku_json_file, platform_json_file):

ports.update(child_ports)

if not ports:
raise Exception("Ports dictionary is empty")
if ports is None:
raise Exception("Ports dictionary is None")

for i in ports.keys():
port_alias_map[ports[i]["alias"]]= i
Expand Down
2 changes: 1 addition & 1 deletion src/sonic-config-engine/sonic-cfggen
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ 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:
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ports

This is a new feature or significant fix. Could you add new testcase to protect it? #Closed

Copy link
Collaborator

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am on it.

Copy link
Contributor Author

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.

if ports is None:
print('Failed to get port config', file=sys.stderr)
sys.exit(1)
deep_update(data, {'PORT': ports})
Copy link
Collaborator

@qiluo-msft qiluo-msft Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ports

Does it work with:

  1. ports == None
  2. Or, ports == {}

If only one, let's use the explicit contant. #Closed

Copy link
Contributor Author

@liorghub liorghub Jul 26, 2021 •
edited
Loading

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:

  1. ports is not None.
  2. 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.

Expand Down
13 changes: 13 additions & 0 deletions src/sonic-config-engine/tests/test_cfggen_platformJson.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,17 @@
import json
import os
import subprocess
import sys

import tests.common_utils as utils

from unittest import TestCase
from portconfig import get_port_config, INTF_KEY

if sys.version_info.major == 3:
from unittest import mock
else:
import mock

# Global Variable
PLATFORM_OUTPUT_FILE = "platform_output.json"
Expand Down Expand Up @@ -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:{}}))
Copy link
Collaborator

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?

@mock.patch('os.path.isfile', mock.MagicMock(return_value=True))
def test_platform_json_no_interfaces(self):
(ports, _, _) = get_port_config(port_config_file=self.platform_json)
self.assertNotEqual(ports, None)
self.assertEqual(ports, {})