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 2022/11/28 19:58:10 UTC
[GitHub] [trafficcontrol] srijeet0406 opened a new pull request, #7224: Add required capabilities to delivery service structure
srijeet0406 opened a new pull request, #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #ISSUE
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
This PR adds the `required capabilities` into the `deliveryservice` structure.
<!-- **^ Add meaningful description above** --><hr/>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Documentation
- Traffic Ops
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your PR.
If your PR has tests (and most should), provide the steps needed to run the tests.
If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
Make `CRUD` calls to the delivery service API and make sure that you can add, remove, modify and get required permissions as part of the DS structure.
Make sure all the tests pass.
## If this is a bugfix, which Traffic Control versions contained the bug?
<!-- Delete this section if the PR is not a bugfix, or if the bug is only in the master branch.
Examples:
- 5.1.2
- 5.1.3 (RC1)
-->
- master
## PR submission checklist
- [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [x] This PR has documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] codecov[bot] commented on pull request #7224: Add required capabilities to delivery service structure
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#issuecomment-1329705215
# [Codecov](https://codecov.io/gh/apache/trafficcontrol/pull/7224?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7224](https://codecov.io/gh/apache/trafficcontrol/pull/7224?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cba4930) into [master](https://codecov.io/gh/apache/trafficcontrol/commit/ef9aa6da8961b8d986eea1ab4db40fc32da405dd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ef9aa6d) will **decrease** coverage by `12.03%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #7224 +/- ##
===========================================
- Coverage 69.71% 57.68% -12.04%
===========================================
Files 167 55 -112
Lines 5647 2552 -3095
===========================================
- Hits 3937 1472 -2465
+ Misses 1461 991 -470
+ Partials 249 89 -160
```
| Flag | Coverage Δ | |
|---|---|---|
| traffic_ops_integration | `57.68% <ø> (-12.04%)` | :arrow_down: |
| v3 | `57.68% <ø> (ø)` | |
| v4 | `?` | |
| v5 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/trafficcontrol/pull/7224?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [traffic\_ops/v4-client/topology\_queue\_updates.go](https://codecov.io/gh/apache/trafficcontrol/pull/7224/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dHJhZmZpY19vcHMvdjQtY2xpZW50L3RvcG9sb2d5X3F1ZXVlX3VwZGF0ZXMuZ28=) | | |
| [traffic\_ops/v5-client/role.go](https://codecov.io/gh/apache/trafficcontrol/pull/7224/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dHJhZmZpY19vcHMvdjUtY2xpZW50L3JvbGUuZ28=) | | |
| [traffic\_ops/v4-client/federation\_resolver.go](https://codecov.io/gh/apache/trafficcontrol/pull/7224/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dHJhZmZpY19vcHMvdjQtY2xpZW50L2ZlZGVyYXRpb25fcmVzb2x2ZXIuZ28=) | | |
| [traffic\_ops/v5-client/cachegroup.go](https://codecov.io/gh/apache/trafficcontrol/pull/7224/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dHJhZmZpY19vcHMvdjUtY2xpZW50L2NhY2hlZ3JvdXAuZ28=) | | |
| [traffic\_ops/v4-client/acme.go](https://codecov.io/gh/apache/trafficcontrol/pull/7224/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dHJhZmZpY19vcHMvdjQtY2xpZW50L2FjbWUuZ28=) | | |
| [traffic\_ops/v4-client/async\_status.go](https://codecov.io/gh/apache/trafficcontrol/pull/7224/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dHJhZmZpY19vcHMvdjQtY2xpZW50L2FzeW5jX3N0YXR1cy5nbw==) | | |
| [...ic\_ops/v5-client/federation\_federation\_resolver.go](https://codecov.io/gh/apache/trafficcontrol/pull/7224/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dHJhZmZpY19vcHMvdjUtY2xpZW50L2ZlZGVyYXRpb25fZmVkZXJhdGlvbl9yZXNvbHZlci5nbw==) | | |
| [traffic\_ops/v4-client/parameter.go](https://codecov.io/gh/apache/trafficcontrol/pull/7224/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dHJhZmZpY19vcHMvdjQtY2xpZW50L3BhcmFtZXRlci5nbw==) | | |
| [traffic\_ops/v5-client/servercheck.go](https://codecov.io/gh/apache/trafficcontrol/pull/7224/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dHJhZmZpY19vcHMvdjUtY2xpZW50L3NlcnZlcmNoZWNrLmdv) | | |
| [...4-client/deliveryservices\_required\_capabilities.go](https://codecov.io/gh/apache/trafficcontrol/pull/7224/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-dHJhZmZpY19vcHMvdjQtY2xpZW50L2RlbGl2ZXJ5c2VydmljZXNfcmVxdWlyZWRfY2FwYWJpbGl0aWVzLmdv) | | |
| ... and [102 more](https://codecov.io/gh/apache/trafficcontrol/pull/7224/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7224: Add required capabilities to delivery service structure
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#discussion_r1035362614
##########
CHANGELOG.md:
##########
@@ -24,6 +24,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- [#7113](https://github.com/apache/trafficcontrol/pull/7113) *Traffic Portal* Minimize the Server Server Capability part of the *Traffic Servers* section of the Snapshot Diff
### Changed
+- *Traffic Ops* Required Parameters are now a part of the `DeliveryService` structure.
Review Comment:
These need to have PR or issue links now
##########
traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.down.sql:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.
+ */
+
+CREATE TABLE IF NOT EXISTS deliveryservices_required_capability (
+ required_capability TEXT NOT NULL,
+ deliveryservice_id bigint NOT NULL,
+ last_updated timestamp with time zone DEFAULT now() NOT NULL,
+
+ PRIMARY KEY (deliveryservice_id, required_capability)
Review Comment:
Constraints should be explicitly named so they're easy to manipulate as the need arises (except defaults and nullability, since those can be manipulated without knowing their name).
##########
traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.down.sql:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.
+ */
+
+CREATE TABLE IF NOT EXISTS deliveryservices_required_capability (
+ required_capability TEXT NOT NULL,
Review Comment:
Personally, I like leaving datatypes lowercased, but they are technically keywords so it's fair to uppercase them - but you should pick. If `TEXT` is in all caps, then `BIGINT` should be too. Or they can both be lowercase.
##########
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.
+
+ .. versionchanged:: 4.1
Review Comment:
this should be `versionadded`, not `versionchanged`
##########
traffic_ops/testing/api/v4/deliveryservice_requests_test.go:
##########
@@ -408,9 +408,10 @@ func validatePutDSRequestFields(expectedResp map[string]interface{}) utils.CkReq
func CreateTestDeliveryServiceRequests(t *testing.T) {
for _, dsr := range testData.DeliveryServiceRequests {
- resetDS(dsr.Original)
- resetDS(dsr.Requested)
- respDSR, _, err := TOSession.CreateDeliveryServiceRequest(dsr, client.RequestOptions{})
+ dsrV41 := dsr.Upgrade()
Review Comment:
instead of updating on-the-fly, the test data should just use the v41 structure
##########
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[];
Review Comment:
fwiw, this change doesn't strictly require a migration at all. Idk if that's easier or not, but it's worth saying now because once it's in it's pretty much just in.
##########
docs/source/api/v4/deliveryservice_requests.rst:
##########
@@ -163,6 +163,7 @@ The response is an array of representations of :term:`Delivery Service Requests`
"regional": false,
"regionalGeoBlocking": false,
"remapText": null,
+ "requiredCapabilities": null,
Review Comment:
From [our API guidelines](https://traffic-control-cdn.readthedocs.io/en/latest/development/api_guidelines.html#response):
> The proper value of an empty collection is an empty collection.
`requiredCapabilities` is a collection, so the proper way to represent none of them is `[]`, not `null`
##########
traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go:
##########
@@ -939,11 +937,9 @@ func GetDSRequiredCapabilitiesFromID(id int, tx *sql.Tx) ([]string, error) {
caps := []string{}
for rows.Next() {
- var cap string
Review Comment:
this should now return exactly one row, so instead of looping over returned rows it's probably simpler to just use `QueryRow` above
##########
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.
+
+ .. versionchanged:: 4.1
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.
+
+ .. versionchanged:: 4.1
Review Comment:
same as above
##########
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.
+
+ .. versionchanged:: 4.1
Review Comment:
same as above
##########
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.
+
+ .. versionchanged:: 4.1
Review Comment:
same as above
##########
traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.down.sql:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.
+ */
+
+CREATE TABLE IF NOT EXISTS deliveryservices_required_capability (
Review Comment:
this should explicitly place the table in the `public` schema
##########
lib/go-tc/deliveryservices.go:
##########
@@ -1437,6 +1438,8 @@ type DeliveryServiceV50 struct {
// use, because the input is in no way restricted, validated, or limited in
// scope to the Delivery Service.
RemapText *string `json:"remapText" db:"remap_text"`
+ // RequiredCapabilities is an array of capabilities required for this delivery service
Review Comment:
nit: comment should end with punctuation
##########
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.
+
+ .. versionchanged:: 4.1
Review Comment:
same as above
##########
traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.down.sql:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.
+ */
+
+CREATE TABLE IF NOT EXISTS deliveryservices_required_capability (
+ required_capability TEXT NOT NULL,
+ deliveryservice_id bigint NOT NULL,
+ last_updated timestamp with time zone DEFAULT now() NOT NULL,
+
+ PRIMARY KEY (deliveryservice_id, required_capability)
+ );
+
+DO $$
Review Comment:
I don't think plpgsql scripting is necessary for this. You can just [use a query as a source for the `INSERT` statement](https://www.postgresql.org/docs/current/sql-insert.html) e.g. `INSERT INTO foo(bar) SELECT bar FROM foobar`.
##########
traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go:
##########
@@ -1584,12 +1591,37 @@ func Validate(tx *sql.Tx, ds *tc.DeliveryServiceV5) error {
if err := validateTypeFields(tx, ds); err != nil {
errs = append(errs, fmt.Errorf("type fields: %w", err))
}
+ if err := validateRequiredCapabilities(tx, ds); err != nil {
+ errs = append(errs, errors.New("required capabilities: "+err.Error()))
+ }
if len(errs) == 0 {
return nil
}
return util.JoinErrs(errs)
}
+func validateRequiredCapabilities(tx *sql.Tx, ds *tc.DeliveryServiceV5) error {
+ var valid bool
+ query := `SELECT $1 <@ (SELECT ARRAY_AGG(name) FROM server_capability)`
+ if ds.RequiredCapabilities != nil && len(ds.RequiredCapabilities) > 0 {
+ rows, err := tx.Query(query, pq.Array(ds.RequiredCapabilities))
+ if err != nil {
+ return err
+ }
+ defer rows.Close()
+ for rows.Next() {
+ err = rows.Scan(&valid)
+ if err != nil {
+ return err
+ }
+ if !valid {
+ return errors.New("one or more of the required capabilities do not exist")
Review Comment:
what would be _most_ useful for the user is to know _which_ server capabilities don't exist. Using the old method of storing the relationships in a separate table, it was more or less trivial to just add an `EXISTS` condition in a `WHERE` clause, but with arrays the easiest way is with the intarray plugin. But you could also do something like
```sql
SELECT cap
FROM UNNEST($1::text[]) AS caps(cap)
WHERE EXISTS (
SELECT c
FROM (
SELECT UNNEST(required_capabilities)
FROM public.deliveryservice
WHERE id = $2
) AS dscaps(c)
WHERE c = cap
);
```
(untested, just off the top of my head)
##########
lib/go-tc/deliveryservice_requests.go:
##########
@@ -435,6 +435,54 @@ type DeliveryServiceRequestNullable struct {
XMLID *string `json:"-" db:"xml_id"`
}
+// Downgrade will convert an instance of DeliveryServiceRequestV4 to DeliveryServiceRequestV40
+func (dsr DeliveryServiceRequestV4) Downgrade() DeliveryServiceRequestV40 {
Review Comment:
I think the receiver for this should be `DeliveryServiceRequestV41`. Because our minor-version-less structures are moving targets, if we add a 4.2 then we'd want to downgrade that to v4.1 instead of all the way down to 4.0, so that it's easy to make 4.1 responses. But that means that the call signature would change for the V4 DSR, because it now returns V41 instead of V40 but it's still tied to the V4 structure as though nothing had changed.
##########
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 $$;
+
+DROP TABLE public.deliveryservices_required_capability;
Review Comment:
basically all the same comments as the down script apply here as well.
##########
traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.down.sql:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.
+ */
+
+CREATE TABLE IF NOT EXISTS deliveryservices_required_capability (
+ required_capability TEXT NOT NULL,
+ deliveryservice_id bigint NOT NULL,
+ last_updated timestamp with time zone DEFAULT now() NOT NULL,
+
+ PRIMARY KEY (deliveryservice_id, required_capability)
+ );
+
+DO $$
+DECLARE temprow RECORD;
+BEGIN FOR temprow IN
+select id as deliveryservice_id, unnest(required_capabilities) as required_capability from deliveryservice d group by d.id, d.required_capabilities
+ LOOP
+insert into deliveryservices_required_capability ("deliveryservice_id", "required_capability") values (temprow.deliveryservice_id, temprow.required_capability);
Review Comment:
(Postgre)SQL keywords should be in ALL_CAPS
##########
traffic_ops/testing/api/v3/deliveryservices_test.go:
##########
@@ -226,8 +226,9 @@ func TestDeliveryServices(t *testing.T) {
"BAD REQUEST when ADDING TOPOLOGY to DS with DS REQUIRED CAPABILITY": {
EndpointID: GetDeliveryServiceId(t, "ds1"), ClientSession: TOSession,
RequestBody: generateDeliveryService(t, map[string]interface{}{
- "topology": "top-for-ds-req",
- "xmlId": "ds1",
+ "requiredCapabilities": []string{"foo"},
Review Comment:
this should be unnecessary in APIv3, as that field doesn't exist there, so this is just some unrecognized superfluous data.
##########
lib/go-tc/deliveryservice_requests.go:
##########
@@ -435,6 +435,54 @@ type DeliveryServiceRequestNullable struct {
XMLID *string `json:"-" db:"xml_id"`
}
+// Downgrade will convert an instance of DeliveryServiceRequestV4 to DeliveryServiceRequestV40
+func (dsr DeliveryServiceRequestV4) Downgrade() DeliveryServiceRequestV40 {
+ var dsrV40 DeliveryServiceRequestV40
+ dsrV40.Assignee = dsr.Assignee
+ dsrV40.AssigneeID = dsr.AssigneeID
+ dsrV40.Author = dsr.Author
+ dsrV40.AuthorID = dsr.AuthorID
+ dsrV40.ChangeType = dsr.ChangeType
+ dsrV40.CreatedAt = dsr.CreatedAt
+ dsrV40.ID = dsr.ID
+ dsrV40.LastEditedBy = dsr.LastEditedBy
+ dsrV40.LastEditedByID = dsr.LastEditedByID
+ dsrV40.LastUpdated = dsr.LastUpdated
+ if dsr.Original != nil {
+ dsrV40.Original = &dsr.Original.DeliveryServiceV40
+ }
+ if dsr.Requested != nil {
+ dsrV40.Requested = &dsr.Requested.DeliveryServiceV40
+ }
+ dsrV40.Status = dsr.Status
+ dsrV40.XMLID = dsr.XMLID
+ return dsrV40
+}
+
+// Upgrade will convert an instance of DeliveryServiceRequestV40 to DeliveryServiceRequestV4
+func (dsrV40 DeliveryServiceRequestV40) Upgrade() DeliveryServiceRequestV4 {
Review Comment:
Same as above; should probably return a DSRV41 instead of V4
##########
lib/go-tc/deliveryservice_requests.go:
##########
@@ -435,6 +435,54 @@ type DeliveryServiceRequestNullable struct {
XMLID *string `json:"-" db:"xml_id"`
}
+// Downgrade will convert an instance of DeliveryServiceRequestV4 to DeliveryServiceRequestV40
+func (dsr DeliveryServiceRequestV4) Downgrade() DeliveryServiceRequestV40 {
+ var dsrV40 DeliveryServiceRequestV40
+ dsrV40.Assignee = dsr.Assignee
+ dsrV40.AssigneeID = dsr.AssigneeID
+ dsrV40.Author = dsr.Author
+ dsrV40.AuthorID = dsr.AuthorID
+ dsrV40.ChangeType = dsr.ChangeType
+ dsrV40.CreatedAt = dsr.CreatedAt
+ dsrV40.ID = dsr.ID
+ dsrV40.LastEditedBy = dsr.LastEditedBy
+ dsrV40.LastEditedByID = dsr.LastEditedByID
+ dsrV40.LastUpdated = dsr.LastUpdated
+ if dsr.Original != nil {
+ dsrV40.Original = &dsr.Original.DeliveryServiceV40
+ }
+ if dsr.Requested != nil {
+ dsrV40.Requested = &dsr.Requested.DeliveryServiceV40
+ }
+ dsrV40.Status = dsr.Status
+ dsrV40.XMLID = dsr.XMLID
+ return dsrV40
+}
+
+// Upgrade will convert an instance of DeliveryServiceRequestV40 to DeliveryServiceRequestV4
+func (dsrV40 DeliveryServiceRequestV40) Upgrade() DeliveryServiceRequestV4 {
+ var dsrV4 DeliveryServiceRequestV41
+ dsrV4.Assignee = dsrV40.Assignee
+ dsrV4.AssigneeID = dsrV40.AssigneeID
+ dsrV4.Author = dsrV40.Author
+ dsrV4.AuthorID = dsrV40.AuthorID
+ dsrV4.ChangeType = dsrV40.ChangeType
+ dsrV4.CreatedAt = dsrV40.CreatedAt
+ dsrV4.ID = dsrV40.ID
+ dsrV4.LastEditedBy = dsrV40.LastEditedBy
+ dsrV4.LastEditedByID = dsrV40.LastEditedByID
+ dsrV4.LastUpdated = dsrV40.LastUpdated
+ if dsrV40.Original != nil {
+ dsrV4.Original = &DeliveryServiceV4{DeliveryServiceV40: *dsrV40.Original}
Review Comment:
Ideally for these you should just store the address of the result of upgrading/downgrading the underlying DS.
##########
traffic_ops/traffic_ops_golang/api/shared_handlers.go:
##########
@@ -571,41 +588,59 @@ func CreateHandler(creator Creator) http.HandlerFunc {
if sysErr != nil {
code = http.StatusInternalServerError
}
- HandleErr(w, r, inf.Tx.Tx, code, userErr, sysErr)
+ errHandler(w, r, inf.Tx.Tx, code, userErr, sysErr)
return
}
if t, ok := obj.(Tenantable); ok {
authorized, err := t.IsTenantAuthorized(inf.User)
if err != nil {
- HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("checking tenant authorized: "+err.Error()))
+ errHandler(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("checking tenant authorized: "+err.Error()))
return
}
if !authorized {
- HandleErr(w, r, inf.Tx.Tx, http.StatusForbidden, errors.New("not authorized on this tenant"), nil)
+ errHandler(w, r, inf.Tx.Tx, http.StatusForbidden, errors.New("not authorized on this tenant"), nil)
return
}
}
userErr, sysErr, errCode = obj.Create()
if userErr != nil || sysErr != nil {
- HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ errHandler(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
return
}
if err := CreateChangeLog(ApiChange, Created, obj, inf.User, inf.Tx.Tx); err != nil {
- HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, fmt.Errorf("inserting changelog: %w", err))
+ errHandler(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, fmt.Errorf("inserting changelog: %w", err))
return
}
alerts := tc.CreateAlerts(tc.SuccessLevel, obj.GetType()+" was created.")
if alertsObj, hasAlerts := obj.(AlertsResponse); hasAlerts {
alerts.AddAlerts(alertsObj.GetAlerts())
}
- WriteAlertsObj(w, r, http.StatusOK, alerts, obj)
+ successHandler(w, r, http.StatusOK, alerts, obj)
}
}
}
+// DeprecatedCreateHandler creates a net/http.HandlerFunc for the passed Creator object, and adds a deprecation
+// notice, optionally with a passed alternative route suggestion.
+func DeprecatedCreateHandler(creator Creator, alternative *string) http.HandlerFunc {
+ return createHandlerHelper(
+ creator,
+ func(w http.ResponseWriter, r *http.Request, tx *sql.Tx, statusCode int, userErr error, sysErr error) {
+ HandleDeprecatedErr(w, r, tx, statusCode, userErr, sysErr, alternative)
+ },
+ func(w http.ResponseWriter, r *http.Request, statusCode int, alerts tc.Alerts, results interface{}) {
+ depAlerts := CreateDeprecationAlerts(alternative)
+ for _, al := range alerts.Alerts {
+ depAlerts.Alerts = append(depAlerts.Alerts, al)
+ }
Review Comment:
you don't need to do that by hand, there's a method on `tc.Alerts` to do that for you: [`tc.Alerts.AddAlerts`](https://pkg.go.dev/github.com/apache/trafficcontrol/lib/go-tc#Alerts.AddAlerts).
##########
lib/go-tc/deliveryservice_requests.go:
##########
@@ -435,6 +435,54 @@ type DeliveryServiceRequestNullable struct {
XMLID *string `json:"-" db:"xml_id"`
}
+// Downgrade will convert an instance of DeliveryServiceRequestV4 to DeliveryServiceRequestV40
+func (dsr DeliveryServiceRequestV4) Downgrade() DeliveryServiceRequestV40 {
+ var dsrV40 DeliveryServiceRequestV40
+ dsrV40.Assignee = dsr.Assignee
+ dsrV40.AssigneeID = dsr.AssigneeID
+ dsrV40.Author = dsr.Author
+ dsrV40.AuthorID = dsr.AuthorID
+ dsrV40.ChangeType = dsr.ChangeType
+ dsrV40.CreatedAt = dsr.CreatedAt
+ dsrV40.ID = dsr.ID
+ dsrV40.LastEditedBy = dsr.LastEditedBy
+ dsrV40.LastEditedByID = dsr.LastEditedByID
+ dsrV40.LastUpdated = dsr.LastUpdated
+ if dsr.Original != nil {
+ dsrV40.Original = &dsr.Original.DeliveryServiceV40
+ }
+ if dsr.Requested != nil {
+ dsrV40.Requested = &dsr.Requested.DeliveryServiceV40
+ }
+ dsrV40.Status = dsr.Status
+ dsrV40.XMLID = dsr.XMLID
+ return dsrV40
+}
+
+// Upgrade will convert an instance of DeliveryServiceRequestV40 to DeliveryServiceRequestV4
+func (dsrV40 DeliveryServiceRequestV40) Upgrade() DeliveryServiceRequestV4 {
+ var dsrV4 DeliveryServiceRequestV41
+ dsrV4.Assignee = dsrV40.Assignee
+ dsrV4.AssigneeID = dsrV40.AssigneeID
+ dsrV4.Author = dsrV40.Author
Review Comment:
the V4 -> v5 conversion and its inverse are deep copies, but this makes a shallow copy. There's no good reason I can think of why it needs to be deep, but I think if you're going to make it shallow it's best to make it clear since the behavior is going to be different depending on what version you're upgrading/downgrading to/from.
##########
lib/go-tc/deliveryservices.go:
##########
@@ -1726,6 +1731,11 @@ func (ds DeliveryServiceV4) Upgrade() DeliveryServiceV5 {
copy(upgraded.MatchList, *ds.MatchList)
}
copy(upgraded.TLSVersions, ds.TLSVersions)
+ if ds.RequiredCapabilities == nil {
+ upgraded.RequiredCapabilities = nil
Review Comment:
same as above
##########
docs/source/api/v3/deliveryservices_required_capabilities.rst:
##########
@@ -19,6 +19,8 @@
``deliveryservices_required_capabilities``
******************************************
+.. deprecated:: ATCv7
Review Comment:
A few notes on these deprecation notices
- all of APIv3 is deprecated, so it's superfluous to note that for any specific endpoint/method thereof
- If an entire endpoint is being deprecated, you don't also need to individually deprecate each request method it supports
- Generally you want to specify the _API_ version that something was deprecated, unless it's not possible to do that for some reason or it just makes more sense to list the ATC version instead.
- Ideally the notice will come with some information about why it's being deprecated and/or what to use instead. This makes generating changelogs using Sphinx much more useful.
-
##########
traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go:
##########
@@ -850,13 +850,11 @@ func GetRequiredCapabilitiesOfDeliveryServices(ids []int, tx *sql.Tx) (map[int][
dsCaps := make(map[int][]string, len(ids))
q := `
SELECT
- d.id,
- ARRAY_REMOVE(ARRAY_AGG(dsrc.required_capability ORDER BY dsrc.required_capability), NULL) AS required_capabilities
-FROM deliveryservice d
-LEFT JOIN deliveryservices_required_capability dsrc on d.id = dsrc.deliveryservice_id
-WHERE
- d.id = ANY($1)
-GROUP BY d.id
+ ds.id,
+ ARRAY_REMOVE((ds.required_capabilities), NULL) AS required_capabilities
Review Comment:
Just to further expand on what I said earlier about not needing a migration, this collection of required Server Capabilities is represented by an array most everywhere, because that's such a common structure for languages and encoding formats to implement. However, in reality, it's more like a concrete set: It should be impossible to insert `NULL` and there should be no duplicates. The easiest way to satisfy those constraints and never have to worry about it anywhere else is to use a table where the primary key includes the intersection of a Delivery Service and any one of the Server Capabilities it requires, where null entries are not allowed. Of course, that means using array functions and possibly separate queries for manipulating them, so there's a trade-off there.
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -825,10 +826,20 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
}
var joinSubQuery string
- if err = tx.QueryRow(deliveryservice.HasRequiredCapabilitiesQuery, dsID).Scan(&dsHasRequiredCapabilities); err != nil {
+ rows, err := tx.Query(deliveryservice.GetRequiredCapabilitiesQuery, dsID)
+ if err != nil {
err = fmt.Errorf("unable to get required capabilities for deliveryservice %d: %s", dsID, err)
return nil, 0, nil, err, http.StatusInternalServerError, nil
}
+ for rows.Next() {
+ if err = rows.Scan(pq.Array(&requiredCapabilities)); err != nil {
+ err = fmt.Errorf("unable to scan required capabilities for deliveryservice %d: %s", dsID, err)
Review Comment:
creating an error from an error should wrap with the `%w` formatting verb
##########
traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go:
##########
@@ -1584,12 +1591,37 @@ func Validate(tx *sql.Tx, ds *tc.DeliveryServiceV5) error {
if err := validateTypeFields(tx, ds); err != nil {
errs = append(errs, fmt.Errorf("type fields: %w", err))
}
+ if err := validateRequiredCapabilities(tx, ds); err != nil {
+ errs = append(errs, errors.New("required capabilities: "+err.Error()))
+ }
if len(errs) == 0 {
return nil
}
return util.JoinErrs(errs)
}
+func validateRequiredCapabilities(tx *sql.Tx, ds *tc.DeliveryServiceV5) error {
+ var valid bool
+ query := `SELECT $1 <@ (SELECT ARRAY_AGG(name) FROM server_capability)`
+ if ds.RequiredCapabilities != nil && len(ds.RequiredCapabilities) > 0 {
Review Comment:
the nil check here is superfluous, `len(nil)` is zero, so the second check covers it
##########
traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go:
##########
@@ -1584,12 +1591,37 @@ func Validate(tx *sql.Tx, ds *tc.DeliveryServiceV5) error {
if err := validateTypeFields(tx, ds); err != nil {
errs = append(errs, fmt.Errorf("type fields: %w", err))
}
+ if err := validateRequiredCapabilities(tx, ds); err != nil {
+ errs = append(errs, errors.New("required capabilities: "+err.Error()))
+ }
if len(errs) == 0 {
return nil
}
return util.JoinErrs(errs)
}
+func validateRequiredCapabilities(tx *sql.Tx, ds *tc.DeliveryServiceV5) error {
Review Comment:
This function returns both raw database errors and user-friendly errors, but doesn't distinguish between which is which. It should probably return two 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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7224: Add required capabilities to delivery service structure
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#discussion_r1048646070
##########
traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go:
##########
@@ -1584,12 +1591,53 @@ func Validate(tx *sql.Tx, ds *tc.DeliveryServiceV5) error {
if err := validateTypeFields(tx, ds); err != nil {
errs = append(errs, fmt.Errorf("type fields: %w", err))
}
+ userErr, sysErr := validateRequiredCapabilities(tx, ds)
+ if userErr != nil {
+ errs = append(errs, errors.New("required capabilities: "+userErr.Error()))
+ }
+ if sysErr != nil {
+ errs = append(errs, errors.New("reading/ scanning required capabilities: "+sysErr.Error()))
Review Comment:
constructing errors from errors should wrap them e.g. using `fmt.Errorf` with the `%w` formatting parameter.
Also, this is returning a system-only error to the user, by appending it to a list of user-facing errors. If a system error occurs, a `500 Internal Server Error` must be written back to the client, and no information about server-side problems or configuration should be made visible to the client. The best way to do this would be to change this function to return two errors, one system and one user-facing, so that you can handle it appropriately e.g. with `api.HandleErr`, but you could also conceivably handle it by returning a "dummy error" from a package-internal constant and then the caller has to check that the returned error is that error using `errors.Is` and do something special in that case. I'm not convinced that this approach is less work than just changing the function's call signature, but however you want to accomplish that without ignoring the server-side error is fine.
##########
traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go:
##########
@@ -1584,12 +1591,53 @@ func Validate(tx *sql.Tx, ds *tc.DeliveryServiceV5) error {
if err := validateTypeFields(tx, ds); err != nil {
errs = append(errs, fmt.Errorf("type fields: %w", err))
}
+ userErr, sysErr := validateRequiredCapabilities(tx, ds)
+ if userErr != nil {
+ errs = append(errs, errors.New("required capabilities: "+userErr.Error()))
+ }
+ if sysErr != nil {
+ errs = append(errs, errors.New("reading/ scanning required capabilities: "+sysErr.Error()))
+ }
if len(errs) == 0 {
return nil
}
return util.JoinErrs(errs)
}
+func validateRequiredCapabilities(tx *sql.Tx, ds *tc.DeliveryServiceV5) (error, error) {
+ missing := make([]string, 0)
+ var missingCap string
+ query := `SELECT missing
+FROM (
+ SELECT UNNEST($1::TEXT[])
+ EXCEPT
+ SELECT UNNEST(ARRAY_AGG(name)) FROM server_capability) t(missing)
Review Comment:
I think creating this anonymous table is entirely unnecessary. It seems that `SELECT col FROM (*table-valued expression with one column*) t(col)` would always be the same as just the `*table-valued expression with one column*`, because the expression only has one column and you are selecting that column from every row in the expression. Similarly, `UNNEST` converts an array into a set of elements and `ARRAY_AGG` aggregates a set of elements into an array, so they are more or less inverse functions. That is, `UNNEST(ARRAY_AGG(x))` is just the same as `x`, so there's no reason to wrap it those operations.
##########
traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go:
##########
@@ -1584,12 +1591,53 @@ func Validate(tx *sql.Tx, ds *tc.DeliveryServiceV5) error {
if err := validateTypeFields(tx, ds); err != nil {
errs = append(errs, fmt.Errorf("type fields: %w", err))
}
+ userErr, sysErr := validateRequiredCapabilities(tx, ds)
+ if userErr != nil {
+ errs = append(errs, errors.New("required capabilities: "+userErr.Error()))
+ }
+ if sysErr != nil {
+ errs = append(errs, errors.New("reading/ scanning required capabilities: "+sysErr.Error()))
+ }
if len(errs) == 0 {
return nil
}
return util.JoinErrs(errs)
}
+func validateRequiredCapabilities(tx *sql.Tx, ds *tc.DeliveryServiceV5) (error, error) {
+ missing := make([]string, 0)
+ var missingCap string
+ query := `SELECT missing
+FROM (
+ SELECT UNNEST($1::TEXT[])
+ EXCEPT
+ SELECT UNNEST(ARRAY_AGG(name)) FROM server_capability) t(missing)
+`
+ if len(ds.RequiredCapabilities) > 0 {
+ rows, err := tx.Query(query, pq.Array(ds.RequiredCapabilities))
+ if err != nil {
+ return nil, err
+ }
+ defer rows.Close()
+ for rows.Next() {
+ err = rows.Scan(&missingCap)
+ if err != nil {
+ return nil, err
+ }
+ missing = append(missing, missingCap)
+ }
+ if len(missing) > 0 {
+ var msg string
+ for _, m := range missing {
+ msg = msg + m + " "
+ }
+ userErr := fmt.Errorf("the following capabilities do not exist: %s", msg)
Review Comment:
there's a standard library function for this: [`strings.Join`](https://pkg.go.dev/strings#Join)
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -825,10 +826,20 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
}
var joinSubQuery string
- if err = tx.QueryRow(deliveryservice.HasRequiredCapabilitiesQuery, dsID).Scan(&dsHasRequiredCapabilities); err != nil {
+ rows, err := tx.Query(deliveryservice.GetRequiredCapabilitiesQuery, dsID)
Review Comment:
Does the `HasRequiredCapabilitiesQuery` not work here anymore? I would assume that's because of the structural changes to the table(s). So should the old query just be deleted, since we know it doesn't work?
It looks like this still returns a single row, so you could keep it shorter by continuing to use `QueryRow`.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7224: Add required capabilities to delivery service structure
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a diff in pull request #7224: Add required capabilities to delivery service structure
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on code in PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#discussion_r1041501933
##########
traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go:
##########
@@ -850,13 +850,11 @@ func GetRequiredCapabilitiesOfDeliveryServices(ids []int, tx *sql.Tx) (map[int][
dsCaps := make(map[int][]string, len(ids))
q := `
SELECT
- d.id,
- ARRAY_REMOVE(ARRAY_AGG(dsrc.required_capability ORDER BY dsrc.required_capability), NULL) AS required_capabilities
-FROM deliveryservice d
-LEFT JOIN deliveryservices_required_capability dsrc on d.id = dsrc.deliveryservice_id
-WHERE
- d.id = ANY($1)
-GROUP BY d.id
+ ds.id,
+ ARRAY_REMOVE((ds.required_capabilities), NULL) AS required_capabilities
Review Comment:
Yeah I just wanted to reduce the need for extra queries, and handle everything in the DS table. I have made the changes to modify the "null" values to "[]". Lmk if I've missed anything.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a diff in pull request #7224: Add required capabilities to delivery service structure
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on code in PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#discussion_r1048755219
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -825,10 +826,20 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
}
var joinSubQuery string
- if err = tx.QueryRow(deliveryservice.HasRequiredCapabilitiesQuery, dsID).Scan(&dsHasRequiredCapabilities); err != nil {
+ rows, err := tx.Query(deliveryservice.GetRequiredCapabilitiesQuery, dsID)
Review Comment:
The `HasRequiredCapabilitiesQuery` has already been deleted.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 merged pull request #7224: Add required capabilities to delivery service structure
Posted by GitBox <gi...@apache.org>.
ocket8888 merged PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7224: Add required capabilities to delivery service structure
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#discussion_r1042523146
##########
lib/go-tc/deliveryservices.go:
##########
@@ -1632,7 +1632,11 @@ func (ds DeliveryServiceV5) Downgrade() DeliveryServiceV4 {
copy(*downgraded.MatchList, ds.MatchList)
}
copy(downgraded.TLSVersions, ds.TLSVersions)
-
+ if len(ds.RequiredCapabilities) == 0 {
+ downgraded.RequiredCapabilities = make([]string, 0)
+ } else {
+ copy(downgraded.RequiredCapabilities, ds.RequiredCapabilities)
+ }
Review Comment:
This won't work. You can't `copy` more data into a slice than it can hold. So when the destination is a nil slice (which by definition has `len` of zero), it silently does nothing - there's no room to hold any data. So you have to `make` some allocation of data for the new slice either way.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7224: Add required capabilities to delivery service structure
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#discussion_r1036398114
##########
docs/source/api/v3/deliveryservices_required_capabilities.rst:
##########
@@ -19,6 +19,8 @@
``deliveryservices_required_capabilities``
******************************************
+.. deprecated:: ATCv7
Review Comment:
A few notes on these deprecation notices
- all of APIv3 is deprecated, so it's superfluous to note that for any specific endpoint/method thereof
- If an entire endpoint is being deprecated, you don't also need to individually deprecate each request method it supports
- Generally you want to specify the _API_ version that something was deprecated, unless it's not possible to do that for some reason or it just makes more sense to list the ATC version instead.
- Ideally the notice will come with some information about why it's being deprecated and/or what to use instead. This makes generating changelogs using Sphinx much more useful.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7224: Add required capabilities to delivery service structure
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#discussion_r1048764650
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -825,10 +826,20 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth
}
var joinSubQuery string
- if err = tx.QueryRow(deliveryservice.HasRequiredCapabilitiesQuery, dsID).Scan(&dsHasRequiredCapabilities); err != nil {
+ rows, err := tx.Query(deliveryservice.GetRequiredCapabilitiesQuery, dsID)
Review Comment:
Oh, I didn't see that change in `deliveryservices.go`, so I guess it must've been in a different file. My bad.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #7224: Add required capabilities to delivery service structure
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#issuecomment-1352222732
needs conflict resolution to run GHA
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #7224: Add required capabilities to delivery service structure
Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#issuecomment-1717970309
@srijeet0406 `TestDeliveryServicesRequiredCapabilities` is entirely commented out
https://github.com/apache/trafficcontrol/blob/319283608617c7c92db5daf0ccfd0c9a42a08905/traffic_ops/testing/api/v5/deliveryservices_required_capabilities_test.go#L28-L31
Also, the API tests still try to create and delete DS required capabilities.
--
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: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a diff in pull request #7224: Add required capabilities to delivery service structure
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on code in PR #7224:
URL: https://github.com/apache/trafficcontrol/pull/7224#discussion_r1042596304
##########
lib/go-tc/deliveryservices.go:
##########
@@ -1632,7 +1632,11 @@ func (ds DeliveryServiceV5) Downgrade() DeliveryServiceV4 {
copy(*downgraded.MatchList, ds.MatchList)
}
copy(downgraded.TLSVersions, ds.TLSVersions)
-
+ if len(ds.RequiredCapabilities) == 0 {
+ downgraded.RequiredCapabilities = make([]string, 0)
+ } else {
+ copy(downgraded.RequiredCapabilities, ds.RequiredCapabilities)
+ }
Review Comment:
Yeah sorry I copied it from another place and forgot to add the allocations. Fixing 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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org