From 0ec937fd42be7f1d3757eba48fa944627dfe591d Mon Sep 17 00:00:00 2001
From: Lucas Di Pentima <lucas.dipentima@curii.com>
Date: Wed, 6 Jul 2022 16:03:48 -0300
Subject: [PATCH] 18858: Don't immediately exit on existing accounts with empty
 user IDs.

Instead, record the UUIDs and report them as an error after processing
the rest.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima@curii.com>
---
 tools/sync-users/sync-users.go      | 15 +++++++++---
 tools/sync-users/sync-users_test.go | 36 +++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/tools/sync-users/sync-users.go b/tools/sync-users/sync-users.go
index 37b94a9f44..f9f8b6046c 100644
--- a/tools/sync-users/sync-users.go
+++ b/tools/sync-users/sync-users.go
@@ -222,6 +222,7 @@ func doMain(cfg *ConfigParams) error {
 	allUsers := make(map[string]arvados.User)
 	userIDToUUID := make(map[string]string) // Index by email or username
 	dupedEmails := make(map[string][]arvados.User)
+	emptyUserIDs := []string{}
 	processedUsers := make(map[string]bool)
 	results, err := GetAll(cfg.Client, "users", arvados.ResourceListParams{}, &UserList{})
 	if err != nil {
@@ -246,7 +247,9 @@ func doMain(cfg *ConfigParams) error {
 			return err
 		}
 		if uID == "" {
-			return fmt.Errorf("%s is empty for user with uuid %q", cfg.UserID, u.UUID)
+			emptyUserIDs = append(emptyUserIDs, u.UUID)
+			log.Printf("Empty %s found in user %s - ignoring", cfg.UserID, u.UUID)
+			continue
 		}
 		if cfg.CaseInsensitive {
 			uID = strings.ToLower(uID)
@@ -327,7 +330,7 @@ func doMain(cfg *ConfigParams) error {
 
 	log.Printf("User update successes: %d, skips: %d, failures: %d", len(updatesSucceeded), len(updatesSkipped), len(updatesFailed))
 
-	// Report duplicated emails detection
+	var errors []string
 	if len(dupedEmails) > 0 {
 		emails := make([]string, len(dupedEmails))
 		i := 0
@@ -335,7 +338,13 @@ func doMain(cfg *ConfigParams) error {
 			emails[i] = e
 			i++
 		}
-		return fmt.Errorf("skipped %d duplicated email address(es) in the cluster's local user list: %v", len(dupedEmails), emails)
+		errors = append(errors, fmt.Sprintf("skipped %d duplicated email address(es) in the cluster's local user list: %v", len(dupedEmails), emails))
+	}
+	if len(emptyUserIDs) > 0 {
+		errors = append(errors, fmt.Sprintf("skipped %d user account(s) with empty %s: %v", len(emptyUserIDs), cfg.UserID, emptyUserIDs))
+	}
+	if len(errors) > 0 {
+		return fmt.Errorf("%s", strings.Join(errors, "\n"))
 	}
 
 	return nil
diff --git a/tools/sync-users/sync-users_test.go b/tools/sync-users/sync-users_test.go
index 8b5385a321..1e5f688b05 100644
--- a/tools/sync-users/sync-users_test.go
+++ b/tools/sync-users/sync-users_test.go
@@ -434,3 +434,39 @@ func (s *TestSuite) TestFailOnDuplicatedEmails(c *C) {
 	c.Assert(err, NotNil)
 	c.Assert(err, ErrorMatches, "skipped.*duplicated email address.*")
 }
+
+func (s *TestSuite) TestFailOnEmptyUsernames(c *C) {
+	for i := range []int{1, 2} {
+		var user arvados.User
+		err := CreateUser(s.cfg.Client, &user, map[string]string{
+			"email":      fmt.Sprintf("johndoe%d@example.com", i),
+			"username":   "",
+			"first_name": "John",
+			"last_name":  "Doe",
+			"is_active":  "true",
+			"is_admin":   "false",
+		})
+		c.Assert(err, IsNil)
+		c.Assert(user.Username, Equals, fmt.Sprintf("johndoe%d", i))
+		if i == 1 {
+			err = UpdateUser(s.cfg.Client, user.UUID, &user, map[string]string{
+				"username": "",
+			})
+			c.Assert(err, IsNil)
+			c.Assert(user.Username, Equals, "")
+		}
+	}
+
+	s.cfg.Verbose = true
+	data := [][]string{
+		{"johndoe0", "John", "Doe", "0", "0"},
+	}
+	tmpfile, err := MakeTempCSVFile(data)
+	c.Assert(err, IsNil)
+	defer os.Remove(tmpfile.Name())
+	s.cfg.Path = tmpfile.Name()
+	s.cfg.UserID = "username"
+	err = doMain(s.cfg)
+	c.Assert(err, NotNil)
+	c.Assert(err, ErrorMatches, "skipped 1 user account.*with empty username.*")
+}
-- 
2.30.2