Re: [yang-doctors] Yangdoctors last call review of draft-ietf-ospf-yang-09

Ladislav Lhotka <lhotka@nic.cz> wrote:
> Reviewer: Ladislav Lhotka
> Review result: Ready with Issues
> 
> The data model defined in this document is a massive piece of work: it
> consists of 11 YANG modules and defines around 1200 schema nodes. The

11 YANG modules?  Isn't there just one?

I would like to add two quick comments to Lada's review.

o  Remove all "revision" statements except the one that says "initial
   revision".   The idea is to have one revision statement per
   published version (i.e., RFC).

o  Many of the nodes have quite rudimentary descriptions, and the
   "reference" statement is rarely used.  For example:

         leaf lsa-id {
           type yang:dotted-quad;
           mandatory true;
           description "LSA ID.";
         }

   It seems as the description is put there just to keep the tools
   quiet.  I know that it is painful to write good descriptions, but
   it does help the consumer of the data model.

/martin

> "ietf-ospf@2017-10-30" module is compatible with the NMDA architecture.
> 
> **** Comments
> 
> 1. Unless there is a really compelling reason not to do so, the
>    "ietf-ospf" should declare YANG version 1.1. For one,
>    "ietf-routing" that is being augmented by "ietf-ospf" already
>    declares this version. Some of my suggestions below also assume
>    version 1.1.
> 
> 2. The "ietf-ospf" can work only with the new NMDA-compatible
>    revisions of some modules, such as "ietf-interfaces" and
>    "ietf-routing". I understand it is not desirable to import such
>    modules by revision, but at least it should be mentioned in a
>    description attached to every such import.
> 
> 3. Maybe the draft could mention that implementations should supply a
>    default routing domain as a system-controlled resource.
> 
> 4. In "when" expressions, the module uses literal strings for
>    identities. This is known to be problematic, the XPath functions
>    derived-from() or derived-from-or-self() should be used instead.
> 
> 5. Some enumerations, such as "packet-type" and "if-state-type"
>    define enum identifiers with uppercase letters and/or underscores,
>    for example "Database-Description" or "LONG_WAIT". RFC6087bis
>    recommends that only lowercase letters, numbers and dashes. I think
>    this convention should be observed despite the fact that the
>    current names are traditionally used in OSPF specs. The
>    "ietf-routing" module also defines "router-id" even though the
>    documents use "Router ID".
> 
> 6. The types of LSA headers are modelled as integers. While OSPF gurus
>    probably know these numbers by heart, it is not very
>    reader-frienly. So at least some references to documents defining
>    these numbers should be provided, but my suggestion is to consider
>    implementing them with identities. It seems it might also be useful
>    to define some "abstract" identities for these types. For example,
>    if "opaque-lsa" is defined, then the definition of container
>    "opaque" could simply use
> 
>      when "derived-from(../../header/type, 'ospf:opaque-lsa')";
> 
>    instead of
> 
>       when "../../header/type = 9 or "
>               + "../../header/type = 10 or "
>               + "../../header/type = 11";
> 
> 7. The title of sec. 2.9 should be "OSPF Notifications" rather than
>    "OSPF notification".
> 
> 
> _______________________________________________
> yang-doctors mailing list
> yang-doctors@ietf.org
> https://www.ietf.org/mailman/listinfo/yang-doctors
> 