From 769c33851c873920d5c7ee0a67c441950408e23c Mon Sep 17 00:00:00 2001 From: Nikhita Raghunath Date: Sun, 13 Jan 2019 03:13:39 +0530 Subject: [PATCH 1/2] Add a test to check if team members are org members Also ensure that: - a user is not mentioned as both an org admin and org member - a user is not mentioned as both a team maintainer and member - no duplicates exist in the list of team maintainers and members --- Gopkg.lock | 1 + config/BUILD.bazel | 1 + config/config_test.go | 92 +++++++++++++++++++++++++++++++++++++------ 3 files changed, 81 insertions(+), 13 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 3fc4714a85..680ed4b0e0 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -989,6 +989,7 @@ "k8s.io/test-infra/prow/cmd/peribolos", "k8s.io/test-infra/prow/config", "k8s.io/test-infra/prow/config/org", + "k8s.io/test-infra/prow/github", ] solver-name = "gps-cdcl" solver-version = 1 diff --git a/config/BUILD.bazel b/config/BUILD.bazel index 93ea2a1478..bdcfb09cdb 100644 --- a/config/BUILD.bazel +++ b/config/BUILD.bazel @@ -29,6 +29,7 @@ go_test( "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//vendor/k8s.io/test-infra/prow/config:go_default_library", "//vendor/k8s.io/test-infra/prow/config/org:go_default_library", + "//vendor/k8s.io/test-infra/prow/github:go_default_library", ], ) diff --git a/config/config_test.go b/config/config_test.go index c7c01ffaa9..d5b3a816b3 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -27,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/test-infra/prow/config" "k8s.io/test-infra/prow/config/org" + "k8s.io/test-infra/prow/github" "github.com/ghodss/yaml" ) @@ -63,10 +64,11 @@ func loadOrg(dir string) (*org.Config, error) { return &cfg, nil } -func testDuplicates(list []string) error { +func testDuplicates(list sets.String) error { found := sets.String{} dups := sets.String{} - for _, i := range list { + all := list.List() + for _, i := range all { if found.Has(i) { dups.Insert(i) } @@ -78,15 +80,64 @@ func testDuplicates(list []string) error { return nil } -func isSorted(list []string) bool { - items := make([]string, len(list)) - for _, l := range list { +func isSorted(list sets.String) bool { + items := make([]string, list.Len()) + all := list.List() + for _, l := range all { items = append(items, strings.ToLower(l)) } return sort.StringsAreSorted(items) } +func normalize(s sets.String) sets.String { + out := sets.String{} + for i := range s { + out.Insert(github.NormLogin(i)) + } + return out +} + +// testTeamMembers ensures that a user is not a maintainer and member at the same time, +// there are no duplicate names in the list and all users are org members. +// TODO: also ensure that the list is sorted. +func testTeamMembers(teams map[string]org.Team, orgMembers sets.String) []error { + var errs []error + for teamName, team := range teams { + teamMaintainers := sets.NewString(team.Maintainers...) + teamMembers := sets.NewString(team.Members...) + + teamMaintainers = normalize(teamMaintainers) + teamMembers = normalize(teamMembers) + + // check for users in both maintainers and members + if both := teamMaintainers.Intersection(teamMembers); len(both) > 0 { + errs = append(errs, fmt.Errorf("The team %s has users in both maintainer admin and member roles: %s", teamName, strings.Join(both.List(), ", "))) + } + + // check for duplicates + if err := testDuplicates(teamMaintainers); err != nil { + errs = append(errs, fmt.Errorf("The team %s has duplicate maintainers: %v", teamName, err)) + } + if err := testDuplicates(teamMembers); err != nil { + errs = append(errs, fmt.Errorf("The team %s has duplicate members: %v", teamMembers, err)) + } + + // check if all are org members + if missing := teamMaintainers.Difference(orgMembers); len(missing) > 0 { + errs = append(errs, fmt.Errorf("The following maintainers of team %s are not org members: %s", teamName, strings.Join(missing.List(), ", "))) + } + if missing := teamMembers.Difference(orgMembers); len(missing) > 0 { + errs = append(errs, fmt.Errorf("The following members of team %s are not org members: %s", teamName, strings.Join(missing.List(), ", "))) + } + + if team.Children != nil { + errs = append(errs, testTeamMembers(team.Children, orgMembers)...) + } + } + return errs +} + func testOrg(targetDir string, t *testing.T) { cfg, err := loadOrg(targetDir) if err != nil { @@ -98,11 +149,21 @@ func testOrg(targetDir string, t *testing.T) { } members := sets.NewString(cfg.Members...) - members.Insert(cfg.Admins...) + admins := sets.NewString(cfg.Admins...) + admins = normalize(admins) + + if both := admins.Intersection(members); len(both) > 0 { + t.Errorf("users in both org admin and member roles: %s", strings.Join(both.List(), ", ")) + } reviewers := sets.NewString(own.Reviewers...) approvers := sets.NewString(own.Approvers...) + members = members.Union(admins) + members = normalize(members) + reviewers = normalize(reviewers) + approvers = normalize(approvers) + if n := len(approvers); n < 5 { t.Errorf("Require at least 5 approvers, found %d: %s", n, strings.Join(approvers.List(), ", ")) } @@ -114,7 +175,6 @@ func testOrg(targetDir string, t *testing.T) { t.Errorf("The following approvers must be members: %s", strings.Join(missing.List(), ", ")) } - admins := sets.NewString(cfg.Admins...) if !admins.Has("k8s-ci-robot") { t.Errorf("k8s-ci-robot must be an admin") } @@ -123,24 +183,30 @@ func testOrg(targetDir string, t *testing.T) { t.Errorf("billing_email must be unset") } - if err := testDuplicates(cfg.Admins); err != nil { + if err := testDuplicates(admins); err != nil { t.Errorf("duplicate admins: %v", err) } - if err := testDuplicates(cfg.Members); err != nil { + if err := testDuplicates(members); err != nil { t.Errorf("duplicate members: %v", err) } - if err := testDuplicates(own.Reviewers); err != nil { + if err := testDuplicates(reviewers); err != nil { t.Errorf("duplicate reviewers: %v", err) } - if err := testDuplicates(own.Approvers); err != nil { + if err := testDuplicates(approvers); err != nil { t.Errorf("duplicate approvers: %v", err) } - if !isSorted(cfg.Admins) { + if !isSorted(admins) { t.Errorf("admins are unsorted") } - if !isSorted(cfg.Members) { + if !isSorted(members) { t.Errorf("members are unsorted") } + + if errs := testTeamMembers(cfg.Teams, members); errs != nil { + for _, err := range errs { + t.Error(err) + } + } } func TestAllOrgs(t *testing.T) { From 5bc6866678c8b78e44feab8a4060668f1dde8d0b Mon Sep 17 00:00:00 2001 From: Nikhita Raghunath Date: Sun, 13 Jan 2019 03:33:36 +0530 Subject: [PATCH 2/2] Fix Atoms' username for k, k-sigs --- config/kubernetes-sigs/org.yaml | 2 +- config/kubernetes/org.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/kubernetes-sigs/org.yaml b/config/kubernetes-sigs/org.yaml index 439041cadf..e6c5a1e471 100644 --- a/config/kubernetes-sigs/org.yaml +++ b/config/kubernetes-sigs/org.yaml @@ -24,7 +24,7 @@ members: - animeshsingh - ant31 - apelisse -- atoms +- Atoms - balajismaniam - BenTheElder - bertinatto diff --git a/config/kubernetes/org.yaml b/config/kubernetes/org.yaml index a8d1eac0dd..5eadbe9587 100644 --- a/config/kubernetes/org.yaml +++ b/config/kubernetes/org.yaml @@ -67,7 +67,7 @@ members: - ash2k - ashcrow - asultan001 -- atoms +- Atoms - austbot - aveshagarwal - awly