rawlinp commented on a change in pull request #5350:
URL: https://github.com/apache/trafficcontrol/pull/5350#discussion_r545979524
##########
File path: traffic_ops/traffic_ops_golang/topology/topologies.go
##########
@@ -150,6 +150,16 @@ func (topology *TOTopology) Validate() error {
rules["empty cachegroups"] =
topology_validation.CheckForEmptyCacheGroups(topology.ReqInfo.Tx,
cacheGroupIds, dsCDNs, false, nil)
rules["required capabilities"] =
topology.validateDSRequiredCapabilities()
+ //Get current Topology-CG for the requested change.
+ topoCachegroupNames := topology.getCachegroupNames()
+ userErr, sysErr, _ =
dbhelpers.CheckTopologyOrgServerCGInDSCG(topology.ReqInfo.Tx.Tx, dsCDNs,
topology.Name, topoCachegroupNames)
+ if userErr != nil {
+ return fmt.Errorf("%s", userErr)
Review comment:
This can just be `return userErr`.
##########
File path: traffic_ops/testing/api/v3/topologies_test.go
##########
@@ -240,6 +241,52 @@ func UpdateTestTopologies(t *testing.T) {
}
}
+func UpdateValidateTopologyORGServerCacheGroup(t *testing.T) {
+ params := url.Values{}
+ params.Set("xmlId", "ds-top")
+
+ //Get the correct DS
+ remoteDS, _, err := TOSession.GetDeliveryServicesV30WithHdr(nil, params)
+ if err != nil {
+ t.Errorf("cannot GET Delivery Services: %v", err)
+ }
+
+ //Assign ORG server to DS
+ assignServer := []string{"denver-mso-org-01"}
+ _, _, err = TOSession.AssignServersToDeliveryService(assignServer,
*remoteDS[0].XMLID)
+ if err != nil {
+ t.Errorf("cannot assign server to Delivery Services: %v", err)
+ }
+
+ //Get Topology node to update and remove ORG server nodes
+ origTopo := *remoteDS[0].Topology
+ resp, _, err := TOSession.GetTopologyWithHdr(origTopo, nil)
+ if err != nil {
+ t.Fatalf("couldn't find any topologies: %v", err)
+ }
+
+ // remove org server cachegroup
+ var p []int
+ newNodes := []tc.TopologyNode{{Id: 0, Cachegroup:
"topology-edge-cg-01", Parents: p, LastUpdated: nil}}
+ if *remoteDS[0].Topology == resp.Name {
+ resp.Nodes = newNodes
+ }
+ _, _, err = TOSession.UpdateTopology(*remoteDS[0].Topology, *resp)
+ if err == nil {
+ t.Fatalf("shouldnot UPDATE topology:%v to %v, but update was a
success", *remoteDS[0].Topology, newNodes[0].Cachegroup)
+ }
Review comment:
Because I ran into a test issue like this the other day, we should add
an `else if` to check that the error message contains `"ORG servers are
assigned to delivery services that use this topology, and their cachegroups
cannot be removed"`.
We want to make sure we're testing _that_ validation specifically, and that
the topology update would otherwise be successful if not for that specific
validation.
##########
File path: traffic_ops/traffic_ops_golang/topology/topologies.go
##########
@@ -150,6 +150,16 @@ func (topology *TOTopology) Validate() error {
rules["empty cachegroups"] =
topology_validation.CheckForEmptyCacheGroups(topology.ReqInfo.Tx,
cacheGroupIds, dsCDNs, false, nil)
rules["required capabilities"] =
topology.validateDSRequiredCapabilities()
+ //Get current Topology-CG for the requested change.
+ topoCachegroupNames := topology.getCachegroupNames()
+ userErr, sysErr, _ =
dbhelpers.CheckTopologyOrgServerCGInDSCG(topology.ReqInfo.Tx.Tx, dsCDNs,
topology.Name, topoCachegroupNames)
+ if userErr != nil {
+ return fmt.Errorf("%s", userErr)
+ }
+ if sysErr != nil {
+ log.Errorf("error while validate topology: %s", sysErr.Error())
Review comment:
In addition to logging the sysErr, we should `return errors.New("unable
to validate topology")`. We don't want to expose system errors to the user, so
we return a generic error here instead (because this method only supports
returning a single error currently).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
