Skip to content

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

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

Merged
qiluo-msft merged 6 commits into from
Aug 23, 2021
Merged

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

Changes from 3 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
3 changes: 0 additions & 3 deletions src/sonic-config-engine/sonic-cfggen
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

@qiluo-msft qiluo-msft Jul 26, 2021

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
Copy Markdown
Contributor Author

@liorghub liorghub Jul 26, 2021

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
Copy Markdown
Contributor Author

@liorghub liorghub Aug 3, 2021

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.

print('Failed to get port config', file=sys.stderr)
sys.exit(1)
deep_update(data, {'PORT': ports})
Copy link
Copy Markdown
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
Copy Markdown
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.


brkout_table = get_breakout_mode(hwsku, platform, args.port_config)
Expand Down