[CC-ing NETMOD]
Hi Balazs, Please see below for responses. (One question pending) I just got back from PTO. Thanks for posting -13 anyways. Kent // shepherd > On Mar 23, 2021, at 6:01 PM, Balázs Lengyel <[email protected]> > wrote: > > Hello Kent, > I am resuming my work on draft-ietf-netmod-yang-instance-file-format. > I removed already agreed items from this mail and provided answers as > BALAZS3 below. > Are you OK with my answers, corrections? What is the next step forward? > > I updated the draft to -13, but did not upload it yet. > Regards Balazs > > > From: Kent Watsen <[email protected] <mailto:[email protected]>> > Sent: 2020. április 9., csütörtök 22:11 > To: Balázs Lengyel <[email protected] > <mailto:[email protected]>> > Cc: [email protected] <mailto:[email protected]> > 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 protected] > <mailto:[email protected]>> wrote: > > 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. > > 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'?> > > BALAZS3: OK > > 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 > <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. > > BALAZS3: OK Thanks but, FWIW, this change must’ve been made before -13 since it doesn’t show up in the 12-to-13 diff. > - 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 > > BALAZS3: ??? Thanks, I see the reference to RFC 8525 in Section 3.2 now. [Also must’ve been added before -13] > > Editorial Issues: > > - 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? > > BALAZS3: OK, changed Thanks, I see s/For/In case of/, which resolves the ambiguity. > On Mar 30, 2020, at 3:22 PM, Kent Watsen <[email protected] > <mailto:[email protected]>> 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... > > > - 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 been 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. > > BALAZS3: To my best knowledge, colon is never accepted in any form in windows > filenames and cannot be escaped. > https://superuser.com/questions/461320/get-a-colon-in-a-windows-filename > <https://superuser.com/questions/461320/get-a-colon-in-a-windows-filename> > I removed the decimal point as that works on windows and Linux. > > 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? > BALAZS3: On another version of Ubuntu tab extension did not work for me. Thank you for providing the link explaining why colons cannot be escaped on Windows. Personal opinion: the underscore usage seems user-unfriendly. I almost seems better if the colons were simply removed. The "silver lining” is that, presumably, the form using “timestamps” will be rare, and even more-so when “semver” names become the norm. BTW, I assume that you wish to publish this as-is and follow-up later with an update to adjust for the “semver” work - correct? > - 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? > BALAZS3: OK. Added out-of-band documentation. It was discussed and this was > the proposed solution. Some people insisted that this should be optional. > This is the best way to indicate that. Thanks, I see this: [PS: also must’ve been in an update before -13] description "This feature indicates that inline content-schema option is supported. Support for this feature might be documented only via out-of-band documentation."; > - "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. > BALAZS3: OK. This is not strictly needed, but it improves readability. Added > min-elements 1 to the other leaf-list. Thanks. > 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… > BALAZS3: It is optional. Section 2.1. Specifying the Content Schema > specifies this. E.g. if you are sending diagnostics data every 10 seconds > with the same filename, it is not needed to include the content schema every > time. Okay. > - 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? > BALAZS3: OK, use unspecified Thanks. > Structural issues: > > - 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... > > BALAZS3: It is now. Please check Thanks! > - 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... > - BTW, missing word “in”: s/The example illustrates UC[125] Section 1 > /The example illustrates UC[125] in Section 1/ > > BALAZS3: OK Thanks. > - 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”? > BALAZS3: OK. Added container inline-content-schema Thanks - better. > Editorial issues: > > - 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 UC > titles are again incorrect... > BALAZS3: OK, Figure postambles Removed. 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). > BALAZS3: OK Thanks. > 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”? > BALAZS3: OK. Reference removed, replaced by a reference to an example. Okay. > - 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"? > BALAZS3: OK, changed to reflects Thanks. > BTW, missing “in": s/illustrates UC1 Section 1/illustrates UC1 in Section 1/ > BALAZS3: OK Thanks. > - 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. > BALAZS3: OK, changed to reflects 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. > BALAZS3: OK 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. > BALAZS3: OK 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. > BALAZS3: OK 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. > BALAZS3: OK Thanks. > - “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 > BALAZS3: OK Thanks. > - “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? > BALAZS3: Copied from rfc6991. It is meant to ensure that an identifier does > not start with the string xml. > typedef yang-identifier { > type string { > length "1..max"; > pattern '[a-zA-Z_][a-zA-Z0-9\-_.]*'; > pattern '.|..|[^xX].*|.[^mM].*|..[^lL].*'; > } > Okay. > - 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? > > BALAZS3: OK, changed to > In case of 'instance data sets ... Thanks! K.
_______________________________________________ netmod mailing list [email protected] https://www.ietf.org/mailman/listinfo/netmod
