Hi Yingzhen, Thank you for addressing my comments. I think we are down to the last few.
Thanks for including the example. It really helps in implementation. However, I believe we have a bug. On the command line below, all the YANG models published by IETF reside in my ~/git/iana/yang-parameters directory and the example you included in the draft is in the file called example-routing-policy.xml. On running the command, I get the following error: % yanglint -s -i -t auto -p ~/git/iana/yang-parameters/ [email protected] example-routing-policy.xml err : Failed to resolve identityref "bgp". (/ietf-routing-policy:routing-policy/policy-definitions/policy-definition[name='export-tagged-BGP']/statements/statement[name='term-0']/conditions/source-protocol) That is because you do not have “bgp” defined anywhere in your module. The only definition for that exists in ietf-bgp module. But you cannot include that module, because it creates a circular dependency (ietf-bgp depends on ietf-routing-policy). Luckily for you, ‘source-protocol’ is not a mandatory field, so if you remove the following line in the example, the example is validated. <source-protocol>bgp</source-protocol> For the remaining point see inline. > On May 27, 2020, at 1:36 PM, Yingzhen Qu <[email protected]> wrote: > > Hi Mahesh, > > Thanks for your nice and patient review. I’ve uploaded version -11 to address > your comments. > > Major changes: > Removed groupings only used in the model and not meant to be used later by > augmentations > Added an xml example in section 11. > Fixed the cross-reference issue. > Changed the “mailto” in the model to “Email” > > Looking at the terms we have so far in section 2, I don’t think it’s > necessary to add a definition for “routing policy”. But this is open for > discussion. It might be obvious to folks who know and understand routing what "routing policy" means. But it is not obvious to anyone who knows YANG modeling what it means. I am not going to hold up the review because of this, but I would highly encourage the authors to come up with a definition and include it in the draft. Cheers. > > Thanks, > Yingzhen > > From: Mahesh Jethanandani <[email protected]> > Date: Sunday, May 24, 2020 at 11:39 AM > To: Yingzhen Qu <[email protected]> > Cc: "[email protected]" <[email protected]>, > "[email protected]" > <[email protected]>, "[email protected]" > <[email protected]> > Subject: Re: Yangdoctors early review of draft-ietf-rtgwg-policy-model-09 > > [Trimming the list to open issues] > > Hi Yingzhen, > > >> On May 23, 2020, at 1:21 PM, Yingzhen Qu <[email protected] >> <mailto:[email protected]>> wrote: >> >> Hi Mahesh, >> >> Thanks for the review, very much appreciated. >> >> I submitted -10 version to address your comments: >> https://datatracker.ietf.org/doc/draft-ietf-rtgwg-policy-model/ >> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fdraft-ietf-rtgwg-policy-model%2F&data=02%7C01%7Cyingzhen.qu%40futurewei.com%7Cc70b253b70a247ec74aa08d80011ce8c%7C0fee8ff2a3b240189c753a1d5591fedc%7C1%7C0%7C637259423777478171&sdata=ZN3wyJ8RziT2DBS7WCNgiffg8yDO25jMSrPcMF4TDlM%3D&reserved=0> >> >> >> Note: There are YANG validation errors because yanglint can't find model >> "ietf-if-extensions", not sure why. >> yanglint SO 1.6.7: yanglint --verbose -p {rfclib} -p {draftlib} -p {tmplib} >> {model} -i: >> err : Data model "ietf-if-extensions" not found. >> err : Importing "ietf-if-extensions" module into "ietf-routing-policy" >> failed. >> err : Module "ietf-routing-policy" parsing failed. > > ietf-if-extensions is part of the draft-ietf-netmod-inft-ext-yang draft, > which is still a draft. It should ideally be in the {draftlib} path variable. > I would suggest a open an issue with the tools team to have them track it. > > I am assuming of course that you are able to validate the model locally on > your machine. > … > > >> The document includes a section on Terminology and Notation. However it >> does >> not define what is a "routing policy" or provide a reference to it. >> [YQ]: I'm not sure there is a standard definition of "routing policy". I'll >> do some >> research on this and see whether we should add it to the terminology. > > Looking at how the draft describes it, how about something like this? > > “A routing policy defines whether a routing protocol imports a route into the > routing table, modifies it, or exports a route from the routing table." > > … > > >> Section 10.1 - Routing policy model >> >> The model uses a lot of groupings, most of them used only once in the >> model. >> That makes the model difficult to understand. Unless these groupings are >> meant >> for use by other models, they should be folded into the main container. >> Take >> this grouping for example: >> >> grouping policy-definitions { >> description >> "This grouping provides policy definitions"; >> >> leaf name { >> type string; >> description >> "Name of the top-level policy definition -- this name >> is used in references to the current policy"; >> } >> } >> >> For one the description of the grouping is not correct. Secondly, it is >> used >> only once in the model and that too to define a key of a list. >> >> [YQ]: This model was initiated by OpenConfig, we've removed some groupings >> Since this become an IETF draft. I removed "policy-definitions" and >> "policy-statements" >> groups. > > The general rule is that if the grouping is used only once in the model, and > not meant for use outside the module, that it should not be a grouping. > > For example, ‘grouping neighbor-set’ is used only once in the model, and I > doubt it is meant for use outside of this module. That grouping should be > collapsed into the model where it gets used. Same for ‘grouping tag-set’. > These are just two examples. There are many more. > > >> >> The YANG model imports multiple models from other drafts. It provides >> normative >> references to the imported models in Section 2.2. (A nit here is that it >> missing the cross-reference in the table for if-l3-vlan), > > A nit. if-l3-vlan is still missing the cross-reference (and the hyperlink) to > the draft. Also why idnits is giving you an error at the bottom. > > >> but does not provide >> references to them in the model. See Section 3.9 of RFC 8407. Please add >> something like this in the model. >> >> import ietf-inet-types { >> prefix "inet"; >> reference >> "RFC 6991: Common YANG Data Types."; >> } >> [YQ]: fixed. >> >> Please drop <mailto:....> and just keep the e-mail address. That tag >> works only >> when embedded within a HTML document. >> [YQ]: do you mean the "editor" in the model? > > I meant to say you should drop the word “mailto:"; from the list of e-mail > addresses of the editors. That tag works inside a HTML document, not inside a > YANG model. > > >> >> A pyang compilation of the model with —ietf and —lint option was clean. >> However, there seems to be an issue with the example in Section 11. >> Instead of >> actually including an example, it is showing the path to what should have >> been >> the example. I was therefore not able to validate the example against the >> model. >> [YQ]: I can't find this example in OpenConfig Github anymore, so removed >> this section. > > Hmm. That leaves the model without any examples, and examples are important. > > I was able to find the example in an earlier version of your own draft (01). > I would suggest that you take that example and modify it to make it > compatible with the latest version of the model. If you need help, let me > know. > > >> >> Checking references for intended status: Proposed Standard >> >> ---------------------------------------------------------------------------- >> >> (See RFCs 3967 and 4897 for information about using normative >> references >> to lower-maturity documents in RFCs) >> >> == Unused Reference: 'I-D.ietf-netmod-sub-intf-vlan-model' is defined on >> line 1620, but no explicit reference was found in the text > > The reason you are getting this error is because you are lacking a normative > reference to I-D.ietf-netmod-sub-intf-vlan-model in the document. Goes back > to the nit I point up earlier. > > I would suggest that once you have fixed all the issues in the draft, that > you re-run idnits locally on the machine. > > >> >> -- No information found for draft-ietf-netmod-intf-ext-yang - is the >> name >> correct? >> >> -- Possible downref: Normative reference to a draft: ref. >> 'I-D.ietf-netmod-intf-ext-yang' >> >> -- No information found for draft-ietf-netmod-sub-intf-vlan-model - is >> the >> name correct? >> >> -- Possible downref: Normative reference to a draft: ref. >> 'I-D.ietf-netmod-sub-intf-vlan-model' >> >> [YQ]: there is dependency on both 'I-D.ietf-netmod-intf-ext-yang' and >> 'I-D.ietf-netmod-sub-intf-vlan-model'. >> >> Summary: 0 errors (**), 0 flaws (~~), 9 warnings (==), 5 comments >> (--).
_______________________________________________ rtgwg mailing list [email protected] https://www.ietf.org/mailman/listinfo/rtgwg
