Add commands to manage Teams and Members #47
Conversation
Callum0x50
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive team and member management capabilities to the CLI tool, allowing users to create/delete teams and add/remove team members programmatically.
Key changes:
- Added team commands for creating and deleting teams with validation
- Added member subcommands for adding and removing team members with role assignment
- Extended the client interface with new API methods for team and member operations
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/rootcommand/rootcommand.go |
Wires up new team create/delete commands and member subcommand group |
pkg/commands/teams/create.go |
Implements team creation with name/description validation |
pkg/commands/teams/delete.go |
Implements team deletion by name lookup |
pkg/commands/teams/member/add.go |
Implements adding members to teams with role assignment and helper functions for ID lookups |
pkg/commands/teams/member/remove.go |
Implements removing members from teams using helper functions |
pkg/client/teams.go |
Adds REST client methods for team CRUD operations and member management |
pkg/client/client.go |
Extends client interfaces with TeamsClient and new MemberClient interface |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| var team *client.Team | ||
| for _, t := range teams { | ||
| if t.Name == teamName { |
There was a problem hiding this comment.
The team name comparison in the delete command uses exact match (==) while the add/remove member commands use case-insensitive comparison (strings.EqualFold). This inconsistency could lead to confusion where users can find a team using case-insensitive matching in member commands but cannot delete it with the same casing. Consider using strings.EqualFold here for consistency.
| func getTeamIdByName(ctx context.Context, set clientset.ClientSet, teamName string) (string, error) { | ||
| teams, err := set.PlatformClient.ListTeams(ctx) | ||
| if err != nil { | ||
| return "", redact.Errorf("could not list teams: %w", redact.Safe(err)) | ||
| } | ||
|
|
||
| var team *client.Team | ||
| for _, t := range teams { | ||
| if strings.EqualFold(t.Name, teamName) { | ||
| team = &t | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if team == nil { | ||
| return "", redact.Errorf("team not found: %s", teamName) | ||
| } | ||
|
|
||
| return team.ID, nil | ||
| } | ||
|
|
||
| func getUserIdByUpn(ctx context.Context, set clientset.ClientSet, upn string) (string, error) { | ||
| users, err := set.PlatformClient.ListUsers(ctx) | ||
| if err != nil { | ||
| return "", redact.Errorf("could not list users: %w", redact.Safe(err)) | ||
| } | ||
|
|
||
| var user *client.User | ||
| for _, u := range users { | ||
| if strings.EqualFold(u.UPN, upn) { | ||
| user = &u | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if user == nil { | ||
| return "", redact.Errorf("user not found: %s", upn) | ||
| } | ||
|
|
||
| return user.ID, nil | ||
| } |
There was a problem hiding this comment.
The helper functions getTeamIdByName and getUserIdByUpn are duplicated in both add.go and remove.go. These should be extracted to a shared utility file (e.g., member/util.go or member/helpers.go) to follow the DRY principle and make maintenance easier.
| func getTeamIdByName(ctx context.Context, set clientset.ClientSet, teamName string) (string, error) { | |
| teams, err := set.PlatformClient.ListTeams(ctx) | |
| if err != nil { | |
| return "", redact.Errorf("could not list teams: %w", redact.Safe(err)) | |
| } | |
| var team *client.Team | |
| for _, t := range teams { | |
| if strings.EqualFold(t.Name, teamName) { | |
| team = &t | |
| break | |
| } | |
| } | |
| if team == nil { | |
| return "", redact.Errorf("team not found: %s", teamName) | |
| } | |
| return team.ID, nil | |
| } | |
| func getUserIdByUpn(ctx context.Context, set clientset.ClientSet, upn string) (string, error) { | |
| users, err := set.PlatformClient.ListUsers(ctx) | |
| if err != nil { | |
| return "", redact.Errorf("could not list users: %w", redact.Safe(err)) | |
| } | |
| var user *client.User | |
| for _, u := range users { | |
| if strings.EqualFold(u.UPN, upn) { | |
| user = &u | |
| break | |
| } | |
| } | |
| if user == nil { | |
| return "", redact.Errorf("user not found: %s", upn) | |
| } | |
| return user.ID, nil | |
| } | |
| // getTeamIdByName and getUserIdByUpn have been moved to util.go for reuse. |
| func (c *RestClient) DeleteTeam(ctx context.Context, request DeleteTeamRequest) error { | ||
| body, err := json.Marshal(request) | ||
| if err != nil { | ||
| return fmt.Errorf("could not marshal request: %w", err) | ||
| } | ||
|
|
||
| req, err := c.createAuthenticatedRequest(ctx, "DELETE", c.baseURI+"/api/v1/teams/"+request.TeamId, bytes.NewReader(body)) | ||
| if err != nil { |
There was a problem hiding this comment.
The DeleteTeam function includes the teamId in the URL path but also marshals the entire DeleteTeamRequest struct (which contains the TeamId field) as the request body. This is redundant - DELETE requests typically don't need a body when the resource identifier is already in the URL path. Consider passing nil as the body instead, similar to how RemoveTeamMember is implemented.
| func (c *RestClient) DeleteTeam(ctx context.Context, request DeleteTeamRequest) error { | |
| body, err := json.Marshal(request) | |
| if err != nil { | |
| return fmt.Errorf("could not marshal request: %w", err) | |
| } | |
| req, err := c.createAuthenticatedRequest(ctx, "DELETE", c.baseURI+"/api/v1/teams/"+request.TeamId, bytes.NewReader(body)) | |
| if err != nil { | |
| func (c *RestClient) DeleteTeam(ctx context.Context, teamId string) error { | |
| req, err := c.createAuthenticatedRequest(ctx, "DELETE", c.baseURI+"/api/v1/teams/"+teamId, nil) | |
| if err != nil { |
| return cmd | ||
| } | ||
|
|
||
| func validateOptions(options CreateOptions) error { |
There was a problem hiding this comment.
The function name validateOptions is too generic and could cause naming conflicts since it's not exported. In add.go, there's already a validateOptions function, and in remove.go there's validateRemoveOptions. Consider using a more specific name like validateCreateOptions to maintain consistency and clarity.
| if teamName == "" { | ||
| return errEmptyName | ||
| } | ||
|
|
There was a problem hiding this comment.
The function retrieves all teams from the API to find one by name. This could be inefficient if there are many teams. Consider adding a query parameter to the API or implement pagination if the API supports it. At minimum, document this potential performance limitation.
| // NOTE: This retrieves all teams to find one by name, which may be inefficient if there are many teams. | |
| // Consider updating the API to support querying by name or pagination to improve performance. |
| type MemberClient interface { | ||
| AddTeamMember(ctx context.Context, teamId string, request []AddTeamMemberRequest) error | ||
| RemoveTeamMember(ctx context.Context, teamId string, memberId string) error | ||
| } |
There was a problem hiding this comment.
The member management methods (AddTeamMember and RemoveTeamMember) are defined in a separate MemberClient interface, but these are logically part of team management. Consider consolidating these methods into the TeamsClient interface for better cohesion, as members are a sub-resource of teams. This would align with the pattern used for GetTeamMembers which is already in TeamsClient.
| teams, err := set.PlatformClient.ListTeams(ctx) | ||
| if err != nil { | ||
| return "", redact.Errorf("could not list teams: %w", redact.Safe(err)) | ||
| } |
There was a problem hiding this comment.
The function retrieves all teams from the API to find one by name. This could be inefficient if there are many teams. Consider adding a query parameter to the API or implement pagination if the API supports it. At minimum, document this potential performance limitation.
| users, err := set.PlatformClient.ListUsers(ctx) | ||
| if err != nil { | ||
| return "", redact.Errorf("could not list users: %w", redact.Safe(err)) |
There was a problem hiding this comment.
The function retrieves all users from the API to find one by UPN. This could be inefficient with a large user base. Consider adding a query parameter to the API or implement pagination if the API supports it. At minimum, document this potential performance limitation.
| teamId, err := getTeamIdByName(ctx, set, options.Team) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| options.TeamId = teamId | ||
| } | ||
|
|
||
| if options.UserId == "" { | ||
| userId, err := getUserIdByUpn(ctx, set, options.User) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| options.UserId = userId | ||
| } |
There was a problem hiding this comment.
The remove.go file references getTeamIdByName and getUserIdByUpn helper functions (lines 39 and 47) but these functions are not defined in this file. These functions are only defined in add.go, which means remove.go depends on add.go being in the same package. While this works in Go, it's better practice to explicitly define shared helper functions in a separate file to make dependencies clearer and improve maintainability.
| return redact.Errorf("could not add team member: %w", redact.Safe(err)) | ||
| } | ||
|
|
||
| ux.Fsuccess(cmd.OutOrStdout(), "added user: %s (%s) to team: %s (%s) \n", options.User, options.UserId, options.Team, options.TeamId) |
There was a problem hiding this comment.
There is an extra space before the newline character in the success message. This is inconsistent with other success messages in the codebase (see delete.go line 58 and remove.go line 64). Remove the trailing space for consistency.
| ux.Fsuccess(cmd.OutOrStdout(), "added user: %s (%s) to team: %s (%s) \n", options.User, options.UserId, options.Team, options.TeamId) | |
| ux.Fsuccess(cmd.OutOrStdout(), "added user: %s (%s) to team: %s (%s)\n", options.User, options.UserId, options.Team, options.TeamId) |
No description provided.