Re: [yang-doctors] Yangdoctors early review of draft-ietf-opsawg-vpn-common-02

[[e-mail]]

Hi Radek,

All good points. Many thanks. 

All the comments were addressed in a new revision, except the one related to rt-rd/rd-choice. Distinct names are used to distinguish auto-assigned values vs assigned from a pool. 

FWIW, the new revision can be seen at: 

URL:            https://www.ietf.org/archive/id/draft-ietf-opsawg-vpn-common-03.txt 
Status:         https://datatracker.ietf.org/doc/draft-ietf-opsawg-vpn-common/ 
Htmlized:       https://datatracker.ietf.org/doc/html/draft-ietf-opsawg-vpn-common 
Htmlized:       https://tools.ietf.org/html/draft-ietf-opsawg-vpn-common-03 
Diff:           https://www.ietf.org/rfcdiff?url2=draft-ietf-opsawg-vpn-common-03

Cheers,
Med

> -----Message d'origine-----
> De : Radek Krejčí via Datatracker [mailto:[e-mail]]
> Envoyé : vendredi 18 décembre 2020 13:14
> À : [e-mail]
> Cc : [e-mail]; [e-mail]
> Objet : Yangdoctors early review of draft-ietf-opsawg-vpn-common-02
> 
> Reviewer: Radek Krejčí
> Review result: Ready with Issues
> 
> *** draft ***
> 
> Seems well written with a clear statement about its purpose and
> describing the groupings of the module.
> 
> *** module ***
> 
> * layout
> Use pyang's `--yang-canonical` option to unify the order of the
> statements and make the order canonical - e.g. starting the
> groupings with description improves readability of the module. Using
> pyang to print the module would also remove probably forgotten
> comment `//L2xMs`.
> 
> * typo
> - Module's description: s/Section 4.c/Section 4/
> 
> * features
> Almost none of the features is actually used in the module, which
> might be fine, but there are identities referring to the same areas
> as the features, so I believe that these identities should have if-
> feature statement referring to the appropriate feature defined in
> the module.
> 
> * enumeration typedefs
> Since the enumeration cannot be extended, are you really sure, that
> the enumeration types you define are really complete forever? I
> would say that address families, negotiation modes and  control
> modes might need extensions in the modules that will use those
> types. Defining it as identityref with specified identities instead
> of enums would solve the problem.
> 
> * `service-status` and `status-timestamp` groupings Both groupings
> seem to have config false meaning.  But only the service-
> status/status/oper-status container is defined as config false. The
> uses statement doesn't have its own config statement, so if you want
> to place the mentioned groupings into config true data, an extra
> grouping or refine will be required. The common sense of the
> groupings is config false, so define them this way. If there will be
> some exception to make them config true, the uses can refine it in
> such an exceptional case.
> 
> * grouping rt-rd/rd-choice
> There are 2 cases (pool-assigned and full-autoassigned) which seem
> to have a node with the same meaning, but since there would be a
> name collision, they have different names `rd-assign` and `rd-
> assigneed`. I don't think that this is a good solution. If they have
> the same meaning, they should have also the same name. I see 2
> possible solutions: - move the leafs in the cases into a container -
> it adds another level in data, but allows a common name `rd-
> assigned` - move the `rd-assigned` leaf outside the choice. If it
> makes sense, it can be constrained by must/when to the (non)-
> presence of the nodes from the choice. The main point here is that
> the rd-assigned status is then always at the same place.
> 
> * grouping group/
> The grouping seems odd to me, like it misses something or it might
> be just by too generic name. When looking into the `placement-
> constraint` grouping, it seems that it refers to the groupings, but
> since they are separated, it is not possible to refer to the group-
> id explicitly. Is the `group` grouping intended to be instantiated
> standalone, without `placement-constraint`? If not, join them
> together and refer the group-id from target-flavor via leafref.
> 
> 


_________________________________________________________________________________________________________________________

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.