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

[Kent Watsen <[e-mail]>]

[CC NETCONF]

K.


> On Apr 14, 2021, at 8:27 PM, Kent Watsen <[e-mail]> wrote:
> 
> Hi Benoit (and Balazs),
> 
> The text-formatting below is pretty garbled, but we’re almost done and it’s almost understandable ;)
> 
> Please see more comments below.
> 
> K.
> 
> 
>> On Apr 7, 2021, at 9:44 AM, Benoit <[e-mail] <mailto:[e-mail]>> wrote:
>> 
>> btw, a mistake with the year 22021 has been corrected in my temp 016 version.
> 
> Good catch!  :)
> 
> 
>> Regards, Benoit
>>> Hi Kent,
>>> 
>>> The draft has been posted.
>>> 
>>> Regards, Benoit
>>> 
>>> 
>>> -------- Forwarded Message --------
>>> Subject:	Re: [netconf] Shepherd review of draft-ietf-netconf-notification-capabilities
>>> Date:	Fri, 2 Apr 2021 21:58:43 +0200
>>> From:	Benoit <[e-mail]> <mailto:[e-mail]>
>>> To: 	Kent Watsen <[e-mail]> <mailto:[e-mail]>, Benoit <[e-mail]> <mailto:[e-mail]>
>>> CC: 	[e-mail] <mailto:[e-mail]> <[e-mail]> <mailto:[e-mail]>
>>> 
>>> Hi Kent,
>>> 
>>> Removed what's agreed upon.
>>> See inline.
>>> 
>>>> Balazs and Benoit,
>>>> 
>>>> Thanks for the update and the response. B Please see below for followups.
>>>> 
>>>> K. B // 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 itb paragraph, but still without a reference. B Admittedly, I think references in Abstracts are discouraged but, as it stands, readers may be confused. B Note that searching for "YANG Instance Data File Formatb other matching because only other matching string is in the b B If you donb import the term in the b
>>> Done. 
>>> Added to the terminology section:
>>> B B B B B B B B B The terms "YANG instance data file format", "instance data", and 
>>> B B B B B B B B B B "instance data set" are used as defined in 
>>> B B B B B B B B B <xref target="I-D.ietf-netmod-yang-instance-file-format"/>.
> 
> WFM
> 
> 
>>>> 
>>>> 
>>>>> - s/"Subscription to YANG Datastores" (YANG-Push)/Subscription
>>>>> to YANG Notifications for Datastore Updates (RFC 8641)/
>>>>> BALAZS: OK
>>>> 
>>>> Thanks, but now that sentence ends 'augmentingB the "ietf-system-capabilitiesb should either be 'augmenting "ietf-system-capabilitiesb "ietf-system-capabilitiesb
>>>> 
>>>> 
>>> Done. 'augmenting "ietf-system-capabilitiesb
>>>> 
>>>>> - 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.b class=""> 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 specifyb 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.
> 
> Fine.
> 
> 
>>>> 
>>>> 
>>>>> - rather than have the b class=""> point for system level capabilitiesb an empty
>>>>> b 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. B Adding an empty choice preserves the data model as is. B Itb you have:
>>>> 
>>>> OLD:
>>>> B B //B augmentation point for system level capabilities
>>>> 
>>>> NEW:
>>>> B B choice system-capabilities {
>>>> B B B // mandatory true; B // must at least one exist?
>>>> B B B descriptionB 
>>>> B B B B B "augmentation point for system level capabilities.b
>>>> B B }
>>>> 
>>> In our opinion,B 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.
> 
> Okay.
> 
> 
>>>>> - rather than have the b class=""> augmentation point for datastore or data node level capabilitiesb why not have an empty b same
>>>>> description statement?
>>>>> BALAZS: see above.
>>>> 
>>>> Also see above B ;)
>>> Also see above ;)
>>>> 
>>>> 
>>>>> - is the comment "The special value '/' denotes all data nodes in the
>>>>> datastore.b consistent with XPath 1.0 expressions.
>>>>> BALAZS: we are consistent witjh the NACM RFC, which says:
>>>>> B B B B B B B B B B B B B B B B B B B The special value '/' refers to all possible
>>>>> B B B B B B B B B B B B B B B B B B B datastore contents.";
>>>> 
>>>> Then the comment should say b Section ???, the special valueb
>>> Done.
>>> B B B B B B B B B leaf node-selector {
>>> B B B B B B B B B B B type nacm:node-instance-identifier;
>>> B B B B B B B B B B B description
>>> B B B B B B B B B B B B B "Selects the data nodes for which capabilities are
>>> B B B B B B B B B B B B B B specified. The special value '/' denotes all data nodes
>>> B B B B B B B B B B B B B B in the datastore, as specified in path YANG object in 
>>> B B B B B B B B B B B B B B data definition statement [RFC8341];
>>> B B B B B B B B B B B reference
>>> B B B B B B B B B B B B B B B "RFC 8341: Network Configuration Access Control Model";
>>> B B B B B B B B B }
> 
> Looking at the new draft, I see the added clarification (thanks), but it’s a bit difficult to read.  Would this help?
> 
> OLD: as specified in path YANG object in data definition statement [RFC8341]
> 
> NEW: consistent with the “path” leaf node on page 41 in [RFC8341].
> 
> 
> 
>>>>> - this command detected an issue: pyang --ietf ietf-system-capabilities\@2020-03-23.yang.
>>>>> (Ib block in the module
>>> $ pyang --ietf ietf-system-capabilities.yang
>>> $ 
>>> $ pyang -v 
>>> pyang 2.4.0
>>> 
>>>>> b class=""> text directly from `pyang --ietf-help`.
>>>> 
>>>> No response?
>>> That was an oversight. Done.
> 
> Thanks.
> 
> 
>>>>> - 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
> 
> Thanks, but be mindful that tool’s opinion is not always perfect and that discretion can override it where preferred.  The primary intent is to show that an effort was made...
> 
> In particular, the closing “; for the "import ietf-yang-library” statement is on a line by itself.  IDK if the tool is suggesting this, but I think it better if the closing “; were brought up to the previous line.
> 
> 
> 
>>>>> - 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 b name is NOT followed by the
>>>>> word b 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 donb
>>> 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.
> 
> Thanks.
> 
> 
> 
>>>>> - the YANG Doctor review suggested adding comments regarding
>>>>> if imports needed to be implemented. I see such comments were
>>>>> added in -06 but theyb removed. I strongly
>>>>> second Ladab 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? B Oh well.
>>> 
>>> Thanks for your review.
>>> 
>>> I'll be posting a new version soon.
> 
> No problem, thanks.
> 
> K.
> 
> 
>>> Regards, Balazs and Benoit