rawlinp commented on a change in pull request #2862: Add Traffic Ops Golang federations endpoints URL: https://github.com/apache/trafficcontrol/pull/2862#discussion_r222472382
########## File path: traffic_ops/traffic_ops_golang/federations/allfederations.go ########## @@ -0,0 +1,132 @@ +package federations + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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. + */ + +import ( + "database/sql" + "errors" + "net/http" + + "github.com/apache/trafficcontrol/lib/go-tc" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api" +) + +func GetAll(w http.ResponseWriter, r *http.Request) { + inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil) + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) + return + } + defer inf.Close() + + feds := []FedInfo{} + err := error(nil) + allFederations := []tc.IAllFederation{} + + if cdnParam, ok := inf.Params["cdnName"]; ok { + cdnName := tc.CDNName(cdnParam) + feds, err = getAllFederationsForCDN(inf.Tx.Tx, cdnName) + if err != nil { + api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("federations.GetAll getting all federations: "+err.Error())) + return + } + allFederations = append(allFederations, tc.AllFederationCDN{CDNName: &cdnName}) Review comment: So, I understand mimicking the Perl version exactly, but I think this _might_ be something we don't really need to transfer from the Perl version. Since the Perl version is `internal`, we might have some more leeway in dropping this behavior because I believe that `internal` distinction means that only components within TC should use it. As far as I can tell, TR is the only internal consumer of this API, and I'm pretty sure it will reject any federations response that contains `{"cdnName": "foo"}` in the list of federations. This is because TR requires the "deliveryService" and "mappings" keys in each object in the list and throws an exception if any object in the list doesn't have those keys. Being able to get rid of this behavior also removes the heterogeneous response problem with this endpoint, and I think if we find a dependency on this weird behavior somewhere as we transition components to this new API, we should just remove that dependency on getting back that `tc.AllFederationCDN` object as it should be totally unnecessary anyways. I won't hold up the PR for this, since we can still remove this weird behavior later, but do you agree that it can be removed? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
