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

[L2 switch mode] Update l2switch.j2 template #5981

Merged
merged 4 commits into from
Nov 21, 2020
Merged

[L2 switch mode] Update l2switch.j2 template #5981

merged 4 commits into from
Nov 21, 2020

Conversation

shi-su
Copy link
Contributor

@shi-su shi-su commented Nov 19, 2020 •
edited by qiluo-msft
Loading

- Why I did it
The l2switch.j2 template does not include all fields for PORT. This could be incompatible with the 201911 image or later.

- How I did it
Update l2switch.j2 template and add a unit test.

- How to verify it

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

  • 201811
  • 201911
  • 202006

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@shi-su shi-su marked this pull request as draft November 19, 2020 22:50
@lguohan lguohan requested a review from qiluo-msft November 19, 2020 22:55
qiluo-msft
qiluo-msft reviewed Nov 19, 2020
View reviewed changes
src/sonic-config-engine/data/l2switch.j2 Outdated
@@ -6,8 +6,11 @@
{%- if ns.firstPrinted %},{% endif %}

"{{ key }}": {
"alias": "{{ value.alias }}",
"lanes": "{{ value.lanes }}",
{%- for keyPort,valuePort in value.items() %}
Copy link
Collaborator

@qiluo-msft qiluo-msft Nov 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

keyPort [](start = 20, length = 7)

If keyPort == 'admin_status', is it a problem? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added check to resolve the scenario with admin_status exists.

qiluo-msft
qiluo-msft reviewed Nov 19, 2020
View reviewed changes
src/sonic-config-engine/data/l2switch.j2 Outdated
@@ -6,8 +6,11 @@
{%- if ns.firstPrinted %},{% endif %}

"{{ key }}": {
"alias": "{{ value.alias }}",
"lanes": "{{ value.lanes }}",
{%- for keyPort,valuePort in value.items() %}
Copy link
Collaborator

@qiluo-msft qiluo-msft Nov 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

, [](start = 27, length = 1)

Add a blank after ,. Also some more occurrences in the whole file. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Nov 19, 2020 •
edited
Loading 

    argument = '-k Mellanox-SN2700 --preset l2 -p ' + self.t0_port_config

Could you add another test case for Mellanox-SN3800-D112C8 in the same way? #WontFix

Refers to: src/sonic-config-engine/tests/test_j2files.py:99 in dd1f4d9. [](commit_id = dd1f4d9, deletion_comment = False)

qiluo-msft
qiluo-msft requested changes Nov 19, 2020
View reviewed changes
Copy link
Collaborator

@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.

As comments

@shi-su
Copy link
Contributor Author

shi-su commented Nov 20, 2020 •
edited
Loading 

    argument = '-k Mellanox-SN2700 --preset l2 -p ' + self.t0_port_config

Could you add another test case for Mellanox-SN3800-D112C8 in the same way?

Refers to: src/sonic-config-engine/tests/test_j2files.py:99 in dd1f4d9. [](commit_id = dd1f4d9, deletion_comment = False)

I think I understand your point: different device types have different sources of port configuration data, and it would be better to capture the difference. However, in this particular test, the data source is explicitly specified (tests/t0-sample-port-config.ini). So only changing the device type cannot test out the sources of port configuration data, and in this test setup, it does not seem the source of port configuration data could follow the -k <device> option. In using the command in SONiC, the -k <device> option should specify the data, and rendering the template should not be dependent on what kind of device it is. If you think the test case is indeed necessary, I think I need to copy the port config ini file for the device here and write another ground-truth json file for it. Let me know what you think.

@shi-su shi-su marked this pull request as ready for review November 20, 2020 05:38
@shi-su
Copy link
Contributor Author

shi-su commented Nov 20, 2020

retest vsimage please

@shi-su shi-su merged commit e0781f4 into sonic-net:master Nov 21, 2020
@shi-su shi-su deleted the l2switch branch November 21, 2020 19:35
abdosi pushed a commit that referenced this pull request Dec 16, 2020
@shi-su @abdosi
[L2 switch mode] Update l2switch.j2 template (#5981)
ff1a60f 
- Why I did it
The l2switch.j2 template does not include all fields for PORT. This could be incompatible with the 201911 image or later.

- How I did it
Update l2switch.j2 template and add a unit test.
santhosh-kt pushed a commit to santhosh-kt/sonic-buildimage that referenced this pull request Feb 25, 2021
@shi-su @santhosh-kt
[L2 switch mode] Update l2switch.j2 template (sonic-net#5981)
f5ae923 
- Why I did it
The l2switch.j2 template does not include all fields for PORT. This could be incompatible with the 201911 image or later.

- How I did it
Update l2switch.j2 template and add a unit test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiluo-msft qiluo-msft qiluo-msft approved these changes

Assignees
No one assigned
Labels
Included in 201911 Branch Request for 201911 Branch :shipit: Request for 202006 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shi-su @qiluo-msft @abdosi