Skip to content

Commit

Permalink
Merge pull request #30392 from MadhavJivrajani/revert-skip-removals-impl
Browse files Browse the repository at this point in the history 
peribolos: revert implementation of `--skip-removals` flag
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Aug 17, 2023
2 parents 5835943 + 9e7b3ad commit a755575
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 137 deletions.
44 changes: 13 additions & 31 deletions prow/cmd/peribolos/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,7 @@ type options struct {
ignoreSecretTeams bool
allowRepoArchival bool
allowRepoPublish bool
// TODO(MadhavJivrajani): this is a temporary mitigation to help
// fix a configuration mishap:
// https://kubernetes.slack.com/archives/CHGFYJVAN/p1690906660582369
// There is no real use for this, remove once things are in good shape.
skipRemovals bool
github flagutil.GitHubOptions
github flagutil.GitHubOptions

logLevel string
}
Expand Down Expand Up @@ -99,7 +94,6 @@ func (o *options) parseArgs(flags *flag.FlagSet, args []string) error {
flags.BoolVar(&o.fixRepos, "fix-repos", false, "Create/update repositories if set")
flags.BoolVar(&o.allowRepoArchival, "allow-repo-archival", false, "If set, archiving repos is allowed while updating repos")
flags.BoolVar(&o.allowRepoPublish, "allow-repo-publish", false, "If set, making private repos public is allowed while updating repos")
flags.BoolVar(&o.skipRemovals, "skip-removals", false, "If set, peribolos does not make removals of any kind")
flags.StringVar(&o.logLevel, "log-level", logrus.InfoLevel.String(), fmt.Sprintf("Logging level, one of %v", logrus.AllLevels))
o.github.AddCustomizedFlags(flags, flagutil.ThrottlerDefaults(defaultTokens, defaultBurst))
if err := flags.Parse(args); err != nil {
Expand Down Expand Up @@ -144,20 +138,12 @@ func (o *options) parseArgs(flags *flag.FlagSet, args []string) error {
return fmt.Errorf("--fix-team-repos requires --fix-teams")
}

if o.skipRemovals && !(o.fixTeams || o.fixTeamRepos || o.fixTeamMembers || o.fixOrg || o.fixOrgMembers) {
return fmt.Errorf("--skip-removals requires atleast one of --fix-teams, --fix-team-repos, --fix-team-members, --fix-org, --fix-org-members")
}

level, err := logrus.ParseLevel(o.logLevel)
if err != nil {
return fmt.Errorf("--log-level invalid: %w", err)
}
logrus.SetLevel(level)

if o.skipRemovals {
logrus.Warnln("--skip-removals is being used. This flag is not supported and will be removed")
}

return nil
}

Expand Down Expand Up @@ -510,7 +496,7 @@ func configureOrgMembers(opt options, client orgClient, orgName string, orgConfi
return err
}

return configureMembers(have, want, invitees, opt, adder, remover)
return configureMembers(have, want, invitees, adder, remover)
}

type memberships struct {
Expand All @@ -535,7 +521,7 @@ func (m *memberships) normalize() {
m.super = normalize(m.super)
}

func configureMembers(have, want memberships, invitees sets.Set[string], opts options, adder func(user string, super bool) error, remover func(user string) error) error {
func configureMembers(have, want memberships, invitees sets.Set[string], adder func(user string, super bool) error, remover func(user string) error) error {
have.normalize()
want.normalize()
if both := want.super.Intersection(want.members); len(both) > 0 {
Expand All @@ -558,11 +544,9 @@ func configureMembers(have, want memberships, invitees sets.Set[string], opts op
}
}

if !opts.skipRemovals {
for u := range remove {
if err := remover(u); err != nil {
errs = append(errs, err)
}
for u := range remove {
if err := remover(u); err != nil {
errs = append(errs, err)
}
}

Expand Down Expand Up @@ -866,7 +850,7 @@ func configureOrg(opt options, client github.Client, orgName string, orgConfig o
logrus.Infof("Skipping team repo permissions configuration")
continue
}
if err := configureTeamRepos(client, opt, githubTeams, name, orgName, team); err != nil {
if err := configureTeamRepos(client, githubTeams, name, orgName, team); err != nil {
return fmt.Errorf("failed to configure %s team %s repos: %w", orgName, name, err)
}
}
Expand Down Expand Up @@ -1090,7 +1074,7 @@ func configureTeamAndMembers(opt options, client github.Client, githubTeams map[
// Configure team members
if !opt.fixTeamMembers {
logrus.Infof("Skipping %s member configuration", name)
} else if err = configureTeamMembers(client, opt, orgName, gt, team, opt.ignoreInvitees); err != nil {
} else if err = configureTeamMembers(client, orgName, gt, team, opt.ignoreInvitees); err != nil {
if opt.confirm {
return fmt.Errorf("failed to update %s members: %w", name, err)
}
Expand Down Expand Up @@ -1167,7 +1151,7 @@ type teamRepoClient interface {
}

// configureTeamRepos updates the list of repos that the team has permissions for when necessary
func configureTeamRepos(client teamRepoClient, opts options, githubTeams map[string]github.Team, name, orgName string, team org.Team) error {
func configureTeamRepos(client teamRepoClient, githubTeams map[string]github.Team, name, orgName string, team org.Team) error {
gt, ok := githubTeams[name]
if !ok { // configureTeams is buggy if this is the case
return fmt.Errorf("%s not found in id list", name)
Expand Down Expand Up @@ -1205,9 +1189,7 @@ func configureTeamRepos(client teamRepoClient, opts options, githubTeams map[str
var err error
switch permission {
case github.None:
if !opts.skipRemovals {
err = client.RemoveTeamRepoBySlug(orgName, gt.Slug, repo)
}
err = client.RemoveTeamRepoBySlug(orgName, gt.Slug, repo)
case github.Admin:
err = client.UpdateTeamRepoBySlug(orgName, gt.Slug, repo, github.RepoAdmin)
case github.Write:
Expand All @@ -1226,7 +1208,7 @@ func configureTeamRepos(client teamRepoClient, opts options, githubTeams map[str
}

for childName, childTeam := range team.Children {
if err := configureTeamRepos(client, opts, githubTeams, childName, orgName, childTeam); err != nil {
if err := configureTeamRepos(client, githubTeams, childName, orgName, childTeam); err != nil {
updateErrors = append(updateErrors, fmt.Errorf("failed to configure %s child team %s repos: %w", orgName, childName, err))
}
}
Expand Down Expand Up @@ -1258,7 +1240,7 @@ func teamInvitations(client teamMembersClient, orgName, teamSlug string) (sets.S
}

// configureTeamMembers will add/update people to the appropriate role on the team, and remove anyone else.
func configureTeamMembers(client teamMembersClient, opts options, orgName string, gt github.Team, team org.Team, ignoreInvitees bool) error {
func configureTeamMembers(client teamMembersClient, orgName string, gt github.Team, team org.Team, ignoreInvitees bool) error {
// Get desired state
wantMaintainers := sets.New[string](team.Maintainers...)
wantMembers := sets.New[string](team.Members...)
Expand Down Expand Up @@ -1327,5 +1309,5 @@ func configureTeamMembers(client teamMembersClient, opts options, orgName string

want := memberships{members: wantMembers, super: wantMaintainers}
have := memberships{members: haveMembers, super: haveMaintainers}
return configureMembers(have, want, invitees, opts, adder, remover)
return configureMembers(have, want, invitees, adder, remover)
}
117 changes: 11 additions & 106 deletions prow/cmd/peribolos/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,6 @@ func TestOptions(t *testing.T) {
logLevel: "debug",
},
},
{
name: "reject --skip-removals without a fix flag",
args: []string{"--config-path=foo", "--skip-removals"},
},
}

for _, tc := range cases {
Expand Down Expand Up @@ -336,15 +332,14 @@ func (c *fakeClient) RemoveTeamMembershipBySlug(org, teamSlug, user string) erro

func TestConfigureMembers(t *testing.T) {
cases := []struct {
name string
want memberships
have memberships
remove sets.Set[string]
members sets.Set[string]
supers sets.Set[string]
invitees sets.Set[string]
err bool
skipRemovals bool
name string
want memberships
have memberships
remove sets.Set[string]
members sets.Set[string]
supers sets.Set[string]
invitees sets.Set[string]
err bool
}{
{
name: "forgot to remove duplicate entry",
Expand Down Expand Up @@ -409,21 +404,6 @@ func TestConfigureMembers(t *testing.T) {
members: sets.New[string]("new-member"),
supers: sets.New[string]("new-admin"),
},
{
name: "some of everything; skip removals",
have: memberships{
super: sets.New[string]("keep-admin", "drop-admin"),
members: sets.New[string]("keep-member", "drop-member"),
},
want: memberships{
members: sets.New[string]("keep-member", "drop-member", "new-member"),
super: sets.New[string]("keep-admin", "drop-admin", "new-admin"),
},
remove: sets.Set[string]{},
members: sets.New[string]("new-member"),
supers: sets.New[string]("new-admin"),
skipRemovals: true,
},
{
name: "ensure case insensitivity",
have: memberships{
Expand Down Expand Up @@ -473,7 +453,7 @@ func TestConfigureMembers(t *testing.T) {
return nil
}

err := configureMembers(tc.have, tc.want, tc.invitees, options{skipRemovals: tc.skipRemovals}, adder, remover)
err := configureMembers(tc.have, tc.want, tc.invitees, adder, remover)
switch {
case err != nil:
if !tc.err {
Expand Down Expand Up @@ -662,22 +642,6 @@ func TestConfigureOrgMembers(t *testing.T) {
addMembers: []string{"new-member"},
addAdmins: []string{"new-admin"},
},
{
name: "some of everything; skipRemovals",
config: org.Config{
Admins: []string{"keep-admin", "new-admin"},
Members: []string{"keep-member", "new-member"},
},
opt: options{
maximumDelta: 0.5,
skipRemovals: true,
},
admins: []string{"keep-admin", "drop-admin"},
members: []string{"keep-member", "drop-member"},
remove: []string{},
addMembers: []string{"new-member"},
addAdmins: []string{"new-admin"},
},
{
name: "do not reinvite",
config: org.Config{
Expand Down Expand Up @@ -1304,7 +1268,6 @@ func TestConfigureTeamMembers(t *testing.T) {
invitees sets.Set[string]
team org.Team
slug string
skipRemovals bool
}{
{
name: "fail when listing fails",
Expand Down Expand Up @@ -1335,19 +1298,6 @@ func TestConfigureTeamMembers(t *testing.T) {
addMembers: sets.New[string]("new-member"),
addMaintainers: sets.New[string]("new-maintainer"),
},
{
name: "some of everything; skip removals",
team: org.Team{
Maintainers: []string{"keep-maintainer", "new-maintainer"},
Members: []string{"keep-member", "new-member"},
},
maintainers: sets.New[string]("keep-maintainer", "drop-maintainer"),
members: sets.New[string]("keep-member", "drop-member"),
remove: sets.Set[string]{},
addMembers: sets.New[string]("new-member"),
addMaintainers: sets.New[string]("new-maintainer"),
skipRemovals: true,
},
{
name: "do not reinvitee invitees",
team: org.Team{
Expand Down Expand Up @@ -1399,7 +1349,7 @@ func TestConfigureTeamMembers(t *testing.T) {
newAdmins: sets.Set[string]{},
newMembers: sets.Set[string]{},
}
err := configureTeamMembers(fc, options{skipRemovals: tc.skipRemovals}, "", gt, tc.team, tc.ignoreInvitees)
err := configureTeamMembers(fc, "", gt, tc.team, tc.ignoreInvitees)
switch {
case err != nil:
if !tc.err {
Expand Down Expand Up @@ -2371,7 +2321,6 @@ func TestConfigureTeamRepos(t *testing.T) {
failRemove bool
expected map[string][]github.Repo
expectedErr bool
skipRemovals bool
}{
{
name: "githubTeams cache not containing team errors",
Expand Down Expand Up @@ -2460,50 +2409,6 @@ func TestConfigureTeamRepos(t *testing.T) {
{Name: "admin", Permissions: github.RepoPermissions{Pull: true}},
}},
},
{
name: "change in permission on existing gets updated; changing to none leads to deletion",
githubTeams: map[string]github.Team{"team": {ID: 1, Slug: "team"}},
teamName: "team",
team: org.Team{
Repos: map[string]github.RepoPermissionLevel{
"read": github.None, // this should be deleted
"write": github.Write,
"admin": github.Read,
},
},
existingRepos: map[string][]github.Repo{"team": {
{Name: "read", Permissions: github.RepoPermissions{Pull: true}},
{Name: "write", Permissions: github.RepoPermissions{Pull: true, Triage: true, Push: true}},
{Name: "admin", Permissions: github.RepoPermissions{Pull: true, Triage: true, Push: true, Maintain: true, Admin: true}},
}},
expected: map[string][]github.Repo{"team": {
{Name: "write", Permissions: github.RepoPermissions{Pull: true, Triage: true, Push: true}},
{Name: "admin", Permissions: github.RepoPermissions{Pull: true}},
}},
},
{
name: "change in permission on existing gets updated; changing to none leads to deletion; skipRemovals",
githubTeams: map[string]github.Team{"team": {ID: 1, Slug: "team"}},
teamName: "team",
team: org.Team{
Repos: map[string]github.RepoPermissionLevel{
"read": github.None, // this should be deleted, but won't happen because skipRemovals
"write": github.Write,
"admin": github.Read,
},
},
existingRepos: map[string][]github.Repo{"team": {
{Name: "read", Permissions: github.RepoPermissions{Pull: true}},
{Name: "write", Permissions: github.RepoPermissions{Pull: true, Triage: true, Push: true}},
{Name: "admin", Permissions: github.RepoPermissions{Pull: true, Triage: true, Push: true, Maintain: true, Admin: true}},
}},
expected: map[string][]github.Repo{"team": {
{Name: "read", Permissions: github.RepoPermissions{Pull: true}},
{Name: "write", Permissions: github.RepoPermissions{Pull: true, Triage: true, Push: true}},
{Name: "admin", Permissions: github.RepoPermissions{Pull: true}},
}},
skipRemovals: true,
},
{
name: "omitted requirement gets removed",
githubTeams: map[string]github.Team{"team": {ID: 1, Slug: "team"}},
Expand Down Expand Up @@ -2612,7 +2517,7 @@ func TestConfigureTeamRepos(t *testing.T) {
failUpdate: testCase.failUpdate,
failRemove: testCase.failRemove,
}
err := configureTeamRepos(&client, options{skipRemovals: testCase.skipRemovals}, testCase.githubTeams, testCase.teamName, "org", testCase.team)
err := configureTeamRepos(&client, testCase.githubTeams, testCase.teamName, "org", testCase.team)
if err == nil && testCase.expectedErr {
t.Errorf("%s: expected an error but got none", testCase.name)
}
Expand Down

0 comments on commit a755575

Please sign in to comment.