srijeet0406 commented on code in PR #7596:
URL: https://github.com/apache/trafficcontrol/pull/7596#discussion_r1258625198
##########
lib/go-tc/federation_resolver.go:
##########
@@ -50,6 +51,36 @@ type FederationResolver struct {
TypeID *uint `json:"typeId,omitempty" db:"type"`
}
+// FederationResolverV5 [V5] - is an alias for the Federal Resolver struct
response used for the latest minor version associated with APIv5.
Review Comment:
nit: Do we need the text within the brackets?
##########
traffic_ops/testing/api/v5/federation_resolvers_test.go:
##########
@@ -29,6 +29,7 @@ import (
"github.com/apache/trafficcontrol/lib/go-util/assert"
"github.com/apache/trafficcontrol/traffic_ops/testing/api/utils"
"github.com/apache/trafficcontrol/traffic_ops/toclientlib"
+
Review Comment:
nit: I don't think we need a blank line here, because it belongs to the same
import block
##########
lib/go-tc/federation_resolver.go:
##########
@@ -50,6 +51,36 @@ type FederationResolver struct {
TypeID *uint `json:"typeId,omitempty" db:"type"`
}
+// FederationResolverV5 [V5] - is an alias for the Federal Resolver struct
response used for the latest minor version associated with APIv5.
+type FederationResolverV5 = FederationResolverV50
+
+// FederationResolverV50 [V50]- is used for RFC3339 format timestamp in
FederationResolver which represents a resolver record for a CDN Federation for
APIv50.
Review Comment:
Same comment here
##########
traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go:
##########
@@ -103,6 +101,7 @@ func Create(w http.ResponseWriter, r *http.Request) {
}
// Read is the handler for GET requests to /federation_resolvers (and
/federation_resolvers/{{ID}}).
+// here based on request V5 is identified to get list of federation resolver
for APIv5
Review Comment:
I don't think you need this godoc comment here.
##########
traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go:
##########
@@ -136,13 +135,28 @@ func Read(w http.ResponseWriter, r *http.Request) {
} else {
log.Warnf("Couldn't get config %v", e)
}
+
+ // Based on version we load types - for version 5 and above we use
FederationResolverV5
+ var resolvers []interface{}
+ if inf.Version != nil && inf.Version.Major >= 5 && inf.Version.Minor >=
0 {
Review Comment:
This is fine, but there is a utility function that can be used in place of
this if statement like this:
`if inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 5, Minor: 0})`
##########
traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go:
##########
@@ -136,13 +135,28 @@ func Read(w http.ResponseWriter, r *http.Request) {
} else {
log.Warnf("Couldn't get config %v", e)
}
+
+ // Based on version we load types - for version 5 and above we use
FederationResolverV5
+ var resolvers []interface{}
+ if inf.Version != nil && inf.Version.Major >= 5 && inf.Version.Minor >=
0 {
+ frData := []tc.FederationResolverV5{} // FR type data
+ for _, item := range frData {
+ resolvers = append(resolvers, item)
+ }
+ } else {
+ frData := []tc.FederationResolver{} // PR type data
+ for _, item := range frData {
+ resolvers = append(resolvers, item)
+ }
+ }
Review Comment:
This entire code block is not needed, because, on lines 142 and 147, you're
declaring a new variable (array), and on the subsequent lines, you're
traversing over that array. There are 0 entries in the array, so traversing
through it is needless.
##########
lib/go-tc/federation_resolver.go:
##########
@@ -50,6 +51,36 @@ type FederationResolver struct {
TypeID *uint `json:"typeId,omitempty" db:"type"`
}
+// [V5] FederationResolverV5 is an alias for the Federal Resolver struct
response used for the latest minor version associated with api major version 5.
+type FederationResolverV5 = FederationResolverV50
+
+// [V50] FederationResolverV50 is used for RFC3339 format timestamp in
FederationResolver which represents a resolver record for a CDN Federation.
+type FederationResolverV50 struct {
+ ID *uint `json:"id" db:"id"`
+ IPAddress *string `json:"ipAddress" db:"ip_address"`
+ LastUpdated *time.Time `json:"lastUpdated,omitempty" db:"last_updated"`
+ Type *string `json:"type"`
+ TypeID *uint `json:"typeId,omitempty" db:"type"`
+}
+
+// [V5] FederationResolversV5Response is an alias for the FederationResolvers
struct response used for the latest minor version associated with api major
version 5.
+type FederationResolversV5Response = FederationResolversV50Response
+
+// [V50] GET request to its /federation_resolvers endpoint for APIv5.
+type FederationResolversV50Response struct {
+ Alerts
+ Response []FederationResolverV5 `json:"response"`
+}
+
+// [V5] FederationResolverV5Response represents struct response used for the
latest minor version associated with api major version 5.
+type FederationResolverV5Response = FederationResolverV50Response
+
+// [V50] POST request to its /federation_resolvers endpoint APIv5.
+type FederationResolverV50Response struct {
Review Comment:
And here.
##########
lib/go-tc/federation_resolver.go:
##########
@@ -50,6 +51,36 @@ type FederationResolver struct {
TypeID *uint `json:"typeId,omitempty" db:"type"`
}
+// [V5] FederationResolverV5 is an alias for the Federal Resolver struct
response used for the latest minor version associated with api major version 5.
+type FederationResolverV5 = FederationResolverV50
+
+// [V50] FederationResolverV50 is used for RFC3339 format timestamp in
FederationResolver which represents a resolver record for a CDN Federation.
+type FederationResolverV50 struct {
+ ID *uint `json:"id" db:"id"`
+ IPAddress *string `json:"ipAddress" db:"ip_address"`
+ LastUpdated *time.Time `json:"lastUpdated,omitempty" db:"last_updated"`
+ Type *string `json:"type"`
+ TypeID *uint `json:"typeId,omitempty" db:"type"`
+}
+
+// [V5] FederationResolversV5Response is an alias for the FederationResolvers
struct response used for the latest minor version associated with api major
version 5.
+type FederationResolversV5Response = FederationResolversV50Response
Review Comment:
Doesn't look like this comment was addressed :)
##########
traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go:
##########
@@ -162,19 +176,31 @@ func Read(w http.ResponseWriter, r *http.Request) {
}
defer rows.Close()
- var resolvers = []tc.FederationResolver{}
for rows.Next() {
- var resolver tc.FederationResolver
- if err := rows.Scan(&resolver.ID, &resolver.IPAddress,
&resolver.LastUpdated, &resolver.Type); err != nil {
- userErr, sysErr, errCode = api.ParseDBError(err)
- if sysErr != nil {
- sysErr = fmt.Errorf("federation_resolver
scanning: %v", sysErr)
+ // Based on version we load types - for version 5 and above we
use FederationResolverV5
+ if inf.Version != nil && inf.Version.Major >= 5 &&
inf.Version.Minor >= 0 {
Review Comment:
You could use the pre defined utility function here, but it's your call.
##########
traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go:
##########
@@ -162,19 +176,31 @@ func Read(w http.ResponseWriter, r *http.Request) {
}
defer rows.Close()
- var resolvers = []tc.FederationResolver{}
for rows.Next() {
- var resolver tc.FederationResolver
- if err := rows.Scan(&resolver.ID, &resolver.IPAddress,
&resolver.LastUpdated, &resolver.Type); err != nil {
- userErr, sysErr, errCode = api.ParseDBError(err)
- if sysErr != nil {
- sysErr = fmt.Errorf("federation_resolver
scanning: %v", sysErr)
+ // Based on version we load types - for version 5 and above we
use FederationResolverV5
+ if inf.Version != nil && inf.Version.Major >= 5 &&
inf.Version.Minor >= 0 {
+ var resolver tc.FederationResolverV5
+ if err := rows.Scan(&resolver.ID, &resolver.IPAddress,
&resolver.LastUpdated, &resolver.Type); err != nil {
+ userErr, sysErr, errCode = api.ParseDBError(err)
+ if sysErr != nil {
+ sysErr =
fmt.Errorf("federation_resolver scanning: %v", sysErr)
+ }
+ api.HandleErr(w, r, tx, errCode, userErr,
sysErr)
+ return
}
- api.HandleErr(w, r, tx, errCode, userErr, sysErr)
- return
+ resolvers = append(resolvers, resolver)
Review Comment:
Instead of repeating this block twice, here's what I'd do:
Scan the struct into a v4 struct as it was being done earlier. Then, check
for the version, and if it's >= 5.0, then convert the v4 struct into a v5
struct before appending it to the interface array `resolvers`.
##########
lib/go-tc/federation_resolver.go:
##########
@@ -50,6 +51,36 @@ type FederationResolver struct {
TypeID *uint `json:"typeId,omitempty" db:"type"`
}
+// [V5] FederationResolverV5 is an alias for the Federal Resolver struct
response used for the latest minor version associated with api major version 5.
+type FederationResolverV5 = FederationResolverV50
+
+// [V50] FederationResolverV50 is used for RFC3339 format timestamp in
FederationResolver which represents a resolver record for a CDN Federation.
+type FederationResolverV50 struct {
+ ID *uint `json:"id" db:"id"`
+ IPAddress *string `json:"ipAddress" db:"ip_address"`
+ LastUpdated *time.Time `json:"lastUpdated,omitempty" db:"last_updated"`
+ Type *string `json:"type"`
+ TypeID *uint `json:"typeId,omitempty" db:"type"`
+}
+
+// [V5] FederationResolversV5Response is an alias for the FederationResolvers
struct response used for the latest minor version associated with api major
version 5.
+type FederationResolversV5Response = FederationResolversV50Response
+
+// [V50] GET request to its /federation_resolvers endpoint for APIv5.
+type FederationResolversV50Response struct {
+ Alerts
+ Response []FederationResolverV5 `json:"response"`
+}
+
+// [V5] FederationResolverV5Response represents struct response used for the
latest minor version associated with api major version 5.
+type FederationResolverV5Response = FederationResolverV50Response
Review Comment:
Same here
##########
lib/go-tc/federation_resolver.go:
##########
@@ -50,6 +51,36 @@ type FederationResolver struct {
TypeID *uint `json:"typeId,omitempty" db:"type"`
}
+// [V5] FederationResolverV5 is an alias for the Federal Resolver struct
response used for the latest minor version associated with api major version 5.
+type FederationResolverV5 = FederationResolverV50
+
+// [V50] FederationResolverV50 is used for RFC3339 format timestamp in
FederationResolver which represents a resolver record for a CDN Federation.
+type FederationResolverV50 struct {
+ ID *uint `json:"id" db:"id"`
+ IPAddress *string `json:"ipAddress" db:"ip_address"`
+ LastUpdated *time.Time `json:"lastUpdated,omitempty" db:"last_updated"`
+ Type *string `json:"type"`
+ TypeID *uint `json:"typeId,omitempty" db:"type"`
+}
+
+// [V5] FederationResolversV5Response is an alias for the FederationResolvers
struct response used for the latest minor version associated with api major
version 5.
+type FederationResolversV5Response = FederationResolversV50Response
+
+// [V50] GET request to its /federation_resolvers endpoint for APIv5.
+type FederationResolversV50Response struct {
Review Comment:
This one as well.
--
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]
