Re: [ippm] Yangdoctors early review of draft-ietf-ippm-ioam-yang-03

[Tommy Pauly <[e-mail]>]

Thanks for the review, Andy!

Authors, can you create a revision of the document to incorporate this feedback?

Best,
Tommy

> On May 3, 2022, at 3:54 PM, Andy Bierman via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Andy Bierman
> Review result: Ready with Issues
> 
> 
> Draft: draft-ietf-ippm-ioam-yang-03
> Module: ietf-ioam
> Revision: 2022-01-25
> 
> Major Issues: none
> 
> Minor Issues:
> 
> 1) when-stmts using identities
> 
> The preferred method is to use the 'derived-from-or-self'
> https://datatracker.ietf.org/doc/html/rfc7950#section-10.4.2
> 
> OLD:
> 
>  when "../filter-type = 'ioam:acl-filter'";
> 
> 
> NEW:
> 
>  when "derived-from-or-self(../filter-type, 'ioam:acl-filter')";
> 
> This correction allows for derived identities,
> not just the specified identity.  The problem is that the
> prefix to module mapping is only defined for the XPath function,
> not for a plain string. The 'ioam' prefix must be resolved correctly
> by the YANG compiler.
> 
> 2)  /ioam/ioam-profiles/admin-config/enabled leaf description
> 
>        description
>          "When true, IOAM configuration is enabled for the system.
>           Meanwhile, the IOAM data-plane functionality is enabled.";
> 
> This object needs more clarification.
> 
>  - this object must be true before anything in the
>    /ioam/ioam-profiles/ioam-profile subtree can be edited.
>  - This means it cannot be changed in the same edit as the
>    objects it affects.
>  - if false, any config in place is used (is this true?)
>  - The 2nd sentence is not clear how it relates to the YANG module.
>  - Need to be clear (in terms of YANG datastore editing)
>    what true or false exactly means.
> 
> 
> 3) Terse descriptions
> 
> A few description-stmts are too terse.
> There must be something that can be said in a full sentence.
> 
>      leaf node-action {
>        type ioam-node-action;
>        description "node action";
>      }
> 
> 4) Use interface-ref data type
> 
> Object /ioam/ioam-info/available-interface/if-name
> 
> OLD:
> 
>     type leafref {
>        path "/if:interfaces/if:interface/if:name";
>     }
> 
> NEW:
> 
>     type if:interface-ref;
> 
> 5) Use of ordered-by user
> 
> List /ioam/ioam-profiles/ioam-profile is user-ordered.
> Describe all server processing requirements that are
> related to the order of these list entries.
> 
> 6) Use of plain 'string' as a list key
> 
> List key /ioam/ioam-profiles/ioam-profile/profile-name is
> an unconstrained string.
> 
> Most servers do not allow corner-case string values
> (such as an empty string).
> 
> Consider using yang:yang-identifier type or at least
> length 1..max to disallow empty strings.
> 
> 7) No mandatory functionality
> 
> There is a YANG feature for each profile so all profiles
> are optional-to-implement.
> 
> Is this because there is no WG agreement on any mandatory
> functionality?  Some explanation that a server is expected
> to support at least one profile should be added in this case.
> 
> 
> Nits
> ------
> 
> 1) Some description text starts with a lowercase letter,
>   and should be capitalized.
> 
> 2) Some sentences start with 'And ...' which could be
>   added to the previous sentence instead.
> 
> 3) Remove the 'mandatory true;' statement in leaf profile-name.
>   A list key leaf is already mandatory.
> 
> 
> 
> _______________________________________________
> ippm mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/ippm