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

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
> yang-doctors@ietf.org
> https://www.ietf.org/mailman/listinfo/yang-doctors
> .
>