> On Sep 18, 2025, at 13:39, Acee Lindem <[email protected]> wrote: > > Hi Yingzhen, Chris > >> On Sep 18, 2025, at 12:34 PM, Yingzhen Qu <[email protected]> wrote: >> >> Hi Chris, >> >> Currently there is a range statement when configuring the algo-number, but >> for the tlvs in the LSDB a uint8 is used without range so it can accommodate >> whatever value returned. This way, we can make sure to configure a correct >> algo-number, and in case something goes wrong in the database we can see it. > > > For the read-write node, I think we should keep the range. If it were to ever > change, we’d need to expand it rather than contract it and that should be > backward compatible.
I think any change widen or shrink is not backward compatible (why I don't like them :) -- however, I'm not pushing for this change, just wanted to highlight why I don't like them in general. :) I definitely agree that for the state based on values from the wire we should continue to not have a range statement (and maybe not even mention the expected range in the description -- or mention that it's the "currently expected" but not required range). Thanks, Chris. > > Thanks, > Acee > > >> >> Are you suggesting we should remove the range for configuration? A wrong >> configuration may still be rejected by the router. >> >> I'll update all names to "algo-number" for consistency. >> >> Thanks, >> Yingzhen >> >> >> On Thu, Sep 18, 2025 at 4:55 AM Christian Hopps <[email protected]> wrote: >> Michal Vasko <[email protected]> writes: >> >> > Hi Yingzhen, >> > >> > I do not understand why you want to keep the 2 nodes in >> > `ietf-isis-link-attr` the way they are. Whether configuration or >> > state data, by using statements such as `range` you can formally and >> > exactly specify the allowed value space, why would you not want to >> > use it? If you put this information in the `description`, it is >> > understandable only by a human. Also, why not be consistent and use >> > the `range` for one node but not the other. Finally, duplicating >> > information is always redundant. >> > >> > In any case, I am only providing YANG-specific advice which you are >> > essentially free not to follow but at least another opinion of a YANG >> > doctor may be helpful. >> >> [as wg-member] >> >> As an aside, I'm not a big fan of range statements like these here since >> they are often ripe for introducing backward incompatible changes when the >> range is updated someday by another RFC. I like to, whenever possible, let >> the current standard say the value (e.g., 128-255 here), and let the module >> hold more values (the full uint8 which is given by the on-the-wire >> encoding), then if the standard is ever updated the module continues to work >> unchanged. Restricting values in the YANG modules should bring more value to >> the table IMO. >> >> In any case, if these are updated please use the same leaf-name for all of >> them since they contain the same semantic value. So, for example, name them >> all `algo-number` since it's used that way under the `flex-algo` container, >> but not `algo-number` `flex-algo-number`, and `flex-algo`. >> >> Thanks, >> Chris. >> >> >> > >> > Regards, >> > Michal >> > >> > On 18. 9. 2025 0:24, Yingzhen Qu wrote: >> > >> > Hi Michal, >> > >> > Thanks for the review. I've updated the JSON example in the >> > appendix. However, I don't think those >> > nits in ietf-isis-flex-alog need to be fixed. Details below >> > inline. >> > >> > Thanks, >> > Yingzhen >> > >> > On Wed, Sep 17, 2025 at 1:06 AM Michal Vasko <[email protected]> >> > wrote: >> > >> > >> > Hi Yingzhen, >> > >> > it seems the minor issues in `ietf-isis-link-attr` were fixed >> > but the ones in `ietf-isis-flex-algo` were not: >> > >> > ietf-isis-flex-algo >> > - leaf flex-algo - range in description instead of using the >> > "range" statement >> > >> > [Yingzhen]: This is read only data. The description is only for >> > information. >> > >> > >> > - leaf algo-number - redundant range mentioned in the >> > description >> > >> > [Yingzhen]: I don't consider this as a nit. >> > >> > >> > Also, the XML data example now validates but the JSON one >> > seems to be using syntax rules unknown to me and definitely >> > not the ones in RFC 7951. I have attached a valid YANG data >> > JSON example. >> > >> > [Yingzhen]: I've updated the JSON example. >> > >> > >> > Regards, >> > Michal >> > >> > On 12. 9. 2025 8:13, Yingzhen Qu wrote: >> > >> > Hi Michal, >> > >> > Thanks for the review. I have uploaded a new version to >> > address your comments. >> > >> > I fixed all the editorial issues in your comments and >> > updated the examples. Please let me know if you have any >> > other questions or comments. >> > >> > Thanks, >> > Yingzhen >> > >> > On Fri, Aug 29, 2025 at 12:17 AM Michal Vaško via >> > Datatracker <[email protected]> wrote: >> > >> > Document: draft-ietf-lsr-isis-flex-algo-yang >> > Title: YANG Model for IS-IS Application-Specific Link >> > Attributes and Flexible >> > Algorithm Reviewer: Michal Vaško Review result: Ready >> > with Issues >> > >> > Looking at the 2 YANG modules, they are in a good >> > shape and I found only a few >> > nits (without having knowledge of the ISIS routing >> > protocol). However, the XML >> > and JSON example YANG data are invalid, which needs >> > to be addressed. >> > >> > ietf-isis-link-attr >> > - leaf udabm-length - 2nd line of description has >> > extra space >> > - leaf transition - 2nd line of description has extra >> > space >> > - leaf unidirectional-link-delay - units mentioned in >> > description instead of >> > "units" statement - uses >> > application-specific-link-attributes-sub-tlv - second >> > usage has extra indent >> > >> > ietf-isis-flex-algo >> > - leaf flex-algo - range in description instead of >> > using the "range" statement >> > - leaf algo-number - redundant range mentioned in the >> > description >> > >> > Appendix A invalid (yanglint used): >> > - none of the XML prefixes of the used identities are >> > defined >> > - there are leafrefs to the ietf-te module data, >> > which should ideally be >> > included so that the examples can successfully be >> > validated or at least the >> > used ietf-te data referenced, if found in another RFC >> > - "At least one >> > area-address must be configured." >> > (/ietf-routing:routing/control-plane-protocols/ >> > control-plane-protocol[type='ietf-isis:isis'][name= >> > 'default']/ietf-isis:isis) >> > >> > Appendix B invalid: >> > - some of the prefixes used are not valid module >> > names (iana-metric-type and >> > iana-algo-types) - similar leafref problems - similar >> > explicit validation error >> > message >> > >> > >> > > _______________________________________________ Lsr mailing list -- [email protected] To unsubscribe send an email to [email protected]
