[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4695: Cachegroups API updates for Topologies

[GitBox <gi...@apache.org>]

zrhoffman commented on a change in pull request #4695:
URL: https://github.com/apache/trafficcontrol/pull/4695#discussion_r426876021



##########
File path: traffic_ops/client/cachegroup.go
##########
@@ -189,3 +189,24 @@ func (to *Session) DeleteCacheGroupByID(id int) (tc.Alerts, ReqInf, error) {
 	}
 	return alerts, reqInf, nil
 }
+
+func (to *Session) GetCacheGroupsByQueryParams(qparams url.Values) ([]tc.CacheGroupNullable, ReqInf, error) {
+	route := API_CACHEGROUPS
+	if len(qparams) > 0 {
+		route += "?" + qparams.Encode()
+	}
+
+	resp, remoteAddr, err := to.request(http.MethodGet, route, nil)
+	reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr}
+	if err != nil {
+		return nil, reqInf, err
+	}
+	defer resp.Body.Close()

Review comment:
       Consider instead deferring `log.Close()` to handle this error.

##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -427,6 +432,7 @@ func (cg *TOCacheGroup) Read() ([]interface{}, error, error, int) {
 		"name":      dbhelpers.WhereColumnInfo{"cachegroup.name", nil},
 		"shortName": dbhelpers.WhereColumnInfo{"cachegroup.short_name", nil},
 		"type":      dbhelpers.WhereColumnInfo{"cachegroup.type", nil},
+		"topology":  dbhelpers.WhereColumnInfo{"topology_cachegroup.topology", nil},

Review comment:
       This can be
   
   ```go
   		"topology":  {"topology_cachegroup.topology", nil},
   ```
   
   or
   
   ```go
   		"topology":  {Column: "topology_cachegroup.topology"},
   ```

##########
File path: traffic_ops/client/cachegroup.go
##########
@@ -189,3 +189,24 @@ func (to *Session) DeleteCacheGroupByID(id int) (tc.Alerts, ReqInf, error) {
 	}
 	return alerts, reqInf, nil
 }
+
+func (to *Session) GetCacheGroupsByQueryParams(qparams url.Values) ([]tc.CacheGroupNullable, ReqInf, error) {

Review comment:
       Nit: Go doc?

##########
File path: traffic_ops/testing/api/v3/cachegroups_test.go
##########
@@ -70,16 +72,47 @@ func GetTestCacheGroupsByName(t *testing.T) {
 		if err != nil {
 			t.Errorf("cannot GET CacheGroup by name: %v - %v", err, resp)
 		}
+		if *resp[0].Name != *cg.Name {
+			t.Errorf("name expected: %s, actual: %s", *cg.Name, *resp[0].Name)
+		}
 	}
 }
 
 func GetTestCacheGroupsByShortName(t *testing.T) {
 	for _, cg := range testData.CacheGroups {
-		resp, _, err := TOSession.GetCacheGroupNullableByName(*cg.ShortName)
+		resp, _, err := TOSession.GetCacheGroupNullableByShortName(*cg.ShortName)
 		if err != nil {
 			t.Errorf("cannot GET CacheGroup by shortName: %v - %v", err, resp)
 		}
+		if *resp[0].ShortName != *cg.ShortName {
+			t.Errorf("short name expected: %s, actual: %s", *cg.ShortName, *resp[0].ShortName)
+		}
+	}
+}
+
+func GetTestCacheGroupsByTopology(t *testing.T) {
+	for _, top := range testData.Topologies {
+		qparams := url.Values{}
+		qparams.Set("topology", top.Name)

Review comment:
       It might not improve readability, but this can be shortened to
   
   ```go
   		qparams := url.Values{"topology": []string{top.Name}}
   ```




----------------------------------------------------------------
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:
users@infra.apache.org