Subject: Re: [Netconf] RtgDir review: draft-ietf-netconf-nmda-netconf-06

From: Martin Bjorklund <[email-masked]>
Date: Tue, 07 Aug 2018 12:36:06 +0200 (CEST)

Hi,

Kent Watsen <[email-masked]> wrote:
> Hi Lou,
> 
> Thanks for your review!
> 
> > Summary:
> >
> > I have some minor concerns about this document that I think should be 
> > resolved before publication.
> >
> > Comments:
> >
> > The document is is generally well written and easy to read.  There are 
> > several places where I'm sure the authors know exactly what they intend, 
> > but the text could be revised to help along those less familiar with the 
> > work.  There is also one miss-marked RFC Update reference.
> 
> Just to be sure, all these issues are discussed below, right?
> 
> 
> > Major Issues:
> >
> > <none>
> >
> > Minor Issues:
> >
> > - Cover/Abstract
> >    Updates: 7950
> >
> >    The update to
> >    RFC 7950 requires the usage of I-D.ietf-netconf-rfc7895bis by NETCONF
> >    servers implementing the Network Management Datastore Architecture.
> >
> > If I read this and the referenced document correctly, this is saying 
> > that I-D.ietf-netconf-rfc7895bis updates which version of YANG library 
> > is supported by implementations RFC7950 that support NMDA.  If this is 
> > the correct reading, this document doesn't update RFC7950, but rather 
> > I-D.ietf-netconf-rfc7895bis updates 7950. (this omission was noted in a 
> > separate message.)
> 
> The last paragraph in Section 2 says this:
> 
>    This document updates [RFC7950], Section 5.6.4, to allow servers to
>    advertise the capability :yang-library:1.1 instead of :yang-
>    library:1.0, and to implement the subtree "/yang-library"
>    [I-D.ietf-netconf-rfc7895bis] instead of "/modules-state".
> 
> Note that RFC 7950, Section 5.6.4 says:
> 
>    A NETCONF server MUST announce the modules it implements (see
>    Section 5.6.5) by implementing the YANG module "ietf-yang-library"
>    defined in [RFC7895] and listing all implemented modules in the
>    "/modules-state/module" list.
> 
> Which is what this document changes.
> 
> 
> 
> > - section 3.1.1.
> >
> >    The "config-filter" parameter can be used to retrieve only "config
> >    true" or "config false" nodes.
> >
> >    also
> >          leaf config-filter {
> >            type boolean;
> >            description
> >              "Filter for nodes with the given value for their
> >               'config' property.";
> >          }
> >
> > So this means:
> >     absent = provide all
> >     true = provide only true
> >     false = provide only false
> >
> > Right? Either way, I think this could be clarified a bit.  At least say 
> > what behavior is expected when the leaf is omitted.
> 
> OLD:
>              "Filter for nodes with the given value for their
>               'config' property.";
> NEW:
>              "Filter for nodes with the given value for their
>               'config' property.  For example, when set to 
>               'true', only 'config true' nodes are returned.
>               When unset, all nodes are returned.";
> 
> Okay?

In order to match the rest of the text I suggest:

NEW:

          "Filter for nodes with the given value for their
           'config' property.  If this leaf is not present, all
           nodes are selected.

           For example, when this leaf is set to 'true', only 'config
           true' nodes are selected.";

> > Nits:
> >
> > - the orders of sections 3.1.1.1. and 3.1.1.2. should be reversed to 
> > match the module ordering.
> 
> ...or change the order in the module, which I think might be preferred.

I suggest we swap 3.1.1.1 and 3.1.1.2.

> > - Section 3.1.2:
> >
> >     The "default-operation" parameter is a copy of the
> >     "default-operation" parameter of the <edit-config> operation.
> >
> >     The "edit-content" choice mirrors the "edit-content" choice of the
> >     <edit-config> operation.  Note, however, that the "config" element in
> >     the "edit-content" choice of <edit-data> uses "anydata" (introduced
> >     in YANG 1.1) while the "config" element in the "edit-content" choice
> >     of <edit-config> used "anyxml".
> >
> > It's fine to say that these nodes mirror <edit-config> nodes, but this 
> > document should at least summarize the function of each, e.g.,
> >      The "default-operation" parameter selects the default operation
> >      for this request. It is a copy....
> 
> NEW
>      The "default-operation" parameter selects the default operation to
>      use.  It is a copy of the "default-operation" parameter of the 
>      <edit-config> operation.
> 
>      The "edit-content" parameter specifies the content for the edit 
>      operation.  It mirrors the "edit-content" choice of the
>      <edit-config> operation.  Note, however, that the "config" element in
>      the "edit-content" choice of <edit-data> uses "anydata" (introduced
>      in YANG 1.1) while the "config" element in the "edit-content" choice
>      of <edit-config> used "anyxml".

Ok.

I will make these edits to the document.

/martin

> Okay?
> Thanks,
> Kent // co-author