Hi, Ebben Aries <exa@xxxxxxxxxxx> wrote: > 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. I have added references for now. > > > - 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. Ok, done. > > > - :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. Ok. After some discussion with my co-authors, we suggest that we remove the ":with-origin" capability, and just use the feature "origin". In fact, the YANG module ietf-netconf-nmda already uses this feature to control both the "with-origin" and "origin-filter" parameters, and the feature is advertised on servers that support the "origin" annotation. The YANG module doesn't even mention the "with-origin" capability. > Per draft-ietf-netmod-revised-datastores, it appears that the population > of origin metadata is mandatory however the wording appears pretty vague > around such. The architecure document defines this concept, but I think that different protocols may or may not expose this; for example, a protocol for constrained devices might choose to not support this at all. > 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. /martin
