ocket8888 commented on code in PR #6996:
URL: https://github.com/apache/trafficcontrol/pull/6996#discussion_r936070291
##########
lib/go-tc/server_server_capability.go:
##########
@@ -27,6 +27,11 @@ type ServerServerCapability struct {
ServerCapability *string `json:"serverCapability"
db:"server_capability"`
}
+type MultipleServerCapabilities struct {
+ ServerID *int `json:"serverId" db:"server"`
Review Comment:
I don't think this property is optional, so it need not be a pointer.
Also; nit but `serverId` should be `serverID` to be properly `camelCased`
##########
traffic_ops/traffic_ops_golang/routing/routes.go:
##########
@@ -131,6 +131,9 @@ func Routes(d ServerData) ([]Route, http.Handler, error) {
* 4.x API
*/
+ // Assign Multiple Server Capabilities
+ {Version: api.Version{Major: 4, Minor: 1}, Method:
http.MethodPut, Path: `multiple_server_capabilities/{id}$`, Handler:
server.AssignMultipleServerCapabilities, RequiredPrivLevel:
auth.PrivLevelOperations, RequiredPermissions: []string{"SERVER:UPDATE",
"SERVER:CREATE", "SERVER:READ", "SERVER-CAPABILITY:READ"}, Authenticated:
Authenticated, Middlewares: nil, ID: 40792419258},
Review Comment:
Do you need to be able to create servers to use this route? Currently,
assigning a single Capability to a single server does not require
`SERVER:CREATE`.
##########
traffic_ops/v4-client/endpoints.go:
##########
@@ -22,6 +22,6 @@ package client
// Versions are ordered latest-first.
func apiVersions() []string {
return []string{
- "4.0",
+ "4.1",
Review Comment:
This should list both minor versions, not just the latest.
##########
lib/go-tc/server_server_capability.go:
##########
@@ -27,6 +27,11 @@ type ServerServerCapability struct {
ServerCapability *string `json:"serverCapability"
db:"server_capability"`
}
+type MultipleServerCapabilities struct {
Review Comment:
This structure could use a GoDoc
##########
traffic_ops/v4-client/server_server_capabilities.go:
##########
@@ -53,3 +56,11 @@ func (to *Session) GetServerServerCapabilities(opts
RequestOptions) (tc.ServerSe
reqInf, err := to.get(apiServerServerCapabilities, opts, &resp)
return resp, reqInf, err
}
+
+// AssignMultipleServerCapability assign multiple server capabilities to a
server
Review Comment:
nit: GoDoc comments should be complete sentences ending with punctuation.
##########
traffic_ops/traffic_ops_golang/server/servers_server_capability.go:
##########
@@ -441,3 +442,87 @@ func getDSTenantIDsByIDs(tx *sqlx.Tx, dsIDs []int64)
([]DSTenant, error) {
return dsTenantIDs, nil
}
+
+// AssignMultipleServerCapabilities helps assign multiple server capabilities
to a given server
Review Comment:
nit: GoDoc comments should be complete sentences ending with punctuation.
##########
traffic_ops/traffic_ops_golang/server/servers_server_capability.go:
##########
@@ -441,3 +442,87 @@ func getDSTenantIDsByIDs(tx *sqlx.Tx, dsIDs []int64)
([]DSTenant, error) {
return dsTenantIDs, nil
}
+
+// AssignMultipleServerCapabilities helps assign multiple server capabilities
to a given server
+func AssignMultipleServerCapabilities(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"},
[]string{"id"})
+ tx := inf.Tx.Tx
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ var msc tc.MultipleServerCapabilities
+ if err := json.NewDecoder(r.Body).Decode(&msc); err != nil {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, err, nil)
+ return
+ }
+
+ // Check existence prior to checking type
+ _, exists, err := dbhelpers.GetServerNameFromID(tx,
int64(*msc.ServerID))
+ if err != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil,
err)
+ }
+ if !exists {
+ userErr := fmt.Errorf("server %v does not exist", *msc.ServerID)
+ api.HandleErr(w, r, tx, http.StatusNotFound, userErr, nil)
+ return
+ }
+
+ // Ensure type is correct
+ correctType := true
+ if err := tx.QueryRow(scCheckServerTypeQuery(),
msc.ServerID).Scan(&correctType); err != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil,
fmt.Errorf("checking server type: %v", err))
+ return
+ }
+ if !correctType {
+ userErr := fmt.Errorf("server %v has an incorrect server type.
Server capabilities can only be assigned to EDGE or MID servers", *msc.ServerID)
Review Comment:
Same as above RE: nit: formatting parameter is more general than it could be
##########
traffic_ops/traffic_ops_golang/server/servers_server_capability.go:
##########
@@ -441,3 +442,87 @@ func getDSTenantIDsByIDs(tx *sqlx.Tx, dsIDs []int64)
([]DSTenant, error) {
return dsTenantIDs, nil
}
+
+// AssignMultipleServerCapabilities helps assign multiple server capabilities
to a given server
+func AssignMultipleServerCapabilities(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"},
[]string{"id"})
+ tx := inf.Tx.Tx
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ var msc tc.MultipleServerCapabilities
+ if err := json.NewDecoder(r.Body).Decode(&msc); err != nil {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, err, nil)
+ return
+ }
+
+ // Check existence prior to checking type
+ _, exists, err := dbhelpers.GetServerNameFromID(tx,
int64(*msc.ServerID))
+ if err != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil,
err)
+ }
+ if !exists {
+ userErr := fmt.Errorf("server %v does not exist", *msc.ServerID)
Review Comment:
nit: formatting parameter is more general than it could be, should use `%d`
instead of `%v`.
##########
docs/source/api/v4/multiple_server_capabilities.rst:
##########
@@ -0,0 +1,81 @@
+..
+..
+.. Licensed 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.
+..
+
+.. _to-api-multiple_server_capabilities:
+
+********************************
+``multiple_server_capabilities``
+********************************
+
+``PUT``
+========
+Associates a list of :term:`Server Capability` to a server.
+
+:Auth. Required: Yes
+:Roles Required: "admin" or "operations"
+:Permissions Required: SERVER:UPDATE, SERVER:CREATE, SERVER:READ,
SERVER-CAPABILITY:READ
+:Response Type: Object
+
+Request Structure
+-----------------
+:serverId: The integral, unique identifier of a server to be
associated with a :term:`Server Capability`
+:serverCapability: List of :term:`Server Capability`'s name to associate
+
+.. code-block:: http
+ :caption: Request Example
+
+ PUT /api/4.1/multiple_server_capabilities/1 HTTP/1.1
+ Host: trafficops.infra.ciab.test
+ User-Agent: curl/7.47.0
+ Accept: */*
+ Cookie: mojolicious=...
+ Content-Length: 84
+ Content-Type: application/json
+
+ {
+ "serverId": 1,
+ "serverCapability": ["test", "disk"]
+ }
Review Comment:
This block mixes real indentation with spaces, the code block should be
indented using real indentation, although its content can uses spaces to offset
itself from the end of the block's indentation.
##########
traffic_ops/v4-client/server_server_capabilities.go:
##########
@@ -16,17 +16,20 @@ package client
*/
import (
+ "fmt"
"net/url"
"strconv"
"github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/traffic_ops/toclientlib"
)
-// apiServerServerCapabilities is the API version-relative path to the
-// /server_server_capabilities API endpoint.
+// apiServerServerCapabilities is the API version-relative path to the
/server_server_capabilities API endpoint.
Review Comment:
Why'd you modify this comment?
##########
docs/source/api/v4/multiple_server_capabilities.rst:
##########
@@ -0,0 +1,81 @@
+..
+..
+.. Licensed 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.
+..
+
+.. _to-api-multiple_server_capabilities:
+
+********************************
+``multiple_server_capabilities``
+********************************
+
Review Comment:
There should be a `.. versionadded::` directive here that lets readers know
this was only added in 4.1 and won't be available in 4.0.
##########
traffic_ops/traffic_ops_golang/server/servers_server_capability.go:
##########
@@ -441,3 +442,87 @@ func getDSTenantIDsByIDs(tx *sqlx.Tx, dsIDs []int64)
([]DSTenant, error) {
return dsTenantIDs, nil
}
+
+// AssignMultipleServerCapabilities helps assign multiple server capabilities
to a given server
+func AssignMultipleServerCapabilities(w http.ResponseWriter, r *http.Request) {
+ inf, userErr, sysErr, errCode := api.NewInfo(r, []string{"id"},
[]string{"id"})
+ tx := inf.Tx.Tx
+ if userErr != nil || sysErr != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr)
+ return
+ }
+ defer inf.Close()
+
+ var msc tc.MultipleServerCapabilities
+ if err := json.NewDecoder(r.Body).Decode(&msc); err != nil {
+ api.HandleErr(w, r, tx, http.StatusBadRequest, err, nil)
+ return
+ }
+
+ // Check existence prior to checking type
+ _, exists, err := dbhelpers.GetServerNameFromID(tx,
int64(*msc.ServerID))
+ if err != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil,
err)
+ }
+ if !exists {
+ userErr := fmt.Errorf("server %v does not exist", *msc.ServerID)
+ api.HandleErr(w, r, tx, http.StatusNotFound, userErr, nil)
+ return
+ }
+
+ // Ensure type is correct
+ correctType := true
+ if err := tx.QueryRow(scCheckServerTypeQuery(),
msc.ServerID).Scan(&correctType); err != nil {
+ api.HandleErr(w, r, tx, http.StatusInternalServerError, nil,
fmt.Errorf("checking server type: %v", err))
Review Comment:
creating an error from an error should use a wrapping formatting parameter -
`%w` - instead of `%v`
##########
docs/source/api/v4/multiple_server_capabilities.rst:
##########
@@ -0,0 +1,81 @@
+..
+..
+.. Licensed 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.
+..
+
+.. _to-api-multiple_server_capabilities:
+
+********************************
+``multiple_server_capabilities``
+********************************
+
+``PUT``
+========
+Associates a list of :term:`Server Capability` to a server.
+
+:Auth. Required: Yes
+:Roles Required: "admin" or "operations"
+:Permissions Required: SERVER:UPDATE, SERVER:CREATE, SERVER:READ,
SERVER-CAPABILITY:READ
+:Response Type: Object
+
+Request Structure
+-----------------
+:serverId: The integral, unique identifier of a server to be
associated with a :term:`Server Capability`
+:serverCapability: List of :term:`Server Capability`'s name to associate
+
+.. code-block:: http
+ :caption: Request Example
+
+ PUT /api/4.1/multiple_server_capabilities/1 HTTP/1.1
+ Host: trafficops.infra.ciab.test
+ User-Agent: curl/7.47.0
+ Accept: */*
+ Cookie: mojolicious=...
+ Content-Length: 84
+ Content-Type: application/json
+
+ {
+ "serverId": 1,
+ "serverCapability": ["test", "disk"]
+ }
+
+Response Structure
+------------------
+:serverId: The integral, unique identifier of the newly associated
server
+:serverCapability: List of :term:`Server Capability`'s name
+
+.. code-block:: http
+ :caption: Response Example
+
+ HTTP/1.1 200 OK
+ Access-Control-Allow-Credentials: true
+ 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-Type: application/json
+ Set-Cookie: mojolicious=...; Path=/; Expires=Mon, 8 Aug 2022 17:40:54
GMT; Max-Age=3600; HttpOnly
+ Whole-Content-Sha512:
eQrl48zWids0kDpfCYmmtYMpegjnFxfOVvlBYxxLSfp7P7p6oWX4uiC+/Cfh2X9i3G+MQ36eH95gukJqOBOGbQ==
+ X-Server-Name: traffic_ops_golang/
+ Date: Mon, 02 Aug 2022 22:15:11 GMT
+ Content-Length: 157
+
+ {
+ "alerts": [{
+ "text": "Multiple Server Capabilities assigned to a server:1",
+ "level": "success"
+ }],
+ "response": {
+ "serverId": 1,
+ "serverCapability": ["test", "disk"]
+ }
+ }
Review Comment:
Same as above RE: indentation
--
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]
