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

Fix yang validation failure when table contains empty value #10431

Merged
merged 5 commits into from
Apr 7, 2022

Conversation

wen587
Copy link
Contributor

@wen587 wen587 commented Apr 1, 2022

Why I did it

Fix #9746

How I did it

Split the check condition based on non-exist and zero length.

How to verify it

Run verification script when table contains empty value

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111

Description for the changelog

Link to config_db schema for YANG module changes

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

@@ -593,7 +593,11 @@ def _xlateContainerInContainer(self, model, yang, configC, table):
ccontainer = model
#print(ccontainer['@name'])
yang[ccontainer['@name']] = dict()
if not configC.get(ccontainer['@name']):
if configC.get(ccontainer['@name']) is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

configC.get(ccontainer['@name']) is None

How about

if ccontainer['@name'] not in configC:

BTW, you can extract ccontainer['@name'] as a variable, no need to calculate many times.

And remove #print above.

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.

LGTM

if not configC.get(ccontainer['@name']):
ccName = ccontainer['@name']
yang[ccName] = dict()
if ccName not in configC:
Copy link
Collaborator

@praveen-li praveen-li Apr 4, 2022

Choose a reason for hiding this comment

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

JFMI: Inner container does not exist in config.

Copy link
Collaborator

@praveen-li praveen-li left a comment

Choose a reason for hiding this comment

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

One Dev Changes is needed.

Will it be possible for you to add a test case for this please, may be in same reference under which you found the issue. Or simply by adding a config in https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-yang-models/tests/files/sample_config_db.json

self.sysLog(msg="xlateProcessListOfContainer: {}".format(ccontainer['@name']))
self._xlateContainer(ccontainer, yang[ccontainer['@name']], \
configC[ccontainer['@name']], table)
if len(ccName) == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

if len(configC[ccName]) ==0:

i.e if value is zero, delete the key. Here, delete the key means processing is done,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I saw TACPLUS table already has configuration in sample_config_db.json.
I added a UT for field global with empty value.
From where I saw the issue, it failed at GCU test after other test configured TACPLUS table.
I also added a empty json apply GCU test sonic-net/sonic-mgmt#5459 for simply checking the configDB.

@@ -304,7 +304,7 @@
"switch_id": "2",
"switch_type": "voq",
"max_cores": "8",
"sub_role": "FrondEnd",
Copy link
Collaborator

Choose a reason for hiding this comment

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

[10:18 PM] Praveen Chaudhary
in this file: src/sonic-yang-models/tests/files/sample_config_db.json

[10:18 PM] Praveen Chaudhary
we need global {} config for test to exercise this code

[10:18 PM] Praveen Chaudhary
plz add that and we are good

Copy link
Collaborator

@praveen-li praveen-li left a comment

Choose a reason for hiding this comment

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

LGTM

@wen587 wen587 merged commit d83ae1e into sonic-net:master Apr 7, 2022
liushilongbuaa pushed a commit to liushilongbuaa/sonic-buildimage that referenced this pull request Jun 20, 2022
Related work items: #49, #58, #107, sonic-net#247, sonic-net#249, sonic-net#277, sonic-net#593, sonic-net#597, sonic-net#1035, sonic-net#2130, sonic-net#2150, sonic-net#2165, sonic-net#2169, sonic-net#2178, sonic-net#2179, sonic-net#2187, sonic-net#2188, sonic-net#2191, sonic-net#2195, sonic-net#2197, sonic-net#2198, sonic-net#2200, sonic-net#2202, sonic-net#2206, sonic-net#2209, sonic-net#2211, sonic-net#2216, sonic-net#7909, sonic-net#8927, sonic-net#9681, sonic-net#9733, sonic-net#9746, sonic-net#9850, sonic-net#9967, sonic-net#10104, sonic-net#10152, sonic-net#10168, sonic-net#10228, sonic-net#10266, sonic-net#10288, sonic-net#10294, sonic-net#10313, sonic-net#10394, sonic-net#10403, sonic-net#10404, sonic-net#10421, sonic-net#10431, sonic-net#10437, sonic-net#10445, sonic-net#10457, sonic-net#10458, sonic-net#10465, sonic-net#10467, sonic-net#10469, sonic-net#10470, sonic-net#10474, sonic-net#10477, sonic-net#10478, sonic-net#10482, sonic-net#10485, sonic-net#10488, sonic-net#10489, sonic-net#10492, sonic-net#10494, sonic-net#10498, sonic-net#10501, sonic-net#10509, sonic-net#10512, sonic-net#10514, sonic-net#10516, sonic-net#10517, sonic-net#10523, sonic-net#10525, sonic-net#10531, sonic-net#10532, sonic-net#10538, sonic-net#10555, sonic-net#10557, sonic-net#10559, sonic-net#10561, sonic-net#10565, sonic-net#10572, sonic-net#10574, sonic-net#10576, sonic-net#10578, sonic-net#10581, sonic-net#10585, sonic-net#10587, sonic-net#10599, sonic-net#10607, sonic-net#10611, sonic-net#10616, sonic-net#10618, sonic-net#10619, sonic-net#10623, sonic-net#10624, sonic-net#10633, sonic-net#10646, sonic-net#10655, sonic-net#10660, sonic-net#10664, sonic-net#10680, sonic-net#10683
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[yang] validating TACPLUS with empty “global” fails, while non-empty “global” does not fail
3 participants