Early Review of draft-ietf-opsawg-vpn-common-02
review-ietf-opsawg-vpn-common-02-yangdoctors-early-krejci-2020-12-18-00

*** draft ***

Seems well written with a clear statement about its purpose and describing the
groupings of the module.

*** module ***

* layout
Use pyang's `--yang-canonical` option to unify the order of the statements and
make the order canonical - e.g. starting the groupings with description
improves readability of the module. Using pyang to print the module would also
remove probably forgotten comment `//L2xMs`.

* typo
- Module's description: s/Section 4.c/Section 4/

* features
Almost none of the features is actually used in the module, which might be
fine, but there are identities referring to the same areas as the features, so
I believe that these identities should have if-feature statement referring to
the appropriate feature defined in the module.

* enumeration typedefs
Since the enumeration cannot be extended, are you really sure, that the
enumeration types you define are really complete forever? I would say that
address families, negotiation modes and  control modes might need extensions in
the modules that will use those types. Defining it as identityref with
specified identities instead of enums would solve the problem.

* `service-status` and `status-timestamp` groupings
Both groupings seem to have config false meaning.  But only the
service-status/status/oper-status container is defined as config false. The
uses statement doesn't have its own config statement, so if you want to place
the mentioned groupings into config true data, an extra grouping or refine will
be required. The common sense of the groupings is config false, so define them
this way. If there will be some exception to make them config true, the uses
can refine it in such an exceptional case.

* grouping rt-rd/rd-choice
There are 2 cases (pool-assigned and full-autoassigned) which seem to have a
node with the same meaning, but since there would be a name collision, they
have different names `rd-assign` and `rd-assigneed`. I don't think that this is
a good solution. If they have the same meaning, they should have also the same
name. I see 2 possible solutions: - move the leafs in the cases into a
container - it adds another level in data, but allows a common name
`rd-assigned` - move the `rd-assigned` leaf outside the choice. If it makes
sense, it can be constrained by must/when to the (non)-presence of the nodes
from the choice. The main point here is that the rd-assigned status is then
always at the same place.

* grouping group/
The grouping seems odd to me, like it misses something or it might be just by
too generic name. When looking into the `placement-constraint` grouping, it
seems that it refers to the groupings, but since they are separated, it is not
possible to refer to the group-id explicitly. Is the `group` grouping intended
to be instantiated standalone, without `placement-constraint`? If not, join
them together and refer the group-id from target-flavor via leafref.