Early Review of draft-ietf-ntp-yang-data-model-03
review-ietf-ntp-yang-data-model-03-yangdoctors-early-bierman-2018-10-08-00

Overall:
   - no compiler warnings; passes --ietf check as well

Normative Sections:

sec 1:
   - YANG version cited should be RFC 7950, not 6020.
     YANG 1.1 is used in this document.\

sec 4: Relationship with RFC 7317
  - this section should say how overlapping configuration
    objects interact. Does setting 1 field (e.g., /system/ntp/enabled)
    change the other field at the same time  (e.g., /ntp/enabled)
    It seems the intent is to ignore and deprecate /system/ntp if
    /ntp is implemented.  This section should explain.

sec 5:
  - this section is supposed to have citations for every imported
    YANG module used in the ietf-ntp YANG module


YANG Module Issues:
  1 - Indexing used in /ntp/associations list
     key "address local-mode isConfigured";

     a) will there really be multiple instances per address for
        different association-modes?  The list description-stmt
        should explain.

     b) There can be configured values (isConfigured key) and learned
        entries for the same address and association-modes?  Why isn't
        NMDA used instead? (i.e. intended and operational datastores
        used instead of the isConfigured list key?)

  2 - Purpose of /ntp/access-rules
      There is no explanation for what it means to configure an entry
      here that points at an ACL entry in /acls/acl; The description
      statement needs to specify the details.  Doesn't the ACL module
      just work? How does the /ntp/access-rules/access-rule list
      change behavior?

  3 - Reinvent Key Management
      It does not seem like a good idea for every protocol to
      duplicate key management functionality. The draft should
      explain why other YANG modules related to this work are not
      relevant here

  4 - Reference statements
      There are no reference statements used in any objects or typedes
      Definitions which are intended to match a definition in NTP
      should include a reference-stmt

Minor Issues:
   - typedef names should be singular
      - access-mode not access-modes
      - association-mode not association-modes
   - grouping comman-attributes should be common-attributes
   - mixed-case naming should not be used (isConfigured)
   - indentation used in module needs a lot of corrections
   - some descriptions have incorrect tense (e.g., "NTP is enable")
   - port definition should be based on inet:port-number, not uint16
     OLD:
         type uint16 {
           range "123 | 1025..max";
         }
     NEW:
         type inet:port-number {
           range "123 | 1025..max";
         }

   - /ntp/authentication/trusted-keys
     Why is this list needed? Seems a leaf (e.g., trusted-key (true/false)
     added to the authentication-keys list is sufficient
   - /ntp/clock-state : some leafs should have a units-stmt instead of
     specifying the units in the description-stmt (could also apply elsewhere)
   - Examples should use line continuation convention
     e.g.:
     OLD:
      <transmit-time>10-10-2017 07:33:55.300 Z+05:30
         </transmit-time>
     NEW:
      <transmit-time>10-10-2017 07:33:55.300 Z+05:30\
         </transmit-time>
   - a spell checker should be used; There are many description-stmts
     with spelling errors