Skip to content
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

Allow peribolos to manage repo permissions for teams #12144

Merged

Conversation

@stevekuznetsov
Copy link
Contributor

stevekuznetsov commented Apr 10, 2019

With this patch, the configuration for Peribolos/org management can now
be used to enforce a specific set of repository permissions for the
groups that it manages.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

/assign @fejta @cjwagner

// RepoPermissionLevel is admin, write, read or none.
//
// See https://developer.github.com/v3/repos/collaborators/#review-a-users-permission-level
type RepoPermissionLevel string

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Apr 10, 2019

Author Contributor

Moving this was necessary to break an import cycle

@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/peribolos-teams branch from 1cf0064 to d237eb8 Apr 10, 2019

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

stevekuznetsov commented Apr 10, 2019

Added more unit tests.

@fejta
Copy link
Contributor

fejta left a comment

Cool!

/assign @cblecker @nikhita

Any chance we can gate this new functionality behind a flag?

Alternatively how Christoph/Nikhita, are we interested in using this functionality in k/org immediately?

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

stevekuznetsov commented Apr 10, 2019

If you --dump first it should have no effect on GitHub state, so I thought gating it would not be necessary. WDYT?

@@ -46,6 +46,9 @@ orgs:
- anne
maintainers:
- jane
repos: # Ensure the team has the following permissions levels on repos in the org
some-repo: admin

This comment has been minimized.

Copy link
@fejta

fejta Apr 10, 2019

Contributor

Once we have repo management I feel like this would be better to scope under the repo.

There's no guarantee that a some-repo exists (although I guess one could also argue that there's no guarantee that some-team exists)

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Apr 10, 2019

Author Contributor

I thought about this, too, but:

  1. GitHub's API works through the team, so I figured it's best if we mirror it for code complexity
  2. Running on repo, not on team, removes the nice property for dumps above and will make honoring the flag to ignore teams harder
  3. Running per repo also adds a substantial API burden to list repo state through teams
@fejta

This comment has been minimized.

Copy link
Contributor

fejta commented Apr 10, 2019

Most of the other features are flag-gated:

flags.BoolVar(&o.fixOrg, "fix-org", false, "Change org metadata if set")
flags.BoolVar(&o.fixOrgMembers, "fix-org-members", false, "Add/remove org members if set")
flags.BoolVar(&o.fixTeams, "fix-teams", false, "Create/delete/update teams if set")
flags.BoolVar(&o.fixTeamMembers, "fix-team-members", false, "Add/remove team members if set")

@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/peribolos-teams branch 2 times, most recently from d8569bb to fee9dc1 Apr 10, 2019

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

stevekuznetsov commented Apr 10, 2019

Ahh, missed that -- added a gating flag.

@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/peribolos-teams branch from fee9dc1 to 52df94e Apr 10, 2019

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

stevekuznetsov commented Apr 10, 2019

@fejta updated the PR to rebase on #12154 and added a debugging commit which we can either drop or run in a different PR, whichever you prefer

@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/peribolos-teams branch 3 times, most recently from a5359c7 to 426fe57 Apr 10, 2019

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

stevekuznetsov commented Apr 10, 2019

Fixed the flag test, made repos omitempty for --dump. @fejta @cblecker @nikhita this is ready for review :)

@fejta

fejta approved these changes Apr 11, 2019

Copy link
Contributor

fejta left a comment

/hold

// ListTeamRepos gets a list of team repos for the given team id
//
// https://developer.github.com/v3/teams/#list-team-repos
func (c *Client) ListTeamRepos(id int) ([]Repo, error) {

This comment has been minimized.

Copy link
@fejta

fejta Apr 11, 2019

Contributor

I'd be inclined to put the client changes in their own commit

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Apr 11, 2019

Author Contributor

done

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 11, 2019

LGTM label has been added.

Git tree hash: 7c56277968d7c7901c1e76c2807296b9933f1484

stevekuznetsov added some commits Apr 11, 2019

Add GitHub client support for team repo permissions
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
Allow peribolos to manage repo permissions for teams
With this patch, the configuration for Peribolos/org management can now
be used to enforce a specific set of repository permissions for the
groups that it manages.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
Add logging for peribolos dump
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/peribolos-teams branch from 426fe57 to aad1b54 Apr 11, 2019

@fejta

fejta approved these changes Apr 11, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

stevekuznetsov commented Apr 11, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit db24b30 into kubernetes:master Apr 11, 2019

14 checks passed

cla/linuxfoundation stevekuznetsov 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-file-perms Job succeeded.
Details
pull-test-infra-verify-github-spelling Job succeeded.
Details
pull-test-infra-verify-gofmt Job succeeded.
Details
pull-test-infra-verify-labels Skipped.
pull-test-infra-verify-tslint Skipped.
tide In merge pool.
Details
@cblecker
Copy link
Member

cblecker left a comment

Biggest concern with doing it this way, is the delegation piece. If you delegate management of a team out to someone, then can just add rights to any repo in the org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.