[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5313: Add ability to filter DS's by the active flag

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

rawlinp commented on a change in pull request #5313:
URL: https://github.com/apache/trafficcontrol/pull/5313#discussion_r529069449



##########
File path: traffic_ops/testing/api/v3/deliveryservices_test.go
##########
@@ -396,6 +397,31 @@ func GetTestDeliveryServices(t *testing.T) {
 	}
 }
 
+func GetInactiveTestDeliveryServices(t *testing.T) {
+	actualDSes, _, err := TOSession.GetDeliveryServicesV30WithHdr(nil, nil)
+	if err != nil {
+		t.Errorf("cannot GET DeliveryServices: %v - %v", err, actualDSes)
+	}
+	totalDSes := len(actualDSes)
+	params := url.Values{}
+	params.Set("active", strconv.FormatBool(false))
+	inactiveDSes, _, err := TOSession.GetDeliveryServicesV30WithHdr(nil, params)
+	if err != nil {
+		t.Errorf("cannot GET DeliveryServices: %v - %v", err, actualDSes)
+	}
+	if len(inactiveDSes) != 0 {
+		t.Errorf("expected none of the delivery services to be inactive, but got %v inactive", len(inactiveDSes))

Review comment:
       We shouldn't hardcode data assumptions like this in the tests. Someone might need to add a test in the future that would require an inactive DS, and this test would fail for no reason. Ideally, we should add at least one `active: false` delivery service to tc-fixtures.json, then iterate over all returned DSes and check whether they are all inactive or active, based on the query parameter that was used.

##########
File path: docs/source/api/v3/deliveryservices.rst
##########
@@ -66,6 +66,8 @@ Request Structure
 	| page              | no       | Return the n\ :sup:`th` page of results, where "n" is the value of this parameter, pages are ``limit`` long and the first page is 1.    |
 	|                   |          | If ``offset`` was defined, this query parameter has no effect. ``limit`` must be defined to make use of ``page``.                       |
 	+-------------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+
+    | active            | no       | Show only the :term:`Delivery Services` that have :ref:`ds-active` set or not based on this boolean(whether or not they are active)     |

Review comment:
       I didn't view the rendered docs, but I think this may not render properly (without warnings) because you used spaces instead of tabs. The columns have to line up properly. Also, there should be a space before the `(`.




----------------------------------------------------------------
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