srijeet0406 commented on a change in pull request #5512:
URL: https://github.com/apache/trafficcontrol/pull/5512#discussion_r604495778
##########
File path: traffic_ops/testing/api/v4/cdnfederations_test.go
##########
@@ -119,12 +119,12 @@ func SortTestCDNFederations(t *testing.T) {
id := *data.Response.ID
//Get list of federations for one type of cdn
- resp, _, err :=
TOSession.GetCDNFederationsByNameWithHdrReturnList("cdn1", header)
+ resp, _, err := TOSession.GetCDNFederationsByName("cdn1", header)
if err != nil {
t.Fatalf("Expected no error, but got %v", err.Error())
}
- for i, _ := range resp {
- sortedList = append(sortedList, *resp[i].CName)
+ for i := range resp.Response {
Review comment:
Could you add a nil check before dereferencing `resp`?
##########
File path: traffic_ops/testing/api/v4/cachegroups_test.go
##########
@@ -223,7 +223,7 @@ func GetTestCacheGroupsByName(t *testing.T) {
func GetTestCacheGroupsByShortName(t *testing.T) {
for _, cg := range testData.CacheGroups {
- resp, _, err :=
TOSession.GetCacheGroupNullableByShortName(*cg.ShortName)
+ resp, _, err :=
TOSession.GetCacheGroupByShortName(*cg.ShortName, nil)
Review comment:
could we add a nil check for cg.ShortName here?
##########
File path: traffic_ops/testing/api/v4/cachegroups_test.go
##########
@@ -188,7 +188,7 @@ func CreateTestCacheGroups(t *testing.T) {
}
func GetTestCacheGroups(t *testing.T) {
- resp, _, err := TOSession.GetCacheGroupsByQueryParams(url.Values{})
+ resp, _, err := TOSession.GetCacheGroups(url.Values{}, nil)
Review comment:
Just to keep it consistent with the earlier tests, you can just use
`nil` here.
##########
File path: traffic_ops/testing/api/v4/federation_users_test.go
##########
@@ -49,7 +49,7 @@ func GetTestValidFederationIDUsersIMSAfterChange(t
*testing.T, header http.Heade
currentTime = currentTime.Add(1 * time.Second)
timeStr := currentTime.Format(time.RFC1123)
header.Set(rfc.IfModifiedSince, timeStr)
- _, reqInf, err = TOSession.GetFederationUsersWithHdr(fedIDs[0], header)
+ _, reqInf, err = TOSession.GetFederationUsers(fedIDs[0], header)
Review comment:
Could you add a `len` check like in line 66 below?
##########
File path: traffic_ops/v4-client/user.go
##########
@@ -27,122 +27,94 @@ import (
"github.com/apache/trafficcontrol/traffic_ops/toclientlib"
)
-func (to *Session) GetUsersWithHdr(header http.Header) ([]tc.User,
toclientlib.ReqInf, error) {
+// GetUsers retrieves all (Tenant-accessible) Users stored in Traffic Ops.
+func (to *Session) GetUsers(header http.Header) ([]tc.User,
toclientlib.ReqInf, error) {
data := tc.UsersResponse{}
route := "/users"
inf, err := to.get(route, header, &data)
return data.Response, inf, err
}
-// GetUsers returns all users accessible from current user
-// Deprecated: GetUsers will be removed in 6.0. Use GetUsersWithHdr.
-func (to *Session) GetUsers() ([]tc.User, toclientlib.ReqInf, error) {
- return to.GetUsersWithHdr(nil)
-}
-
-func (to *Session) GetUsersByRoleWithHdr(roleName string, header http.Header)
([]tc.User, toclientlib.ReqInf, error) {
+// GetUsersByRole retrieves all (Tenant-accessible) with the Role that has the
+// given Name.
+func (to *Session) GetUsersByRole(roleName string, header http.Header)
([]tc.User, toclientlib.ReqInf, error) {
data := tc.UsersResponse{}
route := "/users?role=" + url.QueryEscape(roleName)
inf, err := to.get(route, header, &data)
return data.Response, inf, err
}
-// GetUsersByRole returns all users accessible from current user for a given
role
-// Deprecated: GetUsersByRole will be removed in 6.0. Use
GetUsersByRoleWithHdr.
-func (to *Session) GetUsersByRole(roleName string) ([]tc.User,
toclientlib.ReqInf, error) {
- return to.GetUsersByRoleWithHdr(roleName, nil)
-}
-
-func (to *Session) GetUserByIDWithHdr(id int, header http.Header) ([]tc.User,
toclientlib.ReqInf, error) {
+// GetUserByID retrieves the User with the given ID.
+func (to *Session) GetUserByID(id int, header http.Header) ([]tc.User,
toclientlib.ReqInf, error) {
data := tc.UsersResponse{}
route := fmt.Sprintf("/users/%d", id)
inf, err := to.get(route, header, &data)
return data.Response, inf, err
}
-// Deprecated: GetUserByID will be removed in 6.0. Use GetUserByIDWithHdr.
-func (to *Session) GetUserByID(id int) ([]tc.User, toclientlib.ReqInf, error) {
- return to.GetUserByIDWithHdr(id, nil)
-}
-
-func (to *Session) GetUserByUsernameWithHdr(username string, header
http.Header) ([]tc.User, toclientlib.ReqInf, error) {
+// GetUserByUsername retrieves the User with the given Username.
+func (to *Session) GetUserByUsername(username string, header http.Header)
([]tc.User, toclientlib.ReqInf, error) {
data := tc.UsersResponse{}
route := "/users?username=" + url.QueryEscape(username)
inf, err := to.get(route, header, &data)
return data.Response, inf, err
}
-// Deprecated: GetUserByUsername will be removed in 6.0. Use
GetUserByUsernameWithHdr.
-func (to *Session) GetUserByUsername(username string) ([]tc.User,
toclientlib.ReqInf, error) {
- return to.GetUserByUsernameWithHdr(username, nil)
-}
-
-func (to *Session) GetUserCurrentWithHdr(header http.Header) (*tc.UserCurrent,
toclientlib.ReqInf, error) {
+// GetUserCurrent retrieves the currently authenticated User.
+func (to *Session) GetUserCurrent(header http.Header) (tc.UserCurrent,
toclientlib.ReqInf, error) {
route := `/user/current`
resp := tc.UserCurrentResponse{}
reqInf, err := to.get(route, header, &resp)
- if err != nil {
- return nil, reqInf, err
- }
- return &resp.Response, reqInf, nil
-}
-
-// GetUserCurrent gets information about the current user
-// Deprecated: GetUserCurrent will be removed in 6.0. Use
GetUserCurrentWithHdr.
-func (to *Session) GetUserCurrent() (*tc.UserCurrent, toclientlib.ReqInf,
error) {
- return to.GetUserCurrentWithHdr(nil)
+ return resp.Response, reqInf, err
}
// UpdateCurrentUser replaces the current user data with the provided tc.User
structure.
-func (to *Session) UpdateCurrentUser(u tc.User) (*tc.UpdateUserResponse,
toclientlib.ReqInf, error) {
+func (to *Session) UpdateCurrentUser(u tc.User) (tc.UpdateUserResponse,
toclientlib.ReqInf, error) {
Review comment:
I think we should pass the header param here as well.
##########
File path: traffic_ops/testing/api/v4/cdns_test.go
##########
@@ -49,14 +49,14 @@ func TestCDNs(t *testing.T) {
func UpdateTestCDNsWithHeaders(t *testing.T, header http.Header) {
firstCDN := testData.CDNs[0]
// Retrieve the CDN by name so we can get the id for the Update
- resp, _, err := TOSession.GetCDNByNameWithHdr(firstCDN.Name, header)
+ resp, _, err := TOSession.GetCDNByName(firstCDN.Name, header)
if err != nil {
t.Errorf("cannot GET CDN by name: '%s', %v", firstCDN.Name, err)
}
if len(resp) > 0 {
remoteCDN := resp[0]
remoteCDN.DomainName = "domain2"
- _, reqInf, err := TOSession.UpdateCDNByIDWithHdr(remoteCDN.ID,
remoteCDN, header)
+ _, reqInf, err := TOSession.UpdateCDNByID(remoteCDN.ID,
remoteCDN, header)
Review comment:
Keeping with the other refactors, should this just be `UpdateCDN`?
##########
File path: traffic_ops/testing/api/v4/deliveryservices_test.go
##########
@@ -353,7 +353,7 @@ func GetTestDeliveryServicesIMS(t *testing.T) {
futureTime := time.Now().AddDate(0, 0, 1)
time := futureTime.Format(time.RFC1123)
header.Set(rfc.IfModifiedSince, time)
- _, reqInf, err := TOSession.GetDeliveryServicesV4(header, nil)
+ _, reqInf, err := TOSession.GetDeliveryServices(header, nil)
Review comment:
Same comment about the ordering of method arguments here
##########
File path: traffic_ops/testing/api/v4/cachegroups_parameters_test.go
##########
@@ -45,7 +46,9 @@ func CreateTestCacheGroupParameters(t *testing.T) {
// Get Parameter to assign to Cache Group
firstParameter := testData.Parameters[0]
- paramResp, _, err := TOSession.GetParameterByName(firstParameter.Name)
+ params := url.Values{}
+ params.Set("name", firstParameter.Name)
+ paramResp, _, err := TOSession.GetParameters(nil, params)
Review comment:
Could we keep the method signature consistent with respect to the
`header` parameter? Looks like `header` is the last argument in the other
methods, whereas, it is the first one in here.
##########
File path: traffic_ops/testing/api/v4/cachegroups_test.go
##########
@@ -129,7 +129,7 @@ func GetTestCacheGroupsByNameIMS(t *testing.T) {
time := futureTime.Format(time.RFC1123)
header.Set(rfc.IfModifiedSince, time)
for _, cg := range testData.CacheGroups {
- _, reqInf, err :=
TOSession.GetCacheGroupNullableByNameWithHdr(*cg.Name, header)
+ _, reqInf, err := TOSession.GetCacheGroupByName(*cg.Name,
header)
Review comment:
could we add a nil check for cg.Name here?
##########
File path: traffic_ops/testing/api/v4/cachegroups_test.go
##########
@@ -272,15 +272,15 @@ func UpdateTestCacheGroups(t *testing.T) {
cg.ShortName = &expectedShortName
// fix the type id for test
- typeResp, _, err := TOSession.GetTypeByID(*cg.TypeID)
+ typeResp, _, err := TOSession.GetTypeByID(*cg.TypeID, nil)
Review comment:
Same comment about nil checking
--
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]
