-
Notifications
You must be signed in to change notification settings - Fork 695
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
[WIP] Add remove-members helper script #2048
[WIP] Add remove-members helper script #2048
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrbobbytables 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 |
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.
Left some suggestions, thanks for doing this ❤️
|
||
readonly REPO_ROOT="$(git rev-parse --show-toplevel)" | ||
readonly CONFIG_PATH="$REPO_ROOT/config" | ||
readonly DRYRUN="${DRYRUN:-true}" |
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.
Can we make this a flag?
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.
Yeah, this was me being kind of lazy to get it done lol
set -o pipefail | ||
|
||
readonly REPO_ROOT="$(git rev-parse --show-toplevel)" | ||
readonly CONFIG_PATH="$REPO_ROOT/config" |
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.
Can/should we make this configurable? In cases where someone is active in kubernetes-sigs
but not active in kubernetes
, or if we just want to remove members from kubernetes
?
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.
I can do that 👍
/assign @cblecker |
There is one issue I just thought of, it currently won't remove members if they have a comment after their name like |
Thoughts on putting this somewhere aside from hack? The unfortunately named hack folder generally seems to be used for lint-type updates and/or verifications. Isn't this an operational thing? AKA I want to remove Erick from the org and this is the tool to do so, which just happens to be in bash right now. |
Adding on to @fejta's point, I think this is important enough and of sufficient complexity that it should be written in Go. |
I'm okay with that -- I'm a little low on time at the moment, but I'll see what I can do to get something a bit better pushed up. Either way will get something before the next round of clean up. 👍 /close |
@mrbobbytables: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Created #2052 so that we don't forget this. I'm also happy to take a look at it later :) |
This is the script used to generate #2047
It needs some final polish like usage information, but figured I'd share for initial review / vetting alongside #2047
/hold