Re: [i2rs] [yang-doctors] Yangdoctors last call review of draft-ietf-i2rs-yang-network-topo-14

Dear all,

Now implemented in the yangcatalog.org, we have the tree-type metadata.
from https://github.com/xorrkaz/netmod-yang-catalog, to be published 
soon in https://datatracker.ietf.org/doc/draft-clacla-netmod-model-catalog/

      leaf tree-type {
          type enumeration {
            enum split {
              description
                "This module uses a split config/operational state layout.";
            }
            enum nmda-compatible {
              description
                "This module is compatible with the Network Management
    Datastores
                 Architecture (NMDA) and combines config and operational
    state nodes.";
            }
            enum transitional-extra {
              description
                "This module is derived as a '-state' module to allow
    for transitioning
                 to a full NMDA-compliant tree structure.";
            }
            enum openconfig {
              description
                "This module uses the Openconfig data element layout.";
            }
            enum unclassified {
             description
               "This module does not have a data element tree, or it
    does not belong to any category.";
            }
            enum not-applicable {
              description
                "This module is submodule.";
            }
          }
          description
            "The type of data element tree used by the module as it
    relates to the
             Network Management Datastores Architecture.";
          reference
            "draft-dsdt-nmda-guidelines Guidelines for YANG Module
    Authors (NMDA)

Happy to see that the two following I2RS modules are "nmda-compatible"
https://yangcatalog.org:8443/search/modules/ietf-network,2017-06-30,ietf
https://yangcatalog.org:8443/search/modules/ietf-network-topology,2017-06-30,ietf
The following two are: transactional-extra
https://yangcatalog.org:8443/search/modules/ietf-network-state,2017-06-30,ietf
https://yangcatalog.org:8443/search/modules/ietf-network-state,2017-06-30,ietf

We'll work on a nicer GUI later one. The key point for now: the 
tree-type metadata is populated.

Regards, Benoit
> Reviewer: Kent Watsen
> Review result: Almost Ready
>
>
> YANG Doctor review of draft-ietf-i2rs-yang-network-topo-14 (by Kent Watsen)
>
>
> 4 modules are defined in the draft:
> - ietf-network@2017-06-30.yang
> - ietf-network-topology@2017-06-30.yang
> - ietf-network-state@2017-06-30.yang
> - ietf-network-topology-state@2017-06-30.yang
>
> No validation errors from either `pyang` or `yanglint`.
>
> 0 examples are defined in the draft.
>
> The -state modules appear to have all the correct changes
> from their base modules.
>
> Regarding ietf-network@2017-06-30.yang:
> - prefix “nd”, should be “nw”?  (in IANA Considerations also)
> - “import ietf-inet-types” should have a ‘reference’ to RFC 6991
> - remove “WG Chairs”, per the template in rfc6087bis-13 Appendix C.
> - s/Editor/Author/? (this is a preference choice)
> - defines ‘network-ref’ that is unused within module (it’s used by
>    ietf-network-topology)
> - leafref paths don’t include prefix (rfc6087bis S4.2)
>
> Regarding ietf-network-topology@2017-06-30.yang:
> - prefix “lnk” should be “nt” or maybe “nwtp”? (in IANA Considerations also)
> - “import ietf-inet-types” should have a ‘reference’ to RFC 6991
> - “import ietf-network” should have a ‘reference’ to RFC XXXX
> - remove “WG Chairs”, per the template in rfc6087bis-13 Appendix C.
> - s/Editor/Author/? (this is a preference choice)
> - defines grouping link-ref and tp-ref that are unused within module
> - mandatory true for source/dest-node/tp?
> - replace “tp” with “term-pt”?  (both in “-tp” and “tp-“ uses)
>
> Regarding ietf-networ-statek@2017-06-30.yang:
> - similar comments to ietf-network@2017-06-30.yang:
>
> Regarding ietf-network-topology-state@2017-06-30.yang:
> - similar comments to ietf-network-topology@2017-06-30.yang.
>
>
> Comments on draft:
>
> 1) The document still has references to “server-provided”.  Not the YANG
> leaf of course, but in general text.  For instance “The model does allow
> to layer a network that is configured on top of one that is server-provided.”
> I think that the statement is more about values being in <operational>
> than how it was learned.  All uses of “server-provided” should be examined
> for correctness.
>
> 2) This sentence doesn’t make sense to me, how can a server-provided model
> (you mean data?) access any information in conventional datastores?:
>     “An implementation's security policy MAY further restrict what
>     information the server-provided model is allowed to access in
>     standard configuration data-stores,”
> Either way, this text likely should be moved to the Security
> Considerations section.
>
> 3) Should the last paragraph in Section 1 be removed now?  Is the module
> expected to continue to update?
>
> 4) S4.1, P3: the text here says new data nodes are augmented in, but
> the YANG module itself says that only presence containers are allowed.
>
> 5) are the mandatory statement values set appropriately everywhere?
> (e.g., source-node?, source-tp?)
>
> 6) network-type is a presence container, not an identity? No where in
> the draft is there an example showing it being used.
>
> 7) Section 4.4.8., why not use identities?
>
> 8) S4.4.9 is a duplicate of some text in S4.1
>
> 9) This statement is true, but I think misleading in context. “YANG
> requires data nodes to be designated as either configuration or
> operational data, but not both”. It seems that the solution depends
> entirely on config true nodes, and NMDA to present the operational
> value of those config true nodes.  Right?
>
> 10) When using <CODE BEGINS>, you should have a note to the RFC
> Editor to change the date to the date of publication, both in the
> filename as well as in the module’s ’revision’ statement.
>
> 11) IANA Considerations has comments “(RFC form)” - are these supposed
> to be notes to RFC Editor to replace with final RFC designation?
>
> 12) Your Security Considerations section should follow the template here:
> https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines
>
> 13) create examples for the use-cases in Appendix A?  Note, every draft
> should have examples of its YANG modules...
>
>
> Nits:
>
> - why reference RFC6991 in the Introduction?
> - replace “allows to define” with “enables the definition of”
> - in a few places, you refer to “network.yang”, but the
>    file is called “ietf-network.yang”.
> - in a few places, you refer to “network-topology.yang”,
>    but the file is called “ietf-network-topology.yang”.
> - replace “- X1 and X2 - mapping onto a single L3 network element”
>    with “(X1 and X3) mapping onto a single L3 network element (Y2)”
> - replace “data-store” with “datastore”
> - replace “model” with “data model” where it’s not already.
> - replace “the <intended> datastore” with just “<intended>”
> - replace “the intended datastore” with just “<intended>”
> - update Section 2 to also include a reference to RFC 8174 (see RFC 8174)
> - pull the “datastore” term from the revised-datastores draft?
> - NETCONF and YANG don’t need to be terms/defined, since references
>    to their RFCs are provided when they’re used.
> - s/the network.yang module/the “ietf-network” YANG module/
> - your tree-diagram notation in S4.1 isn’t complete.  And you duplicate
>    it in S4.2.  Create a top-level section called “tree diagram notation”
>    that both sections reference?
> - s/allows to represent/allows representation of/
> - s/another container/a presence container called/
> - s/allows to define more/allows definition of more/
> - s/allows also to represent/allows representation of/
> - s/configuration and intended datastores/conventional datastores/
> - s/and show up only/and thus only appear/
> - /ietf-network:networks/network/network-id - being the list’s key,
>    I was expecting this to the first element defined.
> - for the various leafrefs in both models, I see a lot of longish
>    relative paths; suggest you change these to absolute paths if possible
> - /nd:networks/nd:network:/link/link-id is the list key but not the
>    first leaf listed
> - incomplete sentence: “augmentations can in turn against augmenting
>    modules”
> - s/need to specified/need to be specified/
> - s/if the link-to-links mapping known/if the link-to-links mapping
>    are known/
> - s/each link known/each link are known/
> - s/the operational datastore/<operational>/
> - s/into the data model without relying/into <operational> without relying/
> - s/in the following two companion modules/the following two companion modules/
> - s/that represent a state model/to represent the operational state/
>
> Thanks,
> Kent
>
>
>
> _______________________________________________
> yang-doctors mailing list
> yang-doctors@ietf.org
> https://www.ietf.org/mailman/listinfo/yang-doctors