You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/01/30 17:18:50 UTC

[GitHub] [trafficcontrol] zrhoffman opened a new pull request #4357: Document and deprecate /regions/name/{{name}}

zrhoffman opened a new pull request #4357: Document and deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357
 
 
   <!--
   ************ STOP!! ************
   If this Pull Request is intended to fix a security vulnerability, DO NOT submit it! Instead, contact
   the Apache Software Foundation Security Team at security@trafficcontrol.apache.org and follow the
   guidelines at https://www.apache.org/security/ regarding vulnerability disclosure.
   -->
   ## What does this PR (Pull Request) do?
   <!-- Explain the changes you made here. If this fixes an Issue, identify it by
   replacing the text in the checkbox item with the Issue number e.g.
   
   - [x] This PR fixes #9001 OR is not related to any Issue
   
   ^ This will automatically close Issue number 9001 when the Pull Request is
   merged (The '#' is important).
   
   Be sure you check the box properly, see the "The following criteria are ALL
   met by this PR" section for details.
   -->
   
   - [x] This PR fixes #3819 <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
   
   This PR deprecates and documents the following endpoints:
   
   * GET `regions/name/{{name}}`
   * DELETE `regions/name/{{name}}`
   
   This PR also sorts the changelog's list of deprecated/removed endpoints for easier reading.
   
   ## Which Traffic Control components are affected by this PR?
   <!-- Please delete all components from this list that are NOT affected by this
   Pull Request. Also, feel free to add the name of a tool or script that is
   affected but not on the list.
   
   Additionally, if this Pull Request does NOT affect documentation, please
   explain why documentation is not required. -->
   
   - Documentation
   - Traffic Ops
   
   Tests are not included because these endpoints are deprecated and `v1.GetTestRegionsByNamePath()` already tests the GET method.
   
   ## What is the best way to verify this PR?
   <!-- Please include here ALL the steps necessary to test your Pull Request. If
   it includes tests (and most should), outline here the steps needed to run the
   tests. If not, lay out the manual testing procedure and please explain why
   tests are unnecessary for this Pull Request. -->
   * Run the Traffic Ops API tests
   * Build the documentation
   
   ## If this is a bug fix, what versions of Traffic Control are affected?
   <!-- If this PR fixes a bug, please list here all of the affected versions - to
   the best of your knowledge. It's also pretty helpful to include a commit hash
   of where 'master' is at the time this PR is opened (if it affects master),
   because what 'master' means will change over time. For example, if this PR
   fixes a bug that's present in master (at commit hash '2697ebac'), in v3.0.0,
   and in the current 3.0.1 Release candidate (e.g. RC1), then this list would
   look like:
   
   - master (2697ebac)
   - 3.0.0
   - 3.0.1 (RC1)
   
   If you don't know what other versions might have this bug, AND don't know how
   to find the commit hash of 'master', then feel free to leave this section
   blank (or, preferably, delete it entirely).
    -->
   - master (a14d3d4544)
   - 3.1.0
   - 4.0.0 (RC2)
   
   
   ## The following criteria are ALL met by this PR
   <!-- Check the boxes to signify that the associated statement is true. To
   "check a box", replace the space inside of the square brackets with an 'x'.
   e.g.
   
   - [ x] <- Wrong
   - [x ] <- Wrong
   - [] <- Wrong
   - [*] <- Wrong
   - [x] <- Correct!
   
   -->
   
   - [x] This PR includes tests OR I have explained why tests are unnecessary
   - [x] This PR includes documentation OR I have explained why documentation is unnecessary
   - [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
   - [x] This PR includes any and all required license headers
   - [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
   
   
   ## Additional Information
   <!-- If you would like to include any additional information on the PR for
   potential reviewers please put it here.
   
   Some examples of this would be:
   
   - Before and after screenshots/gifs of the Traffic Portal if it is affected
   - Links to other dependent Pull Requests
   - References to relevant context (e.g. new/updates to dependent libraries,
   mailing list records, blueprints)
   
   Feel free to leave this section blank (or, preferably, delete it entirely).
   -->
   
   <!--
   Licensed to the Apache Software Foundation (ASF) under one
   or more contributor license agreements.  See the NOTICE file
   distributed with this work for additional information
   regarding copyright ownership.  The ASF licenses this file
   to you under the Apache License, Version 2.0 (the
   "License"); you may not use this file except in compliance
   with the License.  You may obtain a copy of the License at
   
       http://www.apache.org/licenses/LICENSE-2.0
   
   Unless required by applicable law or agreed to in writing,
   software distributed under the License is distributed on an
   "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
   KIND, either express or implied.  See the License for the
   specific language governing permissions and limitations
   under the License.
   -->

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on issue #4357: Document and deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on issue #4357: Document and deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#issuecomment-580446451
 
 
   @ocket8888 thoughts ^ 
   

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377339949
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/routing/routes.go
 ##########
 @@ -301,6 +301,8 @@ func Routes(d ServerData) ([]Route, []RawRoute, http.Handler, error) {
 		{1.1, http.MethodGet, `regions/name/{name}/?(\.json)?$`, region.GetName, auth.PrivLevelReadOnly, Authenticated, nil, 503583197, noPerlBypass},
 		{1.1, http.MethodPut, `regions/{id}$`, api.UpdateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 226308224, noPerlBypass},
 		{1.1, http.MethodPost, `regions/?$`, api.CreateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 1288334488, noPerlBypass},
+		{1.5, http.MethodDelete, `regions/?$`, api.DeleteHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 2032626758, noPerlBypass},
+		{1.1, http.MethodDelete, `regions/name/{name}$`, handlerToFunc(proxyHandler), auth.PrivLevelReadOnly, Authenticated, nil, 1925881096, noPerlBypass},
 
 Review comment:
   That makes sense, changed in c947d23d9.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377163719
 
 

 ##########
 File path: traffic_ops/app/lib/TrafficOpsRoutes.pm
 ##########
 @@ -745,7 +745,7 @@ sub api_routes {
 	$r->put("/api/$version/regions/:id" => [ id => qr/\d+/ ] )->over( authenticated => 1, not_ldap => 1 )->to( 'Region#update', namespace => $namespace );
 	$r->post("/api/$version/regions")->over( authenticated => 1, not_ldap => 1 )->to( 'Region#create', namespace => $namespace );
 	$r->post("/api/$version/divisions/:division_name/regions")->over( authenticated => 1, not_ldap => 1 )->to( 'Region#create_for_division', namespace => $namespace );
-	$r->delete("/api/$version/regions/:id" => [ id => qr/\d+/ ] )->over( authenticated => 1, not_ldap => 1 )->to( 'Region#delete', namespace => $namespace );
+	$r->delete("/api/$version/regions/:id" => [ id => qr/\d+/ ] )->over( authenticated => 1, not_ldap => 1 )->to( 'Region#delete_by_id', namespace => $namespace );
 	$r->delete("/api/$version/regions/name/:name")->over( authenticated => 1, not_ldap => 1 )->to( 'Region#delete_by_name', namespace => $namespace );
 
 Review comment:
   I think maybe renaming the sub in Region.pm like you did will break this handler. I haven't tested yet, but it seems likely.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376741178
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/api/shared_handlers.go
 ##########
 @@ -298,23 +298,36 @@ func DeleteHandler(deleter Deleter) http.HandlerFunc {
 		obj := reflect.New(objectType).Interface().(Deleter)
 		obj.SetInfo(inf)
 
-		keyFields := obj.GetKeyFieldsInfo() // expecting a slice of the key fields info which is a struct with the field name and a function to convert a string into a interface{} of the right type. in most that will be [{Field:"id",Func: func(s string)(interface{},error){return strconv.Atoi(s)}}]
-		keys := make(map[string]interface{})
-		for _, kf := range keyFields {
-			paramKey := inf.Params[kf.Field]
-			if paramKey == "" {
-				HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("missing key: "+kf.Field), nil)
-				return
+		deleteKeyOptionExists := false
+		if d, ok := obj.(HasDeleteKeyOptions); ok {
+			options := d.DeleteKeyOptions()
+			for key, _ := range options {
+				if inf.Params[key] != "" {
+					deleteKeyOptionExists = true
+					break
+				}
 			}
 
 Review comment:
   Made specifying a delete key in 2ada94e8f2.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on issue #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on issue #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#issuecomment-583573817
 
 
   Okay, now we have Go handlers for both `/regions/name/{{name}}` (proxyHandler, 6e63cde165), which is deprecated, and `/regions?name={{name}}` (d3353a9b46).

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377167328
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/region/regions.go
 ##########
 @@ -44,8 +44,13 @@ func (v *TORegion) ParamColumns() map[string]dbhelpers.WhereColumnInfo {
 		"id":       dbhelpers.WhereColumnInfo{"r.id", api.IsInt},
 	}
 }
-func (v *TORegion) UpdateQuery() string { return updateQuery() }
-func (v *TORegion) DeleteQuery() string { return deleteQuery() }
+func (v *TORegion) UpdateQuery() string     { return updateQuery() }
+
+// DeleteQuery returns a query, including a WHERE clause
 
 Review comment:
   nit: should end in a period

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376573215
 
 

 ##########
 File path: traffic_ops/app/lib/API/Region.pm
 ##########
 @@ -244,21 +244,22 @@ sub delete {
 sub delete_by_name {
 	my $self = shift;
 	my $name     = $self->param('name');
+	my $alt		 = 'DELETE /regions/{{ID}}';
 
 Review comment:
   should be delete by name for alternative 

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377845840
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/routing/routes.go
 ##########
 @@ -301,6 +301,8 @@ func Routes(d ServerData) ([]Route, []RawRoute, http.Handler, error) {
 		{1.1, http.MethodGet, `regions/name/{name}/?(\.json)?$`, region.GetName, auth.PrivLevelReadOnly, Authenticated, nil, 503583197, noPerlBypass},
 		{1.1, http.MethodPut, `regions/{id}$`, api.UpdateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 226308224, noPerlBypass},
 		{1.1, http.MethodPost, `regions/?$`, api.CreateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 1288334488, noPerlBypass},
+		{1.5, http.MethodDelete, `regions/?$`, api.DeleteHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 2032626758, noPerlBypass},
+		{1.1, http.MethodDelete, `regions/name/{name}$`, handlerToFunc(proxyHandler), auth.PrivLevelReadOnly, Authenticated, nil, 1925881096, noPerlBypass},
 
 Review comment:
   Yeah I agree, seems like a fine way to ensure the deprecation notice. It saves him from needing to write a wrapper that inspects the request path.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376582076
 
 

 ##########
 File path: traffic_ops/client/region.go
 ##########
 @@ -159,3 +160,28 @@ func (to *Session) GetRegionByNamePath(name string) ([]tc.RegionName, ReqInf, er
 	}
 	return resp.Response, reqInf, nil
 }
+
+// DELETE a Region
+func (to *Session) DeleteRegionByName(id *int, name *string) (tc.Alerts, ReqInf, error) {
 
 Review comment:
   godoc is off from name of func and nit but I wonder if this should be called DeleteRegion as you can delete it by id or name

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on issue #4357: WIP: Document and deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on issue #4357: WIP: Document and deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#issuecomment-580460935
 
 
   Got it, I will write a  DELETE handler for `/regions/name/{{name}}` but keep it deprecated and add a DELETE handler for `/regions?name={{name}}`.
   
   Title changed to WIP for now.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377853883
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/routing/routes.go
 ##########
 @@ -301,6 +301,8 @@ func Routes(d ServerData) ([]Route, []RawRoute, http.Handler, error) {
 		{1.1, http.MethodGet, `regions/name/{name}/?(\.json)?$`, region.GetName, auth.PrivLevelReadOnly, Authenticated, nil, 503583197, noPerlBypass},
 		{1.1, http.MethodPut, `regions/{id}$`, api.UpdateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 226308224, noPerlBypass},
 		{1.1, http.MethodPost, `regions/?$`, api.CreateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 1288334488, noPerlBypass},
+		{1.5, http.MethodDelete, `regions/?$`, api.DeleteHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 2032626758, noPerlBypass},
+		{1.1, http.MethodDelete, `regions/name/{name}$`, handlerToFunc(proxyHandler), auth.PrivLevelReadOnly, Authenticated, nil, 1925881096, noPerlBypass},
 
 Review comment:
   It's not, and the new handler can delete by either `id` or `name`. However, if we wanted to handle it in Go we would need to add a way for that handler to receive arbitrary alerts. The only shared handler I see that can already do that is `DeprecatedReadHandler()`, so doing that here would mean making a completely new DELETE handler just for this case.
   
   Of course, it would be easier to just make a `region`-specific handler. In this case, though, the new handler cannot be used for a deprecated route.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4357: Document and deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4357: Document and deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r373090421
 
 

 ##########
 File path: docs/source/api/regions_name_name.rst
 ##########
 @@ -0,0 +1,153 @@
+..
 
 Review comment:
   Yeah, there's no point in doing that since we don't want people to use them. It's a waste of time, but providing the docs are well-formed and accurate I wouldn't hold up a PR because of adding docs, IMO. Reviewer's discretion, I suppose.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Document and deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Document and deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r373084819
 
 

 ##########
 File path: docs/source/api/regions_name_name.rst
 ##########
 @@ -0,0 +1,153 @@
+..
 
 Review comment:
   if an endpoint is being deprecated we shouldnt/dont need to add documentation around it 

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377164919
 
 

 ##########
 File path: traffic_ops/client/region.go
 ##########
 @@ -159,3 +160,28 @@ func (to *Session) GetRegionByNamePath(name string) ([]tc.RegionName, ReqInf, er
 	}
 	return resp.Response, reqInf, nil
 }
+
+// DeleteRegion lets you DELETE a Region
+func (to *Session) DeleteRegion(id *int, name *string) (tc.Alerts, ReqInf, error) {
+	v := url.Values{}
+	if id != nil {
+		v.Add("id", strconv.Itoa(*id))
+	}
+	if name != nil {
+		v.Add("name", *name)
+	}
+	URI := apiBase + "/regions"
+	if qStr := v.Encode(); len(qStr) > 0 {
+		URI = fmt.Sprintf("%s?%s", URI, qStr)
+	}
+
+	resp, remoteAddr, err := to.request(http.MethodDelete, URI, nil)
+	reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr}
+	if err != nil {
+		return tc.Alerts{}, reqInf, err
+	}
+	defer resp.Body.Close()
+	var alerts tc.Alerts
+	err = json.NewDecoder(resp.Body).Decode(&alerts)
+	return alerts, reqInf, nil
 
 Review comment:
   You seem to be ignoring the parse error on the line above; should `return alerts, reqInf, err` instead of `return alerts, reqInf, nil`

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376741159
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/api/generic_crud.go
 ##########
 @@ -156,6 +166,37 @@ func GenericUpdate(val GenericUpdater) (error, error, int) {
 	return nil, nil, http.StatusOK
 }
 
+// GenericOptionsDelete does a Delete (DELETE) for the given GenericOptionsDeleter object and type. Unlike
+// GenericDelete, there is no requirement that a specific key is used as the parameter.
+// GenericOptionsDeleter.DeleteKeyOptions() specifies which keys can be used.
+func GenericOptionsDelete(val GenericOptionsDeleter) (error, error, int) {
+	where, _, _, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(val.APIInfo().Params, val.DeleteKeyOptions())
+	if len(errs) > 0 {
+		return util.JoinErrs(errs), nil, http.StatusBadRequest
+	}
+
+	query := val.DeleteQueryBase() + where
+	tx := val.APIInfo().Tx
+	result, err := tx.NamedExec(query, queryValues)
+	if err != nil {
+		return nil, errors.New("deleting " + val.GetType() + ": " + err.Error()), http.StatusInternalServerError
+	}
+
+	if rowsAffected, err := result.RowsAffected(); err != nil {
+		return nil, errors.New("deleting " + val.GetType() + ": getting rows affected: " + err.Error()), http.StatusInternalServerError
+	} else if rowsAffected < 1 {
+		return errors.New("no " + val.GetType() + " with that key found"), nil, http.StatusNotFound
+	} else if rowsAffected > 1 {
+		err := tx.Rollback()
 
 Review comment:
   Right you are, removed in 2ada94e8f2.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376584072
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/api/generic_crud.go
 ##########
 @@ -156,6 +166,37 @@ func GenericUpdate(val GenericUpdater) (error, error, int) {
 	return nil, nil, http.StatusOK
 }
 
+// GenericOptionsDelete does a Delete (DELETE) for the given GenericOptionsDeleter object and type. Unlike
+// GenericDelete, there is no requirement that a specific key is used as the parameter.
+// GenericOptionsDeleter.DeleteKeyOptions() specifies which keys can be used.
+func GenericOptionsDelete(val GenericOptionsDeleter) (error, error, int) {
+	where, _, _, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(val.APIInfo().Params, val.DeleteKeyOptions())
+	if len(errs) > 0 {
+		return util.JoinErrs(errs), nil, http.StatusBadRequest
+	}
+
+	query := val.DeleteQueryBase() + where
+	tx := val.APIInfo().Tx
+	result, err := tx.NamedExec(query, queryValues)
+	if err != nil {
+		return nil, errors.New("deleting " + val.GetType() + ": " + err.Error()), http.StatusInternalServerError
 
 Review comment:
   should this be ParseDBError? that parses some postgres errors into non 500 errors 

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376586379
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/api/shared_handlers.go
 ##########
 @@ -298,23 +298,36 @@ func DeleteHandler(deleter Deleter) http.HandlerFunc {
 		obj := reflect.New(objectType).Interface().(Deleter)
 		obj.SetInfo(inf)
 
-		keyFields := obj.GetKeyFieldsInfo() // expecting a slice of the key fields info which is a struct with the field name and a function to convert a string into a interface{} of the right type. in most that will be [{Field:"id",Func: func(s string)(interface{},error){return strconv.Atoi(s)}}]
-		keys := make(map[string]interface{})
-		for _, kf := range keyFields {
-			paramKey := inf.Params[kf.Field]
-			if paramKey == "" {
-				HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("missing key: "+kf.Field), nil)
-				return
+		deleteKeyOptionExists := false
+		if d, ok := obj.(HasDeleteKeyOptions); ok {
+			options := d.DeleteKeyOptions()
+			for key, _ := range options {
+				if inf.Params[key] != "" {
+					deleteKeyOptionExists = true
+					break
+				}
 			}
 
 Review comment:
   should this error as they said it has delete key options but none was specified in the params?

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376741051
 
 

 ##########
 File path: traffic_ops/client/region.go
 ##########
 @@ -159,3 +160,28 @@ func (to *Session) GetRegionByNamePath(name string) ([]tc.RegionName, ReqInf, er
 	}
 	return resp.Response, reqInf, nil
 }
+
+// DELETE a Region
+func (to *Session) DeleteRegionByName(id *int, name *string) (tc.Alerts, ReqInf, error) {
+	v := url.Values{}
+	if id != nil {
+		v.Add("id", strconv.Itoa(*id))
+	}
+	if name != nil {
+		v.Add("name", *name)
+	}
+	URI := API_v13_REGIONS
 
 Review comment:
   Changed to use `apiBase` in 2ada94e8f2.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377339796
 
 

 ##########
 File path: traffic_ops/app/lib/TrafficOpsRoutes.pm
 ##########
 @@ -745,7 +745,7 @@ sub api_routes {
 	$r->put("/api/$version/regions/:id" => [ id => qr/\d+/ ] )->over( authenticated => 1, not_ldap => 1 )->to( 'Region#update', namespace => $namespace );
 	$r->post("/api/$version/regions")->over( authenticated => 1, not_ldap => 1 )->to( 'Region#create', namespace => $namespace );
 	$r->post("/api/$version/divisions/:division_name/regions")->over( authenticated => 1, not_ldap => 1 )->to( 'Region#create_for_division', namespace => $namespace );
-	$r->delete("/api/$version/regions/:id" => [ id => qr/\d+/ ] )->over( authenticated => 1, not_ldap => 1 )->to( 'Region#delete', namespace => $namespace );
+	$r->delete("/api/$version/regions/:id" => [ id => qr/\d+/ ] )->over( authenticated => 1, not_ldap => 1 )->to( 'Region#delete_by_id', namespace => $namespace );
 	$r->delete("/api/$version/regions/name/:name")->over( authenticated => 1, not_ldap => 1 )->to( 'Region#delete_by_name', namespace => $namespace );
 
 Review comment:
   I can't reproduce the behavior that motivated me to change it. Reverted in 2edc8f445.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376585162
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/api/generic_crud.go
 ##########
 @@ -156,6 +166,37 @@ func GenericUpdate(val GenericUpdater) (error, error, int) {
 	return nil, nil, http.StatusOK
 }
 
+// GenericOptionsDelete does a Delete (DELETE) for the given GenericOptionsDeleter object and type. Unlike
+// GenericDelete, there is no requirement that a specific key is used as the parameter.
+// GenericOptionsDeleter.DeleteKeyOptions() specifies which keys can be used.
+func GenericOptionsDelete(val GenericOptionsDeleter) (error, error, int) {
+	where, _, _, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(val.APIInfo().Params, val.DeleteKeyOptions())
+	if len(errs) > 0 {
+		return util.JoinErrs(errs), nil, http.StatusBadRequest
+	}
+
+	query := val.DeleteQueryBase() + where
+	tx := val.APIInfo().Tx
+	result, err := tx.NamedExec(query, queryValues)
+	if err != nil {
+		return nil, errors.New("deleting " + val.GetType() + ": " + err.Error()), http.StatusInternalServerError
+	}
+
+	if rowsAffected, err := result.RowsAffected(); err != nil {
+		return nil, errors.New("deleting " + val.GetType() + ": getting rows affected: " + err.Error()), http.StatusInternalServerError
+	} else if rowsAffected < 1 {
+		return errors.New("no " + val.GetType() + " with that key found"), nil, http.StatusNotFound
+	} else if rowsAffected > 1 {
+		err := tx.Rollback()
 
 Review comment:
   this should be handled by the HandleError -> https://github.com/apache/trafficcontrol/blob/master/traffic_ops/traffic_ops_golang/api/api.go#L131 

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377339729
 
 

 ##########
 File path: docs/source/api/regions.rst
 ##########
 @@ -165,3 +165,63 @@ Response Structure
 		"lastUpdated": "2018-12-06 02:14:45+00",
 		"name": "Manchester"
 	}}
+
+``DELETE``
+==========
+
+.. versionadded:: 1.5
+
+Deletes a region. If no query parameter is specified, a ``400 Bad Request`` response is returned.
+
+:Auth. Required: Yes
+:Roles Required: "admin" or "operations"
+:Response Type:  Object
+
+Request Structure
+-----------------
+
+.. table:: Request Query Parameters
+
+	+-----------+----------+---------------------------------------------------------------------------------------------------------------+
+	| Name      | Required | Description                                                                                                   |
+	+===========+==========+===============================================================================================================+
+	| id        | no       | Delete :term:`Region` by integral, unique identifier                                                          |
+	+-----------+----------+---------------------------------------------------------------------------------------------------------------+
+	| name      | no       | Delete :term:`Region` by name                                                                                 |
+	+-----------+----------+---------------------------------------------------------------------------------------------------------------+
+
+.. code-block:: http
+	:caption: Request Example
+
+	DELETE /api/1.5/regions?name=Manchester HTTP/1.1
+	User-Agent: curl/7.29.0
+	Host: trafficops.infra.ciab.test
+	Accept: */*
+	Cookie: mojolicious=...
+
+Response Structure
+------------------
+
+.. code-block:: http
+	:caption: Response Example
+
+	HTTP/1.1 200 OK
+	Access-Control-Allow-Credentials: true
+	Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Accept, Set-Cookie, Cookie
+	Access-Control-Allow-Methods: POST,GET,OPTIONS,PUT,DELETE
+	Access-Control-Allow-Origin: *
+	Content-Type: application/json
+	Set-Cookie: mojolicious=eyJhdXRoX2RhdGEiOiJhZG1pbiIsImV4cGlyZXMiOjE1ODEwODM3ODQsImJ5IjoidHJhZmZpY2NvbnRyb2wtZ28tdG9jb29raWUifQ--f6b59c9dde67bcd545bde3bbb4e56b33e23827e9; Path=/; Expires=Fri, 07 Feb 2020 13:56:24 GMT; Max-Age=3600; HttpOnly
 
 Review comment:
   Ack, I missed one. Fixed in 820659e14.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] ocket8888 merged pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357
 
 
   

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Document and deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Document and deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r373089188
 
 

 ##########
 File path: docs/source/api/regions_name_name.rst
 ##########
 @@ -0,0 +1,153 @@
+..
 
 Review comment:
   Okay, rebased and documentation removed in [`f22b0e9c3d~`...`c954e8776b`](https://github.com/apache/trafficcontrol/compare/f22b0e9c3d~...c954e8776b).

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377168578
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/routing/routes.go
 ##########
 @@ -301,6 +301,8 @@ func Routes(d ServerData) ([]Route, []RawRoute, http.Handler, error) {
 		{1.1, http.MethodGet, `regions/name/{name}/?(\.json)?$`, region.GetName, auth.PrivLevelReadOnly, Authenticated, nil, 503583197, noPerlBypass},
 		{1.1, http.MethodPut, `regions/{id}$`, api.UpdateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 226308224, noPerlBypass},
 		{1.1, http.MethodPost, `regions/?$`, api.CreateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 1288334488, noPerlBypass},
+		{1.5, http.MethodDelete, `regions/?$`, api.DeleteHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 2032626758, noPerlBypass},
+		{1.1, http.MethodDelete, `regions/name/{name}$`, handlerToFunc(proxyHandler), auth.PrivLevelReadOnly, Authenticated, nil, 1925881096, noPerlBypass},
 
 Review comment:
   I think normally routes that use the `proxyHandler` have `NoAuth` because the assumption is that the Perl server will handle authentication.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376582808
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/routing/routes.go
 ##########
 @@ -301,6 +301,8 @@ func Routes(d ServerData) ([]Route, []RawRoute, http.Handler, error) {
 		{1.1, http.MethodGet, `regions/name/{name}/?(\.json)?$`, region.GetName, auth.PrivLevelReadOnly, Authenticated, nil, 503583197, noPerlBypass},
 		{1.1, http.MethodPut, `regions/{id}$`, api.UpdateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 226308224, noPerlBypass},
 		{1.1, http.MethodPost, `regions/?$`, api.CreateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 1288334488, noPerlBypass},
+		{1.1, http.MethodDelete, `regions/?$`, api.DeleteHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 2032626758, noPerlBypass},
 
 Review comment:
   this should be version 1.5

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377847576
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/routing/routes.go
 ##########
 @@ -301,6 +301,8 @@ func Routes(d ServerData) ([]Route, []RawRoute, http.Handler, error) {
 		{1.1, http.MethodGet, `regions/name/{name}/?(\.json)?$`, region.GetName, auth.PrivLevelReadOnly, Authenticated, nil, 503583197, noPerlBypass},
 		{1.1, http.MethodPut, `regions/{id}$`, api.UpdateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 226308224, noPerlBypass},
 		{1.1, http.MethodPost, `regions/?$`, api.CreateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 1288334488, noPerlBypass},
+		{1.5, http.MethodDelete, `regions/?$`, api.DeleteHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 2032626758, noPerlBypass},
+		{1.1, http.MethodDelete, `regions/name/{name}$`, handlerToFunc(proxyHandler), auth.PrivLevelReadOnly, Authenticated, nil, 1925881096, noPerlBypass},
 
 Review comment:
   Ok, I guess usually the routes are just left out so be handled by the "route not found handler" which right now is "check TO-Perl". _Explicit_ `proxyHandler` routes are usually only there to handle requests explicitly before they're unintentionally handled by a different route (i.e. GET /cdns/foo being unintentionally handled by GET /cdns because the request matches it). That doesn't seem to be the case here, right?

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376573215
 
 

 ##########
 File path: traffic_ops/app/lib/API/Region.pm
 ##########
 @@ -244,21 +244,22 @@ sub delete {
 sub delete_by_name {
 	my $self = shift;
 	my $name     = $self->param('name');
+	my $alt		 = 'DELETE /regions/{{ID}}';
 
 Review comment:
   should be delete by name for alternative 

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376741063
 
 

 ##########
 File path: docs/source/api/regions.rst
 ##########
 @@ -165,3 +165,61 @@ Response Structure
 		"lastUpdated": "2018-12-06 02:14:45+00",
 		"name": "Manchester"
 	}}
+
+``DELETE``
+==========
+
+Deletes a region. If no query parameter is specified, a ``400 Bad Request`` response is returned.
 
 Review comment:
   Marked in 2ada94e8f2.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377843602
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/routing/routes.go
 ##########
 @@ -301,6 +301,8 @@ func Routes(d ServerData) ([]Route, []RawRoute, http.Handler, error) {
 		{1.1, http.MethodGet, `regions/name/{name}/?(\.json)?$`, region.GetName, auth.PrivLevelReadOnly, Authenticated, nil, 503583197, noPerlBypass},
 		{1.1, http.MethodPut, `regions/{id}$`, api.UpdateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 226308224, noPerlBypass},
 		{1.1, http.MethodPost, `regions/?$`, api.CreateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 1288334488, noPerlBypass},
+		{1.5, http.MethodDelete, `regions/?$`, api.DeleteHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 2032626758, noPerlBypass},
+		{1.1, http.MethodDelete, `regions/name/{name}$`, handlerToFunc(proxyHandler), auth.PrivLevelReadOnly, Authenticated, nil, 1925881096, noPerlBypass},
 
 Review comment:
   well this is just an explicit proxy to the deprecated handler in the perl, either way that route is deprecated and the notices were added in the perl implementation I didnt see any harm in having this in here

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376741126
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/region/name.go
 ##########
 @@ -35,7 +35,13 @@ func GetName(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	defer inf.Close()
-	api.RespWriter(w, r, inf.Tx.Tx)(getName(inf.Tx.Tx, inf.Params["name"]))
+
+	regionNames, err := getName(inf.Tx.Tx, inf.Params["name"])
+	if err != nil {
+		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("getting region: "+err.Error()))
+		return
+	}
+	api.WriteAlertsObj(w, r, http.StatusOK, tc.Alerts{Alerts: []tc.Alert{api.DeprecationWarning("GET /regions?name={{name}}")}}, regionNames)
 
 Review comment:
   Add deprecation notices to the errors in 2ada94e8f2.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377339864
 
 

 ##########
 File path: traffic_ops/client/region.go
 ##########
 @@ -159,3 +160,28 @@ func (to *Session) GetRegionByNamePath(name string) ([]tc.RegionName, ReqInf, er
 	}
 	return resp.Response, reqInf, nil
 }
+
+// DeleteRegion lets you DELETE a Region
+func (to *Session) DeleteRegion(id *int, name *string) (tc.Alerts, ReqInf, error) {
+	v := url.Values{}
+	if id != nil {
+		v.Add("id", strconv.Itoa(*id))
+	}
+	if name != nil {
+		v.Add("name", *name)
+	}
+	URI := apiBase + "/regions"
+	if qStr := v.Encode(); len(qStr) > 0 {
+		URI = fmt.Sprintf("%s?%s", URI, qStr)
+	}
+
+	resp, remoteAddr, err := to.request(http.MethodDelete, URI, nil)
+	reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr}
+	if err != nil {
+		return tc.Alerts{}, reqInf, err
+	}
+	defer resp.Body.Close()
+	var alerts tc.Alerts
+	err = json.NewDecoder(resp.Body).Decode(&alerts)
+	return alerts, reqInf, nil
 
 Review comment:
   Returning the error now in 820659e14.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376619012
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/api/shared_handlers.go
 ##########
 @@ -298,23 +298,36 @@ func DeleteHandler(deleter Deleter) http.HandlerFunc {
 		obj := reflect.New(objectType).Interface().(Deleter)
 		obj.SetInfo(inf)
 
-		keyFields := obj.GetKeyFieldsInfo() // expecting a slice of the key fields info which is a struct with the field name and a function to convert a string into a interface{} of the right type. in most that will be [{Field:"id",Func: func(s string)(interface{},error){return strconv.Atoi(s)}}]
-		keys := make(map[string]interface{})
-		for _, kf := range keyFields {
-			paramKey := inf.Params[kf.Field]
-			if paramKey == "" {
-				HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("missing key: "+kf.Field), nil)
-				return
+		deleteKeyOptionExists := false
+		if d, ok := obj.(HasDeleteKeyOptions); ok {
+			options := d.DeleteKeyOptions()
+			for key, _ := range options {
+				if inf.Params[key] != "" {
+					deleteKeyOptionExists = true
+					break
+				}
 			}
 
 Review comment:
   Having that error makes sense to me.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376581228
 
 

 ##########
 File path: traffic_ops/client/region.go
 ##########
 @@ -159,3 +160,28 @@ func (to *Session) GetRegionByNamePath(name string) ([]tc.RegionName, ReqInf, er
 	}
 	return resp.Response, reqInf, nil
 }
+
+// DELETE a Region
+func (to *Session) DeleteRegionByName(id *int, name *string) (tc.Alerts, ReqInf, error) {
+	v := url.Values{}
+	if id != nil {
+		v.Add("id", strconv.Itoa(*id))
+	}
+	if name != nil {
+		v.Add("name", *name)
+	}
+	URI := API_v13_REGIONS
 
 Review comment:
   could we use ```apiBase + "/regions"``` instead? 

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376581395
 
 

 ##########
 File path: docs/source/api/regions.rst
 ##########
 @@ -165,3 +165,61 @@ Response Structure
 		"lastUpdated": "2018-12-06 02:14:45+00",
 		"name": "Manchester"
 	}}
+
+``DELETE``
+==========
+
+Deletes a region. If no query parameter is specified, a ``400 Bad Request`` response is returned.
 
 Review comment:
   we should have this API marked as version added 1.5 

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376582549
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/region/name.go
 ##########
 @@ -35,7 +35,13 @@ func GetName(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	defer inf.Close()
-	api.RespWriter(w, r, inf.Tx.Tx)(getName(inf.Tx.Tx, inf.Params["name"]))
+
+	regionNames, err := getName(inf.Tx.Tx, inf.Params["name"])
+	if err != nil {
+		api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("getting region: "+err.Error()))
+		return
+	}
+	api.WriteAlertsObj(w, r, http.StatusOK, tc.Alerts{Alerts: []tc.Alert{api.DeprecationWarning("GET /regions?name={{name}}")}}, regionNames)
 
 Review comment:
   this doesnt have deprecation notices on the errors 

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377857377
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/routing/routes.go
 ##########
 @@ -301,6 +301,8 @@ func Routes(d ServerData) ([]Route, []RawRoute, http.Handler, error) {
 		{1.1, http.MethodGet, `regions/name/{name}/?(\.json)?$`, region.GetName, auth.PrivLevelReadOnly, Authenticated, nil, 503583197, noPerlBypass},
 		{1.1, http.MethodPut, `regions/{id}$`, api.UpdateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 226308224, noPerlBypass},
 		{1.1, http.MethodPost, `regions/?$`, api.CreateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 1288334488, noPerlBypass},
+		{1.5, http.MethodDelete, `regions/?$`, api.DeleteHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 2032626758, noPerlBypass},
+		{1.1, http.MethodDelete, `regions/name/{name}$`, handlerToFunc(proxyHandler), auth.PrivLevelReadOnly, Authenticated, nil, 1925881096, noPerlBypass},
 
 Review comment:
   The reason I made it an explicit `proxyHandler` route was to keep `routes.go` useful as a reference for which routes exist. It would be super easy to remove, though.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on issue #4357: WIP: Document and deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on issue #4357: WIP: Document and deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#issuecomment-580460000
 
 
   okay I am fine either way as along as there is a path to delete regions by name 🤷‍♂ 

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377339905
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/region/regions.go
 ##########
 @@ -44,8 +44,13 @@ func (v *TORegion) ParamColumns() map[string]dbhelpers.WhereColumnInfo {
 		"id":       dbhelpers.WhereColumnInfo{"r.id", api.IsInt},
 	}
 }
-func (v *TORegion) UpdateQuery() string { return updateQuery() }
-func (v *TORegion) DeleteQuery() string { return deleteQuery() }
+func (v *TORegion) UpdateQuery() string     { return updateQuery() }
+
+// DeleteQuery returns a query, including a WHERE clause
 
 Review comment:
   Period added in 6e1617fae.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376740999
 
 

 ##########
 File path: traffic_ops/app/lib/API/Region.pm
 ##########
 @@ -244,21 +244,22 @@ sub delete {
 sub delete_by_name {
 	my $self = shift;
 	my $name     = $self->param('name');
+	my $alt		 = 'DELETE /regions/{{ID}}';
 
 	if ( !&is_oper($self) ) {
 		return $self->forbidden();
 
 Review comment:
   Add a deprecation notice in 2ada94e8f2.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376741107
 
 

 ##########
 File path: traffic_ops/client/region.go
 ##########
 @@ -159,3 +160,28 @@ func (to *Session) GetRegionByNamePath(name string) ([]tc.RegionName, ReqInf, er
 	}
 	return resp.Response, reqInf, nil
 }
+
+// DELETE a Region
+func (to *Session) DeleteRegionByName(id *int, name *string) (tc.Alerts, ReqInf, error) {
 
 Review comment:
   Fixed GoDoc and renamed in 2ada94e8f2.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376741134
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/api/generic_crud.go
 ##########
 @@ -156,6 +166,37 @@ func GenericUpdate(val GenericUpdater) (error, error, int) {
 	return nil, nil, http.StatusOK
 }
 
+// GenericOptionsDelete does a Delete (DELETE) for the given GenericOptionsDeleter object and type. Unlike
+// GenericDelete, there is no requirement that a specific key is used as the parameter.
+// GenericOptionsDeleter.DeleteKeyOptions() specifies which keys can be used.
+func GenericOptionsDelete(val GenericOptionsDeleter) (error, error, int) {
+	where, _, _, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(val.APIInfo().Params, val.DeleteKeyOptions())
+	if len(errs) > 0 {
+		return util.JoinErrs(errs), nil, http.StatusBadRequest
+	}
+
+	query := val.DeleteQueryBase() + where
+	tx := val.APIInfo().Tx
+	result, err := tx.NamedExec(query, queryValues)
+	if err != nil {
+		return nil, errors.New("deleting " + val.GetType() + ": " + err.Error()), http.StatusInternalServerError
 
 Review comment:
   Changed to ParseDBError in 2ada94e8f2.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377339827
 
 

 ##########
 File path: traffic_ops/client/region.go
 ##########
 @@ -159,3 +160,28 @@ func (to *Session) GetRegionByNamePath(name string) ([]tc.RegionName, ReqInf, er
 	}
 	return resp.Response, reqInf, nil
 }
+
+// DeleteRegion lets you DELETE a Region
 
 Review comment:
   Added a period and explanation in 6e1617fae.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377842068
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/routing/routes.go
 ##########
 @@ -301,6 +301,8 @@ func Routes(d ServerData) ([]Route, []RawRoute, http.Handler, error) {
 		{1.1, http.MethodGet, `regions/name/{name}/?(\.json)?$`, region.GetName, auth.PrivLevelReadOnly, Authenticated, nil, 503583197, noPerlBypass},
 		{1.1, http.MethodPut, `regions/{id}$`, api.UpdateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 226308224, noPerlBypass},
 		{1.1, http.MethodPost, `regions/?$`, api.CreateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 1288334488, noPerlBypass},
+		{1.5, http.MethodDelete, `regions/?$`, api.DeleteHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 2032626758, noPerlBypass},
+		{1.1, http.MethodDelete, `regions/name/{name}$`, handlerToFunc(proxyHandler), auth.PrivLevelReadOnly, Authenticated, nil, 1925881096, noPerlBypass},
 
 Review comment:
   Why is the 1.1 route proxied to Perl? Reading the code, it appears the new handler can delete by either `id` or `name`, right?

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377861558
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/routing/routes.go
 ##########
 @@ -301,6 +301,8 @@ func Routes(d ServerData) ([]Route, []RawRoute, http.Handler, error) {
 		{1.1, http.MethodGet, `regions/name/{name}/?(\.json)?$`, region.GetName, auth.PrivLevelReadOnly, Authenticated, nil, 503583197, noPerlBypass},
 		{1.1, http.MethodPut, `regions/{id}$`, api.UpdateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 226308224, noPerlBypass},
 		{1.1, http.MethodPost, `regions/?$`, api.CreateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 1288334488, noPerlBypass},
+		{1.5, http.MethodDelete, `regions/?$`, api.DeleteHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 2032626758, noPerlBypass},
+		{1.1, http.MethodDelete, `regions/name/{name}$`, handlerToFunc(proxyHandler), auth.PrivLevelReadOnly, Authenticated, nil, 1925881096, noPerlBypass},
 
 Review comment:
   Ok, well `routes.go` might not even be a true list of all routes that exist currently because I'm not sure we've followed that pattern for other "deprecate but don't rewrite" routes. Not a big deal, just made me confused when I saw it.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] ocket8888 commented on issue #4357: Document and deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on issue #4357: Document and deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#issuecomment-580459039
 
 
   The way I used to do this was with a mentality of "everything gets rewritten to Go, and *then* refactored." So I'd have rewritten it with a Go DELETE handler for `/regions/name/{{name}}` **and** `/regions?name={{name}}` and a deprecation notice wrapper around the former.
   
   I guess the compromise here being: add a 1.5 DELETE handler for `/regions?name={{name}}` and deprecate the original route (`/regions/name/{{name}}`). You can do that by rewriting to Go or not; since you're already making a handler it'd only entail making a wrapper. That's my suggestion, anyway.
   
   I'm +1 on deprecating them, fwiw.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377162079
 
 

 ##########
 File path: docs/source/api/regions.rst
 ##########
 @@ -165,3 +165,63 @@ Response Structure
 		"lastUpdated": "2018-12-06 02:14:45+00",
 		"name": "Manchester"
 	}}
+
+``DELETE``
+==========
+
+.. versionadded:: 1.5
+
+Deletes a region. If no query parameter is specified, a ``400 Bad Request`` response is returned.
+
+:Auth. Required: Yes
+:Roles Required: "admin" or "operations"
+:Response Type:  Object
+
+Request Structure
+-----------------
+
+.. table:: Request Query Parameters
+
+	+-----------+----------+---------------------------------------------------------------------------------------------------------------+
+	| Name      | Required | Description                                                                                                   |
+	+===========+==========+===============================================================================================================+
+	| id        | no       | Delete :term:`Region` by integral, unique identifier                                                          |
+	+-----------+----------+---------------------------------------------------------------------------------------------------------------+
+	| name      | no       | Delete :term:`Region` by name                                                                                 |
+	+-----------+----------+---------------------------------------------------------------------------------------------------------------+
+
+.. code-block:: http
+	:caption: Request Example
+
+	DELETE /api/1.5/regions?name=Manchester HTTP/1.1
+	User-Agent: curl/7.29.0
+	Host: trafficops.infra.ciab.test
+	Accept: */*
+	Cookie: mojolicious=...
+
+Response Structure
+------------------
+
+.. code-block:: http
+	:caption: Response Example
+
+	HTTP/1.1 200 OK
+	Access-Control-Allow-Credentials: true
+	Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Accept, Set-Cookie, Cookie
+	Access-Control-Allow-Methods: POST,GET,OPTIONS,PUT,DELETE
+	Access-Control-Allow-Origin: *
+	Content-Type: application/json
+	Set-Cookie: mojolicious=eyJhdXRoX2RhdGEiOiJhZG1pbiIsImV4cGlyZXMiOjE1ODEwODM3ODQsImJ5IjoidHJhZmZpY2NvbnRyb2wtZ28tdG9jb29raWUifQ--f6b59c9dde67bcd545bde3bbb4e56b33e23827e9; Path=/; Expires=Fri, 07 Feb 2020 13:56:24 GMT; Max-Age=3600; HttpOnly
 
 Review comment:
   API examples shouldn't include actual `mojolicious` cookies, as that may or may not (though hopefully not) constitute a security risk. You can replace the value with `...` like you did above in the request example, or just omit the header entirely (I'd prefer the former, but the latter is allowed).

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r377163924
 
 

 ##########
 File path: traffic_ops/client/region.go
 ##########
 @@ -159,3 +160,28 @@ func (to *Session) GetRegionByNamePath(name string) ([]tc.RegionName, ReqInf, er
 	}
 	return resp.Response, reqInf, nil
 }
+
+// DeleteRegion lets you DELETE a Region
 
 Review comment:
   nit: should end with a period.
   
   Should also mention that only one of the parameters is required.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376741130
 
 

 ##########
 File path: traffic_ops/traffic_ops_golang/routing/routes.go
 ##########
 @@ -301,6 +301,8 @@ func Routes(d ServerData) ([]Route, []RawRoute, http.Handler, error) {
 		{1.1, http.MethodGet, `regions/name/{name}/?(\.json)?$`, region.GetName, auth.PrivLevelReadOnly, Authenticated, nil, 503583197, noPerlBypass},
 		{1.1, http.MethodPut, `regions/{id}$`, api.UpdateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 226308224, noPerlBypass},
 		{1.1, http.MethodPost, `regions/?$`, api.CreateHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 1288334488, noPerlBypass},
+		{1.1, http.MethodDelete, `regions/?$`, api.DeleteHandler(&region.TORegion{}), auth.PrivLevelOperations, Authenticated, nil, 2032626758, noPerlBypass},
 
 Review comment:
   Changed to 1.5 in 2ada94e8f2.

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


With regards,
Apache Git Services

[GitHub] [trafficcontrol] mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}

Posted by GitBox <gi...@apache.org>.
mhoppa commented on a change in pull request #4357: Deprecate /regions/name/{{name}}
URL: https://github.com/apache/trafficcontrol/pull/4357#discussion_r376573457
 
 

 ##########
 File path: traffic_ops/app/lib/API/Region.pm
 ##########
 @@ -244,21 +244,22 @@ sub delete {
 sub delete_by_name {
 	my $self = shift;
 	my $name     = $self->param('name');
+	my $alt		 = 'DELETE /regions/{{ID}}';
 
 	if ( !&is_oper($self) ) {
 		return $self->forbidden();
 
 Review comment:
   can you add the deprecation notice here as well just like you did with other errors below

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


With regards,
Apache Git Services