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. > 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'?> > BALAZS2: OK, thanks 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/ > 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. > > 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. > BALAZS2: OK Thx > - 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. > > 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 > BALAZS2: OK, corrected 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.. > - Appendix B: > - s/For instance data/Instance data/ > 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? > BALAZS2: OK, added quotes Thank you! (I hope you see now how confusing that was to read before…) > - the syntax grammar used in S3, P8 doesn’t make sense - use ABNF? > BALAZS: > > Please fix the grammar. > > 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 > - 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 > > 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). > 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. > Changed semicolon->colon Excellent. > - 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. > > 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? > 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? Yes, but please s/inline content-schema option/‘inline’ case of the 'content-schema’ container/ > - "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. > > That may be true, but it’s equally true for the other leaf-list. It's > inconsistent. > BALAZS2: OK. added min-elements 1; Thank you. > 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… > 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. Gotcha - thanks. > - 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. > > I think the word “undefined” is throwing me. Maybe “unspecified” would be > better? > BALAZS: OK changed to unspecified okay. > - The list under "Metadata SHOULD include:” is not indented. > BALAZS: OK, added > > 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... > BALAZS2: OK 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. > - The three examples should be <section> of their own (e.g., 3.2.x) > BALAZS: OK > > 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... > BALAZS2: OK Much better! > - BTW, missing word “in”: s/The example illustrates UC[125] Section 1 > /The example illustrates UC[125] in Section 1/ > BALAZS2: OK Good. > - 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. > > 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”? > 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? No but please consider asking the list. It is in our collective interest for the format to be readable/understandable... > - remove P3’s forward-reference to S3, P9? > 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. > 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. Okay. > > 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. > > > > > - 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? > > 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... > BALAZS2: OK, Figure” postambles removed. Better. > > - 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? > > 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 > BALAZS2: OK > BTW, I think the period from the end of the previous sentence is meant to > follow the close-parentheses here... > BALAZS2: OK Thanks. > - 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. > > 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). > BALAZS2: OK > > 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”? > BALAZS: OK, changed. Nice! > - 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. > > 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"? > BALAZS2: OK, changed all illustrates to reflects > > BTW, missing “in": s/illustrates UC1 Section 1/illustrates UC1 in Section 1/ > BALAZS: OK Combo fix is good. But having the section begin with "The example reflects…” seems odd. How about "The example provided in this section reflects..."? > - 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. > > Same comment as above. > BALAZS2: OK, Figure tiles removed as you proposed. Thanks. > - 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. > > Same comment as above. > BALAZS2: OK, changed to reflects Thanks. > - 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 > > Same comment as above. > BALAZS2: OK, Figure tiles removed as you proposed. Thanks. > > - 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. > > Same comment as above. > BALAZS2: OK, changed all illustrates to reflects Thanks. > - 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. > > Same comment as above. > BALAZS2: OK, Figure tiles removed as you proposed. Thanks. > 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? > > 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.”; > BALAZS2: OK Thanks. > - :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 > > Fine, but please add "(i.e., YANG modules)” so people will have better clue I didn’t see a response to this comment... > - “type string” statement on lines 109 and 131 are missing a “pattern" > statement. > 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? > 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’. I see…okay…have you tested the new parts of the pattern statement against a boundary conditions? > - 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. > > 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? I didn’t see a response to this comment... > PS: this command produces output: pyang -f yang --keep-comments > --yang-line-length 69 [email protected] > <mailto:[email protected]> > tmp; diff > [email protected] > <mailto:[email protected]> 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]/ > BALAZS2: OK 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
_______________________________________________ netmod mailing list [email protected] https://www.ietf.org/mailman/listinfo/netmod
