[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4966: TO/TP: View servers by topology-based delivery service

[GitBox <gi...@apache.org>]

ocket8888 commented on a change in pull request #4966:
URL: https://github.com/apache/trafficcontrol/pull/4966#discussion_r476596808



##########
File path: docs/source/glossary.rst
##########
@@ -422,6 +422,14 @@ Glossary
 	Tenancies
 		Users are grouped into :dfn:`Tenants` (or :dfn:`Tenancies`) to segregate ownership of and permissions over :term:`Delivery Services` and their resources. To be clear, the notion of :dfn:`Tenancy` **only** applies within the context of :term:`Delivery Services` and does **not** apply permissions restrictions to any other aspect of Traffic Control.
 
+	Topology Node
+	Topology Nodes
+	Parent Topology Node
+	Parent Topology Nodes
+	Child Topology Node
+	Child Topology Nodes
+		Each :term:`Topology Node` is associated with a particular :term:`Cache Group`. In addition, the Topology Node has 0, 1, or 2 Parent Topology Nodes and has 0, 1, or 2 Child Topology Nodes, according to your configuration.

Review comment:
       Don't link to a definition within itself - the defining instance of a term should use the `dfn` text role.

##########
File path: docs/source/api/v3/servers.rst
##########
@@ -36,7 +36,9 @@ Request Structure
 	+============+==========+===================================================================================================================+
 	| cachegroup | no       | Return only those servers within the :term:`Cache Group` that has this :ref:`cache-group-id`                      |
 	+------------+----------+-------------------------------------------------------------------------------------------------------------------+
-	| dsId       | no       | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier |
+	| dsId       | no       | Return only those servers assigned to the :term:`Delivery Service` identified by this integral, unique identifier.|
+	|            | no       | If the Delivery Service has a :term:` Topology` assigned to it, the :ref:`to-api-servers` endpoint will return    |
+	|            | no       | each server whose :ref:`Cache Group` is associated with a :term:`Topology Node` of that Topology.                 |

Review comment:
       `undefined label: cache group (if the link has no caption the label must precede a section header)`

##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -181,6 +181,36 @@ func GetTestServersQueryParameters(t *testing.T) {
 		t.Errorf("Expected a status code of 304, got %v", reqInf.StatusCode)
 	}
 
+	foundTopDs := false
+	const topDsXmlId = "ds-top"
+	for _, ds := range dses {

Review comment:
       `ds` shadows a name from its containing scope

##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -650,10 +673,15 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
 		if userErr != nil || sysErr != nil {
 			return nil, 0, errors.New("Forbidden"), sysErr, http.StatusForbidden, nil
 		}
+
+		var joinSubQuery string
+		if version.Major >= 3 {

Review comment:
       should check that the version is non-null before dereferencing property to avoid segfault
   
   also, though, I don't know why we decided `APIInfo.Version` should be a pointer value. Maybe for plugins? If I were you, I'd check it in the caller and make this function take just a regular `api.Version`. Hopefully at some point we can put that check into `api.NewInfo` and make it not give out possibly null API versions. Since if you can't tell what version someone requested, you already know they requested an invalid URI, so you can just error out right there.

##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -181,6 +181,36 @@ func GetTestServersQueryParameters(t *testing.T) {
 		t.Errorf("Expected a status code of 304, got %v", reqInf.StatusCode)
 	}
 
+	foundTopDs := false
+	const topDsXmlId = "ds-top"
+	for _, ds := range dses {
+		if (*ds.XMLID != topDsXmlId) {

Review comment:
       should check if the pointer is null before dereferencing to avoid segfault




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org