ocket8888 commented on code in PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#discussion_r1047345456
##########
docs/source/api/v4/deliveryservices.rst:
##########
@@ -328,6 +333,10 @@ Request Structure
:regional: A boolean value defining the :ref:`ds-regional`
setting on this :term:`Delivery Service`
:regionalGeoBlocking: A boolean defining the :ref:`ds-regionalgeo`
setting on this :term:`Delivery Service`
:remapText: :ref:`ds-raw-remap`
+:requiredCapabilities: An array of the capabilities that this delivery
service requires.
Review Comment:
Same as above.
##########
docs/source/api/v4/deliveryservices_id.rst:
##########
@@ -249,6 +254,10 @@ Response Structure
:regexRemap: A :ref:`ds-regex-remap`
:regional: A boolean value defining the :ref:`ds-regional`
setting on this :term:`Delivery Service`
:regionalGeoBlocking: A boolean defining the :ref:`ds-regionalgeo` setting
on this :term:`Delivery Service`
+:requiredCapabilities: An array of the capabilities that this delivery
service requires.
Review Comment:
Same as above.
##########
docs/source/api/v4/deliveryservices_required_capabilities.rst:
##########
@@ -19,6 +19,9 @@
``deliveryservices_required_capabilities``
******************************************
+.. deprecated:: 4.1
+ This endpoint will be removed in a future release, in favor of
``required_capabilities`` being a part of :term:`Delivery Services`.
Review Comment:
`required_capabilities` is not a property of a Delivery Service.
`requiredCapabilities` is - but this should probably be a link to the relevant
DS overview subsection.
##########
docs/source/api/v4/deliveryservices.rst:
##########
@@ -495,6 +505,10 @@ Response Structure
:regional: A boolean value defining the :ref:`ds-regional`
setting on this :term:`Delivery Service`
:regionalGeoBlocking: A boolean defining the :ref:`ds-regionalgeo` setting
on this :term:`Delivery Service`
:remapText: :ref:`ds-raw-remap`
+:requiredCapabilities: An array of the capabilities that this delivery
service requires.
Review Comment:
Same as above
##########
lib/go-tc/deliveryservices.go:
##########
@@ -1726,6 +1736,12 @@ func (ds DeliveryServiceV4) Upgrade() DeliveryServiceV5 {
copy(upgraded.MatchList, *ds.MatchList)
}
copy(upgraded.TLSVersions, ds.TLSVersions)
+ if len(ds.RequiredCapabilities) == 0 {
+ upgraded.RequiredCapabilities = make([]string, 0)
Review Comment:
This line duplicates the effect of line 1703 under the conditions checked on
line 1739
##########
docs/source/api/v4/deliveryservices_id.rst:
##########
@@ -81,6 +81,10 @@ Request Structure
:regional: A boolean value defining the :ref:`ds-regional`
setting on this :term:`Delivery Service`
:regionalGeoBlocking: A boolean defining the :ref:`ds-regionalgeo`
setting on this :term:`Delivery Service`
:remapText: :ref:`ds-raw-remap`
+:requiredCapabilities: An array of the capabilities that this delivery
service requires.
Review Comment:
Same as above.
##########
docs/source/api/v4/deliveryservices.rst:
##########
@@ -148,6 +148,10 @@ Response Structure
:regional: A boolean value defining the :ref:`ds-regional`
setting on this :term:`Delivery Service`
:regionalGeoBlocking: A boolean defining the :ref:`ds-regionalgeo` setting
on this :term:`Delivery Service`
:remapText: :ref:`ds-raw-remap`
+:requiredCapabilities: An array of the capabilities that this delivery
service requires.
Review Comment:
"delivery service" should be title-cased like "Delivery Service" because
it's one of the [ATC project-specific
terms](https://traffic-control-cdn.readthedocs.io/en/latest/development/documentation_guidelines.html#terms).
Ideally it'd also be linked as a `term`, but since that's already done in this
section you don't need to if you don't want to.
##########
lib/go-tc/deliveryservice_requests.go:
##########
@@ -435,6 +435,58 @@ type DeliveryServiceRequestNullable struct {
XMLID *string `json:"-" db:"xml_id"`
}
+// Downgrade will convert an instance of DeliveryServiceRequestV41 to
DeliveryServiceRequestV40
Review Comment:
GoDoc should end with punctuation
##########
lib/go-tc/deliveryservices.go:
##########
@@ -1608,7 +1611,8 @@ func (ds DeliveryServiceV5) Downgrade() DeliveryServiceV4
{
},
TLSVersions: make([]string, len(ds.TLSVersions)),
},
- Regional: ds.Regional,
+ Regional: ds.Regional,
+ RequiredCapabilities: ds.RequiredCapabilities,
Review Comment:
This line is unnecessary since it's being set in all cases below - also
possibly confusing because it's the only shallow copy in a sea of deep copies.
Or, you could set this to `make([]string, len(ds.RequiredCapabilities))` here,
and then below you *only* need to handle the case where
`len(ds.RequiredCapabilities)` is greater than zero.
##########
traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.up.sql:
##########
@@ -0,0 +1,30 @@
+/*
+ * 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.
+ */
+
+ALTER TABLE public.deliveryservice
+ ADD COLUMN required_capabilities TEXT[];
+
+DO $$
+DECLARE temprow RECORD;
+BEGIN FOR temprow IN
+SELECT deliveryservice_id, ARRAY_AGG(required_capability) AS
required_capabilities FROM deliveryservices_required_capability drc GROUP BY
drc.deliveryservice_id
+ LOOP
+UPDATE deliveryservice d SET required_capabilities =
temprow.required_capabilities WHERE d.id = temprow.deliveryservice_id;
+END LOOP;
+END $$;
Review Comment:
I think this doesn't require pgpl scripting, I'm fairly sure this can be
written with just plain old SQL - shorter, too.
##########
lib/go-tc/deliveryservice_requests.go:
##########
@@ -435,6 +435,58 @@ type DeliveryServiceRequestNullable struct {
XMLID *string `json:"-" db:"xml_id"`
}
+// Downgrade will convert an instance of DeliveryServiceRequestV41 to
DeliveryServiceRequestV40
+func (dsr DeliveryServiceRequestV41) Downgrade() DeliveryServiceRequestV40 {
+ var dsrV40 DeliveryServiceRequestV40
+ dsrV40.Assignee = copyStringIfNotNil(dsr.Assignee)
+ dsrV40.AssigneeID = copyIntIfNotNil(dsr.AssigneeID)
+ dsrV40.Author = dsr.Author
+ dsrV40.AuthorID = copyIntIfNotNil(dsr.AuthorID)
+ dsrV40.ChangeType = dsr.ChangeType
+ dsrV40.CreatedAt = dsr.CreatedAt
+ dsrV40.ID = copyIntIfNotNil(dsr.ID)
+ dsrV40.LastEditedBy = dsr.LastEditedBy
+ dsrV40.LastEditedByID = copyIntIfNotNil(dsr.LastEditedByID)
+ dsrV40.LastUpdated = dsr.LastUpdated
+ if dsr.Original != nil {
+ dsrV40.Original = new(DeliveryServiceV40)
+ dsrV40.Original = &dsr.Original.DeliveryServiceV40
+ }
+ if dsr.Requested != nil {
+ dsrV40.Requested = new(DeliveryServiceV40)
+ dsrV40.Requested = &dsr.Requested.DeliveryServiceV40
+ }
+ dsrV40.Status = dsr.Status
+ dsrV40.XMLID = dsr.XMLID
+ return dsrV40
+}
+
+// Upgrade will convert an instance of DeliveryServiceRequestV40 to
DeliveryServiceRequestV41
Review Comment:
GoDoc should end with punctuation. Also, it'd be helpful to note that the
Requested and/or Original DSes are shallow-copied in this case.
##########
lib/go-tc/deliveryservice_requests.go:
##########
@@ -435,6 +435,58 @@ type DeliveryServiceRequestNullable struct {
XMLID *string `json:"-" db:"xml_id"`
}
+// Downgrade will convert an instance of DeliveryServiceRequestV41 to
DeliveryServiceRequestV40
Review Comment:
Also, it'd be helpful to note that the Requested and/or Original DSes are
shallow-copied in this case.
##########
traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go:
##########
@@ -897,7 +895,7 @@ func DSRExistsWithXMLID(xmlid string, tx *sql.Tx) (bool,
error) {
return exists, err
}
-// ScanCachegroupsServerCapabilities, given rows of (server ID, CDN ID,
cachegroup name, server capabilities),
+// ScanCachegroupsServerCapabilities : given rows of (server ID, CDN ID,
cachegroup name, server capabilities),
Review Comment:
this was grammatically correct before, the colon should be used to separate
independent clauses, but neither side of this colon is an independent clause.
##########
docs/source/api/v4/servers_id_deliveryservices.rst:
##########
@@ -134,6 +134,10 @@ Response Structure
:regional: A boolean value defining the :ref:`ds-regional` setting
on this :term:`Delivery Service`
:regionalGeoBlocking: A boolean defining the :ref:`ds-regionalgeo` setting on
this :term:`Delivery Service`
:remapText: :ref:`ds-raw-remap`
+:requiredCapabilities: An array of the capabilities that this delivery service
requires.
Review Comment:
Same as above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
