Re: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-optical-impairment-topology-yang-12

[Michal Vaško <[e-mail]>]

Hi,

reactions inline but I also have some new notes.

Line 657: `i` seems like a really bad name, I would suggest `index` or 
at least `idx`.

> Hi Michal,
> Thanks a lot for your careful review and comments.
>
> We have updated the model following your suggestion in most of the indicated points, where we do not follow suggestion we provided a comment on line .
> You can find the YANG updated module and diff with the previous one at https://github.com/ietf-ccamp-wg/draft-ietf-ccamp-optical-impairment-topology-yang/pull/135
>
> See in line  for comments marked [Belotti, Sergio-Italo Busi]
>
> Thanks
> Italo and Sergio
>
>> -----Original Message-----
>> From: Michal Vaško via Datatracker <[e-mail]>
>> Sent: Tuesday, April 4, 2023 11:58 AM
>> To: [e-mail]
>> Cc: [e-mail]; draft-ietf-ccamp-optical-impairment-topology-
>> [e-mail]; [e-mail]
>> Subject: Yangdoctors last call review of draft-ietf-ccamp-optical-impairment-
>> topology-yang-12
>>
>>
>> CAUTION: This is an external email. Please be very careful when clicking links
>> or opening attachments. See http://nok.it/ext for additional information.
>>
>>
>>
>> Reviewer: Michal Vaško
>> Review result: On the Right Track
>>
>> The reviewed YANG module should describe the parameters it intends to
>> describe but there are lots of formal shortcomings in YANG formatting and
>> general consistency. To fix formatting, pyang tool can be used. Description
>> texts should be full sentences. Data examples are not valid (even include a
>> note that they need to be updated), use yanglint to validate them against the
>> YANG module. Other than that, some specific improvements:
>>
>> ## line 170: leaf out-voa
>> - units dB seems redundant, it is part of the used type name
> [Belotti, Sergio-Italo Busi] YES, it's true . We have already defined typedef power-in-db-or-null.
>
>> ## 326 - 379: grouping roadm-add-path
>> ## 426 - 478: grouping roadm-drop-path
>> - all the 5 leaves seems to match exactly those in the grouping roadm-
>> express-path above so I would just put uses in this grouping
>>
> [Belotti, Sergio-Italo Busi] we have created a new grouping roadm-common-path, and then put "uses" in the other different groupings (add, drop, express).
>
>
>> ## 405, 571: leaf roadm-noise-figure
>> - lots of the leaves are using types from l0-types
>> - a local typedef can be defined (if no existing one can be used) that would
>> prevent duplication of all the information and move type specifics out of the
>> data nodes - alternatively a grouping with this leaf can be defined to avoid
>> description duplication but that is up to you, seems unnecessary
>>
> [Belotti, Sergio-Italo Busi] RFC9093-bis specifically hosting common structure for any L0 YANG modules. So, we followed your suggestion defining a new typedef decimal-5-digits-or-null and decimal-5-digits,  not local  but in RFC9093-bis like  for 2 digits.
>
>
>> ## 631: leaf nominal-power-spectral-density
>> - similar problem as the previous one except a grouping makes no sense, just
>> a typedef
>>
> [Belotti, Sergio-Italo Busi] ok , we have defined a typedef decimal-16-digits and then a typedef decimal-16-digits-or-null  in RFC9093-bis like for 2 digits, and 5 digits above.
>
>
>
>> ## 287, 298, 635, 686:
>> - union with an empty type is used a lot and selecting this type is usually
>> explained but not in these cases
>>
> [Belotti, Sergio-Italo Busi] The usage of “empty” type in the module is described on top of the module, in the description :… ” Within this module, if the value of a mandatory attribute is unknown, it MUST be reported using the empty type. If an optional attribute is applicable but its value is unknown, it MUST be reported using the empty type.”.
MV: Okay, I suppose, but then it should be consistent, no specific 
`empty` type explanations because that is what actually confused me. I 
still think it more user-friendly to have it repeated for every leaf, 
though.
>> ## 880: augment "/nw:networks/nw:network/nw:network-types/tet:te-
>> topology"
>> - adding an empty presence container, which does not make sense on its
>> own unless other foreign augments are expected - following 2 augments add
>> into this container, why not merge all 3 augments into one to prevent any
>> confusion?
>>
> [Belotti, Sergio-Italo Busi] This model is augmenting ietf- te-topology , that is also augmenting ietf-network. For this augmentation related to the type of network both are following the guideline in section 4.3 of RFC8345 that, among other things says: “….First, a new network type needs to be defined; this is done by defining a presence container that represents the new network type. The new network type is inserted, by means of augmentation, below the network-types container……”
> So the augmentation with adding an empty presence container has its own meaning.
> The other augmentation you mention have a different xpath , so they cannot be put together with the first augmentation.
MV: Alright, makes sense, so add a reference to the other RFC for it to 
be clear to everyone.
>> ## 899: container otsi-information
>> - including "information" in the identifier seems redundant
>>
>   [Belotti, Sergio-Italo Busi] ok, any suggestion to avoid leaving just otsi as name of container? In the container there is for example the definition of otsi-group that it could represents more than 1 otsi , the idea of adding “information” at the end ot otsi name is to represent the fact the container contains any otsi related “information” e.g. otsi-group . Maybe “information” can be redundant but leaving otsi only does not add clarity.
MV: My point is that `otsi` has the exact same meaning or clarity as 
`otsi-information`, in my opinion. That makes `information` redundant. I 
cannot help with coming up with a better name as this is not my 
expertise at all.
>> ## 1293, 1315, 1335, 1359, 1412, 1468: leaf roadm-path-impairments
>> - all the leaves are "config false" when augments are applied but some have
>> it specified explicitly some not -
>>
>   [Belotti, Sergio-Italo Busi] OK, we have aligned the code : 1315 and 1294 needed to be aligned with “config false”
>
>
>   if the leafref path is changed to an absolute
>> path, the leaf can be in a grouping and used at all the places
>>
>   [Belotti, Sergio-Italo Busi]  see mail thread with YANG doctor https://mailarchive.ietf.org/arch/msg/yang-doctors/8EHiqGRcAxJkv2OtSLcU9a7i848/
> The absolute path should anyway indicate network-id and node-id as keys for the network and node list. So you need again relative path inside. It is not viable to define a new grouping because the relative path is different in the different locations and as said before changing the relative path into an absolute path requires anyway adding relative paths to identify the network-id and the node-id.
>
>
>> ## 1492 - 1509; 1524 - 1541; 1570 - 1579; 1614 - 1623:
>> - both leaves add-path-impairments and drop-path-impairments use the
>> same absolute path leafref so a single typedef can be defined (also used for
>> the leafref in the leaf roadm-path-impairments above) - then both leaves can
>> be put into a grouping and used at all 4 places
>>
>   [Belotti, Sergio-Italo Busi] : as said above It is not viable to define a new grouping because the relative path is different in the different locations and as said before changing the relative path into an absolute path requires anyway adding relative paths to identify the network-id and the node-id.
>
Regards,
Michal