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

Various YANG fixes related to choice statements and the consistency checker #1452

Merged
merged 5 commits into from
Jan 21, 2022
Merged

Various YANG fixes related to choice statements and the consistency checker #1452

merged 5 commits into from
Jan 21, 2022

Conversation

eugeneia
Copy link
Member

This fixes a few oversights regarding the handling and validation of choice bodies.
e96ae1a in particular could use a set of eyes familiar with lib.yang. Was there a purpose for the order parameter that I am not seeing?

Cc @wingo @dpino @xray7224

@eugeneia
lib.yang.data: reorder translated members before printing
19072b4 
This fixes a bug in which data_printer_from_grammar would not print choice case
members when an order is given to body_printer.

XXX: it is not clear to me why some invocations of body_printer take orderings,
since it seems that the pre-ordering always matches the internal ordering logic
of body_printer. \o/
@eugeneia
snabb config: use consistency checker in command parsing
ae4f734 
Used to be that configurations read from the command line or stdin bypassed the
consistency checker.
@eugeneia
lib.yang.path_data: expand choices in consistency checker
05b495e 
This updates the consistency checkers to expand choices and visit the bodies of
all cases.
@eugeneia
lib.yang.data: rework 19072b4 (remove order parameter) add tests
e96ae1a 
This removes the order parameter in the affected functions altogether. Test
suite passes so I am assuming this was dead code from the past.
eugeneia added a commit to eugeneia/snabb that referenced this pull request Dec 12, 2019
@eugeneia
Merge branch 'yang-choice-fixes' (PR snabbco#1452) into vita-next
a996824
@eugeneia eugeneia mentioned this pull request Dec 13, 2019
Merged
@dpino
Copy link
Contributor

dpino commented Jan 21, 2020

About the order argument in the body printer, I think the purpose was to print the keys of an output sorted.

You commented on 19072b4 that the pre-ordering always matches the internal ordering logic
of body_printer. If that's the case, I think removing the order argument is fine. But I would suggest checking the body printers on their different formats (YANG, XML and InfluxDB), to confirm the removal doesn't affect the output.

It seems the only printer tested is InfluxDB. For the other two, I would suggest launching a lwAFTR instance and run several config-get commands (there are several examples in Igalia#843)

$ sudo ./snabb lwaftr run --name lwaftr --conf lwaftr.conf --on-a-stick 82:00.1
$ sudo ./snabb config get lwaftr /softwire-config/external-interface
...
$ sudo ./snabb config get --format=xpath lwaftr /softwire-config/binding-table  
...

Consider also querying the whole configuration ('/) or the binding table ('/softwire-config/binding-table'). If the results keep sorted, removing the order arguments seems right.

It would be great also (probably in an future PR) to add tests for the YANG and XML printers, like the one it currently exists for InfluxDB.

@dpino
Copy link
Contributor

dpino commented Jan 21, 2020

FWIW, the other changes LGTM.

@eugeneia eugeneia mentioned this pull request Nov 18, 2021
Merged
@eugeneia eugeneia merged commit 1360e0b into snabbco:master Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpino dpino dpino approved these changes

Assignees
No one assigned
Labels
merged release-octarina
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eugeneia @dpino