OSRB review suggestions

README.md

@@ -2,14 +2,19 @@
 
 # Nautobot BGP Models Plugin
 
-A plugin for [Nautobot](https://github.com/nautobot/nautobot) to extend the core models with BGP specific models. 
-All types of BGP peerings can be model and managed, whether or not the device is present in Nautobot.
+A plugin for [Nautobot](https://github.com/nautobot/nautobot) extending the core models with BGP-specific models.
 
-> The initial development of this plugin was sponsored by Riot Games, Inc.
+New models enable modeling and management of BGP peerings, whether or not the peer device is present in Nautobot.
+
+| Przemek: is the Riot remark something we want to keep in the public docs
+> The initial development of this plugin was sponsored by Riot Games, Inc. 
 
 ## Data Models
 
-This plugin adds the following new data models into Nautobot:
+This plugin adds the following data models to Nautobot:
+
+| Przemek: Model names on this list are not consistent with the names we use to describe individual models.
+
 - Autonomous System
 - Routing Instance
 - Peer Endpoint
@@ -19,9 +24,10 @@ This plugin adds the following new data models into Nautobot:
 - Peering
 - Peering Role
 
-A key motivation behind this design is the idea that the Source of Truth should take a network-wide view of the BGP configuration, rather than a device-per-device view. This most directly applies to the data models for autonomous systems (ASNs), BGP peerings and network-wide templates (Peer Groups).
+A key motivation behind this design is the idea that the Source of Truth should take a network-wide view of the BGP configuration, rather than a per-device view. This especially applies to the data models for autonomous systems (ASNs), BGP peerings, and network-wide templates (Peer Groups).
+
+The data models introduced by the BGP plugin support the following Nautobot features:
 
-All the data models introduced by the BGP plugin support the following Nautobot features:
 - Rest API
 - GraphQL
 - Custom fields
@@ -31,22 +37,35 @@ All the data models introduced by the BGP plugin support the following Nautobot
 - Custom data validation logic
 - Webhooks
 
-> The data model defined by this plugin takes inspirations from the Openconfig BGP data model (https://yangcatalog.org/api/services/tree/[email protected]) and the RFC 9234 (https://datatracker.ietf.org/doc/rfc9234/)
+> The data models defined in this plugin were inspired by the Openconfig BGP data model (https://yangcatalog.org/api/services/tree/[email protected]) and the RFC 9234 (https://datatracker.ietf.org/doc/rfc9234/)
 
 ### AutonomousSystem
+
 This model represents a network-wide description of a BGP autonomous system (AS). It has fields including the actual AS number (ASN), a description field, foreign key (FK) to a Nautobot `Provider` object, and a FK to a Nautobot `Status` object.
 
 ### BGPRoutingInstance
+
 This model represents a device specific BGP process. It has a mandatory FK to a Nautobot `Device`, mandatory FK to a `AutonomousSystem` and following fields: 
 
-- Router ID (optional, FK to Nautobot `IPAddress`,
+- Router ID (optional, FK to Nautobot `IPAddress`)
 - Description (optional, string)
 - Extra Attributes (optional, JSON)
 
+| Przemek: No mention of the Peer Group Template attribute? Peer Group Template attribute does not show up in the detailed view.
+
 ### PeerEndpoint
-This model represents the configuration of a single device with respect to a single BGP peering. It does not store configuration that must match symmetrically between peer devices (such as a common authentication key). 
 
-Note that in the case of an external peering (device in the locally managed network peering to a remote endpoint belonging to an AS not managed within this network), while there generally will not be a Device record representing the remote endpoint, there will need to be a `PeerEndpoint` record representing it, at a minimum storing the IP address and ASN of the remote endpoint.
+| Przemek: We should probably start with stating that this model is not directly exposed. I was looking for it in the menu. So something along the lines of "PeerEndpoint records are created when Peering instance is created."
+
+This model represents the configuration of a single device with respect to a single BGP peering. 
+
+| Przemek: What does the below mean? Some configuration elements must match for the peering to establish, do we refer to all of them or just some?
+
+It does not store configuration that must match symmetrically between peer devices (such as a common authentication key).
+
+| Przemek: The below might lead user to think that he needs to manually create PeerEndpoint for remote peer. So might need to qualify it and say that this endpoint will be created automatically once Peering object is created.
+
+Note that in the case of an external peering (device in the locally managed network peering to a remote endpoint belonging to an AS not managed within this network), while there generally will not be a Device record representing the remote endpoint, there will need to be a PeerEndpoint record representing it, at a minimum storing the IP address and ASN of the remote endpoint.
 
 `PeerEndpoint` model has a mandatory FK to a BGP Routing Instance (`BGPRoutingInstance`) record, an optional foreign-key relationship to a `PeerGroup`, and additional keys including:
 
@@ -63,17 +82,23 @@ Note that in the case of an external peering (device in the locally managed netw
 - Extra Attributes (optional, JSON)
 
 #### PeerEndpoint Local-IP
+
 To ease the data presentation and consumption, `PeerEndpoint` provides also a property named `local_ip`.
+
+| Przemek: Which GUI view has Local IP Address?
+
 The value of this property will be presented in plugin's Grapical User Interface (GUI), and can be used to render configuration templates.
 
 As Source-IP and Source-Interface could be defined at multiple inheritance levels, each Peer Endpoint will have a `local_ip` determined based on the following order:
+
 1. `PeerEndpoint`'s `source_ip` attribute (if exists)
 2. `Peer Group`'s `source_ip` attribute (if exists)
 3. `PeerEndpoint`'s `source_interface` attribute (if exists)
 4. `PeerGroup`'s `source_interface` attribute (if exists)
 
 ### PeerGroup
-This model represents common configuration for a group of functionally related BGP peers. `PeerGroup` aims to represent device-specific configuration, and it has a mandatory `Name` field, optional FK to a network-wide `PeerGroupTemplate`, and additional fields including:
+
+This model represents common configuration for a group of functionally related BGP peers. Peer Group aims to represent device-specific configuration shared across multiple peerings, and it has a mandatory `Name` field, optional FK to a network-wide `PeerGroupTemplate`, and additional fields including:
 
 - ASN (optional, FK to `AutonomousSystem`)
 - Source IP (optional, FK to Nautobot `IPAddress`, mutually-exclusive with Source Interface)
@@ -87,6 +112,7 @@ This model represents common configuration for a group of functionally related B
 - Extra Attributes (optional, JSON)
 
 ### PeerGroupTemplate
+
 This model represents network-wide configuration for `PeerGroups`. `PeerGroupTemplate` aims to represent a global configuration, and it has a mandatory `Name` field, and following fields:
 
 - ASN (optional, FK to `AutonomousSystem`)
@@ -99,43 +125,63 @@ This model represents network-wide configuration for `PeerGroups`. `PeerGroupTem
 - Extra Attributes (optional, JSON)
 
 ### Extra Attributes
-Additional BGP object's attributes can be modelled by "Extra Attributes". Extra_attributes is a JSON type fields allowing to store data modelled by user.
-Extra attributes follow the inheritance pattern, thus allowing for merging the inherited extra attributes.
 
-Examples of the extra attributes might include:
+Additional BGP object's attributes can be defined in "Extra Attributes" field. Extra attributes is a JSON type field meant to store data defined by user.
+
+Extra attributes follow the inheritance pattern, thus allowing for merging of the inherited extra attributes.
+
+Example of the extra attributes:
+
 ```json
 {"ttl-security": 1, "timers": [6, 20] }
 ```
 
 Extra Attributes are available for following models:
+
 - `PeerEndpoint`
 - `PeerGroup`
 - `PeerGroupTemplate`
 - `BGPRoutingInstance`
 
+| Przemek: This should be included earlier as it's being referred to from other models.
+
 ### AddressFamily
+
 This model represents configuration of a BGP address-family (AFI-SAFI). AddressFamily aims to represent a device specific Address Family instance.
-It has a locally unique AFI (address-family identifier) field, optional VRF field (FK to Nautobot `VRF`) and following fields:
+
+It has a locally unique AFI (address family identifier) field, optional VRF field (FK to Nautobot `VRF`) and following fields:
+
 - Import Policy (optional, string)
 - Export Policy (optional, string)
 
-(*) The network-wide modelling of AddressFamilies will be implemented in the future with `AddressFamilyTemplate` model similar to the `PeerGroupTemplate`.
+(*) The network-wide modeling of AddressFamilies will be implemented in the future with `AddressFamilyTemplate` model similar to the `PeerGroupTemplate`.
+
+| Przemek: Should the below be included in the description of the `PeerEndpoint` model?
+
 The device-specific `PeerEndpoint` custom modeling will be implemented in the future with `PeerEndpointContext` and `PeerGroupContext` models.
 
 ### Peering
+
 This model represents the shared configuration of a single BGP peer relationship between two endpoints. It has FKs to two `PeerEndpoint` records (representing the two devices involved in the peering), and additional fields including:
+
 - Status (FK to Nautobot `Status`)
 
-> The nature of a session as BGP “internal” or “external” is useful in the construction of queries and filters, but does not need to be stored as an actual database attribute (as it is implied by whether the ASNs of the two BGPPeerEndpoints involved are identical or different). It is implemented as a derived property of the `Peering` model.
+> The nature of a session as BGP "internal" or "external" is useful in the construction of queries and filters, but does not need to be stored as an actual database attribute (as it is implied by whether the ASNs of the two BGPPeerEndpoints involved are identical or different). It is implemented as a derived property of the `Peering` model.
+
+| Przemek: We should ideally show two use cases related to peering, one for internal peering, another one for external peering. Form has got a lot of fields and it's not immediately obvious how to use it.
 
 ### PeeringRole
-This model operates similarly to Nautobot’s `Status` and `Tag` models, in that instances of this model describe various valid values for the `role` field on `PeerGroupTemplate`, `PeerGroup` and/or `PeerEndpoint`. This model has fields including a unique name, unique slug, and a HTML color value.
+
+This model operates similarly to Nautobot’s `Status` and `Tag` models, in that instances of this model describe various valid values for the `Role` field used by `PeerGroup` and `PeerSession` records. Similar to those models, this model has fields including a unique name, unique slug, and a HTML color value.
+
+| Przemek: This model should be introduced earlier, since other models refer to it.
 
 ### Inheritance between models
 
-Some models have a built-in inheritance similar to what BGP supports with PeerGroup. The inheritance can take multi-level lookup between BGP objects, in this case the first found object with the assigned attribute will be considered as an inheritance source.
+Some models can inherit attribute values, similar to what BGP supports with Peer Group. The inheritance is built hierarchically. The final attribute value will be taken from the first object in the hierarchy, moving from the top, which has given the attribute value defined.
 
 Example **PeerEndpoint** inheritance details:
+
 - A `PeerEndpoint` inherits `AutonomousSystem` and `extra_attributes` fields from:
   - `PeerGroup`
   - `PeerGroupTemplate`
@@ -150,13 +196,13 @@ Example **PeerEndpoint** inheritance details:
 - A `PeerEndpoint` inherits `source_ip`, `source_interface` fields from:
   - `PeerGroup`
 
-As an example, a `PeerEndpoint` associated with a `PeerGroup` will automatically inherit above attributes of the `PeerGroup` that haven't been defined at the `PeerEndpoint` level. If an attribute is defined on both, the value defined on the `PeerEndpoint` will be used.
+As an example, a `PeerEndpoint` associated with a `PeerGroup` will automatically inherit above attributes of the `PeerGroup` that haven't been defined at the `PeerEndpoint` level. If an attribute is defined at both levels, the value defined in the `PeerEndpoint` will be used.
 
-The inherited values will be automatically displayed in the UI and can be retrieve from the REST API with the additional `?include_inherited=true` parameter.
+The inherited values will be automatically displayed in the UI and can be retrieved from the REST API by adding `?include_inherited=true` parameter.
 
 (*) **BGP models Custom Fields and GraphQL currently does not offer support for BGP Field Inheritance.** GraphQL issue is tracked under https://github.com/nautobot/nautobot-plugin-bgp-models/issues/43 
 
-Following is the complete documentation of the field inheritance pattern. Models are listed from top priority to the least priority, the first model with an assigned attribute value will be used as an inheritance source.
+Following is the complete documentation of the field inheritance hierarchy. Models are ordered with the topmost having the highest priority. The first model with an assigned attribute value will be used as an inheritance source.
 
 **PeerEndpoint**:
 
@@ -223,12 +269,12 @@ PLUGINS_CONFIG = {
     "nautobot_bgp_models": {
         "default_statuses": {
             "AutonomousSystem": ["active", "available", "planned"],
-            "Peering": ["active", "decommissioned", "deprovisioning", "offline", "planned", "provisioning"],
+            "PeerSession": ["active", "decommissioned", "deprovisioning", "offline", "planned", "provisioning"],
         }
     }
 }
 ```
-In the `default_statuses` section, you can define a list of default statuses to make available to `AutonomousSystem` and/or `Peering`. The lists must be composed of valid slugs of existing Status objects.
+In the `default_statuses` section, you can define a list of default statuses to make available to `AutonomousSystem` and/or `PeerSession`. The lists must be composed of valid slugs of existing Status objects.
 
 ## Screenshots
 

docs/cisco_use_case.md

@@ -1,8 +1,10 @@
 # Example use of BGP Models plugin - Cisco BGP Configuration
+
 This document provides an example of generating a Cisco device's desired BGP configuration based on data stored in Nautobot using this plugin. A GraphQL query is used to retrieve the relevant data, which is then rendered through a Jinja2 template to produce the desired configuration.
 
 ## Querying for the data
-In order to retrieve a BGP data, following GraphQL can be issued to a Nautobot.  
+
+In order to retrieve a BGP data, following GraphQL query can be issued to a Nautobot.
 
 ```python
 import pynautobot

docs/juniper_use_case.md

@@ -1,8 +1,10 @@
 # Example use of BGP Models plugin - Juniper BGP Configuration
+
 This document provides an example of generating a Juniper device's desired BGP configuration based on data stored in Nautobot using this plugin. A GraphQL query is used to retrieve the relevant data, which is then rendered through a Jinja2 template to produce the desired configuration.
 
 ## Querying for the data
-In order to retrieve a BGP data, following GraphQL can be issued to a Nautobot.  
+
+In order to retrieve a BGP data, following GraphQL query can be issued to Nautobot.
 
 ```python
 import pynautobot