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

[Ebben Aries <[e-mail]>]

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.

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.
- 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.

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)

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

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)

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

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)

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.
> 
> >
>