-
Notifications
You must be signed in to change notification settings - Fork 687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a test to check if team members are org members #350
Conversation
cde8942
to
68798e4
Compare
@@ -24,7 +24,7 @@ members: | |||
- animeshsingh | |||
- ant31 | |||
- apelisse | |||
- atoms | |||
- Atoms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--- FAIL: TestAllOrgs (0.03s)
--- FAIL: TestAllOrgs/kubernetes-sigs (0.00s)
config_test.go:169: The following members of team kubespray-maintainers are not org members: Atoms
FAIL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a good test case, because we shouldn't care about capitalization. The peribolos code has normalization functions we should use.
org/vendor/k8s.io/test-infra/prow/cmd/peribolos/main.go
Lines 402 to 408 in 847b18b
func normalize(s sets.String) sets.String { | |
out := sets.String{} | |
for i := range s { | |
out.Insert(github.NormLogin(i)) | |
} | |
return out | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we export this as well?
68798e4
to
8cfecba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this config validation stuff should actually be functions in the prow/config/
package that we export, so that they're reusable.
WDYT @fejta?
@@ -24,7 +24,7 @@ members: | |||
- animeshsingh | |||
- ant31 | |||
- apelisse | |||
- atoms | |||
- Atoms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a good test case, because we shouldn't care about capitalization. The peribolos code has normalization functions we should use.
org/vendor/k8s.io/test-infra/prow/cmd/peribolos/main.go
Lines 402 to 408 in 847b18b
func normalize(s sets.String) sets.String { | |
out := sets.String{} | |
for i := range s { | |
out.Insert(github.NormLogin(i)) | |
} | |
return out | |
} |
@@ -87,6 +87,28 @@ func isSorted(list []string) bool { | |||
return sort.StringsAreSorted(items) | |||
} | |||
|
|||
// testTeamMembers checks if team maintainers and members are org members. | |||
// TODO: also ensure that the list is sorted. | |||
func testTeamMembers(teams map[string]org.Team, orgMembers sets.String) []error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add a test case to ensure that a particular user doesn't exist in both the maintainers and members lists at the same time? This is also a condition that peribolos will error on, but the config checker presubmit doesn't handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also added a check for duplicate names in the lists.
8cfecba
to
da62aca
Compare
+1. Can we also make sure that we are sorting before dumping all names? We would be sorted (sorry for the pun, couldn't resist) once we sort all names and add a test for new ones in the CI, but it might prove useful to others who will use peribolos. |
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
da62aca
to
5bc6866
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/lgtm Thanks so much @nikhita! 🎈 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cblecker, nikhita The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This would be useful to fail early if someone who is added as a team maintainer or member is not an org member. See #346 (review) for an example.
This PR also validates:
/assign @cblecker @fejta