ocket8888 commented on code in PR #6593:
URL: https://github.com/apache/trafficcontrol/pull/6593#discussion_r855593827
##########
traffic_ops/traffic_ops_golang/server/detail.go:
##########
@@ -91,7 +91,11 @@ func GetDetailParamHandler(w http.ResponseWriter, r
*http.Request) {
routerPortName = interfaces[0].RouterPortName
}
v11server := tc.ServerDetailV11{}
- v11server.ServerDetail = server.ServerDetail
+ v11server.ServerDetail, err =
dbhelpers.GetServerDetailFromV4(server, inf.Tx.Tx)
+ if err != nil {
+ api.HandleErr(w, r, inf.Tx.Tx,
http.StatusBadRequest, nil, errors.New("failed to GetServerDetailFromV4:
"+err.Error()))
Review Comment:
This error is being passed up as an internal error and nothing is sent to
the client - this should probably be returning a `500 Internal Server Error`
response not `400 Bad Request`.
Also errors constructed from errors should wrap their source error using the
`%w` formatting parameter of `fmt.Errorf`.
##########
traffic_ops/traffic_ops_golang/server/detail.go:
##########
@@ -117,7 +121,11 @@ func GetDetailParamHandler(w http.ResponseWriter, r
*http.Request) {
routerHostName = interfaces[0].RouterHostName
routerPortName = interfaces[0].RouterPortName
}
- v3Server.ServerDetail = server.ServerDetail
+ v3Server.ServerDetail, err =
dbhelpers.GetServerDetailFromV4(server, inf.Tx.Tx)
+ if err != nil {
+ api.HandleErr(w, r, inf.Tx.Tx,
http.StatusBadRequest, nil, errors.New("failed to GetServerDetailFromV4:
"+err.Error()))
Review Comment:
same as above RE: response code and error wrapping
##########
traffic_ops/traffic_ops_golang/server/detail_test.go:
##########
@@ -93,8 +94,6 @@ func TestGetDetailServers(t *testing.T) {
serviceNetmask := util.StrPtr("")
serviceInterface := util.StrPtr("")
serviceMtu := util.StrPtr("")
- //routerHostName := util.StrPtr("")
- //routerPort := util.StrPtr("")
Review Comment:
why were these commented out?
##########
traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go:
##########
@@ -2091,6 +2091,43 @@ func UpdateServerProfileTableForV2V3(id *int, newProfile
*string, origProfile st
return profileName, nil
}
+// GetServerDetailFromV4 function converts server details from V4 to V3/V2
+func GetServerDetailFromV4(sd tc.ServerDetailV40, tx *sql.Tx)
(tc.ServerDetail, error) {
+ var profileDesc *string
+ if err := tx.QueryRow(`SELECT p.description FROM profile p WHERE
p.name=$1`, sd.ProfileNames[0]).Scan(&profileDesc); err != nil {
+ return tc.ServerDetail{}, fmt.Errorf("querying profile
description by profile name: " + err.Error())
+ }
+ return tc.ServerDetail{
+ CacheGroup: sd.CacheGroup,
+ CDNName: sd.CDNName,
+ DeliveryServiceIDs: sd.DeliveryServiceIDs,
+ DomainName: sd.DomainName,
+ GUID: sd.GUID,
+ HardwareInfo: sd.HardwareInfo,
+ HostName: sd.HostName,
+ HTTPSPort: sd.HTTPSPort,
+ ID: sd.ID,
+ ILOIPAddress: sd.ILOIPAddress,
+ ILOIPGateway: sd.ILOIPGateway,
+ ILOIPNetmask: sd.ILOIPNetmask,
+ ILOPassword: sd.ILOPassword,
+ ILOUsername: sd.ILOUsername,
+ MgmtIPAddress: sd.MgmtIPAddress,
+ MgmtIPGateway: sd.MgmtIPGateway,
+ MgmtIPNetmask: sd.MgmtIPNetmask,
+ OfflineReason: sd.OfflineReason,
+ PhysLocation: sd.PhysLocation,
+ Profile: &sd.ProfileNames[0],
+ ProfileDesc: profileDesc,
+ Rack: sd.Rack,
+ Status: sd.Status,
+ TCPPort: sd.TCPPort,
+ Type: "",
Review Comment:
shouldn't `Type` be populated from `sd.Type` like everything else?
##########
traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go:
##########
@@ -2091,6 +2091,43 @@ func UpdateServerProfileTableForV2V3(id *int, newProfile
*string, origProfile st
return profileName, nil
}
+// GetServerDetailFromV4 function converts server details from V4 to V3/V2
+func GetServerDetailFromV4(sd tc.ServerDetailV40, tx *sql.Tx)
(tc.ServerDetail, error) {
+ var profileDesc *string
+ if err := tx.QueryRow(`SELECT p.description FROM profile p WHERE
p.name=$1`, sd.ProfileNames[0]).Scan(&profileDesc); err != nil {
+ return tc.ServerDetail{}, fmt.Errorf("querying profile
description by profile name: " + err.Error())
Review Comment:
errors built from errors should wrap with `%w`
##########
lib/go-tc/servers.go:
##########
@@ -87,8 +87,33 @@ type ServerDetailV30 struct {
// ServerDetailV40 is the details for a server for API v4.
type ServerDetailV40 struct {
- ServerDetail
- ServerInterfaces []ServerInterfaceInfoV40 `json:"interfaces"`
+ CacheGroup *string `json:"cachegroup"
db:"cachegroup"`
+ CDNName *string `json:"cdnName"
db:"cdn_name"`
+ DeliveryServiceIDs []int64
`json:"deliveryservices,omitempty"`
+ DomainName *string `json:"domainName"
db:"domain_name"`
+ GUID *string `json:"guid" db:"guid"`
+ HardwareInfo map[string]string `json:"hardwareInfo"`
Review Comment:
Do you want to just get rid of this? "Hardware Info" as an ATC concept was
removed with APIv1, so this is always null. Since Go treats `null` and
`undefined` the same way, and our API ignores unrecognized request fields, I
wouldn't even really consider that a breaking change.
--
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]
