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

groups: allow restricting creation of new groups in sub-directories #2401

Merged
merged 2 commits into from
Jul 28, 2021

Conversation

nikhita
Copy link
Member

@nikhita nikhita commented Jul 28, 2021

For #460 (comment)

This PR allows restricting creation of new groups in sub-directories
by specifying the restrictions in a config file called restrictions.yaml.

It follows a similar model to the one we use in slack-infra today:
https://github.com/kubernetes/community/blob/master/communication/slack-config/restrictions.yaml

This is done to ensure that approvers in groups/sig-foo/OWNERS don't
accidentally create random official-sounding groups without sufficient review/oversight.

After this PR:

  • Creation of new groups will require changes to restrictions.yaml and
    will require approval from groups/OWNERS.
  • Updates to existing groups can be approved by groups/sig-foo/OWNERS,
    if appropriate restrictions are specified in restrictions.yaml.

If a new group is specified in a sub-directory but restrictions.yaml is not updated, an error will be displayed:

Could not load groups config: couldn't merge groups: cannot define group "[email protected]" in "sig-contributor-experience/groups.yaml"

/assign @spiffxp

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/access Define who has access to what via IAM bindings, role bindings, policy, etc. wg/k8s-infra labels Jul 28, 2021
@k8s-ci-robot k8s-ci-robot requested review from spiffxp and thockin July 28, 2021 12:57
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 28, 2021
@nikhita
Copy link
Member Author

nikhita commented Jul 28, 2021

From #460 (comment):

The former can probably handled by properly sharding groups, and enumerating which use cases don't need all the extra paranoia.

FYI I haven't added the approvers field in groups/sig-foo/OWNERS in this PR because we still need to add more docs around this.

nikhita added 2 commits July 28, 2021 18:37
This commit allows restricting creation of new groups in sub-directories
by specifying the restrictions in a config file called `restrictions.yaml`.

It follows a similar model to the one we use in slack-infra today:
https://github.com/kubernetes/community/blob/master/communication/slack-config/restrictions.yaml

This is done to ensure that approvers in groups/sig-foo/OWNERS don't
accidentally create random official-sounding groups without sufficient review/oversight.

After this commit:
- Creation of new groups will require changes to `restrictions.yaml` and
will require approval from groups/OWNERS.
- Updates to existing groups can be approved by groups/sig-foo/OWNERS,
if appropriate restrictions are specified in `restrictions.yaml`.
@nikhita nikhita force-pushed the groups-restrictions branch from dde5dc8 to 9e3bb6c Compare July 28, 2021 13:07
@ameukam ameukam added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. labels Jul 28, 2021
Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm
Thank you so much!

I don't have my paranoia engine spun up enough to decide if we want to add something like disallowed members so I'll save that for followup on whether we're comfortable enough to add approvers to subdirs

@spiffxp
Copy link
Member

spiffxp commented Jul 28, 2021

/hold cancel

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nikhita, spiffxp

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 28, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1342b20 into kubernetes:main Jul 28, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jul 28, 2021
@spiffxp
Copy link
Member

spiffxp commented Jul 28, 2021

This isn't quite working as expected. Will take a look in hopefully an hour

2021/07/28 11:57:35 couldn't merge groups: cannot define group "[email protected]" in "**/*"

@spiffxp
Copy link
Member

spiffxp commented Jul 29, 2021

Took longer for me to get back here, and somehow it was just now that I realized how untested this thing is. It's a pile of funcs.

#2413 will get us back up and running

I'll have another PR that includes other stuff I did along the way to find that, but let's get back to green first

@nikhita nikhita deleted the groups-restrictions branch July 29, 2021 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/access Define who has access to what via IAM bindings, role bindings, policy, etc. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants