[GitHub] [trafficcontrol] rimashah25 commented on a diff in pull request #7099: Delivery Service Active Flag Rework

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

rimashah25 commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r994182210


##########
traffic_ops/testing/api/v5/deliveryservice_request_comments_test.go:
##########
@@ -178,7 +178,9 @@ func CreateTestDeliveryServiceRequestComments(t *testing.T) {
 		opts.QueryParameters.Set("xmlId", comment.XMLID)
 		resp, _, err := TOSession.GetDeliveryServiceRequests(opts)
 		assert.NoError(t, err, "Cannot get Delivery Service Request by XMLID '%s': %v - alerts: %+v", comment.XMLID, err, resp.Alerts)
-		assert.Equal(t, len(resp.Response), 1, "Found %d Delivery Service request by XMLID '%s, expected exactly one", len(resp.Response), comment.XMLID)
+		if !assert.Equal(t, len(resp.Response), 1, "Found %d Delivery Service request by XMLID '%s, expected exactly one", len(resp.Response), comment.XMLID) {

Review Comment:
   Why continue and not error out?



##########
traffic_ops/v5-client/deliveryservice_requests.go:
##########
@@ -58,31 +58,31 @@ func (to *Session) CreateDeliveryServiceRequest(dsr tc.DeliveryServiceRequestV4,
 		dsr.AuthorID = res.Response[0].ID
 	}
 
-	var ds *tc.DeliveryServiceV4
+	var ds *tc.DeliveryServiceV5
 	if dsr.ChangeType == tc.DSRChangeTypeDelete {
 		ds = dsr.Original
 	} else {
 		ds = dsr.Requested
 	}
 
-	if ds.TypeID == nil && ds.Type.String() != "" {
+	if ds.TypeID <= 0 && ds.Type != nil && *ds.Type != "" {

Review Comment:
   What is the difference between type and typeID. As I understand it is the name and ID of a given type, so why check for int, string and pointer?



##########
traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go:
##########
@@ -1023,6 +1027,7 @@ func scanDSInfoRow(row *sql.Row) (DSInfo, bool, error) {
 		}
 		return DSInfo{}, false, fmt.Errorf("querying delivery service server ds info: %v", err)
 	}
+	di.Active = active == tc.DSActiveStateActive

Review Comment:
   I am so lost here, why is a boolean being assigned a string and then use a comparison operator?



##########
traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go:
##########
@@ -138,7 +162,7 @@ func CreateV30(w http.ResponseWriter, r *http.Request) {
 
 	ds := tc.DeliveryServiceV30{}
 	if err := json.NewDecoder(r.Body).Decode(&ds); err != nil {
-		api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("decoding: "+err.Error()), nil)
+		api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, fmt.Errorf("decoding: %w", err), nil)

Review Comment:
   I see this (changing errors.New() to fmt.Errorf()) change a lot in the PR? Why the change? If the need is justified, shouldn't we refactor others in the code base too?



##########
traffic_ops/testing/api/v5/deliveryservices_keys_test.go:
##########
@@ -67,30 +67,29 @@ func createBlankCDN(cdnName string, t *testing.T) tc.CDN {
 	return cdns.Response[0]
 }
 
-func cleanUp(t *testing.T, ds tc.DeliveryServiceV4, oldCDNID int, newCDNID int, sslKeyVersions []string) {
-	if ds.ID == nil || ds.XMLID == nil {

Review Comment:
   why was the check for XMLID removed?



##########
traffic_ops/testing/api/v4/tc-fixtures.json:
##########
@@ -907,7 +907,7 @@
             "regexRemap": null,
             "regionalGeoBlocking": false,
             "remapText": null,
-            "routingName": "cdn",
+            "routingName": "ccr-ds1",

Review Comment:
   Why set it to an invalid value? What scenario are we trying to test?



##########
traffic_ops/traffic_ops_golang/deliveryservice/safe.go:
##########
@@ -144,19 +141,28 @@ func UpdateSafe(w http.ResponseWriter, r *http.Request) {
 		switch inf.Version.Major {
 		default:
 			fallthrough
+		case 5:
+			api.WriteRespAlertObj(w, r, tc.SuccessLevel, alertMsg, ds)
 		case 4:
-			api.WriteRespAlertObj(w, r, tc.SuccessLevel, alertMsg, dses)
+			api.WriteRespAlertObj(w, r, tc.SuccessLevel, alertMsg, []tc.DeliveryServiceV4{ds.Downgrade()})
 		case 3:
+			legacyDS := ds.Downgrade()
+			legacyDS.LongDesc1 = dses[0].LongDesc1
+			legacyDS.LongDesc2 = dses[0].LongDesc2
+			ret := legacyDS.DowngradeToV31()
 			if inf.Version.Minor >= 1 {
-				api.WriteRespAlertObj(w, r, tc.SuccessLevel, alertMsg, []tc.DeliveryServiceV31{tc.DeliveryServiceV31(ds.DowngradeToV31())})
+				api.WriteRespAlertObj(w, r, tc.SuccessLevel, alertMsg, []tc.DeliveryServiceV31{tc.DeliveryServiceV31(ret)})
 			}
-			api.WriteRespAlertObj(w, r, tc.SuccessLevel, alertMsg, []tc.DeliveryServiceV30{ds.DowngradeToV31().DeliveryServiceV30})
+			api.WriteRespAlertObj(w, r, tc.SuccessLevel, alertMsg, []tc.DeliveryServiceV30{ret.DeliveryServiceV30})
 		case 2:

Review Comment:
   Why do we have case 2 (didn't we get rid of major version v2)?



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

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

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