From 6127ebcdd5614a2481ffe06a6c0953e665939bcc Mon Sep 17 00:00:00 2001 From: Madhav Jivrajani Date: Wed, 16 Aug 2023 11:53:34 +0530 Subject: [PATCH 1/4] Revert "peribolos: fix --skip-removal flag validation" Now that peribolos is back on its feet, we don't need this functionality. This reverts commit 1c2c4cd2bf25a16be50331fd51a29531d270153d. --- prow/cmd/peribolos/main.go | 2 +- prow/cmd/peribolos/main_test.go | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/prow/cmd/peribolos/main.go b/prow/cmd/peribolos/main.go index da6e2c230c0b..abd88640d81f 100644 --- a/prow/cmd/peribolos/main.go +++ b/prow/cmd/peribolos/main.go @@ -144,7 +144,7 @@ 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) { + 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") } diff --git a/prow/cmd/peribolos/main_test.go b/prow/cmd/peribolos/main_test.go index 249851ce7a49..1c83c14be74a 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 { From e43c114aa5a727204ea170737b70cdfc7fb4c210 Mon Sep 17 00:00:00 2001 From: Madhav Jivrajani Date: Wed, 16 Aug 2023 11:54:13 +0530 Subject: [PATCH 2/4] Revert "Update prow/cmd/peribolos/main.go" Now that peribolos is back on its feet, we don't need skip-removals anymore. This reverts commit 77015184ef38f3db509bd0877925124a3347fdc8. --- prow/cmd/peribolos/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prow/cmd/peribolos/main.go b/prow/cmd/peribolos/main.go index abd88640d81f..da9119c235d9 100644 --- a/prow/cmd/peribolos/main.go +++ b/prow/cmd/peribolos/main.go @@ -155,7 +155,7 @@ func (o *options) parseArgs(flags *flag.FlagSet, args []string) error { logrus.SetLevel(level) if o.skipRemovals { - logrus.Warnln("--skip-removals is being used. This flag is not supported and will be removed") + logrus.Warnln("--skip-removals is being used. This flag should eventually be removed") } return nil From 249a3aab24c24496960989dd3a48b6bf13d80d6b Mon Sep 17 00:00:00 2001 From: Madhav Jivrajani Date: Wed, 16 Aug 2023 11:54:50 +0530 Subject: [PATCH 3/4] Revert "peribolos: add warning log when --skip-removals is being used" Now that peribolos is back on its feet, we don't need skip-removals anymore. This reverts commit 508f54058baa36d4e390f507d529de5e32eddf80. --- prow/cmd/peribolos/main.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/prow/cmd/peribolos/main.go b/prow/cmd/peribolos/main.go index da9119c235d9..ec757dbe9a05 100644 --- a/prow/cmd/peribolos/main.go +++ b/prow/cmd/peribolos/main.go @@ -154,10 +154,6 @@ func (o *options) parseArgs(flags *flag.FlagSet, args []string) error { } logrus.SetLevel(level) - if o.skipRemovals { - logrus.Warnln("--skip-removals is being used. This flag should eventually be removed") - } - return nil } From 9e7b3ad78ecd3e26654eb028d41744b93a07afa1 Mon Sep 17 00:00:00 2001 From: Madhav Jivrajani Date: Wed, 16 Aug 2023 11:55:31 +0530 Subject: [PATCH 4/4] Revert "peribolos: add `--skip-removals` flag to peribolos" Now that peribolos is back on its feet, we don't need skip-removals anymore. This reverts commit a0f8390aacfc188be0615a76b0461711235a80ad. --- prow/cmd/peribolos/main.go | 40 ++++------- prow/cmd/peribolos/main_test.go | 113 ++++---------------------------- 2 files changed, 24 insertions(+), 129 deletions(-) diff --git a/prow/cmd/peribolos/main.go b/prow/cmd/peribolos/main.go index ec757dbe9a05..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,10 +138,6 @@ 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) @@ -506,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 { @@ -531,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 { @@ -554,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) } } @@ -862,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) } } @@ -1086,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) } @@ -1163,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) @@ -1201,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: @@ -1222,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)) } } @@ -1254,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...) @@ -1323,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 1c83c14be74a..8a66c3e80be8 100644 --- a/prow/cmd/peribolos/main_test.go +++ b/prow/cmd/peribolos/main_test.go @@ -332,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", @@ -405,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{ @@ -469,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 { @@ -658,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{ @@ -1300,7 +1268,6 @@ func TestConfigureTeamMembers(t *testing.T) { invitees sets.Set[string] team org.Team slug string - skipRemovals bool }{ { name: "fail when listing fails", @@ -1331,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{ @@ -1395,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 { @@ -2367,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", @@ -2456,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"}}, @@ -2608,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) }