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

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

Thanks Ebben for you review and comments.

Cheers,

Sergio

El vie, 21 abr 2023 a las 1:44, Ebben Aries (<[e-mail]>) escribió:

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