[arvados] created: 2.1.0-2691-g15e759482

git repository hosting git at public.arvados.org
Wed Jul 6 21:23:22 UTC 2022


        at  15e759482006d9f689f8afeb40a97e0f15dfe278 (commit)


commit 15e759482006d9f689f8afeb40a97e0f15dfe278
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date:   Wed Jul 6 18:22:55 2022 -0300

    18858: Adds username case matching tests.
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>

diff --git a/tools/sync-users/sync-users_test.go b/tools/sync-users/sync-users_test.go
index 976d62599..119564d4d 100644
--- a/tools/sync-users/sync-users_test.go
+++ b/tools/sync-users/sync-users_test.go
@@ -492,3 +492,70 @@ func (s *TestSuite) TestFailOnEmptyUsernames(c *C) {
 	c.Assert(err, NotNil)
 	c.Assert(err, ErrorMatches, "skipped 1 user account.*with empty username.*")
 }
+
+func (s *TestSuite) TestFailOnDupedUsernameAndCaseInsensitiveMatching(c *C) {
+	for _, i := range []int{1, 2} {
+		var user arvados.User
+		emailPrefix := "john"
+		if i == 1 {
+			emailPrefix = "JOHN"
+		}
+		err := CreateUser(s.cfg.Client, &user, map[string]string{
+			"email":      fmt.Sprintf("%sdoe at example.com", emailPrefix),
+			"username":   "",
+			"first_name": "John",
+			"last_name":  "Doe",
+			"is_active":  "true",
+			"is_admin":   "false",
+		})
+		c.Assert(err, IsNil)
+		c.Assert(user.Username, Equals, fmt.Sprintf("%sdoe", emailPrefix))
+	}
+
+	s.cfg.Verbose = true
+	data := [][]string{
+		{"johndoe", "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"
+	s.cfg.CaseInsensitive = true
+	err = doMain(s.cfg)
+	c.Assert(err, NotNil)
+	c.Assert(err, ErrorMatches, "case insensitive collision for username.*between.*and.*")
+}
+
+func (s *TestSuite) TestSuccessOnUsernameAndCaseSensitiveMatching(c *C) {
+	for _, i := range []int{1, 2} {
+		var user arvados.User
+		emailPrefix := "john"
+		if i == 1 {
+			emailPrefix = "JOHN"
+		}
+		err := CreateUser(s.cfg.Client, &user, map[string]string{
+			"email":      fmt.Sprintf("%sdoe at example.com", emailPrefix),
+			"username":   "",
+			"first_name": "John",
+			"last_name":  "Doe",
+			"is_active":  "true",
+			"is_admin":   "false",
+		})
+		c.Assert(err, IsNil)
+		c.Assert(user.Username, Equals, fmt.Sprintf("%sdoe", emailPrefix))
+	}
+
+	s.cfg.Verbose = true
+	data := [][]string{
+		{"johndoe", "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"
+	s.cfg.CaseInsensitive = false
+	err = doMain(s.cfg)
+	c.Assert(err, IsNil)
+}

commit da532b4d0a1939bbfa063beaffc53582aa3907d6
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date:   Wed Jul 6 18:02:50 2022 -0300

    18858: Improves test & fixes discovered bug by said improved test.
    
    Also, fixes update decision function to avoid unnecessary calls on
    users that are already inactive but the CSV file list them with admin=true.
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>

diff --git a/tools/sync-users/sync-users.go b/tools/sync-users/sync-users.go
index 45b6e96b8..4b679ffe1 100644
--- a/tools/sync-users/sync-users.go
+++ b/tools/sync-users/sync-users.go
@@ -372,7 +372,7 @@ type userRecord struct {
 
 func needsUpdating(user arvados.User, record userRecord) bool {
 	userData := userRecord{"", user.FirstName, user.LastName, user.IsActive, user.IsAdmin}
-	recordData := userRecord{"", record.FirstName, record.LastName, record.Active, record.Admin}
+	recordData := userRecord{"", record.FirstName, record.LastName, record.Active, record.Active && record.Admin}
 	return userData != recordData
 }
 
@@ -444,7 +444,6 @@ func ProcessRecord(cfg *ConfigParams, record userRecord, userIDToUUID map[string
 			}
 		}
 	}
-	allUsers[record.UserID] = user
 	if createRequired {
 		log.Printf("Created user %q", record.UserID)
 	}
diff --git a/tools/sync-users/sync-users_test.go b/tools/sync-users/sync-users_test.go
index e5cbb77d1..976d62599 100644
--- a/tools/sync-users/sync-users_test.go
+++ b/tools/sync-users/sync-users_test.go
@@ -349,6 +349,24 @@ func (s *TestSuite) TestUserCreationAndUpdate(c *C) {
 
 func (s *TestSuite) TestDeactivateUnlisted(c *C) {
 	localUserUuidRegex := regexp.MustCompile(fmt.Sprintf("^%s-tpzed-[0-9a-z]{15}$", s.cfg.ClusterID))
+
+	var user1 arvados.User
+	for _, nr := range []int{1, 2} {
+		var newUser arvados.User
+		err := CreateUser(s.cfg.Client, &newUser, map[string]string{
+			"email":      fmt.Sprintf("user%d at example.com", nr),
+			"first_name": "Example",
+			"last_name":  fmt.Sprintf("User%d", nr),
+			"is_active":  "true",
+			"is_admin":   "false",
+		})
+		c.Assert(err, IsNil)
+		c.Assert(newUser.IsActive, Equals, true)
+		if nr == 1 {
+			user1 = newUser // for later confirmation
+		}
+	}
+
 	users, err := ListUsers(s.cfg.Client)
 	c.Assert(err, IsNil)
 	previouslyActiveUsers := 0
@@ -364,19 +382,21 @@ func (s *TestSuite) TestDeactivateUnlisted(c *C) {
 			previouslyActiveUsers++
 		}
 	}
-	// At least 3 active users: System root, Anonymous and the current user.
-	// Other active users should exist from fixture.
+	// Active users: System root, Anonymous, current user and the
+	// ones just created (other active users may exist from fixture).
 	c.Logf("Initial active users count: %d", previouslyActiveUsers)
-	c.Assert(previouslyActiveUsers > 3, Equals, true)
+	c.Assert(previouslyActiveUsers > 5, Equals, true)
 
-	s.cfg.DeactivateUnlisted = true
-	s.cfg.Verbose = true
+	// Here we omit user2 at example.com from the CSV file.
 	data := [][]string{
-		{"user1 at example.com", "Example", "User1", "0", "0"},
+		{"user1 at example.com", "Example", "User1", "1", "0"},
 	}
 	tmpfile, err := MakeTempCSVFile(data)
 	c.Assert(err, IsNil)
 	defer os.Remove(tmpfile.Name())
+
+	s.cfg.DeactivateUnlisted = true
+	s.cfg.Verbose = true
 	s.cfg.Path = tmpfile.Name()
 	err = doMain(s.cfg)
 	c.Assert(err, IsNil)
@@ -388,6 +408,7 @@ func (s *TestSuite) TestDeactivateUnlisted(c *C) {
 		fmt.Sprintf("%s-tpzed-000000000000000", s.cfg.ClusterID): true,
 		fmt.Sprintf("%s-tpzed-anonymouspublic", s.cfg.ClusterID): true,
 		s.cfg.CurrentUser.UUID: true,
+		user1.UUID:             true,
 	}
 	remainingActiveUUIDs := map[string]bool{}
 	seenUserEmails := map[string]bool{}
@@ -404,9 +425,10 @@ func (s *TestSuite) TestDeactivateUnlisted(c *C) {
 			currentlyActiveUsers++
 		}
 	}
-	// 3 active users remaining: System root, Anonymous and the current user.
+	// 4 active users remaining: System root, Anonymous, the current user
+	// and user1 at example.com
 	c.Logf("Active local users remaining: %v", remainingActiveUUIDs)
-	c.Assert(currentlyActiveUsers, Equals, 3)
+	c.Assert(currentlyActiveUsers, Equals, 4)
 }
 
 func (s *TestSuite) TestFailOnDuplicatedEmails(c *C) {

commit c592128fda794f2679a117a881c2f7d86ae091e0
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date:   Wed Jul 6 16:36:18 2022 -0300

    18858: Fixes positional argument retrieval on sync-users & sync-groups.
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>

diff --git a/tools/sync-groups/sync-groups.go b/tools/sync-groups/sync-groups.go
index e1e054a66..daec5c995 100644
--- a/tools/sync-groups/sync-groups.go
+++ b/tools/sync-groups/sync-groups.go
@@ -181,8 +181,10 @@ func ParseFlags(config *ConfigParams) error {
 	// Input file as a required positional argument
 	if flags.NArg() == 0 {
 		return fmt.Errorf("please provide a path to an input file")
+	} else if flags.NArg() > 1 {
+		return fmt.Errorf("please provide just one input file argument")
 	}
-	srcPath := &os.Args[flags.NFlag()+1]
+	srcPath := &os.Args[len(os.Args)-1]
 
 	// Validations
 	if *srcPath == "" {
diff --git a/tools/sync-users/sync-users.go b/tools/sync-users/sync-users.go
index f9f8b6046..45b6e96b8 100644
--- a/tools/sync-users/sync-users.go
+++ b/tools/sync-users/sync-users.go
@@ -123,8 +123,10 @@ func ParseFlags(cfg *ConfigParams) error {
 	// Input file as a required positional argument
 	if flags.NArg() == 0 {
 		return fmt.Errorf("please provide a path to an input file")
+	} else if flags.NArg() > 1 {
+		return fmt.Errorf("please provide just one input file argument")
 	}
-	srcPath := &os.Args[flags.NFlag()+1]
+	srcPath := &os.Args[len(os.Args)-1]
 
 	// Validations
 	if *srcPath == "" {
diff --git a/tools/sync-users/sync-users_test.go b/tools/sync-users/sync-users_test.go
index 1e5f688b0..e5cbb77d1 100644
--- a/tools/sync-users/sync-users_test.go
+++ b/tools/sync-users/sync-users_test.go
@@ -102,7 +102,7 @@ func (s *TestSuite) TestParseFlagsWithoutPositionalArgument(c *C) {
 }
 
 func (s *TestSuite) TestParseFlagsWrongUserID(c *C) {
-	os.Args = []string{"cmd", "-user-id=nickname", "/tmp/somefile.csv"}
+	os.Args = []string{"cmd", "-user-id", "nickname", "/tmp/somefile.csv"}
 	err := ParseFlags(&ConfigParams{})
 	c.Assert(err, NotNil)
 	c.Assert(err, ErrorMatches, ".*user ID must be one of:.*")
@@ -122,7 +122,7 @@ func (s *TestSuite) TestParseFlagsWithPositionalArgument(c *C) {
 
 func (s *TestSuite) TestParseFlagsWithOptionalFlags(c *C) {
 	cfg := ConfigParams{}
-	os.Args = []string{"cmd", "-verbose", "-deactivate-unlisted", "-user-id=username", "/tmp/somefile.csv"}
+	os.Args = []string{"cmd", "-verbose", "-deactivate-unlisted", "-user-id", "username", "/tmp/somefile.csv"}
 	err := ParseFlags(&cfg)
 	c.Assert(err, IsNil)
 	c.Assert(cfg.Path, Equals, "/tmp/somefile.csv")

commit 0ec937fd42be7f1d3757eba48fa944627dfe591d
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date:   Wed Jul 6 16:03:48 2022 -0300

    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 at curii.com>

diff --git a/tools/sync-users/sync-users.go b/tools/sync-users/sync-users.go
index 37b94a9f4..f9f8b6046 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 8b5385a32..1e5f688b0 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 at 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.*")
+}

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list