search

IETF-TEAS-WG / ietf-teas-yang-path-computation

0 stars 4 forks source link

Yangdoctors early review of draft-ietf-teas-yang-path-computation-11 #83

Closed italobusi closed 3 years ago

italobusi commented 3 years ago

See https://mailarchive.ietf.org/arch/msg/teas/0P4BAX5Rb-qOVSRr1ODqZEUwqhU/

Reviewer: Martin Björklund Review result: Ready with Nits

o General

The language is called "YANG", not "Yang".

Authors> Ok

o 1.2. Tree Diagram

The text says:

A simplified graphical representation of the data model is used in
section 6.1 of this this document.  The meaning of the symbols in
these diagrams is defined in [RFC8340].

Tree diagrams are used also in chapter 5. I suggest:

Tree diagrams used in this document follow the notation defined in
[RFC8340].

Authors> OK

o Tree diagrams in general

You can use pyang -f tree --tree-line-length 68 ... in order to avoid long lines in the RFC.

Authors> we are using the same pyang string but with 69 limit, but it seems not always (or for all lines) working. Why 68 and not 69? Why something should change?

o 6.1

This section presents a fully expanded tree diagram of the module. Tree diagrams are mainly used to give an overview of a module's structure. The tree diagram in this section spans 13 pages and is quite hard to read.

I also note that a majority of the nodes in this tree diagram come from the expansion of groupings that aren't defined in this document. Hence, I suggest that you might want to run:

pyang -f tree --tree-line-length 68 \
  --tree-print-groupings --tree-no-expand-uses

Authors> we have tried but it seems that also the groupings defined in the module are not expanded and we think at least these groupings should be expanded in the tree.

o There are a number of groupings that are used only once, and do not seem to be defined to be reused by other modules, e.g., "requested-info", "requested-state", "svec-metrics-bounds" and more.

If they are intended to be reused, it should be made clear in their description statements. If not, I think they should be inlined and removed.

Authors> We think that it is sometimes necessary to introduce groupings even if they are not re-used to make the YANG code structured and readable. This is the same logic in other programming, where "helper" functions are used to code complex logics.

Authors> Following this approach we have kept the following groupings:

and removed the following groupings:

o grouping svec-exclude

This grouping has an ordered-by user list. Why is this list user ordered? If the order matters, it should be explained how it matters.

Authors> you're right. Deleted "ordered-by user list". No reason for that.

Also, the index leaf has this description:

 "XRO subobject index"

What is "XRO"? Is this description sufficiently clear?

Authors> We have removed the index since lists in rpc input do not need it

o path-request

In the path-request, there is construct for path-refs:

               list primary-reverse-path-ref {
                 key index;
                 min-elements 1;
                 description
                   "The list of primary reverse paths that
                    reference this path as a candidate
                    secondary reverse path";
                 leaf index {
                   type uint32;
                   description
                     "The index used by the
                      primary-reverse-path-ref list";
                 }

What is this index? Is it only used as an arbitrary index, or something else? If it is an arbitraty index, it should be explained in the descriptions.

Also note that lists in rpc input don't need an index.

Authors> OK, deleted. We have removed index both in "primary reverse path ref" and "primary path ref".

o Validation

The module fails YANG validation, but that is really due to errors in ietf-te@2020-07-12.yang. Specifially, the leafref in the grouping "path-compute-info" must have prefixes in its path. Without prefixes, the path refers to nodes in the module that uses the grouping. (same for other groupings in that module).

o Layout

I suggest you run the module through

pyang -f yang --yang-canonical --yang-line-length 68

in order to have the module indented and formatted consistently.

This will make the RFC editor's job easier.

Authors> We are using " pyang --strict --ietf --max-line-length 69 -f tree --tree-line-length=69 -o ietf-te-path-computation.tree ietf-te-path-computation.yang". Is something wrong with this string command?

/martin

italobusi commented 3 years ago

See also Tom Petch's input: https://mailarchive.ietf.org/arch/msg/teas/3wyfP3_xl-6od_yMafsCVs-a_gE/

Just to make it more obvious what Martin is saying, teas-yang-te-25 is invalid. I think that the invalid grouping that Martin mentions that causes validation to fail first appeared in -23 (along with so much else that I have yet to catch up with it:-(

Tom Petch

tsaad-dev commented 3 years ago

Updated tsaad-dev/te#114 Please take a look.