[netconf] Shepherd review of draft-ietf-netconf-notification-capabilities

[Kent Watsen <[e-mail]>]

[WG/Authors: this draft passed WGLC, what happened?!]


Authors, please find my shepherd review below.


Title:
  - it would be best for the title to not use the “YANG-Push” term before it’s been imported.
    How about this?
    OLD: YANG Modules for describing System Capabilities and Yang-Push Notification Capabilities
    NEW: YANG Modules describing Capabilities for Systems and Notifications for Datastore Updates
    NEW2: YANG Modules describing Capabilities for Systems and Datastore Updates Notifications

    Be sure to update any change in the title in the YANG modules as well!

    Note: https://en.wikipedia.org/wiki/YANG#Standards-track_data_models has
              a preference for "Data Models” over “Modules”, but I left that part alone
              since it’s not unheard of either...


Abbreviated title:
  - "YANG-Push Notification Capabilities” is not an accurate description...



Abstract:

  - module names should be in quotes when introduced

  - s/This document defines two YANG modules:/This document 
    defines two YANG modules,"ietf-system-capabilities” and 
    "ietf-notification-capabilities”.

  - s/YANG Instance Data/YANG Instance Data (I-D.ietf-netmod-yang-instance-file-format)/

  - s/"Subscription to YANG Datastores" (YANG-Push)/Subscription
    to YANG Notifications for Datastore Updates (RFC 8641)/

  - Actually (in ref to the above two comments), the sentence structure in p2 seems
    backwards.  Better would be to say "The module can be used to report capability
    information from the server at run-time or implementation-time, per 
    I-D.ietf-netmod-yang-instance-file-format.”



Section 1:

  - Why isn’t the “Introduction” Section ‘1’ as is common in RFCs?

  - Each term should be in double-quotes when first introduced, or use 
    hanging text like other RFCs do.

  - don’t capitalize terms unless they appear at the beginning of a sentence,
    unless they’re capitalized throughout the entire document consistently!
    Which they’re not here.v Personally, I can never capitalize them 
    consistently, so I just normal sentence-capitalization rules…

 - Neither "Run-time information” nor "Implementation-time information”
   constitute terms.  Seriously, no one is going to care this document for
   those terms.  Also not that "Run-time information” is only used once, as
   "Implementation-time information” would be as well if the text in the two
   back-to-back paragraphs were rearranged slightly.

 - "On-change notification capability” is not a term and it’s not used anywhere
   in the document



Section 2:

  - The first sentence uses the word “capabilities” without explanation (and
     it’s not a term).  Given that this document defines the “capabilities”, I’m
    expecting a term to be defined for it.  S3 provides examples, but it seems
    too far away…thoughts?

    Maybe S1 and S2 could be collapsed:

    OLD:
       Systems implementing a server and/or a publisher often have
       capabilities that are not defined by the YANG model itself.  There is
       a need to publish this capability information as it is part of the
       contract between the server and client.

    NEW:
       Servers and/or a publishers often have capabilities, values describing
       operational behavior, that need to be conveyed to clients, which is 
       enabled by the YANG modules described in this document. <blank line>

  - the bullet list should be indented (i.e., put inside another list with symbols="empty”)


Section 2.1:
  - remove all instances of “we” throughout the document.   s/we/this document/?
  - why is there a “2.1” if there isn’t at least a “2.2”?  (Is this meant to actually be a section 3?)
  - the bullet lists should be indented (i.e., put inside another list with symbols="empty”)
  - P4 (the paragraph beginning "Publishers have limitations”) doesn’t seem to say anything
    concrete (i.e., relavent to the draft).  Actually, the same can be said for P5.
  - P6 and P7 appear to be (unnecessary) elaborations of bullet-point #2 above.
  - s/Clients of a server, subscribers to a publisher/Clients of a server/ (note: that subscribers are clients)
  - the last 2-3 paragraphs seem to go into the weeds wrt the focus of this draft…and the whole
    Implementation-time vs Runtime tradeoff is repeated in Section 3.  Honestly, I don’t think
    this draft needs to discuss this at all but, if you must, factor it out into a separate section
    and keep it short, e.g., as in Section 3.


Section 3:
  - why doesn’t this section come before Section 2.1 (which maybe should actually be a Section 3)?
      - System-level capabilities is the “base” model, in hence should be introduced first…
  - actually, after reading the entirely of Section 3, I no understand the purpose of Section 2.1
      - recommend 1) extracting the middle part out into a new “providing data” section where the
        Implementation-time vs Runtime options are discussed and 2) moving the remainder to 
        End of the Introduction section…
  - s/The module ietf-system-capabilities/The module "ietf-system-capabilities"/
  - s/is defined to provide a structure/provides a structure/
  - s/any YANG related system capability/YANG-defined capabilities/
  - s/The module ietf-notification-capabilities/The module "ietf-notification-capabilities”/
  - s/"Subscription to YANG Datastores" (YANG-Push)/Subscription 
    to YANG Notifications for Datastore Updates [RFC8641]/
  - Re: "on system/publisher level” - why is “publisher” mentioned in Section 4 at all?


Section 4:
  - s/The module ietf-system-capabilities/The module "ietf-system-capabilities"/
  - s/is defined to provide a structure/provides a structure/
  - s/that can be used to specify/that can be used to discover (as read-only operational state)/
  - s/any YANG related system capability/YANG-defined capabilities/
  - P3 should be P2 (like they are in the YANG module)
  - the presence of P2 is confusing, if “this module itself does not contain any capabilities”
      - I think you want to say something like the “structure” provides augmentation points
        for capabilities to be defined at different levels: a) system, when foo, b) datastore,
        when bar, and c) specific data nodes, the baz. (where foo/bar/baz describe the
        augmentation points used) - right?
  - s/config=false/“config false”/


Section 4.2:  ([e-mail])

  - should the “description” statement for "import ietf-yang-library”
    1) s/a revision/a backwards-compatable revision/ and 2) state that
    the module must be implemented?  (Better, see my other comment
    regarding the YANG Doctor;s comment)
  - s/RFC RFC8525/RFC 8525/
  - OLD:
      This module specifies a module intended to contain system
      capabilities. System capabilities may include capabilities of a
      NETCONF or RESTCONF server or a notification publisher.
    NEW:
      This module specifies a structure to specify system
      capabilities for s server or a publisher.
  - s/capabilities it only/capabilities, it only/
  - s/should be augmented in/are augmented in/
  - be sure there is a blank line between paragraphs and numbered
    list items, and/or list items are indented (this comment applies to
    *all* “description” statements)
  - list item (4) is missing punctuation and the first “sentence” doesn’t
    form a sentence.
  - "RFC XXX: YANG-Push Notification Capabilities” has both an 
    incorrect number of X’s as well as incorrect document title.
  - rather than have the “description” statement "// augmentation
    point for system level capabilities”, why not have an empty
    “choice” statement with the same description statement?
  - re: "Only individual datastores”, is “individual” the right word?  
    Would “specific” or “leaf” be better?
  - rather than have the “description” statement "// augmentation
    augmentation point for datastore or data node level capabilities”,
     why not have an empty “choice” statement with the same
     description statement?
  - is the comment "The special value '/' denotes all data nodes in the
    datastore.” should state if it’s a CLR or simply consistent with 
    XPath 1.0 expressions.
  - this command detected an issue: pyang --ietf ietf-system-capabilities\@2020-03-23.yang.
    (I’m unsure why, exactly, as I see the keywords block in the module…suggest copying
    text directly from `pyang --ietf-help`.


Section 5
  - s/The YANG module ietf-notification-capabilities is defined to provide/The YANG module "ietf-notification-capabilities” provides/

Section 5.2:  ([e-mail])
  - P1 doesn’t specify all the normative imports
  - be sure there is a blank line between paragraphs and numbered
    list items, and/or list items are indented (this comment applies to
    *all* “description” statements)
  - s/If, different/If different/
  - "RFC XXX: YANG-Push Notification Capabilities” has both an 
    incorrect number of X’s as well as incorrect document title.
  - s/config=true/‘config true’/
  - s/config=false/‘config false’/
  - "supported for none” - nonsensical, rephrase.
  - s/ or all data nodes/, or all data nodes/  (also try to fix the sentence so it makes sense)
  - s/ or a set of/, or a set of/
  - s/update-too-big/'update-too-big’/
  - many of the “reference” statements are being misused.  Move content
    information into the “description” and only set the reference per 
    https://tools.ietf.org/html/rfc7950#section-7.21.4
  - s/and the subtree below them/including any subtrees that may exist below them/
  - s/on-change notifications/'on-change' notifications/

  - this command detected a number of issues: pyang -f yang --keep-comments 
    --yang-line-length 69 ietf-notification-capabilities\@2020-03-23.yang > tmp; 
    diff ietf-notification-capabilities\@2020-03-23.yang tmp. (whitespace at end of lines)



Section 7.2 (really 5.2, but I didn’t catch it until here)

  - the “inc” prefix seems odd, perhaps wrong, how about “notc” (to match “sysc”)?




Appendix A

  -  the example is wrapped in the <CODE BEGINS>/<CODE ENDS>, but now that `rfcstrip` has the “-a” parameter, these are no longer needed - agreed?  Please move the filename augment to the “name” attribute in the <artwork> element for both examples.
  - ideally there are subsections (i.e., A.1 and A.2) delineating each example
  - s/config=false/“config false”/
  - please reorder the examples so that the system-capability examples precedes the notification-capability example 

  - in both examples:
    - s/following instance-data/following instance data/ (or "instance data set”?)
        ^-- also add a reference to I-D.ietf-netmod-yang-instance-file-format
     - please remove the "<!-- revision date, contact, etc. —>” comment
     - s/config=false/“config false”/
     - s/config=true/"config true”/
     - consider putting quotes around words like “on-change” and “periodic”
     - s/only 2/only two/


Other:

  - nowhere in the document is there a request to the RFC Editor to set the “XXXX”, “YYYY”, and “YYY" strings.  Perhaps other strings need to be converted as well...

  - through draft (in both body and YANG modules>, bracket datastore names with
    angle brackets (e.g., s/running/<running>/) wherever the datastore name is NOT
    followed by word “datastore”.   When the datastore name is NOT followed by the
    word “datastore", the datastore name should be in quotes, where it makes sense
    to do so.  BTW, I note the none of the datastore names are imported as terms or
    otherwise introduced where first used...

  - the YANG Doctor review suggested adding comments regarding
    if imports needed to be implemented.  I see such comments were
    added in -06 but they’ve subsequently been removed.  I strongly
    second Lada’s suggestion to include a minimal YANG Library
    instance somewhere, not only to document which modules need 
    to be implemented, but also to document to totality of them, as I
    was unpleasantly surprised by how many there were (12 in total!)



Kent // shepherd