Re: [yang-doctors] Yangdoctors last call review of draft-ietf-netmod-syslog-model-17

["Clyde Wildes (cwildes)" <[e-mail]>]

Kent and Benoit,

All of Kent’s feedback was addressed AFAIK. I can go back over each item to check if needed.

Thanks,

Clyde

On 9/12/17, 8:12 AM, "Benoit Claise (bclaise)" <[e-mail]> wrote:

    Kent,
    
    The question is: your feedback has been addressed in the version 17?
    
    Regards, Benoit
    > On 9/12/2017 3:58 PM, Kent Watsen wrote:
    >> Reviewer: Kent Watsen
    >> Review result: Ready with Issues
    >>
    >>
    >> [This is from my review posted to netmod ML on July 12]
    >>
    >> As shepherd, yang doctor, and individual contributor, following is
    >> my LC/YD review.
    >>
    >> 1. Because I know this draft will not be presented in Prague, I first
    >> checked to see if it was NMDA-compatible.  The draft contains just
    >> one module, and it only contains config true nodes (no config false
    >> nodes).  There is no companion "-state" module in the Appendix. As
    >> far as I can tell, all this is accurate, as I don't believe this
    >> module needs to do anything special to be NMDA compatible. Agreed?
    >>
    >> 2. the abstract seems just a little bland.  Is there any way to beef
    >> it up with a sentence or two?
    >>
    >> 3. S1, P1, last sentence.  s/the messages/these messages/?
    >>
    >> 4. S1, P3, 1st sentence: "and processes those"?  - rewrite sentence?
    >>
    >> 5. S1 as a whole.  I'm a bit unclear what this section is doing.  It
    >> seems to be a general summary of Syslog (RFC5424).  Do we need this 
    >> here?
    >>
    >> 6. S1.1: you should also reference RFC8174 here.
    >>
    >> 7. S1.2: three terms come from 5424, but only one has its definition
    >>     provided.  This seems inconsistent...
    >>
    >> 8. S2: s/6020/7950/
    >>
    >> 9. S3, P3: this paragraph is hard to read due to the previous paragraph
    >> talking about proprietary features.  Maybe replace the beginning of the
    >> sentence to read "Some optional features are defined in this document
    >> to specify"?
    >>
    >> 10. S3, P4: The diagram appears to show multiple originators, not
    >> just one, so s/an originator/originators/?  Also, I don't think
    >> either of the commas are needed.
    >>
    >> 11. S3, P6: This paragraph starts a new aspect of the design, right?
    >> This is likely just a text-rendering issue, but the transition from
    >> the diagram above (Figure 1) to this line is not visible.  Can you
    >> provide a transition sentence?
    >>
    >> 12. S3, P8: I'm having trouble understanding the pseudocode. What
    >> happens if S and/or F are not present?  Can S or F ever not be
    >> present? - looking at the tree diagram, it seems like they might
    >> always be set to something in the model.
    >>
    >> 13. S3.1, P1: RFC 6087 did not define tree diagram notation, and
    >> rfc6087bis references the tree-diagram draft.  I don't think that
    >> it is safe for this draft to reference the tree-diagram draft, as
    >> that draft is unstable (the notation may change).  You should
    >> probably copy/paste the Tree Diagram Notation section found in
    >> other drafts today (especially mine).
    >>
    >> 14. S3.1: is /syslog/actions/remote/destination/tls/ missing an
    >> 'address' leaf?
    >>
    >> 15. S4.1, P1: Doesn't the module import *groupings* from ietf-keystore
    >> and ietf-tls-client?
    >>
    >> 16. S4.1, though it's not in 6087bis, I think that it is best
    >> practice for 'import' statements to include a 'reference'
    >> substatement:
    >>
    >>    import ietf-keystore {
    >>      prefix ks;
    >>      reference
    >>        "RFC YYYY: Keystore Model";
    >>    }
    >>
    >> 17. S4.1, imports that are used for groupings only should use a
    >> revision statement:
    >>
    >>    import ietf-tls-client {
    >>      prefix tlsc;
    >>      revision-date YYYY-MM-DD; // stable grouping definitions
    >>      reference
    >>        "RFC ZZZZ: TLS Client and Server Models";
    >>    }
    >>
    >> 18. S4.1, can you put the beginning of the 'organization' (i.e. "IETF")
    >> on the next line, s/NETCONF Data Modeling Language/Network Modeling/,
    >> and put a blank line in after the 'organization' line?
    >>
    >> 19. S4.1, in the 'severity-filter' grouping, why does leaf 'severity'
    >> have values set for enums 'none' and 'all'?  When would these values
    >> be used, as opposed to the enum's name string?  If you do need values,
    >> then shouldn't 'none' be 2147483647 (so nothing can be greater than it)
    >> and 'all' be -2147483648 (so everything is greater than it)?
    >>
    >> 20. S7: can you indent the two blocks of details so the whole thing
    >> reads better?
    >>
    >> 21. S8: please rework so this section so it matches the new template
    >> at: https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines
    >>
    >> 22. S8.1: it would be better if the third paragraph was moved up to
    >> become the first paragraph.
    >>
    >>
    >> DISCLAIMER: I'm not a syslog expert, but have interacted with it,
    >> including structured-syslog, over the years.
    >>
    >> Kent
    >>
    >>
    >> _______________________________________________
    >> yang-doctors mailing list
    >> [e-mail]
    >> https://www.ietf.org/mailman/listinfo/yang-doctors
    >> .
    >>
    >
    > _______________________________________________
    > yang-doctors mailing list
    > [e-mail]
    > https://www.ietf.org/mailman/listinfo/yang-doctors
    > .
    >