Re: [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-ntw-attachment-circuit-04

2024-02-09 Thread mohamed . boucadair
Hi all, 

FWIW, the proposed changes are now implemented in the published version: 
https://datatracker.ietf.org/doc/html/draft-ietf-opsawg-ntw-attachment-circuit-05.

Cheers,
Med

> -Message d'origine-
> De : BOUCADAIR Mohamed INNOV/NET
> Envoyé : jeudi 25 janvier 2024 13:46
> À : 'Martin Björklund' ; yang-doct...@ietf.org
> Cc : draft-ietf-opsawg-ntw-attachment-circuit@ietf.org;
> opsawg@ietf.org
> Objet : RE: Yangdoctors early review of draft-ietf-opsawg-ntw-
> attachment-circuit-04
> 
> Hi Martin,
> 
> Thanks for the review.
> 
> Please see inline.
> 
> Cheers,
> Med
> 
> > -Message d'origine-
> > De : Martin Björklund via Datatracker  Envoyé :
> > mercredi 24 janvier 2024 16:20 À : yang-doct...@ietf.org Cc :
> > draft-ietf-opsawg-ntw-attachment-circuit@ietf.org;
> > opsawg@ietf.org
> > Objet : Yangdoctors early review of draft-ietf-opsawg-ntw-
> attachment-
> > circuit-04
> >
> > Reviewer: Martin Björklund
> > Review result: Ready with Issues
> >
> > Here is my YANG doctor's review of draft-ietf-opsawg-ntw-attachment-
> > circuit-04.
> >
> > o  There are several typedefs defined on the form:
> >
> >   typedef attachment-circuit-reference {
> > type leafref {
> >   path "/nw:networks/nw:network/nw:node/ac-ntw:ac/ac-ntw:name";
> > }
> >
> >   Note that this path spans three lists (network, node, ac).  Unless
> >   it is guaranteed that the `ac-ntw:ame` value is unique within all
> >   networks and all nodes, this typedef won't work (or rather, it may
> >   refer to more than one ac).
> >
> 
> [Med] Good catch. We don't have that constraint on the "ac-ntw:name".
> Please see the candidate changes to fix this at: https://author-
> tools.ietf.org/api/iddiff?url_1=https://boucadair.github.io/attachment
> -circuit-model/draft-ietf-opsawg-ntw-attachment-
> circuit.txt_2=https://boucadair.github.io/attachment-circuit-
> model/boucadair-patch-5/draft-ietf-opsawg-ntw-attachment-circuit.txt.
> The use of groupings for references is consistent with the approach in
> RFC 8345.
> 
> >
> > o typedef ac-profile-reference {
> > type leafref {
> >   path "/nw:networks/nw:network/ac-profile/name";
> > }
> >
> >   The nodes should have prefixes:
> >
> >   path "/nw:networks/nw:network/ac-ntw:ac-profile/ac-ntw:name";
> >
> 
> [Med] Fixed.
> 
> >
> > o   leaf site-of-origin {
> >   when "../address-family = 'vpn-common:ipv4' "
> >  + "or 'vpn-common:dual-stack'" {
> >
> > leaf ipv6-site-of-origin {
> >   when "../address-family = 'vpn-common:ipv6' "
> >  + "or 'vpn-common:dual-stack'" {
> >
> >
> >Use 'derived-from-or-self' instead of comparison.
> >
> 
> [Med] Agree. Fixed.
> 
> >
> > o  Some lists have plural-names: routing-profiles, ipv4-lan-
> prefixes,
> >ipv6-lan-prefixes.  Usually lists should have singular names.
> >
> 
> [Med] Fixed.
> 
> > o typedef encryption-profile-reference {
> > ...
> > description
> >   "Defines a type to an encryption profile for referencing
> >purposes.";
> >   }
> >
> >   Perhaps "Defines a reference to an encryption profile"?
> >
> >   (Same for 4 more typedefs)
> >
> >
> 
> [Med] Works for me. Thanks.
> 
> >
> > /martin
> >


Ce message et ses pieces jointes peuvent contenir des informations 
confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce 
message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages 
electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou 
falsifie. Merci.

This message and its attachments may contain confidential or privileged 
information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete 
this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been 
modified, changed or falsified.
Thank you.
___
OPSAWG mailing list
OPSAWG@ietf.org
https://www.ietf.org/mailman/listinfo/opsawg


Re: [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-ntw-attachment-circuit-04

2024-01-25 Thread mohamed . boucadair
Hi Martin,

Thanks for the review. 

Please see inline.

Cheers,
Med

> -Message d'origine-
> De : Martin Björklund via Datatracker 
> Envoyé : mercredi 24 janvier 2024 16:20
> À : yang-doct...@ietf.org
> Cc : draft-ietf-opsawg-ntw-attachment-circuit@ietf.org;
> opsawg@ietf.org
> Objet : Yangdoctors early review of draft-ietf-opsawg-ntw-attachment-
> circuit-04
> 
> Reviewer: Martin Björklund
> Review result: Ready with Issues
> 
> Here is my YANG doctor's review of draft-ietf-opsawg-ntw-attachment-
> circuit-04.
> 
> o  There are several typedefs defined on the form:
> 
>   typedef attachment-circuit-reference {
> type leafref {
>   path "/nw:networks/nw:network/nw:node/ac-ntw:ac/ac-ntw:name";
> }
> 
>   Note that this path spans three lists (network, node, ac).  Unless
>   it is guaranteed that the `ac-ntw:ame` value is unique within all
>   networks and all nodes, this typedef won't work (or rather, it may
>   refer to more than one ac).
> 

[Med] Good catch. We don't have that constraint on the "ac-ntw:name". Please 
see the candidate changes to fix this at: 
https://author-tools.ietf.org/api/iddiff?url_1=https://boucadair.github.io/attachment-circuit-model/draft-ietf-opsawg-ntw-attachment-circuit.txt_2=https://boucadair.github.io/attachment-circuit-model/boucadair-patch-5/draft-ietf-opsawg-ntw-attachment-circuit.txt.
 The use of groupings for references is consistent with the approach in RFC 
8345.

> 
> o typedef ac-profile-reference {
> type leafref {
>   path "/nw:networks/nw:network/ac-profile/name";
> }
> 
>   The nodes should have prefixes:
> 
>   path "/nw:networks/nw:network/ac-ntw:ac-profile/ac-ntw:name";
> 

[Med] Fixed.

> 
> o   leaf site-of-origin {
>   when "../address-family = 'vpn-common:ipv4' "
>  + "or 'vpn-common:dual-stack'" {
> 
> leaf ipv6-site-of-origin {
>   when "../address-family = 'vpn-common:ipv6' "
>  + "or 'vpn-common:dual-stack'" {
> 
> 
>Use 'derived-from-or-self' instead of comparison.
> 

[Med] Agree. Fixed.

> 
> o  Some lists have plural-names: routing-profiles, ipv4-lan-prefixes,
>ipv6-lan-prefixes.  Usually lists should have singular names.
> 
 
[Med] Fixed.

> o typedef encryption-profile-reference {
> ...
> description
>   "Defines a type to an encryption profile for referencing
>purposes.";
>   }
> 
>   Perhaps "Defines a reference to an encryption profile"?
> 
>   (Same for 4 more typedefs)
> 
> 

[Med] Works for me. Thanks.

> 
> /martin
> 


Ce message et ses pieces jointes peuvent contenir des informations 
confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce 
message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages 
electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou 
falsifie. Merci.

This message and its attachments may contain confidential or privileged 
information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete 
this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been 
modified, changed or falsified.
Thank you.
___
OPSAWG mailing list
OPSAWG@ietf.org
https://www.ietf.org/mailman/listinfo/opsawg