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

[Ebben Aries <[e-mail]>]

Hi Sergio - apologies for the delay

I ran validations against the -17 version and read through the draft
updates and it looks good from my end

Thx

/ebben

On Apr 19 14:22 PM, Sergio Aguilar Romero wrote:
> [External Email. Be cautious of content]
> 
> 
> Hello Ebben,
> 
> Hope you are doing well.
> 
> Do you have any update of this review? 
> 
> Thanks.
> 
> Best regards,
> 
> Sergio
> 
> 
>     On Apr 5, 2023, at 1:09 AM, Sergio Aguilar Romero
>     <[e-mail]> wrote:
> 
> 
>     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.
> 
> 
> 
> 
> 
> 
> 
>