Re: [lp-wan] Yangdoctors early review of draft-ietf-lpwan-schc-compound-ack-14

[Sergio Aguilar Romero <[e-mail]>]

Hello Ebben,

Hope you are doing well.

Thanks for your review.

We have published a new version (-17) addressing all the comments.

Please let us know if they are addressed correctly, specially the IANA Consideration text.

Best regards,

Authors Compound ACK draft

> On Apr 1, 2023, at 5:43 PM, Ebben Aries <[e-mail]> wrote:
> 
> Hi Sergio,
> 
> I've gone ahead and re-reviewed based off the latest -16 publish and
> appears quite a few items were not addressed as previously suggested.
> See below.
> 
> Validation
> - The modules still fail to validate.  Below was not actually addressed
>  as your `derived-from()` function should rather be
>  `derived-from-or-self()`.  You can validate this data as well using
>  yanglint.
> 

Changed. We used derive-from-or-self as in RFC9363. We are not sure how to valide it.


> Module 'description' stmt
> - This is now failing the pyang IETF checks.  Please cut/paste and
>  modify the text output from `pyang --ietf-help` underneath your module
>  description as there are some precise string matches failing here.

Changed the copyright and pyang --ietf is ok.


> - For your module description here, you are getting very precise in your
>  wording here about introducing "a new leaf".  This is best left for
>  revision descriptions otherwise any future edits will require
>  modification of the module description.  Stick w/ the overall
>  intention of this module which should not change if there are revision
>  updates.
> 

We added some descriptions to the different element and reference to RFC8724.  

> Overall 'description' statements
> - I see a few were updated but there are still quite a few that remain
>  that do not use correct capitalization or alignment.  Please look at
>  other published modules for what this entails.  You want to be
>  descriptive and ensure you have consistent alignment (you can adjust
>  some by running the module through a linter such as pyang)
> 

The description is started with a capital letter and ended it with a dot. Also we pass pyang -f yang --max-line-length=72 to reformat. 

> Section 5.1
> - You have updated the revision in the module but the filename now
>  contains the old revision-date (just after CODE BEGINS).  Please
>  adjust this otherwise tools like rfcstrip will produce a file that
>  conflicts w/ the revision in the module

We updated the module name.

> 
> Section 5.2
> - This was still not addressed in -16.  This is an incomplete and
>  incorrect representation of the tree diagram.  Please correct this per
>  RFC8340 (You are still missing the other augmented leaf)

We fixed the tree, adding the two leafs. 
 

> 
> Section 7
> - Please follow RFC8407 and include the full security considerations

We have added the security consideration (like in RFC9363), with consideration of the new module.

> 
> Section 8
> - You do have IANA considerations.  You are introducing a new YANG
>  module and a new namespace so you have a URI + a module name
>  registration that needs to be taken care of.  See your parent
>  dependency as an example (RFC9363)
> 

We have modified the IANA Considerations to include the URi and module.
We leave blank the reference RFC, because in the examples we found it was the number of current document.


> Thx
> 
> /ebben
> 
> On Mar 28 16:15 PM, Sergio Aguilar Romero wrote:
>> [External Email. Be cautious of content]
>> 
>> 
>> Hello,
>> 
>> Thanks for your review.
>> 
>> We have published a new version (-16).
>> 
>> Some comments inline.
>> 
>> Best regards,
>> 
>> Authors Compound ACK draft
>> 
>> 
>>> On Mar 20, 2023, at 4:57 PM, Ebben Aries via Datatracker <[e-mail]> wrote:
>>> 
>>> Reviewer: Ebben Aries
>>> Review result: On the Right Track
>>> 
>>> 1 module in this draft:
>>> - [e-mail]
>>> 
>>> YANG compiler errors or warnings (pyang 2.5.3, yanglint 2.1.55, yangson 1.4.16)
>>> - No compiler errors or warnings for tree outputs
>>> - Instance data however fails validation (see below)
>>> 
>>> Module [e-mail]
>>> - Overall, the module is small (2 leaf augments to ietf-schc) and concise.
>>> There are some style nits (alignment/spacing) that can be cleaned up by
>>> running the module through a linter and adding back to the draft code blocks
>> 
>> The revision performed to version -14 by Robert Wilton included the pyang output that was copied into version -15.
>> 
>> 
>>> - As mentioned above, instance data will fail to validate due to the following
>>> when stmt
>>> 
>>>   when "derived-from(../schc:fragmentation-mode,"
>>>   +" 'schc:fragmentation-mode-ack-on-error')";
>>> 
>>> Should rather be:
>>> 
>>>   when "derived-from-or-self(../schc:fragmentation-mode," +
>>>        "'schc:fragmentation-mode-ack-on-error')";
>>> - Since you are applying the same when restriction to 2 leaf nodes here, I
>>> would recommend grouping the leaf nodes and gating the augment by way of
>>> uses + when
>>> 
>>> e.g.
>>> 
>>> augment "/schc:schc/schc:rule/schc:nature/schc:fragmentation" +
>>>         "/schc:mode/schc:ack-on-error" {
>>>   description
>>>     "";
>>> 
>>>   uses ack-on-error {
>>>     when "derived-from-or-self(./schc:fragmentation-mode, " +
>>>          "'schc:fragmentation-mode-ack-on-error')";
>>>   }
>>> }
>>> 
>> 
>> We have fixed the “+” location.
>> 
>> 
>> 
>>> General comments on the draft/modules:
>>> - Section 5.2 - It appears that only 1 augment is conveyed here and the Figure
>>> 10 line is out of place.  This section should conform to RFC8340 and be
>>> labeled "Tree Diagram" as seen in other published drafts/RFCs
>> 
>> We have modified Figure 10 label.
>> 
>> 
>>> - Module contact information - Feel free to include authors as stated in
>>> RFC8407 Section 4.8
>> 
>> One more author was added.
>> 
>>> - Module description - Put the description of the module at the top and the
>>> Copyright information beneth with correct line breaks and removal of the
>>> asterisk delimeter
>> 
>> Module description was move to the top and asterisk have been removed. When the new version was uploaded, we have a warning saying that the IETF Trust Copyright statement seems to be missing, but it is there.
>> 
>>> - For all description statements in the module, use correct capitilization and
>>> be as descriptive as possible without being overly verbose.
>> 
>> Some description was added.
>> 
>>> - Section 7 - Include the full Security Considerations as is in any other
>>> drafts/RFCs related to YANG modules vs. point to a related RFC
>> 
>> The draft mentions that same security considerations defined in RFC8724  applied. We added RFC9363 to the security considerations.
>> 
>>> - Section 8 - IANA Considerations.  You are introducing a new module that
>>> contains a new namespace so will require registering a new URI.  This will
>>> need to follow the same process and contain the same verbiage as other
>>> related drafts/RFCs
>>> 
>> 
>> We understand that this document has no IANA actions.
>> 
>> 
>>> Example validated instance data after the when stmt fix:
>>> 
>>> <schc xmlns="urn:ietf:params:xml:ns:yang:ietf-schc"
>>> xmlns:schc-compound-ack="urn:ietf:params:xml:ns:yang:ietf-lpwan-schc-compound-ack">
>>> <rule>
>>>   <rule-id-value>100</rule-id-value>
>>>   <rule-id-length>1</rule-id-length>
>>>   <rule-nature>nature-fragmentation</rule-nature>
>>>   <fragmentation-mode>fragmentation-mode-ack-on-error</fragmentation-mode>
>>>   <direction>di-up</direction>
>>>   <fcn-size>2</fcn-size>
>>>   <schc-compound-ack:bitmap-format>schc-compound-ack:bitmap-compound-ack</schc-compound-ack:bitmap-format>
>>>   <schc-compound-ack:last-bitmap-compression>true</schc-compound-ack:last-bitmap-compression>
>>> </rule>
>>> </schc>
>>> 
>>> {
>>>   "ietf-schc:schc": {
>>>       "rule": [
>>>           {
>>>               "rule-id-value": 100,
>>>               "rule-id-length": 1,
>>>               "rule-nature": "ietf-schc:nature-fragmentation",
>>>               "fragmentation-mode":
>>>               "ietf-schc:fragmentation-mode-ack-on-error", "direction":
>>>               "ietf-schc:di-up", "fcn-size": 2,
>>>               "ietf-lpwan-schc-compound-ack:bitmap-format":
>>>               "ietf-lpwan-schc-compound-ack:bitmap-compound-ack",
>>>               "ietf-lpwan-schc-compound-ack:last-bitmap-compression": true
>>>           }
>>>       ]
>>>   }
>>> }
>>> 
>> 
>> Great.
>> 
>>> 
>>