[GitHub] [trafficcontrol] TaylorCFrey commented on a change in pull request #5603: Removing non existent route /tools/flash_and_close/{message}

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

TaylorCFrey commented on a change in pull request #5603:
URL: https://github.com/apache/trafficcontrol/pull/5603#discussion_r588777221



##########
File path: traffic_ops/traffic_ops_golang/crconfig/handler.go
##########
@@ -257,9 +256,9 @@ func snapshotHandler(w http.ResponseWriter, r *http.Request, deprecated bool) {
 
 // SnapshotOldGUIHandler creates the CRConfig JSON and writes it to the snapshot table in the database. The response emulates the old Perl UI function. This should go away when the old Perl UI ceases to exist.
 func SnapshotOldGUIHandler(w http.ResponseWriter, r *http.Request) {
-	inf, userErr, sysErr, _ := api.NewInfo(r, []string{"cdn"}, nil)
+	inf, userErr, sysErr, code := api.NewInfo(r, []string{"cdn"}, nil)

Review comment:
       NIT: The other handlers label this variable with `errCode` instead of just `code`

##########
File path: traffic_ops/traffic_ops_golang/crconfig/handler.go
##########
@@ -286,7 +284,7 @@ func SnapshotOldGUIHandler(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if err := Snapshot(inf.Tx.Tx, crConfig, tm); err != nil {
-		writePerlHTMLErr(w, r, inf.Tx.Tx, errors.New(r.RemoteAddr+" making CRConfig: "+err.Error()), err)
+		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, err, errors.New(r.RemoteAddr+" making CRConfig: "+err.Error()))

Review comment:
       Same here, we want to pass `nil` instead of `err` since this is a sysErr not a userErr.

##########
File path: traffic_ops/traffic_ops_golang/crconfig/handler.go
##########
@@ -296,11 +294,5 @@ func SnapshotOldGUIHandler(w http.ResponseWriter, r *http.Request) {
 	}
 
 	api.CreateChangeLogRawTx(api.ApiChange, "Snapshot of CRConfig performed for "+cdn, inf.User, inf.Tx.Tx)
-	http.Redirect(w, r, "/tools/flash_and_close/"+url.PathEscape("Successfully wrote the CRConfig.json!"), http.StatusFound)
-}
-
-func writePerlHTMLErr(w http.ResponseWriter, r *http.Request, tx *sql.Tx, logErr error, err error) {
-	log.Errorln(logErr.Error())
-	tx.Rollback()

Review comment:
       Interesting. I haven't seen many transaction rollbacks. Is this something we still need to account for before redirecting?

##########
File path: traffic_ops/traffic_ops_golang/crconfig/handler.go
##########
@@ -296,11 +294,5 @@ func SnapshotOldGUIHandler(w http.ResponseWriter, r *http.Request) {
 	}
 
 	api.CreateChangeLogRawTx(api.ApiChange, "Snapshot of CRConfig performed for "+cdn, inf.User, inf.Tx.Tx)
-	http.Redirect(w, r, "/tools/flash_and_close/"+url.PathEscape("Successfully wrote the CRConfig.json!"), http.StatusFound)
-}
-
-func writePerlHTMLErr(w http.ResponseWriter, r *http.Request, tx *sql.Tx, logErr error, err error) {
-	log.Errorln(logErr.Error())
-	tx.Rollback()
-	http.Redirect(w, r, "/tools/flash_and_close/"+url.PathEscape("Error: "+err.Error()), http.StatusFound)
+	http.Redirect(w, r, client.API_v13_CDNs+"/"+cdn+"/snapshot", http.StatusFound)

Review comment:
       So this just feels weird to me. Including the _client_ which is based on the TO api seems almost cyclical? Don't we want to simply redirect akin to:
   ```
   http.Redirect(w, r, "/api/1.1/cdns/"+cdn+"/snapshot", http.StatusSeeOther)
   ```
   I know we're just trying to use a const version here, but I dunno. Should we make it a const in this package so we aren't relying on a v1 client? I do like the idea, once removing v1 api and client that this will automatically be picked up, but something just itches the wrong way for me here.
   

##########
File path: traffic_ops/traffic_ops_golang/crconfig/handler.go
##########
@@ -271,11 +270,10 @@ func SnapshotOldGUIHandler(w http.ResponseWriter, r *http.Request) {
 	}
 
 	cdn := inf.Params["cdn"]
-
 	// We never store tm_path, even though low API versions show it in responses.
 	crConfig, err := Make(inf.Tx.Tx, cdn, inf.User.UserName, r.Host, inf.Config.Version, inf.Config.CRConfigUseRequestHost, false)
 	if err != nil {
-		writePerlHTMLErr(w, r, inf.Tx.Tx, errors.New(r.RemoteAddr+" making CRConfig: "+err.Error()), err)
+		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, err, errors.New(r.RemoteAddr+" making CRConfig: "+err.Error()))

Review comment:
       The method signature for `api.HandleErr` is:
   ```
   func HandleErr(w http.ResponseWriter, r *http.Request, tx *sql.Tx, statusCode int, userErr error, sysErr error)
   ```
   So, we don't want to pass the `err` into the Handler because it will be assumed to be a userErr. Perhaps `api.HandleErr` is a bit too general handling all types of errors. I might argue for a `api.HandleUserErr` and a `api.HandleServerErr`. But in any case, for the time being we want to pass `nil` along instead.
   ```
   api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New(r.RemoteAddr+" making CRConfig: "+err.Error()))
   ```




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