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

[YANG] Enhance the port yang model with new port fields: adv_speeds, interface_type and adv_interface_types #6948

Merged
merged 11 commits into from
May 20, 2021

Conversation

Junchao-Mellanox
Copy link
Collaborator

@Junchao-Mellanox Junchao-Mellanox commented Mar 3, 2021

- Why I did it
Enhance the port yang model with new port fields: adv_speeds, interface_types and adv_interface_types
Refer to HLD sonic-net/SONiC#732

- How I did it
Add fields adv_speeds, interface_type, adv_interface_types

- How to verify it
Add yang unit test and verify build is not broken.

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

  • 201811
  • 201911
  • 202006
  • 202012

- Description for the changelog

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

@lguohan lguohan added the YANG YANG model related changes label Mar 3, 2021
@Junchao-Mellanox
Copy link
Collaborator Author

Hi @lguohan, @praveen-li, could you please review again? Thanks.

@lguohan
Copy link
Collaborator

lguohan commented Mar 4, 2021

@Junchao-Mellanox, there are unanswered question, can you address them?

@Junchao-Mellanox
Copy link
Collaborator Author

Hi @lguohan, @praveen-li, could you please review again? Thanks.

@Junchao-Mellanox
Copy link
Collaborator Author

Hi @lguohan , could you please kindly review again?

@lguohan
Copy link
Collaborator

lguohan commented Mar 28, 2021

use the keyword description, please refer to the yang model.

@lguohan
Copy link
Collaborator

lguohan commented Apr 1, 2021

can you resolve the conflict? any update on the comments?

Conflicts:
	src/sonic-yang-models/tests/yang_model_tests/test_yang_model.py
	src/sonic-yang-models/tests/yang_model_tests/yangTest.json
…x/sonic-buildimage into port-auto-neg-yang-model

Conflicts:
	src/sonic-yang-models/yang-models/sonic-port.yang
@Junchao-Mellanox
Copy link
Collaborator Author

Hi @lguohan, I fixed all comment, could you please help review again? Thanks,

@liat-grozovik
Copy link
Collaborator

@praveen-li can you please approve? if it is we can go a head and merge.

@lguohan
Copy link
Collaborator

lguohan commented Apr 8, 2021

need to change the title to reflect the pr

@liat-grozovik liat-grozovik changed the title [port auto-neg] Add yang model for port auto-neg feature [YANG] Enhance the port yang model with new port fields: adv_speeds, interface_types and adv_interface_types Apr 8, 2021
@liat-grozovik liat-grozovik changed the title [YANG] Enhance the port yang model with new port fields: adv_speeds, interface_types and adv_interface_types [YANG] Enhance the port yang model with new port fields: adv_speeds, interface_type and adv_interface_types Apr 8, 2021
@liat-grozovik
Copy link
Collaborator

need to change the title to reflect the pr

done. please recheck

@liat-grozovik
Copy link
Collaborator

@praveen-li and @lguohan any further comments?
We are ready with the PRs of the feature and we would like to have the HLD closed before it.


type union {
type uint32 {
range 1..400000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

So there is no other constraint on adv speed?, it can take any number of values. Kindly point to the correct paragraph in HLD for this plz, I will have a look. Thx

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, there is no other constraint for adv_speeds, I am following the field "speed" in the same yang model file. HLD: https://github.com/Junchao-Mellanox/SONiC/blob/port-auto-neg/doc/port_auto_neg/port-auto-negotiation-design.md#config-advertised-speeds

}

leaf-list adv_interface_types {
description "Port advertised interface type, valid value could be a list of stypes:interface_type or all";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kindly point to the correct paragraph in HLD here in comment, thanks again

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I cannot add the HLD paragraph to the comment here because HLD is not approved yet. And HLD approval requires YANG model PR approval.

@Junchao-Mellanox
Copy link
Collaborator Author

Hi @praveen-li, could you please review again?

@anshuv-mfst
Copy link

Hi @prsunny - could you please take a look, thanks.

@keboliu
Copy link
Collaborator

keboliu commented Apr 25, 2021

@prsunny @praveen-li would you please have a look?

Copy link
Collaborator

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

:shipit:

@lguohan
Copy link
Collaborator

lguohan commented May 4, 2021

@praveen-li , lgtm, can you also check?

@anshuv-mfst
Copy link

@praveen-li - could you please review as well.

@keboliu
Copy link
Collaborator

keboliu commented May 11, 2021

@praveen-li would you please check?

@anshuv-mfst
Copy link

Hi @praveen-li - Can you please review and approve at the earliest.

@lguohan lguohan merged commit e17e9f4 into sonic-net:master May 20, 2021
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
…interface_type and adv_interface_types (sonic-net#6948)

Enhance the port yang model with new port fields: adv_speeds, interface_types and adv_interface_types
Refer to HLD sonic-net/SONiC#732
@Junchao-Mellanox Junchao-Mellanox deleted the port-auto-neg-yang-model branch June 12, 2023 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
YANG YANG model related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants