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

Merged
merged 2 commits into from Jan 14, 2019

Conversation

Projects
None yet
4 participants
@nikhita
Copy link
Member

nikhita commented Jan 12, 2019

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:

  • 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

/assign @cblecker @fejta

@k8s-ci-robot k8s-ci-robot requested review from fejta and grodrigues3 Jan 12, 2019

@k8s-ci-robot k8s-ci-robot added size/S size/M and removed size/S labels Jan 12, 2019

@nikhita nikhita force-pushed the nikhita:team-members-test branch from cde8942 to 68798e4 Jan 12, 2019

@@ -24,7 +24,7 @@ members:
- animeshsingh
- ant31
- apelisse
- atoms
- Atoms

This comment has been minimized.

@nikhita

nikhita Jan 12, 2019

Member
--- 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

This comment has been minimized.

@cblecker

cblecker Jan 13, 2019

Member

This is actually a good test case, because we shouldn't care about capitalization. The peribolos code has normalization functions we should use.

func normalize(s sets.String) sets.String {
out := sets.String{}
for i := range s {
out.Insert(github.NormLogin(i))
}
return out
}

This comment has been minimized.

@nikhita

nikhita Jan 13, 2019

Member

Can we export this as well?

@nikhita nikhita force-pushed the nikhita:team-members-test branch from 68798e4 to 8cfecba Jan 12, 2019

@cblecker
Copy link
Member

cblecker left a comment

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

This comment has been minimized.

@cblecker

cblecker Jan 13, 2019

Member

This is actually a good test case, because we shouldn't care about capitalization. The peribolos code has normalization functions we should use.

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 {

This comment has been minimized.

@cblecker

cblecker Jan 13, 2019

Member

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.

This comment has been minimized.

@nikhita

nikhita Jan 13, 2019

Member

Done. Also added a check for duplicate names in the lists.

@nikhita nikhita force-pushed the nikhita:team-members-test branch from 8cfecba to da62aca Jan 13, 2019

@nikhita

This comment has been minimized.

Copy link
Member

nikhita commented Jan 13, 2019

I wonder if this config validation stuff should actually be functions in the prow/config/ package that we export, so that they're reusable.

+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.

nikhita added some commits Jan 12, 2019

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

@nikhita nikhita force-pushed the nikhita:team-members-test branch from da62aca to 5bc6866 Jan 13, 2019

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 14, 2019

@fejta
Copy link
Contributor

fejta left a comment

/lgtm

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Jan 14, 2019

/lgtm
/approve

Thanks so much @nikhita! 🎈

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 14, 2019

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit f6b171c into kubernetes:master Jan 14, 2019

3 of 4 checks passed

tide Not mergeable.
Details
cla/linuxfoundation nikhita authorized
Details
pull-org-test-all Job succeeded.
Details
pull-org-verify-all Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment