From: Kent Watsen <kent+ietf@watsen.net>
Content-Type: text/plain;
	charset=utf-8
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Date: Tue, 31 Mar 2020 01:50:17 +0000
References: <010001712ce4c5fe-04a059c3-ced3-4e6d-8389-5dd7c1257ac2-000000@email.amazonses.com>
To: "netmod@ietf.org" <netmod@ietf.org>
In-Reply-To: <010001712ce4c5fe-04a059c3-ced3-4e6d-8389-5dd7c1257ac2-000000@email.amazonses.com>
Message-ID: <010001712e483a1b-204d92e3-7046-46fb-b6b8-13d8ad4cb9ff-000000@email.amazonses.com>
Feedback-ID: 1.us-east-1.DKmIRZFhhsBhtmFMNikgwZUWVrODEw9qVcPhqJEI2DA=:AmazonSES
Archived-At: <https://mailarchive.ietf.org/arch/msg/netmod/5DfEi8swOmLEPykBpFICJK6398s>
Subject: Re: [netmod] Shepherd review on
 draft-ietf-netmod-yang-instance-file-format-10
Precedence: list

[UPDATE: I just realized that I prematurely stopped the review after the =
YANG module.  Picking up where I left off=E2=80=A6]


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 =E2=80=9Cstructure=E2=80=9D,  the Security =
Considerations doesn=E2=80=99t 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/

  - S6.1. the full registration is not inside the <figure>.  (How could =
happen?)

  - S8.1: I-D.ietf-netmod-yang-data-ext should be listed last, as the =
number it will be the greater than any other...

  - S8.1: RFC8340 is Informative

  - S8.1: agreed that RFC8525 is Normative, but the only place it it =
referenced is in a non-normative section=E2=80=A6please add a ref to it =
from a normative section.

  - S8.2: same comment as before re: floating the I-D refs to the end=E2=80=
=A6

  - Appendix C: remove the unnecessary =E2=80=9CC.1=E2=80=9D section.


Editorial Issues:

  - Appendix B:
     - s/For instance data/Instance data/
     - =E2=80=9C...to avoid are listed.=E2=80=9D - listed where?  =
Section reference?
     - In P2, how is the 2nd sentence connected to the 1st?
     - s/may lead to problems/may lead to the following problems:/?

  - Appendix C:
      - Don=E2=80=99t put =E2=80=9CNon-Normative=E2=80=9D into the =
Section title (move to 1st sentence of section)=20
      - s/We present/This section presents/
      - s/use cases were YANG/use cases where YANG/
      - Actually, this whole sentence is meaningless

I gave up reviewing Section C at C.1.1, since it=E2=80=99s =
non-normative.  Honestly, I'd remove the entire section but, if you want =
to keep it, I suggest reviewing it for issues=E2=80=A6


Kent // shepherd



> On Mar 30, 2020, at 3:22 PM, Kent Watsen <kent+ietf@watsen.net> wrote:
>=20
>=20
> As part of the Shepherd writeup, I read the entire draft and found the =
following issues, which I=E2=80=99d like to see resolved before =
progressing the document.  Most of these issues should have been caught =
be the WG and/or Editors...
>=20
>=20
> Logical Issues:
>=20
>  - S3, P8 defines MUSTs inside a SHOULD, a logical contradiction.
>  - In S3, P8 (the P beginning w/ "The name of=E2=80=9D), text fails to =
indicate what SHOULD be done if both =E2=80=9Crevision=E2=80=9D and =
=E2=80=9Ctimestamp=E2=80=9D are present?
>  - the syntax grammar used in S3, P8 doesn=E2=80=99t make sense - use =
ABNF?
>  - In S3, P8: =E2=80=9Cthe semicolons and the decimal point, if =
present, shall be replaced by underscores=E2=80=9D - why are they not =
escaped?
>  - Example 1 seems semantically invalid?  e.g., =
"ietf-netconf-monitoring=E2=80=9D is in "content-data" though not in =
"modules-state=E2=80=9D.  Also, "module-set-id=E2=80=9D is missing=20
>  - It is unclear how the "inline-content-schema=E2=80=9D feature could =
ever be used.  I.e., there are no protocol-accessible nodes in the =
module=E2=80=A6
>  - "leaf-list inline-module" is "min-elements 1=E2=80=9D and =
"ordered-by user=E2=80=9D, but "leaf-list module=E2=80=9D has neither =
(though it may be that ordering is irrelevant for simple-inline).
>  - "leaf-list inline-module=E2=80=9D has "ordered-by user=E2=80=9D, =
what does that mean in the context of, e.g., UC1?
>  - P4 in the =E2=80=9Cdescription=E2=80=9D statement on line 134 in =
the YANG module makes no sense (grammatically incorrect).  Try breaking =
into multiple sentences.
>  - The last two sentences of the =E2=80=9Cdescription=E2=80=9D =
statement on line 207 in the YANG module contradict each other.
>=20
>=20
> Structural issues:
>=20
>  - Why isn=E2=80=99t the =E2=80=9CIntroduction=E2=80=9D Section =
=E2=80=981=E2=80=99 as is common in RFCs?
>  - the list in S3, P4 is missing symbols.
>  - the two lists in S3, P8 should be indented, or remove the leading =
symbol
>  - The list under "Metadata MUST include:=E2=80=9D is missing symbols.
>  - The list under "Metadata SHOULD include:=E2=80=9D is not indented.
>  - 3.2, Example 1: move the comment under "content-data=E2=80=9D under =
"modules-state=E2=80=9D.  Replace said comment with a new comment that =
says something like =E2=80=9CThis content-data only contains the data =
identified by the =E2=80=98content-schema=E2=80=99 node, despite the =
=E2=80=98module-state=E2=80=99 node indicating that the server =
implements other modules
>  - The three examples should be <section> of their own (e.g., 3.2.x)
>  - The =E2=80=9Cinline=E2=80=9D choice node is generally confusing.  I =
can=E2=80=99t tell if it=E2=80=99s missing container called =E2=80=9Cinlin=
e=E2=80=9D or if the two descendant nodes are poorly named.  In either =
case, it would be best to try to make it more readable.
>=20
>=20
> Editorial issues:
>=20
>  - s/The term Server is/The term =E2=80=9Cserver" is/
>  - s/not available/unavailable/
>  - s/Data is often needed already at/Data is often needed at/
>  - s/needed by groups that do not have a live running server =
available/needed when a live running server is unavailable/.
>  - s/ietf-yang- instance-data/"ietf-yang-instance-data=E2=80=9D/
>  - remove the word =E2=80=9Cwe=E2=80=9D throughout: s/we/this =
document/?
>  - remove P3=E2=80=99s forward-reference to S3, P9?
>  - s/is actually implemented/is implemented/
>  - s/e.g., UC5 documenting diagnostic data/(e.g., UC5 [Section 2])/
>  - s/and state data set/and state data/
>  - s/Config=3Dtrue/configuration (=E2=80=9Cconfig true=E2=80=9D)/
>  - s/Config=3Dfalse/operational state data (=E2=80=9Cconfig false"/
>  - OLD: mandatory, min-elements, require-instance=3Dtrue, must and =
when
>    NEW: =E2=80=9Cmandatory", "min-elements", "require-instance true", =
=E2=80=9C ust=E2=80=9D, and =E2=80=9Cwhen=E2=80=9D
>  - OLD: If revision information inside the data set is present
>    NEW: If =E2=80=9Crevision" information is present inside the data =
set:
>  - 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=E2=80=9D.
>  - s/items MAY also be used/items MAY be used/
>  - s/Metadata MUST include:/\nMetadata MUST include:/
>  - s/Content schema specification/Content schema specification (i.e., =
the "content-schema=E2=80=9D node)/
>  - s/Do not include the content-schema,/Do not include the =
"content-schema=E2=80=9D node=E2=80=9D;/
>  - s/for "UC2 Preloading Data=E2=80=9D/for UC2 [Section 2],/
>  - S3.1.1 P2 doesn=E2=80=99t makes sense to me (esp. the verdt ref, =
which likely should be removed or better explained)
>  - s/The same-schema-as-file leaf/The "same-schema-as-file" leaf/
>  - s/In Use Case 6/in UC6 [Section 2]/
>  - S3.2 P1: s/examples use/example uses/ and s/for better =
formatting//.  =20
>  - s/is based on "UC1, Documenting Server Capabilities=E2=80=9D/exemplif=
ies UC1 [Section 2]/
>  - s/(a shortened)/a/g
>  - For Ex1: s/uses the inline method/uses the =E2=80=9Cinline" method/
>  - s/- Use case 1, Documenting server capabilities/Exemplifying UC1/
>  - s/is based on "UC2, Preloading Default Configuration=E2=80=9D/exempli=
fies UC2 [Section 2]/
>  - For Ex2: s/uses the inline method/uses the "simplified-inline" =
method/
>  - s/- Use case 2, Preloading access control data/Exemplifying UC2/
>  - s/is based on UC5 Storing diagnostics data/exemplifies UC5 [Section =
2]/
>  - s/statistics about NETCONF/statistics about the NETCONF server/
>  - s/- UC5 Storing diagnostics data/Exemplifying UC5/
>=20
>=20
> Editorial issues inside the YANG module:
>  - "description" statement on line 74: rephrase to make more sense.
>  - :description" statement on line 92: so confusing.  Just write =
=E2=80=9CThe =E2=80=98revision' of the 'ietf-yang-instance-data=E2=80=99 =
module used to encode this 'instance-data-set=E2=80=99.=E2=80=9D
>  - =E2=80=9Cdescription" statement on line 100: s/content =
schema/schema (i.e., YANG modules)/?
>  - =E2=80=9Ctype string=E2=80=9D statement on lines 109 and 131 are =
missing a =E2=80=9Cpattern" statement.
>  - =E2=80=9Cdescription" statement on line 110: should this be mostly =
the same as the description statement of line 134, sans the bit =
regarding features, deviations, etc.?
>  - line 152: s/ietf-yang-library@2019-01-04/revision "2019-01-04=E2=80=9D=
 of the "ietf-yang-library=E2=80=9D module/?
>  - P2 in the =E2=80=9Cdescription" statements on lines 220 and 249: =
s/For instance data sets/Instance data sets/
>  - =E2=80=9Cdescription" statement on line 256: s/Modules/modules/ and =
s/content-schema-spec/=E2=80=98content-schema=E2=80=99/
>=20
>=20
> YANG module formatting issues:
>=20
>  - On line 19: s/  reference/reference/
>  - On line 21: s/}/  }/
>  - On line 235: s/pattern/  pattern/
>=20
>=20
>=20
> Kent // shepherd
>=20
>=20
>=20
> _______________________________________________
> netmod mailing list
> netmod@ietf.org
> https://www.ietf.org/mailman/listinfo/netmod