Re: [Lsr] [Last-Call] Yangdoctors last call review of draft-ietf-lsr-yang-isis-reverse-metric-01

Christian Hopps <chopps@chopps.org> Fri, 18 December 2020 18:37 UTC

Return-Path: <chopps@chopps.org>
X-Original-To: lsr@ietfa.amsl.com
Delivered-To: lsr@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 5F36D3A0BA4; Fri, 18 Dec 2020 10:37:04 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.897
X-Spam-Level:
X-Spam-Status: No, score=-1.897 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_NONE=0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Y83oqwPZyPWo; Fri, 18 Dec 2020 10:37:03 -0800 (PST)
Received: from smtp.chopps.org (smtp.chopps.org [54.88.81.56]) by ietfa.amsl.com (Postfix) with ESMTP id D7CD83A0B92; Fri, 18 Dec 2020 10:37:02 -0800 (PST)
Received: from [192.168.2.5] (047-050-069-038.biz.spectrum.com [47.50.69.38]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by smtp.chopps.org (Postfix) with ESMTPSA id 60A3C6092B; Fri, 18 Dec 2020 18:37:01 +0000 (UTC)
From: Christian Hopps <chopps@chopps.org>
Message-Id: <1FEE65FE-A0CE-405D-A584-DEB9AA43205B@chopps.org>
Content-Type: multipart/signed; boundary="Apple-Mail=_84BB079F-F251-4B7D-A05C-E7BB39FC0DD8"; protocol="application/pgp-signature"; micalg="pgp-sha512"
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Date: Fri, 18 Dec 2020 13:37:00 -0500
In-Reply-To: <160812077773.23729.6946762665789150965@ietfa.amsl.com>
Cc: Christian Hopps <chopps@chopps.org>, yang-doctors@ietf.org, lsr@ietf.org, draft-ietf-lsr-yang-isis-reverse-metric.all@ietf.org, last-call@ietf.org
To: Ladislav Lhotka <ladislav.lhotka@nic.cz>
References: <160812077773.23729.6946762665789150965@ietfa.amsl.com>
X-Mailer: Apple Mail (2.3608.120.23.2.4)
Archived-At: <https://mailarchive.ietf.org/arch/msg/lsr/JeqfqZI-Mj3sjP5Ww0j-63d7muI>
Subject: Re: [Lsr] [Last-Call] Yangdoctors last call review of draft-ietf-lsr-yang-isis-reverse-metric-01
X-BeenThere: lsr@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Link State Routing Working Group <lsr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/lsr>, <mailto:lsr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/lsr/>
List-Post: <mailto:lsr@ietf.org>
List-Help: <mailto:lsr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/lsr>, <mailto:lsr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 18 Dec 2020 18:37:05 -0000


> On Dec 16, 2020, at 7:12 AM, Ladislav Lhotka via Datatracker <noreply@ietf.org> wrote:
> 
> Reviewer: Ladislav Lhotka
> Review result: Ready with Issues
> 
> **** General comments
> 
> - YANG module "ietf-isis-reverse-metric" is in a good shape except for
>  one issue described below.
> - I appreciate the three examples of instance data in appendices (two
>  in XML representation, one in JSON).
> 
> **** Specific comments
> 
> - In three places, the module uses "when" expressions with plain
>  string equality test applied to identityref values, such as:
> 
>     when "../rt:type = 'isis:isis'"
> 
>  This is known to be fragile, which can demonstrated on the JSON
>  example in appendix A.3: It has
> 
>     "type": "ietf-isis:isis"
> 
>  which makes the "when" expression false because of the different
>  prefix, and the corresponding augment doesn't happen.
> 
>  The above "when" expression should use the XPath function
>  "derived-from-or-self" that is defined in RFC 7950:
> 
>     when "derived-from-or-self(../rt:type, 'isis:isis')"

Will change.

> 
> - The "tlv16-reverse-metric" grouping is used only once in the
>  module. Unless it is expected to be used elsewhere, I would
>  recommend to remove this grouping and use its contents directly.

As this was describing TLV data I figured it might be useful to other modules that work with TLVs.

> 
> - The phrase "YANG XML data" is somewhat misleading, although it is
>  probably often used in informal discussions. I would suggest "XML
>  instance data" instead.

Updated.


> 
> **** Nits
> 
> - Abstract & Introduction: s/routeing/routing/

I'm actually taking that from the standard which names the protocol. :)

> 
> - Appending A.3: s/YANG XML data/JSON instance data/

Fixed.

Thanks,
Chris.

> 
> 
> 
> --
> last-call mailing list
> last-call@ietf.org
> https://www.ietf.org/mailman/listinfo/last-call
>