-
Notifications
You must be signed in to change notification settings - Fork 25
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
Cleanup permissions for shipyard maintained repos #211
Cleanup permissions for shipyard maintained repos #211
Conversation
…rs, remove github-mgmt pull permissions
…move github-mgmt stewards pull permissions
…ewards pull permissions, add shipyard as maintainers
…ams, reduce admins, remove github-mgmt stewards pull permissions
…e github-mgmt stewards pull permissions
…stewards pull permissions
The following access changes will be introduced as a result of applying the plan: Access Changes
|
Before merge, verify that all the following plans are correct. They will be applied as-is after the merge. Terraform plansipfs
|
teams: | ||
admin: | ||
- helia-dev | ||
pull: | ||
- github-mgmt stewards | ||
maintain: | ||
- shipyard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@achingbrain @SgtPooki I added Shipyard as maintainers for some repos where there were no ambient admin permissions for the "admin" or w3dt-stewards teams. It might be that these are unnecessary or should just be push permissions. Happy to downgrade if you think that makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be fine, and we haven't touched helia-cli in a while, and likely won't
push_restrictions: | ||
- /aschmahmann | ||
- /gmasgras | ||
- /thattommyhall | ||
- ipfs/kubo-maintainers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ipfs/ipdx how can I set this to push restrictions on, but with no associated group? IIUC there's nothing you can do anyway to stop admins from pushing (or maintainers from pushing with approval) so adding groups here seems unnecessary provided you can keep the restrictions enabled.
admin: | ||
- lidel | ||
push: | ||
- dennis-tra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dennis-tra I'm removing your permissions here and there will be a follow up PR to add you to the IPFS org and from there you can get added to any teams you need to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks for the heads-up! 👍
pull: | ||
- github-mgmt stewards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ipfs/ipdx any idea why these pull permissions got added everywhere in fe64a02?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! This is because github-mgmt stewards
group is designated as moderator and security manager as per #189
So, unfortunately, they're going to come back, but you don't necessarily have to restore them yourself. The apply should go through anyway, and the config will be updated during the weekly sync.
maintain: | ||
- shipyard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lidel WDYT about giving push to repos Go for rainbow and someguy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably ok, that group already has "maintain" in boxo, so push will make it easier for existing community to submit PRs.
ps. there is a separate long-term meta-worry in that there is way too many people in https://github.com/orgs/ipfs/teams/repos-go, and if our intention is to limit security risks / access, we should plan to subset that group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fine to me (only searched for my username and looked through removals or items where I was left). I really wish it was easier to see the specific repo the changes are for
teams: | ||
admin: | ||
- helia-dev | ||
pull: | ||
- github-mgmt stewards | ||
maintain: | ||
- shipyard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be fine, and we haven't touched helia-cli in a while, and likely won't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
pull: | ||
- github-mgmt stewards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! This is because github-mgmt stewards
group is designated as moderator and security manager as per #189
So, unfortunately, they're going to come back, but you don't necessarily have to restore them yourself. The apply should go through anyway, and the config will be updated during the weekly sync.
@aschmahmann should we merge this on monday? |
Co-authored-by: Piotr Galar <[email protected]>
Summary
This is a general cleanup of permissions across a number of repos that Shipyard maintains, also adds a shipyard team. The general forms of the changes look like:
Why do you need this?
Overall the number of admins in these repos seems much higher than necessary, especially given the ability to escalate via github-mgmt if needed. For repos shipyard maintains I'd like permissions scoped down where possible. This seems like the easiest cleanup although I wouldn't be surprised if we want to shrink permissions more and/or look at more repos than just the ones I've covered here.
What else do we need to know?
TODO: tag people with permission reductions once CI generates the list
DRI: myself
Reviewer's Checklist