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

From: Benoit <[email-masked]>
Date: Fri, 2 Apr 2021 21:58:43 +0200

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 <[email-masked] 
>> <mailto:[email-masked]>> 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.

---

From: Kent Watsen <[email-masked]>
Date: Thu, 15 Apr 2021 00:55:04 +0000

[CC NETCONF]

K.

> On Apr 14, 2021, at 8:27 PM, Kent Watsen <[email-masked]> 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 <[email-masked] <mailto:[email-masked]>> 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 <[email-masked]> <mailto:[email-masked]>
>>> To: 	Kent Watsen <[email-masked]> <mailto:[email-masked]>, Benoit <[email-masked]> <mailto:[email-masked]>
>>> CC: 	[email-masked] <mailto:[email-masked]> <[email-masked]> <mailto:[email-masked]>
>>> 
>>> Hi Kent,
>>> 
>>>> 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 <[email-masked] <mailto:[email-masked]>> wrote:
>>>>> 
>>>>> Hi Kent,
>>>>> 
>>>>> - 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. 
>>> 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
> 
> 
>>>> 
>>>> 
>>>> 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 
>>>> 
>>>>> - 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.
>>> 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 ;)
>>>> 
>>>> 
>>>>> datastore.b consistent with XPath 1.0 expressions.
>>>>> 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
>>>>> b class=""> text directly from `pyang --ietf-help`.
>>>> 
>>>> No response?
> 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?
> 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
>>>> 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).
>>> Added a reference to RFC 8342 with the first occurrence of datastore in the intro.
> 
> Thanks.
> 
> 
> 
>>>>> 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
>>>>> 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.
>>> 
> No problem, thanks.

---

From: Benoit Claise <[email-masked]>
Date: Fri, 16 Apr 2021 15:42:21 +0200

Hi Kent,

Thanks for your review.
See inline, after some clean up.
I addressed your two remaining comments and I posted a draft version: 
draft-ietf-netconf-notification-capabilities-16

>> Looking at the new draft, I see the added clarification (thanks), but 
>> it’s a bit difficult to read.  Would this help?
>>
Done.
Did not include the double quotes around path to avoid troubles.
>>>>>> ietf-notification-capabilities\@2020-03-23.yang tmp. (whitespace 
>>>>>> at end of lines)
>>>>>
>>>>> No response?
>> 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.
Corrected.

Regards, Benoit