Re: [yang-doctors] [Netconf] Yangdoctors last call review of draft-ietf-netconf-nmda-netconf-03

Martin Bjorklund <mbj@tail-f.com> Fri, 23 February 2018 15:43 UTC

Return-Path: <mbj@tail-f.com>
X-Original-To: yang-doctors@ietfa.amsl.com
Delivered-To: yang-doctors@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 3C6CF127241; Fri, 23 Feb 2018 07:43:39 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.911
X-Spam-Level:
X-Spam-Status: No, score=-1.911 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-0.001, T_RP_MATCHES_RCVD=-0.01] 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 1_C52Xdk6hDU; Fri, 23 Feb 2018 07:43:37 -0800 (PST)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id 642821271DF; Fri, 23 Feb 2018 07:43:34 -0800 (PST)
Received: from localhost (h-80-27.A165.priv.bahnhof.se [212.85.80.27]) by mail.tail-f.com (Postfix) with ESMTPSA id DF76D1AE02BE; Fri, 23 Feb 2018 16:43:32 +0100 (CET)
Date: Fri, 23 Feb 2018 16:43:32 +0100 (CET)
Message-Id: <20180223.164332.1710999237635438920.mbj@tail-f.com>
To: exa@juniper.net
Cc: yang-doctors@ietf.org, ietf@ietf.org, draft-ietf-netconf-nmda-netconf.all@ietf.org, netconf@ietf.org
From: Martin Bjorklund <mbj@tail-f.com>
In-Reply-To: <151934407669.22591.51198627609392228@ietfa.amsl.com>
References: <151934407669.22591.51198627609392228@ietfa.amsl.com>
X-Mailer: Mew version 6.7 on Emacs 24.5 / Mule 6.0 (HANACHIRUSATO)
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/v8B9Hk06L4bVwDw1UlhWvmcnvPQ>
Subject: Re: [yang-doctors] [Netconf] Yangdoctors last call review of draft-ietf-netconf-nmda-netconf-03
X-BeenThere: yang-doctors@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: Email list of the yang-doctors directorate <yang-doctors.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/yang-doctors/>
List-Post: <mailto:yang-doctors@ietf.org>
List-Help: <mailto:yang-doctors-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 23 Feb 2018 15:43:39 -0000

Hi,

Thank you for this review.  Comments inline.

Ebben Aries <exa@juniper.net> wrote:
> Reviewer: Ebben Aries
> Review result: Almost Ready
> 
> 1 module in this draft:
> - ietf-netconf-nmda@2018-02-05.yang
> 
> YANG validation errors or warnings (from pyang 1.7.3 and yanglint 0.14.69)
> - ietf-netconf-nmda@2018-02-05.yang:171: warning: RFC 6087: 4.10,4.12:
>   statement "enum" should have a "description" substatement (From pyang 1.7.3)

Yes, this is a warning b/c it is a SHOULD in 6087.  In this case, the
enum is described together with the rest in the main description.  I
think it gives a better overall description of the type.

> 0 examples are provided in this draft (section 3.12 of
> draft-ietf-netmod-rfc6087bis-18)
> Module ietf-netconf-nmda@2018-02-05.yang:
> - Note: For the following imports, there are no references to the supporting
>   documents as suggested in rfc6087bis however this item is currently under
>   discussion on both usefulness/possible formatting
>   - import "ietf-yang-types" should reference RFC 6991 per (not as a comment)
>     https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
>   - import "ietf-inet-types" should reference RFC 6991 per (not as a comment)
>     https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
>   - import "ietf-datastores" should reference I-D.ietf-netmod-revised-datastores per
>     https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
>   - import "ietf-origin" should reference I-D.ietf-netmod-revised-datastores per
>     https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
>   - import "ietf-netconf" should reference RFC 6241 per
>     https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7
>   - import "ietf-netconf-with-defaults" should reference RFC 6243 per
>     https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#section-4.7

I think we have to wait for the outcome of that discussion.  I would
prefer to add the reference statements.

> - Module description, revision and feature definition should contain note to
>   RFC Ed. to change placeholder reference to RFC when assigned
>   https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-18#appendix-C

Ok.

> - Security Considerations looks good and is adjusted to account for NETCONF
>   only as well as addressing the additional RPCs introduced
> - L82: suggested replacement of text:
>   s/, i.e., the filter criteria are logically ANDed/. Multiple filters are
>   processed as a logical AND operation/

Hmm.  This misses the "i.e." - the original text explains the sentence
just before it.  What do others think?  Maybe leave this to the RFC
editor?

> - L127: suggested replacement of text:
>   s/then the get-data/the get-data/

We have "if ... then ..." in several places.  Maybe also leave this to
the RFC editor?

> General comments/nits on draft text:
> - As mentioned above, there are 0 examples in this draft.  There should be XML
>   RPC examples of the 2 newly defined RPCs as well as usage examples of the
>   augments of RFC6241 RPCs

This is a SHOULD in 6087.  I don't think that examples for these
operations would add much value.


> - :with-origin    urn:ietf:params:netconf:capability:with-origin:1.0
>   Wouldn't it make more sense to have this URN defined in
>   I-D.ietf-netmod-revised-datastores rather?  In addition, should this
>   capability and feature be rather defined as 'origin' in the 'ietf-origin'
>   module?  As defined today, this feature is used as a clause for
>   'origin-filter' as well as 'with-origin' inputs to get-data however these
>   inputs appear to be mutually exclusive.
>   IMHO, this should be detached from this specific draft which focuses on
>   NETCONF specific base extensions for datastores as the returning and
>   filtering of origin metadata can be transport independent.

The capability is NETCONF-specific, and should be defined here, imo.
As for the feature, it is not as clear.  It is not obvious that it
should be defined in ietf-origin.  (and since that doc is at the RFC
editor, it might be late to change, unless it is really urgent).

> - L308: suggested replacement of text:
>   s/parameter is optional to support/parameter is optional and dependant off
>   of the servers support for the 'origin' feature/ (dependant off the previous
>   comment)

Do you mean that if we don't move the origin feature, this text should
not change?

> - L320: "ietf-netconf-nmda" (see Section 4). - This should be an actual
>   reference as in line #306

I don't understand this comment.  It is a proper reference...?

> - L345: "default-operation" parameter of the <edit-config> operation - This
>   should at a bare minimum call reference to the appropriate section in
>   RFC6241

There is a reference for <edit-config> to RFC 6241 in the first
paragraph of this section.  I think that is how references should be
done, but I might be wrong.  I am sure the RFC editor will fix this,
if needed.

> - To the previous point, since there is a large amount of reference and
>   comparison to RFC6241 definitions, any references should be tagged
>   appropriately

I am not sure I understand what you mean with "tagged appropriately".
Could you provide OLD / NEW text for these occurences?


/martin