zrhoffman commented on code in PR #7774:
URL: https://github.com/apache/trafficcontrol/pull/7774#discussion_r1323533904
##########
lib/go-atscfg/headerrewritedotconfig.go:
##########
@@ -120,11 +120,14 @@ func MakeHeaderRewriteDotConfig(
ds := &DeliveryService{}
for _, ids := range deliveryServices {
- if ids.XMLID == nil {
+ if ids.Active == tc.DSActiveStateInactive {
+ continue
+ }
+ if &ids.XMLID == nil || ids.XMLID == "" {
Review Comment:
No need for this nil check, it is guaranteed to be non-nil.
##########
lib/go-atscfg/hostingdotconfig.go:
##########
@@ -118,15 +118,18 @@ func MakeHostingDotConfig(
filteredDSes := []DeliveryService{}
for _, ds := range deliveryServices {
- if ds.Active == nil || ds.Type == nil || ds.XMLID == nil ||
ds.CDNID == nil || ds.ID == nil || ds.OrgServerFQDN == nil {
+ if &ds.Active == nil || ds.Active == "" || ds.Type == nil ||
&ds.XMLID == nil || &ds.CDNID == nil || ds.ID == nil || ds.OrgServerFQDN == nil
{
Review Comment:
No need to check `&ds.Active == nil`, `ds.Active` is a value and not a
pointer.
##########
lib/go-atscfg/hostingdotconfig.go:
##########
@@ -118,15 +118,18 @@ func MakeHostingDotConfig(
filteredDSes := []DeliveryService{}
for _, ds := range deliveryServices {
- if ds.Active == nil || ds.Type == nil || ds.XMLID == nil ||
ds.CDNID == nil || ds.ID == nil || ds.OrgServerFQDN == nil {
+ if &ds.Active == nil || ds.Active == "" || ds.Type == nil ||
&ds.XMLID == nil || &ds.CDNID == nil || ds.ID == nil || ds.OrgServerFQDN == nil
{
Review Comment:
Instead of checking `&ds.XMLID == nil`, check `ds.XMLID == ""`
##########
lib/go-atscfg/hostingdotconfig.go:
##########
@@ -178,7 +181,7 @@ func MakeHostingDotConfig(
seenOrigins := map[string]struct{}{}
for _, ds := range filteredDSes {
- if ds.OrgServerFQDN == nil || ds.XMLID == nil ||
ds.Active == nil {
+ if ds.OrgServerFQDN == nil || &ds.XMLID == nil ||
&ds.Active == nil {
Review Comment:
`ds.XMLID` and `ds.Active` are values, not pointers. They should be checked
for the empty string, `""`.
##########
lib/go-atscfg/hostingdotconfig.go:
##########
@@ -118,15 +118,18 @@ func MakeHostingDotConfig(
filteredDSes := []DeliveryService{}
for _, ds := range deliveryServices {
- if ds.Active == nil || ds.Type == nil || ds.XMLID == nil ||
ds.CDNID == nil || ds.ID == nil || ds.OrgServerFQDN == nil {
+ if &ds.Active == nil || ds.Active == "" || ds.Type == nil ||
&ds.XMLID == nil || &ds.CDNID == nil || ds.ID == nil || ds.OrgServerFQDN == nil
{
Review Comment:
Instead of checking if `&ds.CDNID == nil`, check if `ds.CDNID == 0`.
##########
lib/go-atscfg/meta.go:
##########
@@ -208,7 +208,7 @@ func addMetaObjConfigDir(
nameTopologies := makeTopologyNameMap(topologies)
for _, ds := range dses {
- if ds.XMLID == nil {
+ if &ds.XMLID == nil {
Review Comment:
This condition should be `ds.XMLID == ""`
##########
lib/go-tc/totest/deliveryservices.go:
##########
@@ -23,19 +23,19 @@ import (
"strconv"
"testing"
+ "github.com/apache/trafficcontrol/lib/go-tc"
"github.com/apache/trafficcontrol/lib/go-util/assert"
- toclient "github.com/apache/trafficcontrol/traffic_ops/v4-client"
+ toclient "github.com/apache/trafficcontrol/traffic_ops/v5-client"
)
func CreateTestDeliveryServices(t *testing.T, cl *toclient.Session, td
TrafficControl) {
for _, ds := range td.DeliveryServices {
- ds = ds.RemoveLD1AndLD2()
- if ds.XMLID == nil {
+ if &ds.XMLID == nil {
Review Comment:
This condition should be `ds.XMLID == ""`
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -778,13 +778,13 @@ type dsesSortByName []DeliveryService
func (s dsesSortByName) Len() int { return len(s) }
func (s dsesSortByName) Swap(i, j int) { s[i], s[j] = s[j], s[i] }
func (s dsesSortByName) Less(i, j int) bool {
- if s[i].XMLID == nil {
+ if &s[i].XMLID == nil || s[i].XMLID == "" {
Review Comment:
The `&s[i].XMLID == nil` check can be removed.
##########
lib/go-atscfg/regexremapdotconfig.go:
##########
@@ -108,19 +108,19 @@ func deliveryServicesToCDNDSes(dses []DeliveryService)
(map[tc.DeliveryServiceNa
warnings := []string{}
sDSes := map[tc.DeliveryServiceName]cdnDS{}
for _, ds := range dses {
- if ds.OrgServerFQDN == nil || ds.QStringIgnore == nil ||
ds.XMLID == nil {
- if ds.XMLID == nil {
+ if ds.OrgServerFQDN == nil || ds.QStringIgnore == nil ||
&ds.XMLID == nil {
+ if &ds.XMLID == nil {
Review Comment:
Instead of checking `&ds.XMLID == nil`, check `ds.XMLID == ""`.
##########
lib/go-atscfg/parentdotconfig.go:
##########
@@ -778,13 +778,13 @@ type dsesSortByName []DeliveryService
func (s dsesSortByName) Len() int { return len(s) }
func (s dsesSortByName) Swap(i, j int) { s[i], s[j] = s[j], s[i] }
func (s dsesSortByName) Less(i, j int) bool {
- if s[i].XMLID == nil {
+ if &s[i].XMLID == nil || s[i].XMLID == "" {
return true
}
- if s[j].XMLID == nil {
+ if &s[j].XMLID == nil || s[j].XMLID == "" {
Review Comment:
The `&s[j].XMLID == nil` check can be removed.
##########
lib/go-atscfg/remapdotconfig.go:
##########
@@ -1161,27 +1161,27 @@ func makeFQDN(hostRegex string, ds *DeliveryService,
server string, cdnDomain st
re = strings.Replace(re, `.*`, ``, -1)
hName := server
- if ds.Type.IsDNS() {
- if ds.RoutingName == nil {
+ if tc.DSType(*ds.Type).IsDNS() {
+ if &ds.RoutingName == nil || ds.RoutingName == "" {
Review Comment:
The `&ds.RoutingName == nil` check can be removed.
##########
lib/go-atscfg/servercachedotconfig.go:
##########
@@ -48,15 +48,15 @@ func makeCacheDotConfigMid(
dses := map[tc.DeliveryServiceName]serverCacheConfigDS{}
for _, ds := range deliveryServices {
- if ds.XMLID == nil || ds.Active == nil || ds.OrgServerFQDN ==
nil || ds.Type == nil {
+ if &ds.XMLID == nil || &ds.Active == nil || ds.OrgServerFQDN ==
nil || ds.Type == nil {
Review Comment:
Instead of checking `&ds.Active == nil`, check `ds.Active == ""`.
##########
cache-config/t3c-generate/cfgfile/all.go:
##########
@@ -38,9 +38,15 @@ func GetAllConfigs(
toData *t3cutil.ConfigData,
cfg config.Cfg,
) ([]t3cutil.ATSConfigFile, error) {
- if toData.Server.HostName == nil {
+ if &toData.Server.HostName == nil || toData.Server.HostName == "" {
Review Comment:
No need for this nil check, it is guaranteed to be non-nil.
##########
lib/go-atscfg/meta.go:
##########
@@ -304,17 +304,17 @@ func getURISignedDSes(dses
map[tc.DeliveryServiceName]DeliveryService) ([]tc.Del
warnings = append(warnings, "got delivery service with
no id, skipping!")
continue
}
- if ds.XMLID == nil {
+ if &ds.XMLID == nil {
Review Comment:
This condition should be `ds.XMLID == ""`
##########
lib/go-atscfg/regexrevalidatedotconfig.go:
##########
@@ -70,19 +70,19 @@ func MakeRegexRevalidateDotConfig(
}
warnings := []string{}
- if server.CDNName == nil {
+ if &server.CDN == nil || server.CDN == "" {
return Cfg{}, makeErr(warnings, "server CDNName missing")
}
params := paramsToMultiMap(filterParams(globalParams,
RegexRevalidateFileName, "", "", ""))
dsNames := map[string]struct{}{}
for _, ds := range deliveryServices {
- if ds.XMLID == nil {
+ if &ds.XMLID == nil {
Review Comment:
This condition should be `ds.XMLID == ""`
##########
lib/go-atscfg/meta.go:
##########
@@ -356,29 +359,32 @@ func filterConfigFileDSes(server *Server,
deliveryServices []DeliveryService, de
warnings = append(warnings, "got
deliveryservice with nil id, skipping!")
continue
}
- if ds.XMLID == nil {
+ if &ds.XMLID == nil {
warnings = append(warnings, "got
deliveryservice with nil xmlId (name), skipping!")
continue
}
if _, ok := dssMap[*ds.ID]; !ok && ds.Topology == nil {
continue
}
- dses[tc.DeliveryServiceName(*ds.XMLID)] = ds
+ dses[tc.DeliveryServiceName(ds.XMLID)] = ds
}
} else {
for _, ds := range deliveryServices {
if ds.ID == nil {
warnings = append(warnings, "got
deliveryservice with nil id, skipping!")
continue
}
- if ds.XMLID == nil {
+ if &ds.XMLID == nil {
Review Comment:
This condition should be `ds.XMLID == ""`
##########
lib/go-atscfg/sslservernamedotyaml.go:
##########
@@ -342,22 +342,22 @@ func dsUsesServer(
ds *DeliveryService,
server *Server,
dss []DeliveryServiceServer,
- nameTopologies map[TopologyName]tc.Topology,
- cacheGroups map[tc.CacheGroupName]tc.CacheGroupNullable,
+ nameTopologies map[TopologyName]tc.TopologyV5,
+ cacheGroups map[tc.CacheGroupName]tc.CacheGroupNullableV5,
serverCapabilities map[int]map[ServerCapability]struct{},
dsRequiredCapabilities map[int]map[ServerCapability]struct{},
) (bool, error) {
- if ds.XMLID == nil || *ds.XMLID == "" {
+ if &ds.XMLID == nil || ds.XMLID == "" {
Review Comment:
The `&ds.XMLID == nil` check can be removed.
##########
lib/go-atscfg/remapdotconfig.go:
##########
@@ -341,12 +341,12 @@ func getServerConfigRemapDotConfigForMid(
}
}
- if !ds.Type.UsesMidCache() && (!hasTopology || *ds.Topology ==
"") {
+ if !tc.DSType(*ds.Type).UsesMidCache() && (!hasTopology ||
*ds.Topology == "") {
Review Comment:
If `ds.Type` is not guaranteed to be non-nil, it needs a nil check.
##########
lib/go-atscfg/meta.go:
##########
@@ -356,29 +359,32 @@ func filterConfigFileDSes(server *Server,
deliveryServices []DeliveryService, de
warnings = append(warnings, "got
deliveryservice with nil id, skipping!")
continue
}
- if ds.XMLID == nil {
+ if &ds.XMLID == nil {
Review Comment:
This condition should be `ds.XMLID == ""`
##########
lib/go-atscfg/meta.go:
##########
@@ -356,29 +359,32 @@ func filterConfigFileDSes(server *Server,
deliveryServices []DeliveryService, de
warnings = append(warnings, "got
deliveryservice with nil id, skipping!")
continue
}
- if ds.XMLID == nil {
+ if &ds.XMLID == nil {
warnings = append(warnings, "got
deliveryservice with nil xmlId (name), skipping!")
continue
}
if _, ok := dssMap[*ds.ID]; !ok && ds.Topology == nil {
continue
}
- dses[tc.DeliveryServiceName(*ds.XMLID)] = ds
+ dses[tc.DeliveryServiceName(ds.XMLID)] = ds
}
} else {
for _, ds := range deliveryServices {
if ds.ID == nil {
warnings = append(warnings, "got
deliveryservice with nil id, skipping!")
continue
}
- if ds.XMLID == nil {
+ if &ds.XMLID == nil {
warnings = append(warnings, "got
deliveryservice with nil xmlId (name), skipping!")
continue
}
- if ds.CDNID == nil || *ds.CDNID != *server.CDNID {
+ if &ds.CDNID == nil || ds.CDNID != server.CDNID {
Review Comment:
The `&ds.CDNID == nil` check can be removed.
##########
lib/go-atscfg/remapdotconfig.go:
##########
@@ -984,24 +984,24 @@ func remapFilterDSes(server *Server, dss
[]DeliveryServiceServer, dses []Deliver
if ds.LastHeaderRewrite == nil {
ds.LastHeaderRewrite = util.StrPtr("")
}
- if ds.XMLID == nil {
+ if &ds.XMLID == nil {
Review Comment:
Instead of checking `&ds.XMLID == nil`, check `ds.XMLID == "`.
##########
lib/go-atscfg/servercachedotconfig.go:
##########
@@ -48,15 +48,15 @@ func makeCacheDotConfigMid(
dses := map[tc.DeliveryServiceName]serverCacheConfigDS{}
for _, ds := range deliveryServices {
- if ds.XMLID == nil || ds.Active == nil || ds.OrgServerFQDN ==
nil || ds.Type == nil {
+ if &ds.XMLID == nil || &ds.Active == nil || ds.OrgServerFQDN ==
nil || ds.Type == nil {
Review Comment:
Instead of checking `&ds.XMLID == nil`, check `ds.XMLID == ""`.
##########
lib/go-atscfg/remapdotconfig.go:
##########
@@ -984,24 +984,24 @@ func remapFilterDSes(server *Server, dss
[]DeliveryServiceServer, dses []Deliver
if ds.LastHeaderRewrite == nil {
ds.LastHeaderRewrite = util.StrPtr("")
}
- if ds.XMLID == nil {
+ if &ds.XMLID == nil {
warnings = append(warnings, "got Delivery Service with
nil XMLID, skipping!")
continue
} else if ds.Type == nil {
- warnings = append(warnings, "got Delivery Service
'"+*ds.XMLID+"' with nil Type, skipping!")
+ warnings = append(warnings, "got Delivery Service
'"+ds.XMLID+"' with nil Type, skipping!")
continue
- } else if ds.DSCP == nil {
- warnings = append(warnings, "got Delivery Service
'"+*ds.XMLID+"' with nil DSCP, skipping!")
+ } else if &ds.DSCP == nil {
+ warnings = append(warnings, "got Delivery Service
'"+ds.XMLID+"' with nil DSCP, skipping!")
continue
} else if ds.ID == nil {
- warnings = append(warnings, "got Delivery Service
'"+*ds.XMLID+"' with nil ID, skipping!")
+ warnings = append(warnings, "got Delivery Service
'"+ds.XMLID+"' with nil ID, skipping!")
continue
- } else if ds.Active == nil {
- warnings = append(warnings, "got Delivery Service
'"+*ds.XMLID+"' with nil Active, skipping!")
+ } else if &ds.Active == nil {
+ warnings = append(warnings, "got Delivery Service
'"+ds.XMLID+"' with nil Active, skipping!")
Review Comment:
Instead of checking `&ds.Active == nil`, check `ds.Active == ""`.
##########
lib/go-atscfg/remapdotconfig.go:
##########
@@ -984,24 +984,24 @@ func remapFilterDSes(server *Server, dss
[]DeliveryServiceServer, dses []Deliver
if ds.LastHeaderRewrite == nil {
ds.LastHeaderRewrite = util.StrPtr("")
}
- if ds.XMLID == nil {
+ if &ds.XMLID == nil {
warnings = append(warnings, "got Delivery Service with
nil XMLID, skipping!")
continue
} else if ds.Type == nil {
- warnings = append(warnings, "got Delivery Service
'"+*ds.XMLID+"' with nil Type, skipping!")
+ warnings = append(warnings, "got Delivery Service
'"+ds.XMLID+"' with nil Type, skipping!")
continue
- } else if ds.DSCP == nil {
- warnings = append(warnings, "got Delivery Service
'"+*ds.XMLID+"' with nil DSCP, skipping!")
+ } else if &ds.DSCP == nil {
+ warnings = append(warnings, "got Delivery Service
'"+ds.XMLID+"' with nil DSCP, skipping!")
Review Comment:
Instead of checking `&ds.DSCP == nil`, check `ds.DSCP == ""`.
--
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]
