Subject: Re: [netmod] Shepherd review on draft-ietf-netmod-yang-instance-file-format-10

From: Balázs Lengyel <[email-masked]>
Date: Wed, 8 Apr 2020 12:06:25 +0000

Hello Kent,
Thanks for the review. See answers below.
I tried to address all you comments, sorry if I missed something. 
I updated the draft and uploaded a -11 version. Please check/advance it.

One question I could not settle: XML2RFC does not accept
      <?rfc include='reference.I-D.netmod-yang-module-versioning'?>      
Only
      <?rfc include='reference.I-D.verdt-netmod-yang-module-versioning'?>      
Why ? Please help.
Regards Balazs

-----Original Message-----
From: netmod <[email-masked]> On Behalf Of Kent Watsen
Sent: 2020. március 31., kedd 3:50
To: [email-masked]
Subject: Re: [netmod] Shepherd review on draft-ietf-netmod-yang-instance-file-format-10

[UPDATE: I just realized that I prematurely stopped the review after the YANG module.  Picking up where I left off…]

Structural Issues:

 - S5 contains an mix of important and unimportant information.   I think that the most important thing to state that the module defines an offline format that MAY contain security sensitive information, and thus safe handling is advised.  Maybe also say something about because the YANG module only defines a “structure”,  the Security Considerations doesn’t follow the template specified in https://tools.ietf.org/html/rfc8407#section-3.7.1).  For instance: s/is designed as a wrapper specifying a format and a metadata header for YANG instance data defined by the content-schema/specifies an offline format/
BALAZS: Most of text was required to be put there by earlier reviewers (Mostly Juergen and Acee Lindem) and sent to the mailing list.
I added that we do not follow the security template for YANG models.
  - S6.1. the full registration is not inside the <figure>.  (How could happen?)
BALAZS: Corrected
  - S8.1: I-D.ietf-netmod-yang-data-ext should be listed last, as the number it will be the greater than any other...
BALAZS: The order of references is decided by Xml2Rfc. In the draft-..xml it is the last.
  - S8.1: RFC8340 is Informative
BALAZS: OK
  - S8.1: agreed that RFC8525 is Normative, but the only place it it referenced is in a non-normative section…please add a ref to it from a normative section.
BALAZS: It is referenced from the YANG module which is normative.
  - S8.2: same comment as before re: floating the I-D refs to the end…
BALAZS: The order of references is decided by Xml2Rfc. In the draft-..xml it is the last.
  - Appendix C: remove the unnecessary “C.1” section.
BALAZS: OK

Editorial Issues:

  - Appendix B:
     - s/For instance data/Instance data/
BALAZS: Sorry, that would make the sentence incorrect.
     - “...to avoid are listed.” - listed where?  Section reference?
BALAZS: In the next list in the same chapter. Added "below".
     - In P2, how is the 2nd sentence connected to the 1st?
BALAZS: Separated them into 2 paragraphs. Instance data can be produced automatically or by some design activity. I was told by a previous reviewer that I should provide these guidelines only for the latter. However the guidelines are valid and important as proven by experience.
     - s/may lead to problems/may lead to the following problems:/?
BALAZS: OK
  - Appendix C:
      - Don’t put “Non-Normative” into the Section title (move to 1st sentence of section) 
BALAZS: OK, changed (earlier it was requested to have it in the title.)
      - s/We present/This section presents/
      - s/use cases were YANG/use cases where YANG/
      - Actually, this whole sentence is meaningless
BALAZS: removed  

I gave up reviewing Section C at C.1.1, since it’s non-normative.  Honestly, I'd remove the entire section but, if you want to keep it, I suggest reviewing it for issues…

Kent // shepherd

> On Mar 30, 2020, at 3:22 PM, Kent Watsen <[email-masked]> wrote:
> 
> 
> As part of the Shepherd writeup, I read the entire draft and found the following issues, which I’d like to see resolved before progressing the document.  Most of these issues should have been caught be the WG and/or Editors...
> 
> 
> Logical Issues:
> 
>  - S3, P8 defines MUSTs inside a SHOULD, a logical contradiction.
BALAZS: Changed to SHOULD
>  - In S3, P8 (the P beginning w/ "The name of”), text fails to indicate what SHOULD be done if both “revision” and “timestamp” are present?
BALAZS: IMHO the preference depends on the use case, so a general guidance cannot be given.
>  - the syntax grammar used in S3, P8 doesn’t make sense - use ABNF?
BALAZS: 
>  - In S3, P8: “the semicolons and the decimal point, if present, shall be replaced by underscores” - why are they not escaped?
BALAZS: This is a file name. Escaping in file names does not always work (depending on the filesystem and tools used). This is more simple and understandable
>  - Example 1 seems semantically invalid?  e.g., "ietf-netconf-monitoring” is in "content-data" though not in "modules-state”.  Also, "module-set-id” is missing 
BALAZS: OK, added. 
The original example is valid, just illogical. It follows the YANG modules defined in the content schema; it is allowed to have partial data sets, so some modules may be omitted from modules-state. However you are right this is illogical.
module-set-id does not need to be there. Partial data sets are allowed.
>  - It is unclear how the "inline-content-schema” feature could ever be used.  I.e., there are no protocol-accessible nodes in the module…
BALAZS: The system can declare in supported/not-supported in design documentation. E.g. in UC2, Preloading Default Configuration the designer preparing instance data, can decide to use or not use the inline-content-schema based on this.
>  - "leaf-list inline-module" is "min-elements 1” and "ordered-by user”, but "leaf-list module” has neither (though it may be that ordering is irrelevant for simple-inline).
BALAZS: ordered-by  removed. It doesn't really mean anything. In this case there is no chance of the system reordering a list a CLI/Netconf/Restconf client provided.
Min-elements is not needed for simplified-inline as the case will only be selected if there is at least one "module" leaf-list entry. It is needed for inline because otherwise the case could contain an " inline-schema" anydata section and no "inline-module" entries. That would not be usable.
>  - "leaf-list inline-module” has "ordered-by user”, what does that mean in the context of, e.g., UC1?
BALAZS: OK. Removed. In this case there is no chance of the system reordering a list a CLI/Netconf/Restconf client provided.
>  - P4 in the “description” statement on line 134 in the YANG module makes no sense (grammatically incorrect).  Try breaking into multiple sentences.
BALAZS: OK, Reworded.
>  - The last two sentences of the “description” statement on line 207 in the YANG module contradict each other.
BALAZS: Why ? I don't see the contradiction. If you know a single datastore specify it. If not omit the leaf. If the leaf is omitted, the situation is unknown.
> 
> 
> Structural issues:
> 
>  - Why isn’t the “Introduction” Section ‘1’ as is common in RFCs?
BALAZS: OK, rearranged sections
>  - the list in S3, P4 is missing symbols.
BALAZS: OK, added
>  - the two lists in S3, P8 should be indented, or remove the leading symbol
BALAZS: OK removed symbols
>  - The list under "Metadata MUST include:” is missing symbols.
BALAZS: OK, added
>  - The list under "Metadata SHOULD include:” is not indented.
BALAZS: OK, added
>  - 3.2, Example 1: move the comment under "content-data” under "modules-state”.  Replace said comment with a new comment that says something like “This content-data only contains the data identified by the ‘content-schema’ node, despite the ‘module-state’ node indicating that the server implements other modules
BALAZS: Removed comment, it is not needed
>  - The three examples should be <section> of their own (e.g., 3.2.x)
BALAZS: OK
>  - The “inline” choice node is generally confusing.  I can’t tell if it’s missing container called “inline” or if the two descendant nodes are poorly named.  In either case, it would be best to try to make it more readable.
BALAZS: Yes it is complicated. Some members of Netmod (I think Rob W.) Asked for a full, powerful, flexible way of documenting the content schema. In some cases it is needed.
> 
> 
> Editorial issues:
> 
>  - s/The term Server is/The term “server" is/
BALAZS: OK
>  - s/not available/unavailable/
BALAZS: OK
>  - s/Data is often needed already at/Data is often needed at/
BALAZS: OK
>  - s/needed by groups that do not have a live running server available/needed when a live running server is unavailable/.
BALAZS: OK
>  - s/ietf-yang- instance-data/"ietf-yang-instance-data”/
BALAZS: OK
>  - remove the word “we” throughout: s/we/this document/?
BALAZS: OK
>  - remove P3’s forward-reference to S3, P9?
BALAZS:  Sorry, I did not find this. Could you specify the text around it
>  - s/is actually implemented/is implemented/
BALAZS: OK
>  - s/e.g., UC5 documenting diagnostic data/(e.g., UC5 [Section 2])/
BALAZS: I prefer to use the short name of the use case instead of the reference. IMHO it provides information instantly without a look-up. Is that a problem?
>  - s/and state data set/and state data/
BALAZS: OK
>  - s/Config=true/configuration (“config true”)/
BALAZS: OK 
>  - s/Config=false/operational state data (“config false"/
BALAZS: OK 
>  - OLD: mandatory, min-elements, require-instance=true, must and when
>    NEW: “mandatory", "min-elements", "require-instance true", “ ust”, and “when”
BALAZS: OK
>  - OLD: If revision information inside the data set is present
>    NEW: If “revision" information is present inside the data set:
BALAZS: OK
>  - OLD: If the leaf "name" is present in the instance data header, this MUST be used.
>    NEW: If the leaf "name" is present in the instance data header, its value should be used for the "instance-data-set-name”.
BALAZS: OK 
>  - s/items MAY also be used/items MAY be used/
BALAZS: I would to keep "also" to indicated that they can be used as additional possibilities.
>  - s/Metadata MUST include:/\nMetadata MUST include:/
BALAZS: OK
>  - s/Content schema specification/Content schema specification (i.e., the "content-schema” node)/
BALAZS: OK
>  - s/Do not include the content-schema,/Do not include the "content-schema” node”;/
BALAZS: OK
>  - s/for "UC2 Preloading Data”/for UC2 [Section 2],/
BALAZS: I prefer to use the short name of the use case instead of the reference. IMHO it provides information instantly without a look-up. Is that a problem?
>  - S3.1.1 P2 doesn’t makes sense to me (esp. the verdt ref, which likely should be removed or better explained)
BALAZS: This was explicitly requested by 2 members of the verdt team. I tried to amend the text to make it more understandable, however IMHO we should not try to explain the usage of revision label here. Also this is just an example.
>  - s/The same-schema-as-file leaf/The "same-schema-as-file" leaf/
BALAZS: OK
>  - s/In Use Case 6/in UC6 [Section 2]/
BALAZS: OK
>  - S3.2 P1: s/examples use/example uses/ and s/for better formatting//.   
BALAZS: OK. Originally  I intended to use it in multiple places, but it was unneeded.
>  - s/is based on "UC1, Documenting Server Capabilities”/exemplifies UC1 [Section 2]/
BALAZS: Exemplifying is an uncommon word I find ugly. Is the current text hard to understand or misleading?  
I changed it to " The following example illustrates ..." I hope that's OK.
>  - s/(a shortened)/a/g
BALAZS: OK
>  - For Ex1: s/uses the inline method/uses the “inline" method/
BALAZS: OK
>  - s/- Use case 1, Documenting server capabilities/Exemplifying UC1/
BALAZS: Exemplifying is an uncommon word I find ugly. Is the current text hard to understand or misleading?  
IMHO the string stating  the name of the use case is more helpful then a reference, that needs to be looked up.
I changed it to " The following example illustrates ..." I hope that's OK.
>  - s/is based on "UC2, Preloading Default Configuration”/exemplifies UC2 [Section 2]/
BALAZS: Exemplifying is an uncommon word I find ugly. Is the current text hard to understand or misleading?  
I changed it to " The following example illustrates ..." I hope that's OK.
>  - For Ex2: s/uses the inline method/uses the "simplified-inline" method/
BALAZS: OK
>  - s/- Use case 2, Preloading access control data/Exemplifying UC2/
BALAZS: Exemplifying is an uncommon word I find ugly. Is the current text hard to understand or misleading?  
IMHO the string stating  the name of the use case is more helpful then a reference, that needs to be looked up
.>  - s/is based on UC5 Storing diagnostics data/exemplifies UC5 [Section 2]/
BALAZS: OK. but I changed it to: exemplifies UC5, Storing diagnostics data. IMHO the string stating  the name of the use case is more helpful then a reference, that needs to be looked up.
I changed it to " The following example illustrates "UC2, Preloading ..." I hope that's OK.
>  - s/statistics about NETCONF/statistics about the NETCONF server/
BAALZS: OK
>  - s/- UC5 Storing diagnostics data/Exemplifying UC5/
BALAZS: Exemplifying is an uncommon word I find ugly. Is the current text hard to understand or misleading?  
IMHO the string stating  the name of the use case is more helpful then a reference, that needs to be looked up.>
I changed it to " The following example illustrates ..." I hope that's OK.
> 
> Editorial issues inside the YANG module:
>  - "description" statement on line 74: rephrase to make more sense.
BALAZS: Other people thought it was OK. Any specific suggestion?
>  - :description" statement on line 92: so confusing.  Just write “The ‘revision' of the 'ietf-yang-instance-data’ module used to encode this 'instance-data-set’.”
BALAZS: OK
>  - “description" statement on line 100: s/content schema/schema (i.e., YANG modules)/?
BALAZS: The term "content-schema" is defined in the terminology section.  It defines  
>  - “type string” statement on lines 109 and 131 are missing a “pattern" statement.
BALAZS: OK, Defined it as a typedef.
>  - “description" statement on line 110: should this be mostly the same as the description statement of line 134, sans the bit regarding features, deviations, etc.?
BALAZS: Paragraphs 2,3 are the same. Paragraphs 1, 4,5,6 are really different. Inline is not just the same list with features, it involves one more level of indirection in defining the content schema.  
>  - line 152: s/ietf-yang-library@2019-01-04/revision "2019-01-04” of the "ietf-yang-library” module/?
BALAZS: OK
>  - P2 in the “description" statements on lines 220 and 249: s/For instance data sets/Instance data sets/
BALAZS: The sentence will not make sense unless I change the comma at the end of sentence to a colon.
>  - “description" statement on line 256: s/Modules/modules/ and s/content-schema-spec/‘content-schema’/
BALAZS: OK
> 
> 
> YANG module formatting issues:
> 
>  - On line 19: s/  reference/reference/
BALAZS: OK
>  - On line 21: s/}/  }/
BALAZS: OK
>  - On line 235: s/pattern/  pattern/ 
BALAZS: OK
> Kent // shepherd

---

From: Kent Watsen <[email-masked]>
Date: Thu, 9 Apr 2020 20:11:17 +0000

Hi Balazs,

> On Apr 8, 2020, at 8:06 AM, Balázs Lengyel <[email-masked]> wrote:
> 
> Hello Kent,

Because it’s a working group document now and so uses the “ietf” prefix.  Try this:

     <?rfc include='reference.I-D.ietf-netmod-yang-module-versioning'?>  

> I added that we do not follow the security template for YANG models.

Please add the reference to https://tools.ietf.org/html/rfc8407#section-3.7.1 <https://tools.ietf.org/html/rfc8407#section-3.7.1> per above.

> BALAZS: It is referenced from the YANG module which is normative.

You just added that reference, but not correctly:
  1) the “reference” doesn’t follow the standard format
  2) the paragraph at the top of 3.2 doesn’t also list RFC 8525

> BALAZS: Sorry, that would make the sentence incorrect.

Do you mean it to be “For instance, data” then?   If “instance data” is supposed to be read together, maybe use a hyphen or quotes?

>> - the syntax grammar used in S3, P8 doesn’t make sense - use ABNF?

Please fix the grammar.

> BALAZS: This is a file name. Escaping in file names does not always work (depending on the filesystem and tools used). This is more simple and understandable

No, this is a special case CLR and we never do this.  I see this idea has been in the document since -03, so it must’ve bee discussed, can you point me to the discussion? 

FWIW, my OS doesn’t even require escaping colons.  BTW, they’re “colons” (not semicolons).

> BALAZS: The system can declare in supported/not-supported in design documentation. E.g. in UC2, Preloading Default Configuration the designer preparing instance data, can decide to use or not use t…

When I make statements like this, please see it as an opportunity to improve the document.  In this case, please modify the inline-content-schema’s “description” statement to indicate that the feature is never supported by a server, and that it is intended to be enabled via out-of-band documentation.  BTW, was this discussed by the WG?

> Min-elements is not needed for simplified-inline as the case will only be selected if there is at least one "module" leaf-list entry. It is needed for inline because otherwise the case could contai…

That may be true, but it’s equally true for the other leaf-list.  It's inconsistent.  

BTW, is "choice content-schema-spec” meant to be “mandatory true”?  - because, currently, 'content-schema” doesn’t have to be specified according to the model…

> BALAZS: Why ? I don't see the contradiction. If you know a single datastore specify it. If not omit the leaf. If the leaf is omitted, the situation is unknown.

I think the word “undefined” is throwing me.  Maybe “unspecified” would be better?

>> - The list under "Metadata SHOULD include:” is not indented.

I don’t see it.  The way to do it is by adding a fake “list”, with missing symbols, to put the other list inside...

> BALAZS: Removed comment, it is not needed

Too bad, I found the comments helpful.  Oh well.

>> - The three examples should be <section> of their own (e.g., 3.2.x)

Better, but:
  - the new titles don’t match the UC titles
  - perhaps remove the “UCx,” prefix from the titles?  It looks weird in
    the ToC and they're not needed in the title since the first sentence
    relates the example to the UC already...
  - BTW, missing word “in”:  s/The example illustrates UC[125] Section 1
    /The example illustrates UC[125] in Section 1/

> BALAZS: Yes it is complicated. Some members of Netmod (I think Rob W.) Asked for a full, powerful, flexible way of documenting the content schema. In some cases it is needed.

I’m not saying that it’s purpose is confusing, I’m saying that its poorly named or missing a parent container.  Try looking at your examples with “fresh” eyes.  The node names "inline-module” and “inline-schema” are odd.  It seems like “inline-module” could be “anydata-schema” and "inline-schema” could be “module-data”?

> BALAZS:  Sorry, I did not find this. Could you specify the text around it

   Two formats are specified based on the XML and JSON YANG encodings.
   Later as other YANG encodings (e.g., CBOR) are defined, further
   instance data formats may be specified.

Which is normatively described below.  I’d either delete or move this text down so it’s all together.

FWIW, generally, your writing style involves a lot of prefacing, whereas it’s somewhat more readable to have minimal text possible, ideally most text being in the YANG module themselves.  As an aside, I also sometimes start a document with use-cases (to build support), but then delete the use-cases after adoption.  I find the prevalence of the use-cases here detracting from readability.

> BALAZS: I prefer to use the short name of the use case instead of the reference. IMHO it provides information instantly without a look-up. Is that a problem?

I think I mentioned this above already, but the titles are wrong.  

Myself, I’d remove all the “Figure” postambles; I never title my figures, just more to have to look at and maintain.  In the case, this is where the US titles are again incorrect...

> BALAZS: I would to keep "also" to indicated that they can be used as additional possibilities.

As you wish, but I try to remove filler words.

> BALAZS: I prefer to use the short name of the use case instead of the reference. IMHO it provides information instantly without a look-up. Is that a problem?

Same comment used elsewhere.  Firstly, the titles are incorrect.  Second, the presentation is rather informal, a more formalized version might be:
OLD: (e.g., for "UC2 Preloading Data" the 
NEW: (e.g., for the "Preloading default configuration data" use-case (UC2 in Section 1), the

BTW, I think the period from the end of the previous sentence is meant to follow the close-parentheses here...

> BALAZS: This was explicitly requested by 2 members of the verdt team. I tried to amend the text to make it more understandable, however IMHO we should not try to explain the usage of revision label…

OLD: 
   (e.g., revision labels which can be used as alternative to the revision
   date[I-D.verdt-netmod-yang-module-versioning]). 

NEW:
    (e.g., revision labels, described by [I-D.verdt-netmod-yang-module-versioning]
    as alternative to the revision date). 

BTW, immediately following, the text says "See Section 2.2.”   This doesn’t mean
Anything to me.  Do you want to say something like “An example of the “inline” method is provided in 2.2.1”?

> I changed it to " The following example illustrates ..." I hope that's OK.

I’m unsure if it’s possible for something to be “based on” or “illustrate” a use case.  Illustrate is better though, maybe “reflects” or “epitomizes"?

BTW, missing “in":  s/illustrates UC1 Section 1/illustrates UC1 in Section 1/

> I changed it to " The following example illustrates ..." I hope that's OK.

Same comment as above.

> I changed it to " The following example illustrates ..." I hope that's OK.

Same comment as above.

> IMHO the string stating  the name of the use case is more helpful then a reference, that needs to be looked up

Same comment as above.

> >  - s/is based on UC5 Storing diagnostics data/exemplifies UC5 [Section 2]/

Same comment as above.

> I changed it to " The following example illustrates ..." I hope that's OK.

Same comment as above.

> BALAZS: Other people thought it was OK. Any specific suggestion?

OLD:
      "A data structure to define a format for
       YANG instance data sets. Consists of meta-data about
       the instance data set and the real content-data.”;

NEW:
      "A data structure to define a format for
       YANG instance data.   The majority of the YANG nodes provide
       meta-data about the instance data; the instance data itself is
       is contained only in the 'content-data’ node.”;

> BALAZS: The term "content-schema" is defined in the terminology section.  It defines  

Fine, but please add "(i.e., YANG modules)” so people will have better clue 

> BALAZS: OK, Defined it as a typedef.

Good!  But I’m unsure about the pattern statement (esp. "pattern '.|..|[^xX].*|.[^mM].*|..[^lL].*’;”)…did you copy/paste it from somewhere?

> BALAZS: Paragraphs 2,3 are the same. Paragraphs 1, 4,5,6 are really different. Inline is not just the same list with features, it involves one more level of indirection in defining the content sche…

If you say so,

> BALAZS: The sentence will not make sense unless I change the comma at the end of sentence to a colon.

Hmmm, that didn’t come out very well.  This is the same issue as before, whereby “For instance data...” looks like it should be read “For instance, data…”.  Maybe you can find a better way to express this?

>> - On line 235: s/pattern/  pattern/ 

PS: this command produces output:  pyang -f yang --keep-comments --yang-line-length 69 [email-masked] > tmp; diff [email-masked] tmp

New: missing space: s/artwork folding[I-D.ietf-netmod-artwork-folding]/artwork folding [I-D.ietf-netmod-artwork-folding]/

Kent // shepherd

---

From: Balázs Lengyel <[email-masked]>
Date: Tue, 14 Apr 2020 12:17:37 +0000

See below as BALAZS2. Removed previously agreed issues. Uploaded as -12.

Balazs

P.S. Kent, if further edits are needed, shall I do them via new uploaded versions, or shall I just send the update for checking to you?

From: Kent Watsen <[email-masked]> 
Sent: 2020. április 9., csütörtök 22:11
To: Balázs Lengyel <[email-masked]>
Cc: [email-masked]
Subject: Re: [netmod] Shepherd review on draft-ietf-netmod-yang-instance-file-format-10

Hi Balazs,

On Apr 8, 2020, at 8:06 AM, Balázs Lengyel <[email-masked] <mailto:[email-masked]> > wrote:

Hello Kent,

BALAZS2: OK, thanks

Structural Issues:

Please add the reference to https://tools.ietf.org/html/rfc8407#section-3.7.1 per above.

BALAZS2: OK

BALAZS2: OK, corrected

Editorial Issues:

 - Appendix B:

BALAZS2: OK, added quotes

BALAZS: 

Please fix the grammar.

BALAZS2: OK, Updated grammar.

BALAZS2: Windows doesn’t allow colons in the filename. Although it’s not everyone’s favorite OS, it is pretty widespread. 

For Ubuntu Linux and a bash shell the colon is allowed, but tab extension does not work properly.

Sorry, I don’t remember any discussion on this. Timestamps were discussed, but I don’t find any arguments about this substitution.

Changed semicolon->colon

BALAZS2: It was discussed that this inline-content-schema seems complicated, so it should not be mandatory. After this I introduced the feature. AFAIK no discussion after this.

Actually it might be supported by a server: 

*	preloading configuration data: the server may or may not be able the inline-content-schema. The designers preparing the instance data sets to be loaded onto the server may use this declaration as a design guide
*	if a server also produces instance data files (e.g. UC5  Storing diagnostics data), and I am writing a post-processing tool to handle these files, I would use the support for this feature as an input requirement: does my tool need to support inline-content-schema

While the server will probably not declare support for the ietf-yang-instance-data module and this feature, the support of the statement about feature support would be available in the product documentation.

I changed the description to 

  feature inline-content-schema {

    description

      "This feature indicates that inline content-schema  

          option is supported. Support for this feature might 

         be documented only via out-of-band documentation.";

  }

Is that OK?

BALAZS2: OK. added min-elements 1;  

BALAZS2: No, it is optional. As described in section 2.1 there is an external method to define the content schema outside the instance data file.

      External Method: Do not include the "content-schema" node; the

      user needs to obtain the information through external documents.

BALAZS: OK changed to unspecified

Structural issues:

BALAZS: OK, added

BALAZS2: OK

BALAZS: OK

Better, but:

BALAZS2: OK

BALAZS2: OK

BALAZS2: 

There is a parent container “content-schema” around the choice.

inline-module: the name should contain the word module, because the content is a module for a specific purpose. What it really is: Modules-defining-inline-content-schema, but I didn’t find a good short version for this. Modules-defining-inline-content-schema.

anydata-schema doesn’t sound right because:

*	Each individual leaf-list entry is just one module not a complete schema as a unit. As there are two anydata nodes, it could also be confusing which do we mean.

inline-schema : the name should contain the word schema, because this is what defines the content-schema.

Module-data does not really tell you what this is or what it’s purpose is. It can also be confused with content-data.

IMHO the current names inside a content-schema container are not bad, but any better proposals?

Editorial issues:

BALAZS2: This is not a forward reference. “Later” in this case means in this case means Later in time not later in the document. This sentence was specifically requested by earlier reviewers. How would you word this sentence.

 

BALAZS2: OK, Figure” postambles removed.

BALAZS2: OK

BALAZS2: OK

OLD: 

NEW:

BALAZS2: OK

BALAZS: OK, changed.

BALAZS2: OK, changed all illustrates to reflects

BALAZS: OK

Same comment as above.

BALAZS2: OK, Figure tiles removed as you proposed.

Same comment as above.

BALAZS2: OK, changed to reflects

Same comment as above.

BALAZS2: OK, Figure tiles removed as you proposed.

Same comment as above.
BALAZS2: OK, changed all illustrates to reflects

Same comment as above.
BALAZS2: OK, Figure tiles removed as you proposed.

OLD:

NEW:

BALAZS2: OK

BALAZS: OK

BALAZS2: The initial part, and the section you mention is from RFC6991 I-D.ietf-netmod-yang-module-versioning

          A YANG identifier MUST NOT start with any possible

          combination of the lowercase or uppercase character

          sequence 'xml'.

If you say so,

BALAZS: OK

PS: this command produces output:  pyang -f yang --keep-comments --yang-line-length 69 [email-masked] <mailto:[email-masked]>  > tmp; diff [email-masked] <mailto:[email-masked]>  tmp

BALAZS2: OK

Kent // shepherd

---

From: Kent Watsen <[email-masked]>
Date: Fri, 17 Apr 2020 00:55:40 +0000

Hi Balazs,

> P.S. Kent, if further edits are needed, shall I do them via new uploaded versions, or shall I just send the update for checking to you?

I prefer uploaded versions so I (everyone) can easily see the diffs and verify the changes made.  Thanks for asking.

>      <?rfc include='reference.I-D.ietf-netmod-yang-module-versioning'?>  

Np

> - S5 contains an mix of important and unimportant information.   I think that the most important thing to state that the module defines an offline format that MAY contain security sensitive information, and thus safe handling is advised.  Maybe also say something about because the YANG module only defines a “structure”,  the Security Considerations doesn’t follow the template specified in https://tools.ietf.org/html/rfc8407#section-3.7.1 <https://tools.ietf.org/html/rfc8407#section-3.7.1>).  For instance: s/is designed as a wrapper specifying a format and a metadata header for YANG instance data defined by the content-schema/specifies an offline format/

Thx

>   2) the paragraph at the top of 3.2 doesn’t also list RFC 8525

Getting there…

A)
  OLD:
     The first item is either ietf-yang-library
  NEW:
    The first module listed MUST either 'ietf-yang-library’, from RFC 8525,

B) 
  OLD:
    This YANG module imports typedefs from [RFC6991], identities from
    [RFC8342] and the "structure" extension from
    [I-D.ietf-netmod-yang-data-ext].  It also references [RFC8525].

  NEW:
    This YANG module imports typedefs from [RFC6991], identities from
    [RFC8342], and the "structure" extension from
    [I-D.ietf-netmod-yang-data-ext].  It also references [RFC8525]
    in a “description” statement..

> BALAZS2: OK, added quotes

Thank you!  (I hope you see now how confusing that was to read before…)

> BALAZS2: OK, Updated grammar.

That grammar excludes the possibility of using CBOR, which is indicated as possible in S2, P3...

Also, isn’t this now the case?
   OLD: The name of the instance data file SHOULD be of the form:
   NEW: The name of the instance data file MUST be of the form, defined using ABNF [RFC5234]:

    - and be sure to add a Normative reference to RFC 5234

> BALAZS2: Windows doesn’t allow colons in the filename. Although it’s not everyone’s favorite OS, it is pretty widespread. 

Understood, but that doesn’t explain why escapes can’t work.  Please explain.

> For Ubuntu Linux and a bash shell the colon is allowed, but tab extension does not work properly.

On Bash:
  $ touch a:b
  $ ls a<TAB> ---> replaces “a” with "a\:b” <RETURN>
  a:b

Seems regular to me…what’s the problem?

> Sorry, I don’t remember any discussion on this. Timestamps were discussed, but I don’t find any arguments about this substitution.

Excellent.

> preloading configuration data: the server may or may not be able the inline-content-schema. The designers preparing the instance data sets to be loaded onto the server may use this declaration as a design guide
> if a server also produces instance data files (e.g. UC5  Storing diagnostics data), and I am writing a post-processing tool to handle these files, I would use the support for this feature as an input requirement: does my tool need to support inline-content-schema

Yes, but please s/inline content-schema option/‘inline’ case of the 'content-schema’ container/

> BALAZS2: OK. added min-elements 1;  

Thank you.

>       user needs to obtain the information through external documents.

Gotcha - thanks.

> BALAZS: OK changed to unspecified

okay.

> I don’t see it.  The way to do it is by adding a fake “list”, with missing symbols, to put the other list inside...

Thanks!  But now I’m miffed (not at you), as the various RFC-format versions are rendering this inconsistently.  I was going to suggest also indenting the first list that appears above in the same section, but now I’m unsure.  There appears to be a bug in `xml2rfc` and I think now we should leave it to the editor to sort out.

>     relates the example to the UC already...

Much better!

>     /The example illustrates UC[125] in Section 1/

Good.

> Each individual leaf-list entry is just one module not a complete schema as a unit. As there are two anydata nodes, it could also be confusing which do we mean.

No but please consider asking the list.  It is in our collective interest for the format to be readable/understandable...

> BALAZS2: This is not a forward reference. “Later” in this case means in this case means Later in time not later in the document. This sentence was specifically requested by earlier reviewers. How w…

Okay.

> BALAZS2: OK, Figure” postambles removed.

Better.

> BTW, I think the period from the end of the previous sentence is meant to follow the close-parentheses here...

Thanks.

> Anything to me.  Do you want to say something like “An example of the “inline” method is provided in 2.2.1”?

Nice!

> BTW, missing “in":  s/illustrates UC1 Section 1/illustrates UC1 in Section 1/

Combo fix is good.

But having the section begin with "The example reflects…” seems odd.   How about "The example provided in this section reflects..."?

> BALAZS2: OK, Figure tiles removed as you proposed.

Thanks.

> BALAZS2: OK, changed to reflects

Thanks.

> BALAZS2: OK, Figure tiles removed as you proposed.

Thanks.

> BALAZS2: OK, changed all illustrates to reflects

Thanks.

> BALAZS2: OK, Figure tiles removed as you proposed.

Thanks.

>        is contained only in the 'content-data’ node.”;

Thanks.

> Fine, but please add "(i.e., YANG modules)” so people will have better clue 

I didn’t see a response to this comment...

>           sequence 'xml’.

I see…okay…have you tested the new parts of the pattern statement against a boundary conditions?

> Hmmm, that didn’t come out very well.  This is the same issue as before, whereby “For instance data...” looks like it should be read “For instance, data…”.  Maybe you can find a better way to expre…

I didn’t see a response to this comment...

> PS: this command produces output:  pyang -f yang --keep-comments --yang-line-length 69 [email-masked] <mailto:[email-masked]> > tmp; diff [email-masked] <mailto:[email-masked]> tmp

I didn’t see a response to this comment…. (I’ll defer testing it myself again until everything is fixed)

> New: missing space: s/artwork folding[I-D.ietf-netmod-artwork-folding]/artwork folding [I-D.ietf-netmod-artwork-folding]/

Other:

1) There is no RFC Editor request to assign placeholder values (e.g., XXXX, YYYY, ZZZZ)

2) conflicting assignment with “XXXX" (currently draft vs. artwork-folding draft)
      s/per BCP XXX (RFC XXXX)/per BCP YYY (RFC YYYY)/

3) In the YANG module:

3a) the “reference” statement isn’t set correctly...

  OLD:
       reference
         "YANG Data Structure Extensions: draft-ietf-netmod-yang-data-ext-05";

  NEW:
       reference
         “RFC ZZZZ: YANG Data Structure Extensions”;

3b) please consider ordering the imports in descending RFC order:

  OLD:
    RFC ZZZZ. (i.e. draft-ietf-netmod-yang-data-ext-05)
    RFC 8342
    RFC 6991
    RFC 6991

  NEW:
    RFC 6991
    RFC 6991
    RFC 8342
    RFC ZZZZ. (i.e. draft-ietf-netmod-yang-data-ext-05)

4) Some of the “description” statements in the YANG module are not fully-justified / flow-controlled.  Also, I see two lines like "E.g., ietf-yang-…” that should be indented, I think...

Kent // shepherd