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

[Benoit Claise <[e-mail]>]

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