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

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

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.