Hi, Thank you for this review. Comments inline. Ebben Aries <exa@xxxxxxxxxxx> wrote: > Reviewer: Ebben Aries > Review result: Almost Ready > > 1 module in this draft: > - ietf-netconf-nmda@xxxxxxxxxxxxxxx > > YANG validation errors or warnings (from pyang 1.7.3 and yanglint 0.14.69) > - ietf-netconf-nmda@xxxxxxxxxxxxxxx: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 ietf-netconf-nmda@xxxxxxxxxxxxxxx: > - 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
