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

Don't remove team members with pending invites #10606

Merged
merged 1 commit into from Jan 7, 2019

Conversation

Projects
None yet
4 participants
@cblecker
Copy link
Member

cblecker commented Jan 4, 2019

Found another peribolos bug. Right now if fix-teams is on, and there are pending invites to the org, peribolos will try to remove them from the team, even if they're not a team member. Has to do with the way we were piping invites through the configureTeamAndMembers function.

We're now going to list the invites that are applicable for the team, basically so we can ignore them. But after this change, we can add someone to both the org and teams, and peribolos will add them to the right teams after they accept the invite.

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Jan 4, 2019

/assign @fejta

@@ -415,6 +415,7 @@ func (m *memberships) normalize() {
func configureMembers(have, want memberships, invitees sets.String, adder func(user string, super bool) error, remover func(user string) error) error {
have.normalize()
want.normalize()
invitees = normalize(invitees)

This comment has been minimized.

@nikhita

nikhita Jan 4, 2019

Member

Maybe I'm missing something here...but shouldn't we normalize invitees in configureOrgMembers so that the following (in
configureOrgMembers) will yield the correct result?

if invitees.Has(user) { // Do not add them, as this causes another invite.

The normalized invitees can then be passed onto configureMembers, instead of normalizing here.

adder := func(user string, super bool) error {
if invitees.Has(user) {
if haveInvites.Has(user) {

This comment has been minimized.

@nikhita

nikhita Jan 4, 2019

Member

With the same logic, I think we need to normalize haveInvites as well.

@cblecker cblecker force-pushed the cblecker:team-no-reinvite branch from 283f348 to 1d0edf3 Jan 4, 2019

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Jan 4, 2019

@nikhita Looking at how we do it for org invites, we normalize inside a function, because there's also the possibility of the login being null (if it's a direct e-mail invite). Changed how we're handling teams to do it the same way.

@cblecker cblecker force-pushed the cblecker:team-no-reinvite branch from 1d0edf3 to c2664d6 Jan 4, 2019

@fejta

fejta approved these changes Jan 7, 2019

Copy link
Contributor

fejta left a comment

/lgtm
/hold

if err != nil {
return fmt.Errorf("failed to configure %s teams: %v", orgName, err)
}
}
return nil
}

func configureTeamAndMembers(client *github.Client, fixMembers bool, githubTeams map[string]github.Team, name, orgName string, team org.Team, invitees sets.String, parent *int) error {
func configureTeamAndMembers(opt options, client *github.Client, githubTeams map[string]github.Team, name, orgName string, team org.Team, parent *int) error {

This comment has been minimized.

@fejta

fejta Jan 7, 2019

Contributor

Why pass in opt instead of fixTeamMembers?

This comment has been minimized.

@cblecker

cblecker Jan 7, 2019

Member

I left this for readability, mostly. When I was working on this, I had copied the entire orgInvitations function which had some opt checks.. those checks were redundant this far into the fix-members process, so I stripped them.. but this just makes the entire set of options available, if we in the future need to check an option further into the process.

It's possible to revert to only passing the bool in, but IMO this just makes it cleaner.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 7, 2019

LGTM label has been added.

Git tree hash: ba280318d75769e96b18eceecc803632e4dbafa6

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 7, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, fejta

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

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented Jan 7, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 71c31c7 into kubernetes:master Jan 7, 2019

12 checks passed

cla/linuxfoundation cblecker authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Skipped
pull-test-infra-lint Job succeeded.
Details
pull-test-infra-verify-bazel Job succeeded.
Details
pull-test-infra-verify-codegen Job succeeded.
Details
pull-test-infra-verify-config Job succeeded.
Details
pull-test-infra-verify-deps Skipped
pull-test-infra-verify-gofmt Job succeeded.
Details
pull-test-infra-verify-govet Job succeeded.
Details
pull-test-infra-verify-labels Skipped
tide In merge pool.
Details

@cblecker cblecker deleted the cblecker:team-no-reinvite branch Jan 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment