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

Add remove-members helper tool implemented in go #2575

Closed
wants to merge 15 commits into from
Closed
Prev Previous commit
Next Next commit
document the tool
  • Loading branch information
daemon1024 committed Mar 15, 2021
commit 1dab05fc76b5d4f4aa0778a42af565043448e0e7
7 changes: 7 additions & 0 deletions hack/remove-members.go
Original file line number Diff line number Diff line change
@@ -37,6 +37,7 @@ func main() {

}

//readMemberList reads the list of members to be removed from the given filepath
func readMemberList(path string) ([]string, error) {
file, err := os.Open(path)
Copy link
Member

Choose a reason for hiding this comment

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

@mrbobbytables is the inactive member list a yaml file? how is that generated?

Copy link
Member

Choose a reason for hiding this comment

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

It's been a text file with 1 member per line. The list is generated from this sheet after some manual vetting: https://docs.google.com/spreadsheets/d/1jqxMOo9f1EG72i4sGE7g6RlNPvQ4_qfOx9K-UM4YsLw/edit

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! One more follow up question - how does one generate this sheet?

Want to remove you as a single point of failure :)

Copy link
Member

Choose a reason for hiding this comment

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

The big stuff to regen it are in the notes tab in the sheet 👍 I thought it was owned/under the sig contribex shared dir, but it looks like I'm the only owner atm. I'll move it over in a bit.

if err != nil {
@@ -52,6 +53,7 @@ func readMemberList(path string) ([]string, error) {
return members, scanner.Err()
}

//removeMembers walks through the config directory and removes the occurences of the given member name
func removeMembers(memberList []string, configPath string) error {
for _, member := range memberList {
var orgs, teams []string
@@ -65,6 +67,7 @@ func removeMembers(memberList []string, configPath string) error {
if info.IsDir() {
return nil
}

if matched, err := filepath.Match("*.yaml", filepath.Base(path)); err != nil {
return err
} else if matched {
@@ -73,6 +76,7 @@ func removeMembers(memberList []string, configPath string) error {
return err
}

//Record the org/team name when a member is removed from it
if removed {
count++
if info.Name() == "org.yaml" {
@@ -94,6 +98,7 @@ func removeMembers(memberList []string, configPath string) error {

fmt.Printf("\n Orgs: %v\n Teams: %v\n Number of occurences: %d\n", orgs, teams, count)

//Proceed to committing changes if member is actually removed from somewhere
if count > 0 {
commitRemovedMembers(member, orgs, teams)
}
@@ -113,6 +118,7 @@ func removeMemberFromFile(member string, path string) (bool, error) {

if re.Match(content) {
Copy link
Member

Choose a reason for hiding this comment

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

we can probably count how many times member occurs in the path and return the count. This will ensure that count is accurate.

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean to count the number of matches in the said file?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean to count the number of matches in the said file?

Yeah :)

Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker for this, but potentially something in the future. We can marshal/unmarshal yaml while leaving the comments (go-yaml/yaml#132). This would also let us get team names directly.

Copy link
Member

Choose a reason for hiding this comment

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

@mrbobbytables -- I had a dig at this earlier by parsing the org files using k8s.io/test-infra/prow/config/org.Config, but there seemed to be something off there.

The processed org config when written back to disk had a lot of diff compared to the original file.

We can possibly dig deeper into it later.

Copy link
Member

Choose a reason for hiding this comment

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

It's been a bit since I looked at it , but potentially whitespace =/ since yaml supports some fuzzy inclusion:

foo:
  - bar

is the same as:

foo:
- bar

Theres what people have written (both formats) and what go will want to format it as.

Copy link
Member

Choose a reason for hiding this comment

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

That as well. we may want to bite the bullet with a big initial diff as the subsequent updates should be deterministic .

One other thing was, in order to process comments, the structs in k8s.io/test-infra/prow/config/org.Config should have fields with yaml.Node as field type and then have post-processing helper method. I had taken a step back because of that complexity.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, what I'd probably do at that point is get away from editing by hand, and then have an add-member function or have a make command that ensures proper formatting before commit. Out of scope for this one though^^;;;

Copy link
Member

Choose a reason for hiding this comment

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

@mrbobbytables -- that's (adding members) what I was trying to automate. 😉

But, yes. Let's talk about this in a separate thread.

Copy link
Author

Choose a reason for hiding this comment

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

Adding to whitespaces and comments related changes, The substantial amount of diff is also created due to the difference in order of the yaml properties.
I was trying to parse the yaml files and removing members in a more structured way, I also faced this.

diff --git a/config/kubernetes-incubator/org.yaml b/config/kubernetes-incubator/org.yaml
index baf4ca76..e2da8a87 100644
--- a/config/kubernetes-incubator/org.yaml
+++ b/config/kubernetes-incubator/org.yaml
@@ -1,10 +1,3 @@
-name: Kubernetes Incubator
-description: Kubernetes Incubator
-default_repository_permission: read
-has_organization_projects: true
-has_repository_projects: true
-members_can_create_repositories: false
-billing_email: [email protected]
 admins:
 - cblecker
 - fejta
@@ -15,3 +8,10 @@ admins:
 - nikhita
 - spiffxp
 - thelinuxfoundation
+billing_email: [email protected]
+default_repository_permission: read
+description: Kubernetes Incubator
+has_organization_projects: true
+has_repository_projects: true
+members_can_create_repositories: false
+name: Kubernetes Incubator

It seems to be sorting the fields.

I currently implemented this using kubernetes-sigs/yaml.


//Mofify the file only if it's not a dry run
if dryrun == true {
return true, nil
}
@@ -161,6 +167,7 @@ func commitRemovedMembers(member string, orgs []string, teams []string) {

fmt.Printf("\nCommit Command: %q\n\n", strings.Join(cmd, " "))

//Execute the git command only if not a dry run
if !dryrun {
cmd := exec.Command("git", cmd...)
err := cmd.Run()