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
184 changes: 184 additions & 0 deletions hack/remove-members.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
package main

import (
"bufio"
"flag"
daemon1024 marked this conversation as resolved.
Show resolved Hide resolved
"fmt"
"io/ioutil"
"log"
"os"
"os/exec"
"path/filepath"
"regexp"
"sort"
"strings"
)

var dryrun bool
var repoRoot string

func main() {
flag.StringVar(&repoRoot, "root", ".", "Root of the repository")
daemon1024 marked this conversation as resolved.
Show resolved Hide resolved
flag.BoolVar(&dryrun, "dryrun", true, "Enable Dryrun or not")
daemon1024 marked this conversation as resolved.
Show resolved Hide resolved

flag.Parse()

if len(flag.Args()) < 1 {
daemon1024 marked this conversation as resolved.
Show resolved Hide resolved
fmt.Print("Usage: remove-members [flags] <file-containing-members-list>\n\nFlags:\n\t-root\tstring\tpath to root directory of the repository\n\t-dryrun\tboolean\tEnable/Disable dryrun (Default: Enabled)")
daemon1024 marked this conversation as resolved.
Show resolved Hide resolved
os.Exit(0)
}

memberList, err := readMemberList(flag.Args()[0])
if err != nil {
log.Fatal(err)
}

configPath := repoRoot + "/config"

err = removeMembers(memberList, configPath)
if err != nil {
log.Fatal(err)
}

}

//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 {
return nil, err
}
defer file.Close()

var members []string
scanner := bufio.NewScanner(file)
for scanner.Scan() {
members = append(members, scanner.Text())
}
return members, scanner.Err()
}
Copy link
Member

Choose a reason for hiding this comment

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

How about simplifying this using ioutil.ReadFile as used in removeMemberFromFile?

Note: ioutil.ReadFile has been deprecated in Go 1.16 although it shouldn't be a blocker since this repository still uses Go 1.13.


//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
count := 0
fmt.Print(member)
Copy link
Member

Choose a reason for hiding this comment

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

would be better to log here and a more descriptive log message

Copy link
Author

@daemon1024 daemon1024 Mar 16, 2021

Choose a reason for hiding this comment

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

Should I change all instances of fmt to log?

Currently the output looks like

~/P/o/hack (remove-members-tool|…) ❯❯❯  go run remove-members.go -path=../config/kubernetes members.txt
nikhita
 Orgs: [kubernetes]
 Teams: [sig-contributor-experience sig-release]
 Number of occurences: 3

Commit Command: "commit -m Remove nikhita from the kubernetes org -m Remove nikhita from sig-contributor-experience, sig-release teams"

daemon1024
 Orgs: []
 Teams: []
 Number of occurences: 0

P.s. I don't really want to remove you 😝 just for showing the output 😹

Copy link
Member

Choose a reason for hiding this comment

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

I think fmt is also ok, but would be good to use a more descriptive message :)


err := filepath.Walk(configPath, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
return nil
}

if matched, err := filepath.Match("*.yaml", filepath.Base(path)); err != nil {
return err
} else if matched {
removed, err := removeMemberFromFile(member, path)
if err != nil {
return err
}

//Record the org/team name when a member is removed from it
if removed {
count++
if info.Name() == "org.yaml" {
orgs = append(orgs, filepath.Base(filepath.Dir(path)))
}
if info.Name() == "teams.yaml" {
teams = append(teams, filepath.Base(filepath.Dir(path)))
}
}
}
return nil
})
if err != nil {
return err
}

sort.Strings(orgs)
sort.Strings(teams)

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)
}
}

return nil
}

func removeMemberFromFile(member string, path string) (bool, error) {

content, err := ioutil.ReadFile(path)
if err != nil {
return false, err
}

re := regexp.MustCompile(`(\s+)?- ` + member + `(.*)?`)

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
}

updatedContent := re.ReplaceAll(content, []byte(""))
err = ioutil.WriteFile(path, updatedContent, 0666)

if err != nil {
return false, err
}
daemon1024 marked this conversation as resolved.
Show resolved Hide resolved

cmd := exec.Command("git", "add", path)
err := cmd.Run()

if err != nil {
log.Fatal(err)
}

return true, nil
}

return false, nil

}

func commitRemovedMembers(member string, orgs []string, teams []string) {
cmd := []string{"commit"}

orgCommitMsg := "Remove " + member + " from the "
if len(orgs) == 1 {
orgCommitMsg += orgs[0] + " org"
cmd = append(cmd, "-m", orgCommitMsg)
} else if len(orgs) >= 1 {
orgCommitMsg += strings.Join(orgs, ", ") + " orgs"
cmd = append(cmd, "-m", orgCommitMsg)
}

teamCommitMsg := "Remove " + member + " from "
if len(teams) == 1 {
teamCommitMsg += teams[0] + " team"
cmd = append(cmd, "-m", teamCommitMsg)
} else if len(teams) >= 1 {
teamCommitMsg += strings.Join(teams, ", ") + " teams"
cmd = append(cmd, "-m", teamCommitMsg)
}

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()

if err != nil {
log.Fatal(err)
}
}
}