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

[Kent Watsen <[e-mail]>]

Hi Clyde,

I just did:

1. ok
2. better
3. should be: s/the message/these messages/  [RFC Editor might've caught this]
4. better
5. still feel the same way, but no biggee
6. better, but from 8174, you should add the part "when, and only when, they appear in all capitals, as shown here."
7. fixed
8. fixed
9. you did what I asked, but the result still isn't satisfying...
10. some improvements made in this area, but my ask wasn't among them
11. better
12. better, but I think the 4th line should be indented too, right?
13. better, but I wish you called S1.3 "Tree Diagram Notation"
14. fixed
15. fixed
16. fixed
17. fine
18. still a weird line brake here.  try putting the quoted string on the next line.
19. fixed
20. fixed
21. not fixed (re: yang-security-guidelines)
22. fine

Kent




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