ocket8888 commented on code in PR #7750:
URL: https://github.com/apache/trafficcontrol/pull/7750#discussion_r1309313847
##########
traffic_ops/testing/api/v5/cdn_locks_test.go:
##########
@@ -123,19 +123,21 @@ func TestCDNLocks(t *testing.T) {
ClientSession: opsUserWithLockSession,
RequestOpts:
client.RequestOptions{QueryParameters: url.Values{"hostName":
{"cdn2-test-edge"}}},
RequestBody: map[string]interface{}{
- "config_apply_time":
util.TimePtr(now),
+ "config_apply_time":
util.Ptr(now),
+ "config_update_failed":
util.Ptr(true),
Review Comment:
need not use `util.Ptr` on non-reference field. I actually don't think it's
necessary for either of these, since a non-`nil` value marshals identical to a
non-reference value.
##########
traffic_ops/v5-client/server_update_status.go:
##########
@@ -52,11 +53,11 @@ func (to *Session) SetServerQueueUpdate(serverID int,
queueUpdate bool, opts Req
// SetUpdateServerStatusTimes updates a server's config queue status and/or
reval status.
// Each argument individually is optional, however at least one argument must
not be nil.
-func (to *Session) SetUpdateServerStatusTimes(serverName string,
configApplyTime, revalApplyTime *time.Time, opts RequestOptions) (tc.Alerts,
toclientlib.ReqInf, error) {
+func (to *Session) SetUpdateServerStatusTimes(serverName string,
configApplyTime, revalApplyTime *time.Time, configUpdateFailed,
revalUpdateFailed *bool, opts RequestOptions) (tc.Alerts, toclientlib.ReqInf,
error) {
reqInf := toclientlib.ReqInf{CacheHitStatus:
toclientlib.CacheHitStatusMiss}
var alerts tc.Alerts
- if configApplyTime == nil && revalApplyTime == nil {
+ if configApplyTime == nil && revalApplyTime == nil &&
configUpdateFailed == nil && revalUpdateFailed == nil {
return alerts, reqInf, errors.New("one must be non-nil
(configApplyTime, revalApplyTime); nothing to do")
Review Comment:
error message no longer fully describes erroneous calls
##########
traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go:
##########
@@ -1975,6 +1975,28 @@ WHERE server.id = $2;`
return nil
}
+func SetUpdateFailedForServer(tx *sql.Tx, serverID int64, failed bool) error {
+ query := `
+UPDATE public.server
+SET config_update_failed = $1
+WHERE server.id = $2`
+ if _, err := tx.Exec(query, failed, serverID); err != nil {
+ return fmt.Errorf("setting config update failed for ServerID %d
with value %v: %v", serverID, failed, err)
+ }
+ return nil
+}
+
+func SetRevalFailedForServer(tx *sql.Tx, serverID int64, failed bool) error {
+ query := `
+UPDATE public.server
+SET revalidate_update_failed = $1
+WHERE server.id = $2`
+ if _, err := tx.Exec(query, failed, serverID); err != nil {
+ return fmt.Errorf("setting reval update failed for ServerID %d
with value %v: %v", serverID, failed, err)
Review Comment:
should use wrapping error construction
##########
traffic_ops/app/db/migrations/2023082308560644_server_update_status.up.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.server
+ ADD COLUMN IF NOT EXISTS config_update_failed boolean DEFAULT false,
+ ADD COLUMN IF NOT EXISTS revalidate_update_failed boolean DEFAULT false;
Review Comment:
Since these have default values, `NULL` values have no valid use or meaning,
and scanning into a non-reference struct field would fail for `NULL` values, do
you want to make these columns `NOT NULL`?
##########
traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go:
##########
@@ -1975,6 +1975,28 @@ WHERE server.id = $2;`
return nil
}
+func SetUpdateFailedForServer(tx *sql.Tx, serverID int64, failed bool) error {
+ query := `
+UPDATE public.server
+SET config_update_failed = $1
+WHERE server.id = $2`
+ if _, err := tx.Exec(query, failed, serverID); err != nil {
+ return fmt.Errorf("setting config update failed for ServerID %d
with value %v: %v", serverID, failed, err)
+ }
+ return nil
+}
+
+func SetRevalFailedForServer(tx *sql.Tx, serverID int64, failed bool) error {
Review Comment:
exported symbol should have a GoDoc comment
##########
traffic_ops/testing/api/v5/servers_hostname_update_test.go:
##########
@@ -125,9 +137,15 @@ func validateServerApplyTimes(hostName string,
expectedResp map[string]interface
case "ConfigApplyTime":
assert.RequireNotNil(t,
resp.Response[0].ConfigApplyTime, "Expected ConfigApplyTime to not be nil.")
assert.Equal(t, true,
server.ConfigApplyTime.Equal(expected.(time.Time)), "Expected ConfigApplyTime
to be %v, but got %v", expected, server.ConfigApplyTime)
+ case "ConfigUpdateFailed":
+ assert.RequireNotNil(t,
resp.Response[0].ConfigUpdateFailed, "Expected ConfigUpdateFailed to not be
nil.")
+ assert.Equal(t, expected.(bool),
resp.Response[0].ConfigUpdateFailed, "Expected ConfigUpdateFailed to be %v, but
got %v", expected, server.ConfigUpdateFailed)
case "RevalApplyTime":
assert.RequireNotNil(t,
resp.Response[0].RevalApplyTime, "Expected RevalApplyTime to not be nil.")
assert.Equal(t, true,
server.RevalApplyTime.Equal(expected.(time.Time)), "Expected RevalApplyTime to
be %v, but got %v", expected, server.RevalApplyTime)
+ case "RevalUpdateFailed":
+ assert.RequireNotNil(t,
resp.Response[0].RevalUpdateFailed, "Expected RevalUpdateFailed to not be nil.")
Review Comment:
this check cannot possibly fail because non-reference fields can never be
`nil`
##########
traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go:
##########
@@ -1975,6 +1975,28 @@ WHERE server.id = $2;`
return nil
}
+func SetUpdateFailedForServer(tx *sql.Tx, serverID int64, failed bool) error {
Review Comment:
exported symbol should have a GoDoc comment
##########
traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go:
##########
@@ -1975,6 +1975,28 @@ WHERE server.id = $2;`
return nil
}
+func SetUpdateFailedForServer(tx *sql.Tx, serverID int64, failed bool) error {
+ query := `
+UPDATE public.server
+SET config_update_failed = $1
+WHERE server.id = $2`
+ if _, err := tx.Exec(query, failed, serverID); err != nil {
+ return fmt.Errorf("setting config update failed for ServerID %d
with value %v: %v", serverID, failed, err)
Review Comment:
should use wrapping error construction
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
