Re: [yang-doctors] [Netconf] Yangdoctors last call review of draft-ietf-netconf-nmda-netconf-03

[Benoit Claise <[e-mail]>]

On 2/25/2018 4:14 AM, Ebben Aries wrote:
> Hi Martin - see inline...
>
> On Feb 23 16:43 PM, Martin Bjorklund wrote:
>> Hi,
>>
>> Thank you for this review.  Comments inline.
>>
>> Ebben Aries <[e-mail]> wrote:
>>> Reviewer: Ebben Aries
>>> Review result: Almost Ready
>>>
>>> 1 module in this draft:
>>> - [e-mail]
>>>
>>> YANG validation errors or warnings (from pyang 1.7.3 and yanglint 0.14.69)
>>> - [e-mail]:171: warning: RFC 6087: 4.10,4.12:
>>>    statement "enum" should have a "description" substatement (From pyang 1.7.3)
>> Yes, this is a warning b/c it is a SHOULD in 6087.  In this case, the
>> enum is described together with the rest in the main description.  I
>> think it gives a better overall description of the type.
>>
> I'm fine w/ this - I agree
>
>>> 0 examples are provided in this draft (section 3.12 of
>>> draft-ietf-netmod-rfc6087bis-18)
>>> Module [e-mail]:
>>> - Note: For the following imports, there are no references to the supporting
>>>    documents as suggested in rfc6087bis however this item is currently under
>>>    discussion on both usefulness/possible formatting
>>>    - import "ietf-yang-types" should reference RFC 6991 per (not as a comment)
>>>      https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
>>>    - import "ietf-inet-types" should reference RFC 6991 per (not as a comment)
>>>      https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
>>>    - import "ietf-datastores" should reference I-D.ietf-netmod-revised-datastores per
>>>      https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
>>>    - import "ietf-origin" should reference I-D.ietf-netmod-revised-datastores per
>>>      https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
>>>    - import "ietf-netconf" should reference RFC 6241 per
>>>      https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
>>>    - import "ietf-netconf-with-defaults" should reference RFC 6243 per
>>>      https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23section-2D4.7&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=lYv-rQuwL9GaPVAIXkxrGgMI1AMjZg3yyhxbU2dUzZw&e=
>> I think we have to wait for the outcome of that discussion.  I would
>> prefer to add the reference statements.
>>
> Since we've been actively suggesting to add into the reference
> statements, I suggest to just go ahead w/ the syntax you previously
> described.  If we have a different outcome, then 6087bis can be updated
> as well as modification to modules if time permits.
>
>>> - Module description, revision and feature definition should contain note to
>>>    RFC Ed. to change placeholder reference to RFC when assigned
>>>    https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dietf-2Dnetmod-2Drfc6087bis-2D18-23appendix-2DC&d=DwICAg&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=GIehbDpQlo31lSi6WbnEkA&m=vjCdF0tPthdQp5vicuYUDK234W2DXJSvI9zSM0jaQS4&s=Bsh4kYXQ9T8sGblwrJnk41asnQCqSuUZjzvla2EgA9E&e=
>> Ok.
>>
>>> - Security Considerations looks good and is adjusted to account for NETCONF
>>>    only as well as addressing the additional RPCs introduced
>>> - L82: suggested replacement of text:
>>>    s/, i.e., the filter criteria are logically ANDed/. Multiple filters are
>>>    processed as a logical AND operation/
>> Hmm.  This misses the "i.e." - the original text explains the sentence
>> just before it.  What do others think?  Maybe leave this to the RFC
>> editor?
>>
> We can leave this to the RFC editor
>
>>> - L127: suggested replacement of text:
>>>    s/then the get-data/the get-data/
>> We have "if ... then ..." in several places.  Maybe also leave this to
>> the RFC editor?
>>
> We can leave this to the RFC editor
>
>>> General comments/nits on draft text:
>>> - As mentioned above, there are 0 examples in this draft.  There should be XML
>>>    RPC examples of the 2 newly defined RPCs as well as usage examples of the
>>>    augments of RFC6241 RPCs
>> This is a SHOULD in 6087.  I don't think that examples for these
>> operations would add much value.
Note that it's a SHOULD in a BCP (RFC6087bis)
I heard before the argument that 6087bis was not a standard. It's not 
the case any longer with RFC6087bis, on the next IESG telechat

Regards, Benoit
>>
> More examples is generally a good thing and provides clarity to
> implementors.  While it is a SHOULD, RFC6241 carries examples across all
> sections and propose this extension follow the same format.