Re: [Teas] Yangdoctors early review of draft-ietf-teas-yang-te-34

[Andy Bierman <[e-mail]>]

On Fri, Jan 12, 2024 at 6:56 AM Tarek Saad <[e-mail]> wrote:

> Hi Andy,
>
>
>
> Thank you for the review and comments. We have uploaded rev 35
> <https://datatracker.ietf.org/doc/html/draft-ietf-teas-yang-te-35> that
> addresses comments you have raised. Please see inline for more details [TS].
>
>
>
>
>

I updated the review to Ready.
https://datatracker.ietf.org/doc/review-ietf-teas-yang-te-34-yangdoctors-early-bierman-2023-11-28/


Andy


> *From: *Andy Bierman via Datatracker <[e-mail]>
> *Date: *Tuesday, November 28, 2023 at 3:15 PM
> *To: *[e-mail] <[e-mail]>
> *Cc: *[e-mail] <
> [e-mail]>, [e-mail] <[e-mail]>
> *Subject: *Yangdoctors early review of draft-ietf-teas-yang-te-34
>
> Reviewer: Andy Bierman
> Review result: Ready with Issues
>
>
>
> # ietf-te.yang
>
> ## Summary
>
> -  Well written large module representing a lot of work.
> -  No YANG errors or warnings.
>
> ## Major Issues
>
> -  None
>
> ## Minor Issues
>
> ### leaf hierarchy/dependency-tunnels/dependency-tunnel/name
>
> -  The 'tunnel-ref' typedef is duplicated here.
>
> OLD:
>
>             type leafref {
>               path "/te:te/te:tunnels/te:tunnel/te:name";
>               require-instance false;
>             }
>
> SUGGESTED NEW:
>
>             type tunnel-ref {
>               require-instance false;
>             }
>
>
>
> [TS]: accepted.
>
>
>
> ### list lsp-provisioning-error-infos/lsp-provisioning-error-info
>
> -  This list has no key leaf(s) defined. It is used in 4 places.
>    Although legal, this limits list entry retrieval options
>
>
>
> [TS]: we discussed this amongst authors. We found no strong case to index
> the items in this read-only list. So, we opted to keep as-is.
>
>
>
> -  The description (or reference) should explain when/how a server is
>    expected to add and remove entries from this list.
>
> -  Also applies to /computed-path-error-infos/computed-path-error-info
>
> [TS]: these entries are populated by the backend path computing server and
> are “read-only”. Hence, those entries are not expected to be removed. We
> have expanded description of this in section 5.1.2.
>
>
> ### action tunnel-action
>
> -  description-stmt should explain the purpose of this action
>
> [TS]: updated description to describe the purpose.
>
> -  reference-stmt should be added, if applicable
>
> [TS]: updated.
>
>
>
> ### action protection-external-commands
>
> -  no description-stmt. It should explain the purpose of this action
>
> -  reference-stmt should be added, if applicable
>
> [TS]: updated.
>
>
>
> ### rpc tunnels-path-compute
>
> -  description-stmt should explain the purpose of this operation.
>    It looks like this operation is just a generic API,
>    and the purpose, input, and output parameters are all
>    added by augment.
>
> [TS]: updated description to describe the purpose.
>
> -  reference-stmt should be added, if applicable
> [TS]: updated.
>
>
>
>
> ### rpc tunnels-actions
>
> -  description-stmt should explain the purpose of this operation.
>
> -  reference-stmt should be added, if applicable
> [TS]: updated.
>
>
>
>
> ## Nits
>
> -  General: action and RPC naming is not very consistent.
>
> -  pg 14: the 'tunnels' list
>
>    The YANG list node is named 'tunnel'.
> [TS]: fixed.
>
> # ietf-te-device
>
> ## Summary
>
> -  Well written large module representing a lot of work.
>
> -  No YANG errors or warnings.
>
> ## Major Issues
>
> -  None
>
> ## Minor Issues
>
> ### interface/interface
>
> -  list interface with key leaf interface is confusing.
>    Descriptions for both should be expanded.
> [TS]: addressed by renaming the key to name.
>
>
>
> Regards,
>
> Tarek (for the co-authors)
>
>
>
>
> ## Nits
>
> -  None
>
>