Re: [netconf] Shepherd review of draft-ietf-netconf-notification-capabilities

[Benoit <[e-mail]>]

Hi Kent,

Removed what's agreed upon.
See inline.

> Balazs and Benoit,
>
> Thanks for the update and the response.  Please see below for followups.
>
> K.  // shepherd
>
>
>> On Feb 22, 2021, at 4:20 PM, Benoit <[e-mail] 
>> <mailto:[e-mail]>> wrote:
>>
>> Hi Kent,
>>
>> Sorry it took us so long to update the draft.
>> Below are our answer. We believe we have addressed all comments.
>
>
>> - s/YANG Instance Data/YANG Instance Data 
>> (I-D.ietf-netmod-yang-instance-file-format)/
>> BALAZS: OK
>
> Hmmm, now it’s renamed and moved to the end of the paragraph, but 
> still without a reference.  Admittedly, I think references in 
> Abstracts are discouraged but, as it stands, readers may be confused. 
>  Note that searching for "YANG Instance Data File Format” doesn’t 
> resolve any other matching because only other matching string is in 
> the “Normative References” Section, where it line wraps.  If you don’t 
> put a reference into the abstract, then import the term in the 
> “Terminology” Section.
Done.
Added to the terminology section:
           The terms "YANG instance data file format", "instance data", and
             "instance data set" are used as defined in
           <xref target="I-D.ietf-netmod-yang-instance-file-format"/>.
>
>
>> - s/"Subscription to YANG Datastores" (YANG-Push)/Subscription
>> to YANG Notifications for Datastore Updates (RFC 8641)/
>> BALAZS: OK
>
> Thanks, but not that the sentence ends 'augmenting the 
> "ietf-system-capabilities”.’   This should either be 'augmenting 
> "ietf-system-capabilities”.’ or 'augmenting the 
> "ietf-system-capabilities” module.'
>
>
Done. 'augmenting "ietf-system-capabilities”
>> - Actually (in ref to the above two comments), the sentence structure 
>> in p2 seems
>> backwards. Better would be to say "The module can be used to report 
>> capability
>> information from the server at run-time or implementation-time, per 
>> I-D.ietf-netmod-yang-instance-file-format.”
>> BENOIT: idnits complains when we have references in the abstract. 
>> Removed the references
>
> Ah, yes, right, then import the term?

See above. Added to the terminology section:
>
>> - s/that can be used to specify/that can be used to discover (as 
>> read-only operational state)/
>> BENOIT: DONE
>
> Good, but searching for "hat can be used to specify” resulted in 
> finding two other instances that I believe would benefit from the same 
> update.
Only one was left, in the abstract. Now changed.
Note that I did not add "(as read-only operational state)" in the 
abstract. I left for the explanation in the draft. I'm fear that might 
be distracting and confusing.
>
>
>> - rather than have the “description” statement "// augmentation
>> point for system level capabilities”, why not have an empty
>> “choice” statement with the same description statement?
>> BALAZS: choice would not be a good solution because it would only 
>> allow one set of capabilities, while our goal is to allow different 
>> capability set side by side.
>
> You misunderstand.  Adding an empty choice preserves the data model as 
> is.  It’s just a more "YANG way” to do what you have:
>
> OLD:
>     // augmentation point for system level capabilities
>
> NEW:
>    choice system-capabilities {
>       // mandatory true;  // must at least one exist?
>       description
>          "augmentation point for system level capabilities.”;
>    }
>
In our opinion,  the choice is not an ideal solution. For the choice you 
may only select one case. We, on the other hand, want to allow multiple 
augmentations to be used at the same time. If we used a choice, we could 
not allow multiple different cases/augmentations to be used at the same 
time. It is a question of on-of-many versus any-number.

>
>
>> - rather than have the “description” statement "// augmentation
>> augmentation point for datastore or data node level capabilities”,
>> why not have an empty “choice” statement with the same
>> description statement?
>> BALAZS: see above.
>
> Also see above  ;)
Also see above ;)
>
>
>> - is the comment "The special value '/' denotes all data nodes in the
>> datastore.” should state if it’s a CLR or simply consistent with 
>> XPath 1.0 expressions.
>> BALAZS: we are consistent witjh the NACM RFC, which says:
>>                     The special value '/' refers to all possible
>>                     datastore contents.";
>
> Then the comment should say “As specified in NACM Section ???, the 
> special value…”, right?
Done.
           leaf node-selector {
             type nacm:node-instance-identifier;
             description
               "Selects the data nodes for which capabilities are
                specified. The special value '/' denotes all data nodes
                in the datastore, as specified in path YANG object in
                data definition statement [RFC8341];
             reference
                   "RFC 8341: Network Configuration Access Control Model";
           }

>
>
>> - this command detected an issue: pyang --ietf 
>> ietf-system-capabilities\@2020-03-23.yang.
>> (I’m unsure why, exactly, as I see the keywords block in the module
$ pyang --ietf ietf-system-capabilities.yang
$
$ pyang -v
pyang 2.4.0

>> …suggest copying
>> text directly from `pyang --ietf-help`.
>
> No response?
That was an oversight. Done.
>
>
>
>> - this command detected a number of issues: pyang -f yang 
>> --keep-comments --yang-line-length 69 
>> ietf-notification-capabilities\@2020-03-23.yang > tmp; diff 
>> ietf-notification-capabilities\@2020-03-23.yang tmp. (whitespace at 
>> end of lines)
>
> No response?
That was an oversight. Done for both modules
>
>
>
>
>
>> - through draft (in both body and YANG modules), bracket datastore 
>> names with
>> angle brackets (e.g., s/running/<running>/) wherever the datastore 
>> name is NOT
>> followed by word “datastore”. When the datastore name is NOT followed 
>> by the
>> word “datastore", the datastore name should be in quotes, where it 
>> makes sense
>> to do so. BTW, I note the none of the datastore names are imported as 
>> terms or
>> otherwise introduced where first used...
>> Benoit: Done
>
> I don’t see these changes...
What I did in the previous draft version: I searched for all instances 
of running, and it's always followed by the word datastore. I did the 
same for candidate, operational (there are no instances of startup and 
intended).
I double-checked this new version too.

Added a reference to RFC 8342 with the first occurrence of datastore in 
the intro.

>
>
>> - the YANG Doctor review suggested adding comments regarding
>> if imports needed to be implemented. I see such comments were
>> added in -06 but they’ve subsequently been removed. I strongly
>> second Lada’s suggestion to include a minimal YANG Library
>> instance somewhere, not only to document which modules need to be 
>> implemented, but also to document the totality of them, as I
>> was unpleasantly surprised by how many there were (12 in total!)
>> BALAZS: OK, Included for the 3 imports that need implemented. They 
>> were removed due to comments, but I like the sentences.
>
> Better, but no YANG Library instance?  Oh well.

Thanks for your review.

I'll be posting a new version soon.

Regards, Balazs and Benoit
>
>
> K.
>
>