Hi Martin - see inline... On Feb 23 16:43 PM, Martin Bjorklund wrote: > 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. > I'm fine w/ this - I agree > > 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://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. > 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. > > > - :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. Understood. I guess what I would have imagined is rather a capability indicating that the operational datastore supports the population of 'origin' metadata. Support of that capability would then indicate support of the 'with-origin' parameter which is just a toggle to return metadata or not. Per draft-ietf-netmod-revised-datastores, it appears that the population of origin metadata is mandatory however the wording appears pretty vague around such. This capability exists in both this draft as well as the RESTCONF draft and am trying to understand if origin metadata is mandatory for support in <operational> then why is it necessary to have a capability just for support of this parameter vs. a capability of origin metadata support as a whole? If the capability is meant to convey 'origin' metadata support as a whole, then I think it should be named 'origin' rather than 'with-origin' (which is just the parameter name we are discussing) > 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). > It seems odd to me that we wouldn't define the feature along side the annotation/identity definitions but rather here in a protocol specific module. I think that the population of 'origin' metadata support as a whole should be the feature, this should exist in the ietf-origin module and 'origin-filter' and 'with-origin' parameters would still be gated by this feature that is defined elsewhere. As is defined today, the feature description even indicates server support for the annotation as a whole. feature origin { description "Indicates that the server supports the 'origin' annotation."; reference "RFC XXXX: Network Management Datastore Architecture"; } > > - 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? > The suggestion above is merely to add a bit more description around this parameter being gated off the server's support for populating origin metadata. If the server supports origin metadata, then it is optional and the client can choose to use/not-use. If the server does not support origin metadata, then this parameter is not supported or visible. > > - 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...? > I would assume reference to [I-D.ietf-netmod-revised-datastores] here but there are a number of other spots where this exists as well. This can be left to the RFC editor as well > > - 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. > We can leave this to the RFC editor > > - 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? > Same comment as above. This can be left to the RFC editor and don't believe all definitions need to be referenced. Thx /ebben
