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]
