[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5404: TO unable to unassign ORG server from delivery services if it's the last assigned

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

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



##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/servers/delete.go
##########
@@ -69,7 +79,7 @@ func checkLastServer(dsID, serverID int, tx *sql.Tx) (int, error, error) {
 		return http.StatusInternalServerError, nil, fmt.Errorf("checking if server #%d is the last one assigned to DS #%d: %v", serverID, dsID, err)
 	}
 	if isLast {
-		return http.StatusConflict, fmt.Errorf("removing server #%d from active Delivery Service #%d would leave it with no assigned servers", serverID, dsID), nil
+		return http.StatusConflict, fmt.Errorf("removing server #%d from active Delivery Service #%d would leave it with no REPORTED/ ONLINE assigned servers", serverID, dsID), nil

Review comment:
       Nit but I don't think you need that ` ` after the `/`

##########
File path: traffic_ops/testing/api/v3/topologies_test.go
##########
@@ -279,15 +279,13 @@ func UpdateValidateTopologyORGServerCacheGroup(t *testing.T) {
 
 	}
 
-	//TODO: Need to fix the query in deliveryservice/servers/delete.go for DeleteDeliveryServiceServer() to work correctly.
-
-	// Remove org server assignment and reset DS back to as it was for further testing
-	//params.Set("hostName", "denver-mso-org-01")
-	//serverResp, _, err := TOSession.GetServersWithHdr(&params, nil)
-	//_, _, err = TOSession.DeleteDeliveryServiceServer(*remoteDS[0].ID, *serverResp.Response[0].ID)
-	//if err != nil {
-	//	t.Errorf("cannot delete assigned server from Delivery Services: %v", err)
-	//}
+	//Remove org server assignment and reset DS back to as it was for further testing
+	params.Set("hostName", "denver-mso-org-01")
+	serverResp, _, err := TOSession.GetServersWithHdr(&params, nil)
+	_, _, err = TOSession.DeleteDeliveryServiceServer(*remoteDS[0].ID, *serverResp.Response[0].ID)

Review comment:
       This could segfault in two different ways: `serverResp.Response` could be nil so indexing with `0` would panic, and `serverResp.Response[0].ID` could be nil so dereferencing it would panic. Those conditions should be checked.




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