rawlinp commented on a change in pull request #4644:
URL: https://github.com/apache/trafficcontrol/pull/4644#discussion_r574063310
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -56,6 +56,13 @@ type DeliveryServicesResponseV30 struct {
Alerts
}
+// DeliveryServicesResponseV40 is the type of a response from the
+// /api/3.0/deliveryservices Traffic Ops endpoint.
+type DeliveryServicesResponseV40 struct {
Review comment:
Per my next comment, this should be called `DeliveryServicesResponseV4`
and use the `DeliveryServicesV4` alias for the `Response` field.
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -173,20 +180,70 @@ type DeliveryServiceV11 struct {
XMLID string `json:"xmlId"`
}
+type DeliveryServiceV40 struct {
Review comment:
We need a type alias per major version, e.g. `type DeliveryServiceV4
DeliveryServiceV40`. Then the type alias is what should be used in other
structs and code as well.
That way, when we add `DeliveryServiceV41` with new 4.1 fields, we can just
update the alias to `type DeliveryServiceV4 DeliveryServiceV41` and won't need
to add new "response" structs, etc., for every minor version.
See the comment on the 3.1 alias:
```
// DeliveryServiceNullableV30 is the aliased structure that we should be
using for all api 3.x delivery structure operations
// This type should always alias the latest 3.x minor version struct. For
ex, if you wanted to create a DeliveryServiceV32 struct, you would do the
following:
// type DeliveryServiceNullableV30 DeliveryServiceV32
// DeliveryServiceV32 = DeliveryServiceV31 + the new fields
type DeliveryServiceNullableV30 DeliveryServiceV31
```
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -450,6 +472,177 @@ func createV31(w http.ResponseWriter, r *http.Request,
inf *api.APIInfo, dsV31 t
return &dsV31, http.StatusOK, nil, nil
}
+func createV40(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, dsV40
tc.DeliveryServiceV40) (*tc.DeliveryServiceV40, int, error, error) {
Review comment:
We shouldn't have to create another copy of this function. Instead, the
`createV31` function should call _this_ function, take the result, and convert
it back to a V31 struct to return back to the client.
In `createV40` we don't write `CacheURL` to the DB, so that should instead
be handled by the `createV31` function after getting a successful return value
from `createV40`. If `EnsureParams` handles CacheURL-ey stuff as well, it would
go in `createV31` alone.
Also, `createV40` should cast the `tc.DeliveryServiceV40` into the
`tc.DeliveryServiceV4` alias at the beginning of the method then cast back to
`tc.DeliveryServiceV40` at the end (on L479 and L642, respectively) in order
for all the other functions used by this function to accept the
`tc.DeliveryServiceV4` alias as input. Otherwise, every new API version
requires updating all of these extra functions.
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -852,9 +852,33 @@ func TopologyExists(tx *sql.Tx, name string) (bool, error)
{
return count > 0, err
}
+// CheckTopologyV30 returns an error if the given Topology does not exist or
if one of the Topology's Cache Groups is
+// empty with respect to the Delivery Service's CDN.
+func CheckTopologyV30(tx *sqlx.Tx, ds tc.DeliveryServiceNullableV30) (int,
error, error) {
Review comment:
This copy shouldn't be necessary -- all the "real work" of the DS API
should be handled at the "v4 (latest) layer." I.e. the v1-3 handlers should all
chain up to the v4 handler, where the request is actually handled, then
converted back down to the handler at which the request came in.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -1740,8 +2215,21 @@ func GetDSSelectQuery() string {
return selectQuery()
}
+// getTenantIDV30 returns the tenant Id of the given delivery service. Note it
may return a nil id and nil error, if the tenant ID in the database is nil.
+func getTenantIDV30(tx *sql.Tx, ds *tc.DeliveryServiceNullableV30) (*int,
error) {
Review comment:
per my prior comments, this shouldn't be necessary
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -469,6 +585,97 @@ func (ds *DeliveryServiceNullable) validateTypeFields(tx
*sql.Tx) error {
return nil
}
+func (ds *DeliveryServiceV40) validateTypeFields(tx *sql.Tx) error {
Review comment:
My prior comment applies here as well. No need for another copy -- just
move the existing function to the `DeliveryServiceV4` alias.
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -560,6 +767,40 @@ func (ds *DeliveryServiceNullableV30)
validateTypeFields(tx *sql.Tx) error {
return nil
}
+func (ds *DeliveryServiceV40) Validate(tx *sql.Tx) error {
Review comment:
My prior comment applies here as well. No need for another copy -- just
move the existing function to the `DeliveryServiceV4` alias.
In fact, I'm starting to wonder if we should just convert these methods into
functions that take a `*DeliveryServiceV4` as an argument and move this into
traffic_ops_golang/deliveryservices/deliveryservices.go instead (and make it
private). These methods shouldn't really be exposed -- just the structs/data.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -1066,6 +1282,249 @@ func updateV31(w http.ResponseWriter, r *http.Request,
inf *api.APIInfo, dsV31 *
return dsV31, http.StatusOK, nil, nil
}
+func updateV40(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, dsV40
*tc.DeliveryServiceV40) (*tc.DeliveryServiceV40, int, error, error) {
Review comment:
See my prior comment about `createV40` -- the same pattern applies to
`updateV40` as well.
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -349,6 +425,46 @@ func ParseOrgServerFQDN(orgServerFQDN string) (*string,
*string, *string, error)
return &protocol, &FQDN, port, nil
}
+func (ds *DeliveryServiceV40) Sanitize() {
Review comment:
We shouldn't need to make another copy of this function. Basically, the
existing:
```
func (ds *DeliveryServiceNullableV30) Sanitize() {
```
Should be updated to:
```
func (ds *DeliveryServiceV4) sanitize() {
```
Note that it doesn't actually have to be public (and shouldn't be IMO), and
that it should be on the `DeliveryServiceV4` alias as well.
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -97,7 +97,7 @@ func (ds *TODeliveryService) GetType() string {
// IsTenantAuthorized checks that the user is authorized for both the delivery
service's existing tenant, and the new tenant they're changing it to (if
different).
func (ds *TODeliveryService) IsTenantAuthorized(user *auth.CurrentUser) (bool,
error) {
- return isTenantAuthorized(ds.ReqInfo, &ds.DeliveryServiceNullableV30)
+ return isTenantAuthorizedV30(ds.ReqInfo, &ds.DeliveryServiceNullableV30)
Review comment:
Version-specific functions like this shouldn't be necessary (see prior
comment).
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -595,6 +836,16 @@ func (ds *DeliveryServiceNullableV30) Validate(tx *sql.Tx)
error {
return util.JoinErrs(errs)
}
+func (ds *DeliveryServiceV40) validateTopologyFields() error {
Review comment:
ditto my prior comments
##########
File path: traffic_ops/v4-client/deliveryservice.go
##########
@@ -388,7 +456,20 @@ func (to *Session) UpdateDeliveryServiceV30WithHdr(id int,
ds tc.DeliveryService
return tc.DeliveryServiceNullableV30{}, reqInf,
fmt.Errorf("failed to update Delivery Service #%d; response indicated that %d
were updated", id, len(data.Response))
}
return data.Response[0], reqInf, nil
+}
+// UpdateDeliveryServiceV40WithHdr replaces the Delivery Service identified by
the
+// integral, unique identifier 'id' with the one it's passed.
+func (to *Session) UpdateDeliveryServiceV40WithHdr(id int, ds
tc.DeliveryServiceV40, header http.Header) (tc.DeliveryServiceV40, ReqInf,
error) {
Review comment:
ditto prior comment
##########
File path: traffic_ops/v4-client/deliveryservice.go
##########
@@ -155,6 +155,19 @@ func (to *Session) GetDeliveryServicesV30WithHdr(header
http.Header, params url.
return data.Response, reqInf, err
}
+// GetDeliveryServicesV40WithHdr returns all (tenant-visible) Delivery
Services that
+// satisfy the passed query string parameters. See the API documentation for
+// information on the available parameters.
+func (to *Session) GetDeliveryServicesV40WithHdr(header http.Header, params
url.Values) ([]tc.DeliveryServiceV40, ReqInf, error) {
Review comment:
These methods should also use the `DeliveryServiceV4` alias as
input/output. We should also consider implementing the new proposed TO Go
client best practices (or make a mental note to come back here and change them
once the groundwork is laid out for that).
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -1785,6 +2273,38 @@ func isTenantAuthorized(inf *api.APIInfo, ds
*tc.DeliveryServiceNullableV30) (bo
return true, nil
}
+func isTenantAuthorizedV30(inf *api.APIInfo, ds
*tc.DeliveryServiceNullableV30) (bool, error) {
Review comment:
per my prior comments, this shouldn't be necessary
##########
File path: traffic_ops/v4-client/deliveryservice.go
##########
@@ -261,6 +274,61 @@ func (to *Session) GetDeliveryServiceByXMLIDNullable(XMLID
string) ([]tc.Deliver
return ret, reqInf, err
}
+func (to *Session) CreateDeliveryServiceV40(ds tc.DeliveryServiceV40)
(tc.DeliveryServiceV40, ReqInf, error) {
Review comment:
ditto prior comment
##########
File path: traffic_ops/traffic_ops_golang/server/servers_assignment.go
##########
@@ -390,12 +389,6 @@ INSERT INTO deliveryservice_server (deliveryservice,
server)
} else {
delete = append(delete, param)
}
- param = getConfigFile(cacheURLPrefix, xmlID.String)
Review comment:
We can't get rid of this functionality yet, right? As long as the 3.1
and below APIs exist, the field still exists, and its associated functionality
should be supported?
----------------------------------------------------------------
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]
