[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1025641073
##
traffic_ops/testing/api/utils/utils.go:
##
@@ -129,7 +140,7 @@ type V3TestDataT[B any] struct {
// V4TestData represents the data needed for testing the v4 api endpoints.
type V4TestData struct {
- EndpointIdfunc() int
+ EndpointIDfunc() int
Review Comment:
I'm going to resolve all of these that relate to the conversation on the
above review 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.
To unsubscribe, e-mail: issues-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1025640302
##
traffic_ops/testing/api/utils/utils.go:
##
@@ -119,7 +130,7 @@ type V3TestData struct {
// V3TestDataT represents the data needed for testing the v3 api endpoints.
type V3TestDataT[B any] struct {
- EndpointId func() int
+ EndpointID func() int
Review Comment:
> ...it looks like https://github.com/apache/trafficcontrol/pull/7099 is
missing test coverage for the "PRIMED" DeliveryServiceActiveState.
I see two Delivery Services in the fixtures data that use that active state,
so it seems to be covered to me. It's also worth noting that all Delivery
Services with `active: false` in APIv3 and APIv4 tests are implicitly using
"PRIMED"; technically the new state is "INACTIVE", not "PRIMED".
> ... missing coverage for an invalid DeliveryServiceActiveState being
POSTed or PUT.
That's true. Ideally it should have both.
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1025615404
##
traffic_ops/testing/api/utils/utils.go:
##
@@ -119,7 +130,7 @@ type V3TestData struct {
// V3TestDataT represents the data needed for testing the v3 api endpoints.
type V3TestDataT[B any] struct {
- EndpointId func() int
+ EndpointID func() int
Review Comment:
Removing is very simple (if somewhat annoying because I already did that and
then un-did it) but I don't think that work fully captures what Rima means by
"it would be more work at this point to remove it". It doesn't belong in the
PR, that's true, but it's already been reviewed as a part of this PR. It was a
_simple_ change, but not a _small_ one. So to pull that out into a separate PR
is duplicating work already spent on reviewing those changes here, which isn't
insignificant. Having already volunteered yourself for the task, it's clear
you're prepared to take on that tedium, but consider that the time it took Rima
to review it already will have been wasted, which is why she asked me to put it
back in after I took it out.
At this point I've put it in, taken it out, and put it back in again. I
truly do not care if it goes in this PR, but if I change it again I want to be
assured it'll be the last time. Perhaps get a third reviewer to break the tie.
Or don't if you can come to a consensus without that. But I just need to know
what I have to do.
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099: URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1025603512 ## traffic_ops/traffic_ops_golang/crconfig/deliveryservice.go: ## @@ -145,7 +145,7 @@ FROM deliveryservice AS d INNER JOIN type AS t ON t.id = d.type LEFT OUTER JOIN profile AS p ON p.id = d.profile WHERE d.cdn_id = (select id FROM cdn WHERE name = $1) -AND d.active = true +AND d.active = 'ACTIVE' Review Comment: > Changing all of the Go occurrences in one place would still be possible Sure, but doing that yields a fundamentally broken piece of software, so idk how useful that is. > 3 of the 6 queries I commented on are queries that already use concatenation, so concatenation should work there, as well as the 2 query fragments. Without looking at those concatenations, I would say I probably disagree with their use as well, since I know such instances exist. > For the 3 queries that don't use concatenation, passing tc.DSActiveStateActive as a parameter to the prepared statement would work (and, unlike the other cases, would not require a typecast). > > That said, if you feel that it's more maintainable as-is, I'm fine with you keeping it like that. I think that's actually probably the best course, is to pass it in for statement preparation. That seems more proper than string concatenation and makes the connection between the Go constant and the database enum even more clear. So that's what I'll do. -- 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-unsubscr...@trafficcontrol.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099: URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1025558560 ## traffic_ops/traffic_ops_golang/crconfig/deliveryservice.go: ## @@ -145,7 +145,7 @@ FROM deliveryservice AS d INNER JOIN type AS t ON t.id = d.type LEFT OUTER JOIN profile AS p ON p.id = d.profile WHERE d.cdn_id = (select id FROM cdn WHERE name = $1) -AND d.active = true +AND d.active = 'ACTIVE' Review Comment: The point of using a constant is to avoid misspellings and so you can change it once to change it everywhere - but this being an enum type in the database as well as a Go constant sort of escapes both of those use cases. Because it's an enum it's not possible to insert a misspelling; that would break the API and fail tests etc. And also if the string used to represent "Active" ever changes, it's not possible to change it in only one place, since it would require a database migration. Plus, using string concatenation for building SQL queries - besides always making me a little nervous inherently - means that knowing what you're doing is safe depends on knowing the exact string value of the constant, so an abstraction could be detrimental to that. For all of those reasons I chose deliberately not to use the Go constant in queries. If you still want me to change it I will, I just wanted to explain my reasoning. -- 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-unsubscr...@trafficcontrol.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099: URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1025546374 ## lib/go-tc/copy.go: ## @@ -0,0 +1,112 @@ +package tc Review Comment: In order for that to be true, it would need to change to exported utilities and the usage across files otherwise not touched by this PR will need to be updated. Obviously I'm not afraid to expand the scope of this PR, but I want to be sure you're aware that these are used in other places -- 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-unsubscr...@trafficcontrol.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1021899761
##
traffic_ops/testing/api/v5/tc-fixtures.json:
##
@@ -341,7 +341,7 @@
{
Review Comment:
#7189
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1021893838
##
docs/source/api/v5/deliveryservice_requests.rst:
##
@@ -663,59 +504,315 @@ The response is a full representation of the edited
:term:`Delivery Service Requ
"protocol": 2,
"qstringIgnore": 0,
"rangeRequestHandling": 0,
+ "rangeSliceBlockSize": null,
"regexRemap": null,
"regional": false,
"regionalGeoBlocking": false,
"remapText": null,
"routingName": "video",
+ "serviceCategory": null,
"signed": false,
- "sslKeyVersion": 1,
- "tenantId": 1,
- "type": "HTTP",
- "typeId": 1,
- "xmlId": "demo1",
- "exampleURLs": [
- "http://video.demo1.mycdn.ciab.test;,
- "https://video.demo1.mycdn.ciab.test;
- ],
- "deepCachingType": "NEVER",
- "fqPacingRate": null,
"signingAlgorithm": null,
+ "sslKeyVersion": 1,
"tenant": "root",
+ "tenantId": 1,
+ "tlsVersions": null,
+ "topology": "demo1-top",
"trResponseHeaders": null,
"trRequestHeaders": null,
- "consistentHashRegex": null,
+ "type": "HTTP",
+ "typeId": 1,
+ "xmlId": "demo1"
+ },
+ "requested": {
+ "active": "INACTIVE",
+ "anonymousBlockingEnabled": false,
+ "ccrDnsTtl": null,
+ "cdnId": 2,
+ "cdnName": "CDN-in-a-Box",
+ "checkPath": null,
"consistentHashQueryParams": [
"abc",
"pdq",
"xxx",
"zyx"
],
- "maxOriginConnections": 0,
+ "consistentHashRegex": null,
+ "deepCachingType": "NEVER",
+ "displayName": "Demo 1",
+ "dnsBypassCname": null,
+ "dnsBypassIp": null,
+ "dnsBypassIp6": null,
+ "dnsBypassTtl": null,
+ "dscp": 0,
"ecsEnabled": false,
- "rangeSliceBlockSize": null,
- "topology": "demo1-top",
+ "edgeHeaderRewrite": null,
+ "exampleURLs": [
+ "http://video.demo1.mycdn.ciab.test;,
+ "https://video.demo1.mycdn.ciab.test;
+ ],
"firstHeaderRewrite": null,
+ "fqPacingRate": null,
+ "geoLimit": 0,
+ "geoLimitCountries": null,
+ "geoLimitRedirectURL": null,
+ "geoProvider": 0,
+ "globalMaxMbps": null,
+ "globalMaxTps": null,
+ "httpBypassFqdn": null,
+ "id": 1,
+ "infoUrl": null,
+ "initialDispersion": 1,
"innerHeaderRewrite": null,
+ "ipv6RoutingEnabled": true,
"lastHeaderRewrite": null,
+ "lastUpdated": "2020-02-13T16:43:54Z",
Review Comment:
RFC3339 allows sub-second precision to be either included or omitted. In Go,
you can choose between the two by using `time.RFC3339` vs `time.RFC3339Nano`.
However, if the sub-second parts would all be zero (e.g. `time.Time{}`; the
"zero-value" of a time.Time) they will be left off even if you use
`time.RFC3339Nano`. So all that to say: Our API can and will produce timestamps
in RFC3339 format that include or exclude the sub-second parts, and any
consumer of the API needs to be prepared to handle both scenarios.
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1021880374
##
traffic_ops/app/db/migrations/2022110908494015_ds_active_flag.down.sql:
##
@@ -0,0 +1,74 @@
+/*
+ * 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 active_flag boolean DEFAULT FALSE NOT NULL;
+
+UPDATE public.deliveryservice
+SET active_flag = FALSE
+WHERE active = 'PRIMED' OR active = 'INACTIVE';
+
+UPDATE public.deliveryservice
+SET active_flag = TRUE
+WHERE active = 'ACTIVE';
+
+ALTER TABLE public.deliveryservice DROP COLUMN active;
+ALTER TABLE public.deliveryservice RENAME COLUMN active_flag TO active;
+DROP TYPE public.ds_active_state;
+
+UPDATE public.deliveryservice_request
+SET
+ deliveryservice = jsonb_set(deliveryservice, '{active}', 'true')
+WHERE
+ deliveryservice IS NOT NULL
+ AND
+ deliveryservice ? 'active'
+ AND
+ deliveryservice ->> 'active' = 'ACTIVE';
+UPDATE public.deliveryservice_request
Review Comment:
Need? No. I can add them if you think it improves readability, but there's
no language requirement that they be there.
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099: URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1018590003 ## traffic_ops/testing/api/v4/tc-fixtures.json: ## @@ -907,7 +907,7 @@ "regexRemap": null, "regionalGeoBlocking": false, "remapText": null, -"routingName": "cdn", +"routingName": "ccr-ds1", Review Comment: No, it seems we don't -- 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-unsubscr...@trafficcontrol.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1018500420
##
traffic_ops/testing/api/v5/tc-fixtures.json:
##
@@ -341,7 +341,7 @@
{
Review Comment:
it would seem not. The PR that added that field didn't add them to the
fixtures and the tests all pass fine. I believe that's abusing the fact that if
a field isn't found when unmarshaling, Go will just leave it as the "zero"
value of its type. It's not proper IMO, but it's also outside the scope of my
PR to fix
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099: URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1018455483 ## traffic_ops/testing/api/v3/tc-fixtures.json: ## @@ -858,7 +858,7 @@ "regexRemap": null, "regionalGeoBlocking": false, "remapText": null, -"routingName": "cdn", Review Comment: I don't think so. I really hope the tests don't start failing when I push that up -- 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-unsubscr...@trafficcontrol.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1018452494
##
docs/source/api/v5/deliveryservice_requests.rst:
##
@@ -214,18 +303,19 @@ The request must be a well-formed representation of a
:term:`Delivery Service Re
:caption: Request Example
POST /api/5.0/deliveryservice_requests HTTP/1.1
- User-Agent: python-requests/2.22.0
+ User-Agent: python-requests/2.25.1
Accept-Encoding: gzip, deflate
Accept: */*
Connection: keep-alive
- Cookie: mojolicious=...
- Content-Length: 1979
+ Cookie: access_token=...; mojolicious=...
+ Content-Length: 2011
+ Content-Type: application/json
{
"changeType": "update",
"status": "draft",
"requested": {
- "active": false,
+ "active": "INACTIVE",
Review Comment:
I'm sorry, I'm not sure I know what you mean 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.
To unsubscribe, e-mail: issues-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1018451789
##
docs/source/api/v5/deliveryservice_requests.rst:
##
@@ -663,59 +504,315 @@ The response is a full representation of the edited
:term:`Delivery Service Requ
"protocol": 2,
"qstringIgnore": 0,
"rangeRequestHandling": 0,
+ "rangeSliceBlockSize": null,
"regexRemap": null,
"regional": false,
"regionalGeoBlocking": false,
"remapText": null,
"routingName": "video",
+ "serviceCategory": null,
"signed": false,
- "sslKeyVersion": 1,
- "tenantId": 1,
- "type": "HTTP",
- "typeId": 1,
- "xmlId": "demo1",
- "exampleURLs": [
- "http://video.demo1.mycdn.ciab.test;,
- "https://video.demo1.mycdn.ciab.test;
- ],
- "deepCachingType": "NEVER",
- "fqPacingRate": null,
"signingAlgorithm": null,
+ "sslKeyVersion": 1,
"tenant": "root",
+ "tenantId": 1,
+ "tlsVersions": null,
+ "topology": "demo1-top",
"trResponseHeaders": null,
"trRequestHeaders": null,
- "consistentHashRegex": null,
+ "type": "HTTP",
+ "typeId": 1,
+ "xmlId": "demo1"
+ },
+ "requested": {
+ "active": "INACTIVE",
+ "anonymousBlockingEnabled": false,
+ "ccrDnsTtl": null,
+ "cdnId": 2,
+ "cdnName": "CDN-in-a-Box",
+ "checkPath": null,
"consistentHashQueryParams": [
"abc",
"pdq",
"xxx",
"zyx"
],
- "maxOriginConnections": 0,
+ "consistentHashRegex": null,
+ "deepCachingType": "NEVER",
+ "displayName": "Demo 1",
+ "dnsBypassCname": null,
+ "dnsBypassIp": null,
+ "dnsBypassIp6": null,
+ "dnsBypassTtl": null,
+ "dscp": 0,
"ecsEnabled": false,
- "rangeSliceBlockSize": null,
- "topology": "demo1-top",
+ "edgeHeaderRewrite": null,
+ "exampleURLs": [
+ "http://video.demo1.mycdn.ciab.test;,
+ "https://video.demo1.mycdn.ciab.test;
+ ],
"firstHeaderRewrite": null,
+ "fqPacingRate": null,
+ "geoLimit": 0,
+ "geoLimitCountries": null,
+ "geoLimitRedirectURL": null,
+ "geoProvider": 0,
+ "globalMaxMbps": null,
+ "globalMaxTps": null,
+ "httpBypassFqdn": null,
+ "id": 1,
+ "infoUrl": null,
+ "initialDispersion": 1,
"innerHeaderRewrite": null,
+ "ipv6RoutingEnabled": true,
"lastHeaderRewrite": null,
+ "lastUpdated": "2020-02-13T16:43:54Z",
Review Comment:
I'm not sure what you mean; [that *is* RFC3339
format](https://go.dev/play/p/QZuY0kwsep8).
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1018283500
##
docs/source/api/v5/deliveryservices_id.rst:
##
@@ -400,20 +404,25 @@ Response Structure
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-Encoding: gzip
Content-Type: application/json
- Set-Cookie: mojolicious=...; Path=/; Expires=Mon, 18 Nov 2019 17:40:54
GMT; Max-Age=3600; HttpOnly
- Whole-Content-Sha512:
w9NlQpJJEl56r6iYq/fk8o5WfAXeUS5XR9yDHvKUgPO8lYEo8YyftaSF0MPFseeOk60dk6kQo+MLYTDIAhhRxw==
+ Permissions-Policy: interest-cohort=()
+ Set-Cookie: mojolicious=...; Path=/; Expires=Thu, 29 Sep 2022 22:59:08
GMT; Max-Age=3600; HttpOnly, access_token=...; Path=/; Expires=Thu, 29 Sep 2022
22:59:08 GMT; Max-Age=3600; HttpOnly
+ Vary: Accept-Encoding
X-Server-Name: traffic_ops_golang/
- Date: Tue, 20 Nov 2018 14:56:37 GMT
- Content-Length: 57
+ Date: Thu, 29 Sep 2022 21:59:08 GMT
+ Content-Length: 161
{ "alerts": [
{
"text": "ds was deleted.",
"level": "success"
+ },
+ {
+ "text": "Perform a CDN snapshot then queue updates on
all servers in the cdn for the changes to take effect.",
+ "level": "info"
Review Comment:
Possibly, but that should be a separate effort, since this was added in a
different PR.
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1013301078
##
traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go:
##
@@ -138,7 +162,7 @@ func CreateV30(w http.ResponseWriter, r *http.Request) {
ds := tc.DeliveryServiceV30{}
if err := json.NewDecoder(r.Body).Decode(); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest,
errors.New("decoding: "+err.Error()), nil)
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest,
fmt.Errorf("decoding: %w", err), nil)
Review Comment:
took it out
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099: URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1005068529 ## traffic_ops/app/db/migrations/2022092107561215_ds_explicit_mso.down.sql: ## @@ -0,0 +1,20 @@ +/* + * 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 Review Comment: Those changes don't need to be reverted. Before the change, `multiSiteOrigin` can be one of three values: `true`, `false`, or `null`. The `up` file changes all `null` values to `false` (because `null` and `false` are treated exactly the same, so there was no reason to allow both). The `up` file needed to do that because after it runs, `null` will no longer be allowed. But after `down` runs, `false` is still allowed - and also it's treated the same as `null` so there's no behavioral impact, and finally you also can't know which `false`s used to be `null`s and which were `false` in the first place, so the information needed to restore that state doesn't exist anyway. -- 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-unsubscr...@trafficcontrol.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1005066584
##
traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go:
##
@@ -1023,6 +1027,7 @@ func scanDSInfoRow(row *sql.Row) (DSInfo, bool, error) {
}
return DSInfo{}, false, fmt.Errorf("querying delivery service
server ds info: %v", err)
}
+ di.Active = active == tc.DSActiveStateActive
Review Comment:
A boolean is not being assigned to a string. A boolean is being assigned to
the outcome of comparing two strings. A Delivery Service where `"active": true`
in the old data model is equivalent to `"active": "ACTIVE"` in the new data
model. So if `active` is exactly `tc.DSActiveStateActive`, then `di.Active`
should be `true`. If it's anything else, `di.Active` should be false. Which is
what this line does.
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099: URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1005065463 ## traffic_ops/testing/api/v4/tc-fixtures.json: ## @@ -907,7 +907,7 @@ "regexRemap": null, "regionalGeoBlocking": false, "remapText": null, -"routingName": "cdn", +"routingName": "ccr-ds1", Review Comment: That value is not invalid, what it was set to *before* was actually hiding an error. Changing this was part of the fix for #7094. -- 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-unsubscr...@trafficcontrol.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1005063876
##
traffic_ops/testing/api/v5/deliveryservice_request_comments_test.go:
##
@@ -178,7 +178,9 @@ func CreateTestDeliveryServiceRequestComments(t *testing.T)
{
opts.QueryParameters.Set("xmlId", comment.XMLID)
resp, _, err := TOSession.GetDeliveryServiceRequests(opts)
assert.NoError(t, err, "Cannot get Delivery Service Request by
XMLID '%s': %v - alerts: %+v", comment.XMLID, err, resp.Alerts)
- assert.Equal(t, len(resp.Response), 1, "Found %d Delivery
Service request by XMLID '%s, expected exactly one", len(resp.Response),
comment.XMLID)
+ if !assert.Equal(t, len(resp.Response), 1, "Found %d Delivery
Service request by XMLID '%s, expected exactly one", len(resp.Response),
comment.XMLID) {
Review Comment:
Because although creation of the currently processed comment has failed (or
at best succeeded wrongly), other comments can still be created, which can help
narrow down what the problem was and/or find other problems by continuing to
run other tests.
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1005062709
##
traffic_ops/v5-client/deliveryservice_requests.go:
##
@@ -58,31 +58,31 @@ func (to *Session) CreateDeliveryServiceRequest(dsr
tc.DeliveryServiceRequestV4,
dsr.AuthorID = res.Response[0].ID
}
- var ds *tc.DeliveryServiceV4
+ var ds *tc.DeliveryServiceV5
if dsr.ChangeType == tc.DSRChangeTypeDelete {
ds = dsr.Original
} else {
ds = dsr.Requested
}
- if ds.TypeID == nil && ds.Type.String() != "" {
+ if ds.TypeID <= 0 && ds.Type != nil && *ds.Type != "" {
Review Comment:
The Type is known to humans by its name, so this block was meant to populate
the ID if it wasn't given by using the Type's Name (if that *is* given,
otherwise it's an error). Because `ds.Type` is a pointer, it can be `nil`, so
its value must not be accessed in that case, or a bad error response will be
returned to the user. You also can't look up a Type with a blank Name, because
Type Names can't be blank. The old code called `.String()`, which would
segfault if the `ds.Type` reference was `nil`, so I added the `nil` check to
avoid that. `TypeID` is no longer a pointer, so instead of `nil` I checked for
the "zero" value of the property's datatype (and also invalid values i.e.
negative ones).
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1005059975
##
traffic_ops/testing/api/v5/deliveryservices_keys_test.go:
##
@@ -67,30 +67,29 @@ func createBlankCDN(cdnName string, t *testing.T) tc.CDN {
return cdns.Response[0]
}
-func cleanUp(t *testing.T, ds tc.DeliveryServiceV4, oldCDNID int, newCDNID
int, sslKeyVersions []string) {
- if ds.ID == nil || ds.XMLID == nil {
Review Comment:
Because XMLID can no longer be `nil`. If that check was still there, the
tests wouldn't compile.
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1005057765
##
traffic_ops/traffic_ops_golang/deliveryservice/safe.go:
##
@@ -144,19 +141,28 @@ func UpdateSafe(w http.ResponseWriter, r *http.Request) {
switch inf.Version.Major {
default:
fallthrough
+ case 5:
+ api.WriteRespAlertObj(w, r, tc.SuccessLevel, alertMsg,
ds)
case 4:
- api.WriteRespAlertObj(w, r, tc.SuccessLevel, alertMsg,
dses)
+ api.WriteRespAlertObj(w, r, tc.SuccessLevel, alertMsg,
[]tc.DeliveryServiceV4{ds.Downgrade()})
case 3:
+ legacyDS := ds.Downgrade()
+ legacyDS.LongDesc1 = dses[0].LongDesc1
+ legacyDS.LongDesc2 = dses[0].LongDesc2
+ ret := legacyDS.DowngradeToV31()
if inf.Version.Minor >= 1 {
- api.WriteRespAlertObj(w, r, tc.SuccessLevel,
alertMsg, []tc.DeliveryServiceV31{tc.DeliveryServiceV31(ds.DowngradeToV31())})
+ api.WriteRespAlertObj(w, r, tc.SuccessLevel,
alertMsg, []tc.DeliveryServiceV31{tc.DeliveryServiceV31(ret)})
}
- api.WriteRespAlertObj(w, r, tc.SuccessLevel, alertMsg,
[]tc.DeliveryServiceV30{ds.DowngradeToV31().DeliveryServiceV30})
+ api.WriteRespAlertObj(w, r, tc.SuccessLevel, alertMsg,
[]tc.DeliveryServiceV30{ret.DeliveryServiceV30})
case 2:
Review Comment:
Yes, though when I first made these changes we had not, yet.
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #7099: Delivery Service Active Flag Rework
ocket commented on code in PR #7099:
URL: https://github.com/apache/trafficcontrol/pull/7099#discussion_r1005057324
##
traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go:
##
@@ -138,7 +162,7 @@ func CreateV30(w http.ResponseWriter, r *http.Request) {
ds := tc.DeliveryServiceV30{}
if err := json.NewDecoder(r.Body).Decode(); err != nil {
- api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest,
errors.New("decoding: "+err.Error()), nil)
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest,
fmt.Errorf("decoding: %w", err), nil)
Review Comment:
> Why the change?
Because it preserves the identity of the underlying error that way. Here's
[a playground link to an example program that shows what that
means](https://go.dev/play/p/GOkCOgci1lh). It accomplishes the same thing as
`errors.New(someString+err.Error())` but without destroying any information.
> ...shouldn't we refactor others in the code base too?
In my opinion, yes, and I always request people do it this way in my
reviews. I only changed it in the files I was already editing, though, because
this is done untold thousands of times throughout ATC. Nobody would ever review
that PR. So I just make smaller changes to files as I work on them.
--
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-unsubscr...@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
