fix: optimizing user deletion, improving error handling and logging#698
fix: optimizing user deletion, improving error handling and logging#698rkcoder101 wants to merge 6 commits intogoharbor:mainfrom
Conversation
…etion; improved error and happy path logging Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the harbor user delete command to avoid repeated user-list API calls when deleting multiple users, and to provide clearer success/failure output via RunE.
Changes:
- Switch
user deletetoRunEand return errors instead of only logging. - Resolve usernames to IDs via a single
ListUserscall and delete resolved users concurrently. - Print summarized success/failure results for multi-delete, plus a success message for interactive delete.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/harbor/root/user/delete.go
Outdated
| allUsers, err := api.ListUsers() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to list users: %v", utils.ParseHarborErrorMsg(err)) | ||
| } | ||
| userMap := make(map[string]int64) | ||
| for _, u := range allUsers.Payload { | ||
| userMap[u.Username] = u.UserID |
There was a problem hiding this comment.
api.ListUsers() is called once and only the returned Payload is used to build userMap. Since the Harbor API is paginated, this will only cover a single page and can incorrectly mark valid usernames as "user not found" when there are more users than the default page size. Consider using the same paging loop as user list (set Page=1, PageSize=100, iterate until a short page) to build the map, or add an API helper that fetches all users before resolving names.
| allUsers, err := api.ListUsers() | |
| if err != nil { | |
| return fmt.Errorf("failed to list users: %v", utils.ParseHarborErrorMsg(err)) | |
| } | |
| userMap := make(map[string]int64) | |
| for _, u := range allUsers.Payload { | |
| userMap[u.Username] = u.UserID | |
| userMap := make(map[string]int64) | |
| page := int64(1) | |
| pageSize := int64(100) | |
| for { | |
| allUsers, err := api.ListUsers(page, pageSize) | |
| if err != nil { | |
| return fmt.Errorf("failed to list users: %v", utils.ParseHarborErrorMsg(err)) | |
| } | |
| for _, u := range allUsers.Payload { | |
| userMap[u.Username] = u.UserID | |
| } | |
| if len(allUsers.Payload) < int(pageSize) { | |
| break | |
| } | |
| page++ |
| Use: "delete [username...]", | ||
| Short: "delete user by name", | ||
| Args: cobra.MinimumNArgs(0), | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| // If there are command line arguments, process them concurrently. | ||
| if len(args) > 0 { | ||
| var wg sync.WaitGroup | ||
| errChan := make(chan error, len(args)) // Channel to collect errors | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| if len(args) == 0 { | ||
| userID := prompt.GetUserIdFromUser() | ||
| if err := api.DeleteUser(userID); err != nil { |
There was a problem hiding this comment.
The command help text now says "delete user by name", but when no args are provided it deletes by an interactively-entered user ID (prompt.GetUserIdFromUser). Either update the Short/Use/Long help to reflect both modes (name args vs interactive ID), or switch the prompt to select/enter a username for consistency with the command description.
cmd/harbor/root/user/delete.go
Outdated
| return fmt.Errorf("failed to delete user: %v", utils.ParseHarborErrorMsg(err)) | ||
| } | ||
| fmt.Println("User deleted successfully") | ||
| return nil | ||
| } | ||
|
|
||
| for _, arg := range args { | ||
| // Retrieve user ID by name. | ||
| userID, err := api.GetUsersIdByName(arg) | ||
| if err != nil { | ||
| log.Errorf("failed to get user id for '%s': %v", arg, err) | ||
| continue | ||
| } | ||
| wg.Add(1) | ||
| go func(userID int64) { | ||
| defer wg.Done() | ||
| if err := api.DeleteUser(userID); err != nil { | ||
| errChan <- err | ||
| } | ||
| }(userID) | ||
| allUsers, err := api.ListUsers() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to list users: %v", utils.ParseHarborErrorMsg(err)) | ||
| } |
There was a problem hiding this comment.
In the error wraps, utils.ParseHarborErrorMsg(err) returns a string, but the format uses %v. Using %s (or embedding the string directly) would avoid printing Go formatting artifacts and keep error messages consistent with other command implementations.
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
|
@bupd @NucleoFusion @qcserestipy kindly review this whenever u guys r free. Thanks! |
|
please resolve copilot reviews. |
…led the API to fetch the users Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #698 +/- ##
=========================================
- Coverage 10.99% 7.21% -3.78%
=========================================
Files 173 260 +87
Lines 8671 12953 +4282
=========================================
- Hits 953 935 -18
- Misses 7612 11910 +4298
- Partials 106 108 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
Signed-off-by: Rayyan Khan <rayyanrehman101@gmail.com>
done 👍 kindly review |
2 participants
Description
The PR solves some issues related to user deletion:-
Fixes #697