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