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

On Wed, 2017-12-06 at 12:46 +0100, Martin Bjorklund wrote:
> 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?

Yes, but I meant the complete data model including the existing modules that
must be implemented along with "ietf-ospf", such as "ietf-interface", "ietf-
routing" and "ietf-keychain". I think it is a higher league of data modelling
compared to a stand-alone module - kudos to the authors.

> 
> 
> 
> 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).

Right, the revisions make the module excessively long.

> 
> 
> 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.

Agreed.

Thanks, Lada

> 
> 
> /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
> > 
-- 
Ladislav Lhotka
Head, CZ.NIC Labs
PGP Key ID: 0xB8F92B08A9F76C67