diff --git a/prow/cmd/peribolos/main.go b/prow/cmd/peribolos/main.go index da6e2c230c0b..7fa73df550bd 100644 --- a/prow/cmd/peribolos/main.go +++ b/prow/cmd/peribolos/main.go @@ -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 } @@ -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 { @@ -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 } @@ -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 { @@ -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 { @@ -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) } } @@ -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) } } @@ -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) } @@ -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) @@ -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: @@ -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)) } } @@ -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...) @@ -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) } diff --git a/prow/cmd/peribolos/main_test.go b/prow/cmd/peribolos/main_test.go index 249851ce7a49..8a66c3e80be8 100644 --- a/prow/cmd/peribolos/main_test.go +++ b/prow/cmd/peribolos/main_test.go @@ -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 { @@ -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", @@ -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{ @@ -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 { @@ -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{ @@ -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", @@ -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{ @@ -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 { @@ -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", @@ -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"}}, @@ -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) }