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

Missing container in list support in YANG Model #16704 #18091

Merged
merged 4 commits into from
Mar 21, 2024

Conversation

VladimirKuk
Copy link
Contributor

@VladimirKuk VladimirKuk commented Feb 13, 2024

Fixes "Missing container in list support in YANG Model #16704"

Why I did it
Adds support for container in list

How I did it
Identify container in list's leaf and add its data.

Fixes "Update dhcpv6 option yang model" #16290

Why I did it
Adds support for single "choice" statement in container/list

How I did it
Check if choice data is dictionary (instead of list).

How to verify it
Reconstruction details in bug's description.

Tested branch (Please provide the tested image version)
202311
Description for the changelog
Adds support for container in list to yang parsing

Link to config_db schema for YANG module changes
https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/doc/Configuration.md#dhcp_relay

Signed-off-by: vkuk [vkuk@marvell.com]

Add support container in list

Signed-off-by: vkuk <vkuk@marvell.com>
Copy link

linux-foundation-easycla bot commented Feb 13, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@matiAlfaro
Copy link
Contributor

@wen587 - could you review

@wen587 wen587 requested review from qiluo-msft and ganglyu February 22, 2024 13:19
@wen587
Copy link
Contributor

wen587 commented Feb 22, 2024

Can we add some test for this?

@VladimirKuk
Copy link
Contributor Author

I as far as I can see, all tests use existing yang models. And none of them used this feature before. Can I add dummy yang model just for testing ?

@wen587
Copy link
Contributor

wen587 commented Feb 23, 2024

Hi @VladimirKuk , can you check if the buildimage will pass all checks if rebase your PR upon this DHCP PR: #16290

@VladimirKuk
Copy link
Contributor Author

@wen587 This is failing. Apparently, there was no support for single choice in container/list (only multiple). I i will update the PR with the fix.
Please note, that DHCP PR: #16290 as is, will still fail the tests since it is not changing sample_config_db.json.

Add support container in list

Signed-off-by: vkuk <vkuk@marvell.com>
Added test yang model for single/multiple choice(s) in container/list
Add test yang model for container in list

Signed-off-by: vkuk <vkuk@marvell.com>
@VladimirKuk
Copy link
Contributor Author

@wen587 - could you review.
I've added fix for #16290 and tests for both PRs (previously I missed the option of test in sonic-yang-mgmt).

@wen587 wen587 requested a review from kellyyeh February 29, 2024 01:02
@wen587
Copy link
Contributor

wen587 commented Feb 29, 2024

Hi @kellyyeh , could you check if your sonic-buildimage PR checker pass or not after placing your PR upon this PR?

Copy link
Contributor

@wen587 wen587 left a comment

Choose a reason for hiding this comment

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

lgtm

@VladimirKuk
Copy link
Contributor Author

Hi, @qiluo-msft, @ganglyu, Could you review so the PR can be merged ?

@VladimirKuk
Copy link
Contributor Author

@wen587 Is there someone else who can review this ?

@wen587
Copy link
Contributor

wen587 commented Mar 12, 2024

@qiluo-msft please help review or find someone who can review it.

cases = choice['case']
# If single choice exists in container/list
if isinstance(choices, dict):
cases = choices['case']
Copy link
Collaborator

@qiluo-msft qiluo-msft Mar 12, 2024

Choose a reason for hiding this comment

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

case

Is it possible that 'case' is not in choices dict? will it throw? #Closed

Copy link
Contributor Author

@VladimirKuk VladimirKuk Mar 12, 2024

Choose a reason for hiding this comment

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

My understanding that this should be validated by libyang. Since the choice is already checked, at least one case must be present otherwise yang file won't be valid.

cases = choice['case']
# If single choice exists in container/list
if isinstance(choices, dict):
cases = choices['case']
for case in cases:
self._fillLeafDict(case.get('leaf'), leafDict)
Copy link
Collaborator

@qiluo-msft qiluo-msft Mar 12, 2024

Choose a reason for hiding this comment

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

The 2 lines are duplicated, you may consider reusing. #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.

You are right, but throughout the code every time, there are separation between single element (dict) and multiple (list), they are handled separately.

Copy link
Collaborator

Choose a reason for hiding this comment

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

no easy way to reuse.

Signed-off-by: vkuk <vkuk@marvell.com>
@VladimirKuk VladimirKuk requested a review from qiluo-msft March 12, 2024 21:02
@zhangyanzhao zhangyanzhao added the YANG YANG model related changes label Mar 21, 2024
@zhangyanzhao
Copy link
Collaborator

@qiluo-msft will follow-up.

@qiluo-msft qiluo-msft merged commit 0688aa3 into sonic-net:master Mar 21, 2024
19 checks passed
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