Re: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-rfc9093-bis-04

["Joe Clarke (jclarke)" <[e-mail]>]

Thanks, Segio.  I’ve looked at your GH diffs, and I think you addressed my comments.  Yes, you fixed the quoting inconsistencies.

Your approach to “isolating” the relative paths is interesting, and should make the grouping safer to re-use.  That said, I don’t see how you’re signaling such augmentation would be needed by external consumers.

Joe

From: Sergio Belotti (Nokia) <[e-mail]>
Date: Thursday, May 25, 2023 at 12:14
To: Joe Clarke (jclarke) <[e-mail]>, [e-mail] <[e-mail]>
Cc: [e-mail] <[e-mail]>, [e-mail] <[e-mail]>, [e-mail] <[e-mail]>, Italo Busi <[e-mail]>
Subject: RE: Yangdoctors last call review of draft-ietf-ccamp-rfc9093-bis-04
Hi Joe,

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/ietf-ccamp-layer0-types-ext-RFC9093-bis/pull/70

See in line for comments (marked [Belotti Sergio-Italo Busi].

Thanks
Italo and Sergio

> -----Original Message-----
> From: Joe Clarke via Datatracker <[e-mail]>
> Sent: Tuesday, April 4, 2023 10:40 PM
> To: [e-mail]
> Cc: [e-mail]; [e-mail]; last-
> [e-mail]
> Subject: Yangdoctors last call review of draft-ietf-ccamp-rfc9093-bis-04
>
>
> 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: Joe Clarke
> Review result: Ready with Issues
>
> I have been asked to review this document on behalf of YANG Doctors.
> While I do not have domain experience in layer 0 optical, I found the
> document and YANG module fairly easy to get through.  Since this document
> is a bis for RFC9093, I did a pyang check to make sure that there are no
> backwards-incompatible changes between the 9093 version and this one.  I
> did not find any.  The new module adds new types, identities, and nodes.
>
> In terms of issues, the leafref and must in the transceiver-mode are relative
> back to the transceiver-capabilities grouping, which uses the transceiver-
> mode grouping.  But what if transceiver-mode is used outside of this
> context?  Those references may break.
>
[Belotti, Sergio-Italo Busi] : Changing to relative path is not possible because we need to point to the same network, node, ttp, transceiver through another relative path. We have moved the code with leafref and must statements to the transceiver-capabilities grouping. A similar code (with adjusted relative paths) should be added by anyone using the transceiver-mode grouping elsewhere.

> In terms of nits, I found an inconsistent use of first-letter capitalization and
> period termination in descriptions.
>
[Belotti, Sergio-Italo Busi] OK we have reviewed and made the module more consistent

  I also noted that both the document and
> the module lack an overall summary of changes between this version and the
> 9093 version (and I'm just setting the interrupt bit to make sure the authors
> address that).
 >
[Belotti, Sergio-Italo Busi] We have in fact a specific issue #40 in github for that, saying “ Add description of the changes from RFC 9093 both in the YANG revision statement as well as in Appendix A”. Thanks to remind us.


 There is also an inconsistent use of quotes around various
> units.
>
[Belotti, Sergio-Italo Busi] We think to have fixed inconsistencies.


> I notice that there are some config true nodes that are a union with "empty"
> as being one of the options.  Some of the nodes explain when to use empty,
> but others do not.  I think it would be good to be consistent with the "empty"
> explanation.
>
[Belotti, Sergio-Italo Busi]: We have updated all the descriptions to be consistent. The only config true attribute is the tx-channel-power which in some cases it is also used as config false. We have added a MUST in the description to limit the use of empty value only when the attribute is config false.