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

[Martin Bjorklund <[e-mail]>]

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.

> 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://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
>   - import "ietf-inet-types" should reference RFC 6991 per (not as a comment)
>     https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
>   - import "ietf-datastores" should reference I-D.ietf-netmod-revised-datastores per
>     https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
>   - import "ietf-origin" should reference I-D.ietf-netmod-revised-datastores per
>     https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
>   - import "ietf-netconf" should reference RFC 6241 per
>     https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
>   - import "ietf-netconf-with-defaults" should reference RFC 6243 per
>     https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7

I think we have to wait for the outcome of that discussion.  I would
prefer to add the reference statements.

> - Module description, revision and feature definition should contain note to
>   RFC Ed. to change placeholder reference to RFC when assigned
>   https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#appendix-C

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?

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

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


> - :with-origin    urn:ietf:params:netconf:capability:with-origin:1.0
>   Wouldn't it make more sense to have this URN defined in
>   I-D.ietf-netmod-revised-datastores rather?  In addition, should this
>   capability and feature be rather defined as 'origin' in the 'ietf-origin'
>   module?  As defined today, this feature is used as a clause for
>   'origin-filter' as well as 'with-origin' inputs to get-data however these
>   inputs appear to be mutually exclusive.
>   IMHO, this should be detached from this specific draft which focuses on
>   NETCONF specific base extensions for datastores as the returning and
>   filtering of origin metadata can be transport independent.

The capability is NETCONF-specific, and should be defined here, imo.
As for the feature, it is not as clear.  It is not obvious that it
should be defined in ietf-origin.  (and since that doc is at the RFC
editor, it might be late to change, unless it is really urgent).

> - L308: suggested replacement of text:
>   s/parameter is optional to support/parameter is optional and dependant off
>   of the servers support for the 'origin' feature/ (dependant off the previous
>   comment)

Do you mean that if we don't move the origin feature, this text should
not change?

> - L320: "ietf-netconf-nmda" (see Section 4). - This should be an actual
>   reference as in line #306

I don't understand this comment.  It is a proper reference...?

> - L345: "default-operation" parameter of the <edit-config> operation - This
>   should at a bare minimum call reference to the appropriate section in
>   RFC6241

There is a reference for <edit-config> to RFC 6241 in the first
paragraph of this section.  I think that is how references should be
done, but I might be wrong.  I am sure the RFC editor will fix this,
if needed.

> - To the previous point, since there is a large amount of reference and
>   comparison to RFC6241 definitions, any references should be tagged
>   appropriately

I am not sure I understand what you mean with "tagged appropriately".
Could you provide OLD / NEW text for these occurences?


/martin