rawlinp commented on a change in pull request #5345:
URL: https://github.com/apache/trafficcontrol/pull/5345#discussion_r540290219
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -179,6 +188,11 @@ type DeliveryServiceNullableV30 struct {
ServiceCategory *string `json:"serviceCategory"
db:"service_category"`
}
+type DeliveryServiceV31 struct {
+ DeliveryServiceNullableV30
+ MaxRequestHeaderSize *int `json:"maxRequestHeaderSize"
db:"max_request_header_size"`
+}
+
// Deprecated: Use versioned structures only from now on.
type DeliveryServiceNullable DeliveryServiceNullableV15
Review comment:
So I had some thoughts @ocket8888 @rob05c. I think we may want to
continue using type aliases like this, but only per major version.
Otherwise, let's say we end up with another 5 minor versions in 3.x. That
means we will have 5 different `GetDeliveryServiceV*`,
`UpdateDeliveryServiceV*`, etc, all in the same client package. I don't think
that's what we want the client to look like, right? As a user, I wouldn't want
to have to pick between all those minor version methods. I thought we wanted it
to auto-downgrade minor versions, which means all the client functions should
be using a type alias per major version, e.g. `type DeliveryServiceV3
DeliveryServiceV31`, which we continually update to alias to new minor
versions, e.g. `type DeliveryServiceV3 DeliveryServiceV32`. Does that make
sense?
Since we already have `DeliveryServiceNullableV30`, maybe that should be the
current alias. Then we don't need to add new client methods for V31.
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -335,6 +349,46 @@ func ParseOrgServerFQDN(orgServerFQDN string) (*string,
*string, *string, error)
return &protocol, &FQDN, port, nil
}
+func (ds *DeliveryServiceV31) Sanitize() {
Review comment:
I think we might not need this if we alias `type DeliveryServiceV30
DeliveryServiceV31` like I mention in the previous comment. But if we don't
want to do that for some reason, I think the matching `DeliveryServiceV30`
methods would no longer be in use and could be removed. We should only be
calling `Sanitize`, `Validate`, etc. in the _latest_ handlers.
##########
File path:
traffic_ops/app/db/migrations/2020113000000000_add_max_request_header_size_delivery_service.sql
##########
@@ -0,0 +1,22 @@
+/*
+
+ Licensed 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.
+*/
+
+-- +goose Up
+-- SQL in section 'Up' is executed when this migration is applied
+ALTER TABLE deliveryservice ADD COLUMN max_request_header_size int DEFAULT
131072;
+
+-- +goose Down
+-- SQL section 'Down' is executed when this migration is rolled back
+ALTER TABLE deliveryservice DROP COLUMN max_request_header_size;
Review comment:
missing newline at end of file
##########
File path: traffic_ops/traffic_ops_golang/routing/routes.go
##########
@@ -144,6 +144,13 @@ func Routes(d ServerData) ([]Route, []RawRoute,
http.Handler, error) {
/**
* 3.x API
*/
+ ////DeliveryServices
+ {api.Version{3, 1}, http.MethodGet, `deliveryservices/?$`,
api.ReadHandler(&deliveryservice.TODeliveryService{}), auth.PrivLevelReadOnly,
Authenticated, nil, 22383173943, noPerlBypass},
Review comment:
We should only need 3.1 routes for POST and PUT. The GET handler should
have the minor-version specific handling, and DELETE is version-agnostic.
##########
File path: traffic_ops/v3-client/deliveryservice.go
##########
@@ -34,13 +34,25 @@ const (
// See Also:
https://traffic-control-cdn.readthedocs.io/en/latest/api/v3/deliveryservices.html
API_DELIVERY_SERVICES = apiBase + "/deliveryservices"
+ // API_DELIVERY_SERVICES_V31 is the API path on which Traffic Ops
serves Delivery Service
+ // information. More specific information is typically found on
sub-paths of this.
+ // See Also:
https://traffic-control-cdn.readthedocs.io/en/latest/api/v3/deliveryservices.html
+ API_DELIVERY_SERVICES_V31 = apiBaseV31 + "/deliveryservices"
Review comment:
If we follow my aliasing comment above, I think we would just update
`apiBase` to 3.1, and we wouldn't need the new 3.1-specific client methods
below.
##########
File path: traffic_portal/app/src/common/api/DeliveryServiceService.js
##########
@@ -20,7 +20,7 @@
var DeliveryServiceService = function($http, locationUtils, messageModel, ENV)
{
this.getDeliveryServices = function(queryParams) {
- return $http.get(ENV.api['root'] + 'deliveryservices', {params:
queryParams}).then(
+ return $http.get(ENV.api['deliveryServices'] + 'deliveryservices',
{params: queryParams}).then(
Review comment:
I think we would just update `ENV.api['root']` to 3.1, right
@mitchell852? I think we would prefer to have TP using the same version for all
requests if possible.
##########
File path: traffic_ops/testing/api/v3/deliveryservices_test.go
##########
@@ -69,6 +71,21 @@ func TestDeliveryServices(t *testing.T) {
})
}
+func GetTestDeliveryServicesV31(t *testing.T) {
Review comment:
We shouldn't need new minor-version specific tests if we follow my first
comment above. We could just check this new field in the existing
`GetTestDeliveryServices` test (similar for update).
##########
File path: traffic_ops/testing/api/v3/traffic_control_test.go
##########
@@ -31,6 +31,7 @@ type TrafficControl struct {
DeliveryServiceRequests
[]tc.DeliveryServiceRequest `json:"deliveryServiceRequests"`
DeliveryServiceRequestComments
[]tc.DeliveryServiceRequestComment `json:"deliveryServiceRequestComments"`
DeliveryServices
[]tc.DeliveryServiceNullableV30 `json:"deliveryservices"`
+ DeliveryServicesV31
[]tc.DeliveryServiceV31 `json:"deliveryservicesV31"`
Review comment:
We probably wouldn't need this and the new additions to tc-fixtures.json
either (assuming we alias like I mention in the first comment).
----------------------------------------------------------------
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:
[email protected]
