Hi Martin, Many thanks for the review.
All good points. Please see inline for more context. The changes can also be tracked at: https://www.ietf.org/rfcdiff?url1=draft-ietf-opsawg-sap&url2=https://raw.githubusercontent.com/IETF-OPSAWG-WG/lxnm/master/I-D-sap/draft-ietf-opsawg-sap.txt Cheers, Med > -----Message d'origine----- > De : OPSAWG <[email protected]> De la part de Martin Björklund via > Datatracker > Envoyé : mardi 8 mars 2022 09:43 > À : [email protected] > Cc : [email protected]; [email protected] > Objet : [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-sap-02 > > Reviewer: Martin Björklund > Review result: Ready with Nits > > This is my YANG doctors review of draft-ietf-opsawg-sap-02. I have > reviewed the YANG module only. > > The YANG module has the following structure: > > +--rw service* [sap-type] > +--rw sap-type identityref > +--rw service-attachment-point* [attachment-id] > +--rw attachment-id string > +--rw description? string > +--rw attachment-interface? string > +--rw parent-termination-point? nt:tp-id > +--rw interface-type? identityref > +--rw encapsulation-type? identityref > +--rw role? identityref > +--rw peer-customer-sap-id? string > > o "service" > > The model defines a list "service", with the key "sap-type". This > looks a bit weird - is the term "service" the same thing as "sap"? > I don't think so, and the text in the document talks about "service > type" in section 5. > [Med] Made this change s/sap-type/service-type. > I don't know which term is correct. If the correct term "service", > I suggest you change "sap-type" to "service-type". If the correct > term is "sap", I suggest you change the list name. > > In any case, there can obviously only be one "service" of a given > type, but I assume this is intentional. [Med] Yes. > > > o "service-attachment-point" > > In other places in the model you use "sap" ("peer-customer-sap-id", > "sap-type"). Perhaps simple call this "sap". > > [Med] Works for me. > o "attachment-id" > > In the description of the leaf "attachment-id", it suggests that > this is the identifier of the "service-attachment-point". Normally, > I would suggest to not repeat the name of the list in the names of > the nodes in the list, and thus I would suggest simply "id". > However, in this case it seems the model that this module augments > use this convention, so perhaps call this leaf > "service-attachment-point-id" - which is quite long, perhaps > "sap-id". [Med] went with this one and peer-sap-id. > > Also note the name mismatch "peer-customer-sap-id" and > "attachement-id". > > Also, I note that the type of the id in this document is a string, > whereas in the other models it is an inet:uri. See section 4.4.11 > of RFC 8345. Note that all examples in RFC 8345 violates the "uri" > type, so it it is not clear to me that they really meant uri. My > suggestion is to stick to string for this type. (BTW, the example > in this document also violates the "uri" type, e.g.: > "network-id": "an-id" > is not a valid uri.) > [Med] Good catch. Fixed the example. Thanks. > > o "attachment-interface" vs "interface-type" > > These two leafs define properties of the same object; the name and > type of the "interface to which the SAP is bound". > > This should be reflected in the names of these leafs. Perhaps: > "interface-name" and "interface-type". [Med] I prefer to maintain the OLD name. > > Also, I suggest you put these two leafs together in the data model, > giving: > > ... > +--rw description? string > +--rw parent-termination-point? nt:tp-id > +--rw interface-name? string > +--rw interface-type? identityref > [Med] OK. > > o "interface-type" > > It is not clear to me how this relates to the > "/interfaces/interface/type" from RFC 8343. How are the interface > types defined in iana-if-type mapped to the types defined in this > document? Perhaps they are not? > > [Med] We have considered in the past the use of 8343/7224, but we went finally with the current approach as we want to cover a limited set of things such as bridge domains, bearer references, IRBs, etc. that we are already using in other service modules. The mapping to the detailed interface types as per 7224 will be made by an orchestrator when it translates this network module into devices modules. > o "role" > > The description of "role" says: > > description > "Indicates whether the SAP is an UNI or a NNI."; > > Now, the role is an identity, which is an "extensible enumeration". > The description seems to indicate that this leaf really can have > just two values. If the description is right, perhaps change this > type to an enumeration. Otherwise, change the description. > > [Med] Fair. Changed the description. > o "peer-customer-sap-id" > > The description says: > > Indicates an identifier of the peer's termination > identifier (e.g., Customer Edge (CE)). This > > Is the peer always a "customer sap"? If so, it could be explained > better in the description. If not, the leaf's name is not correct. > > [Med] Changed the name. Thanks. > > /martin > > > > _______________________________________________ > OPSAWG mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/opsawg _________________________________________________________________________________________________________________________ Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration, Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci. This message and its attachments may contain confidential or privileged information that may be protected by law; they should not be distributed, used or copied without authorisation. If you have received this email in error, please notify the sender and delete this message and its attachments. As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified. Thank you. _______________________________________________ OPSAWG mailing list [email protected] https://www.ietf.org/mailman/listinfo/opsawg
