From e3db2d0a33e05da5caa87b71e1e65e44498beb49 Mon Sep 17 00:00:00 2001 From: Paul Gier Date: Fri, 11 Jan 2019 15:29:29 -0600 Subject: [PATCH 1/3] tools/populate-owners: fix test issues - Fix go vet complaining about wrong type in ErrorF - Print console output when there is an error - Configure username and email for test git repo - Ignore difference in commit ID between expected and actual orgRepo --- tools/populate-owners/main.go | 2 +- tools/populate-owners/main_test.go | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/tools/populate-owners/main.go b/tools/populate-owners/main.go index bff63d5053b5..05f9967c51bd 100644 --- a/tools/populate-owners/main.go +++ b/tools/populate-owners/main.go @@ -204,7 +204,7 @@ func get(uri, accept string) (data []byte, status int, err error) { defer response.Body.Close() if response.StatusCode != 200 { - return data, response.StatusCode, fmt.Errorf("failed to fetch %s: %s", uri, response.StatusCode, response.Status) + return data, response.StatusCode, fmt.Errorf("failed to fetch %s: %v %s", uri, response.StatusCode, response.Status) } data, err = ioutil.ReadAll(response.Body) diff --git a/tools/populate-owners/main_test.go b/tools/populate-owners/main_test.go index c77286e79f0a..f54bf701d013 100644 --- a/tools/populate-owners/main_test.go +++ b/tools/populate-owners/main_test.go @@ -168,6 +168,8 @@ func TestExtractOwners(t *testing.T) { for _, args := range [][]string{ {"git", "init"}, + {"git", "config", "user.name", "Test"}, + {"git", "config", "user.email", "test@test.org"}, {"git", "add", "README"}, {"git", "commit", "-m", "Begin versioning"}, } { @@ -177,8 +179,9 @@ func TestExtractOwners(t *testing.T) { "GIT_COMMITTER_DATE=1112911993 -0700", "GIT_AUTHOR_DATE=1112911993 -0700", } - err = cmd.Run() + stdoutStderr, err := cmd.CombinedOutput() if err != nil { + t.Log(string(stdoutStderr)) t.Fatal(err) } } @@ -258,6 +261,8 @@ func TestExtractOwners(t *testing.T) { t.Fatalf("unexpected error: %v does not match %v", err, test.error) } + // Need to override the newly created commit to avoid test failure + orgrepo.Commit = test.expected.Commit assertEqual(t, orgrepo, test.expected) }) } From 0f475599e515166639cc5845f893de816b95c93f Mon Sep 17 00:00:00 2001 From: Paul Gier Date: Fri, 11 Jan 2019 15:09:27 -0600 Subject: [PATCH 2/3] tools/populate-owners: resolve aliases into OWNERS Resolves aliases contained in the OWNERS files based on the contents of the OWNERS_ALIASES file. Also adds optional parameter to only update repositories which match a regex. --- ci-operator/populate-owners.sh | 2 +- tools/populate-owners/main.go | 172 ++++++++++++--------------- tools/populate-owners/main_test.go | 181 +++-------------------------- 3 files changed, 89 insertions(+), 266 deletions(-) diff --git a/ci-operator/populate-owners.sh b/ci-operator/populate-owners.sh index ec23ccaff05a..b675dfdb6da7 100755 --- a/ci-operator/populate-owners.sh +++ b/ci-operator/populate-owners.sh @@ -3,4 +3,4 @@ # This script runs /tools/populate-owners REPO_ROOT="$(git rev-parse --show-toplevel)" && -exec go run "${REPO_ROOT}/tools/populate-owners/main.go" +exec go run "${REPO_ROOT}/tools/populate-owners/main.go" $1 diff --git a/tools/populate-owners/main.go b/tools/populate-owners/main.go index 05f9967c51bd..20132369dbc8 100644 --- a/tools/populate-owners/main.go +++ b/tools/populate-owners/main.go @@ -1,13 +1,14 @@ package main import ( + "flag" "fmt" "io/ioutil" "net/http" "os" "os/exec" "path/filepath" - "reflect" + "regexp" "sort" "strings" @@ -281,70 +282,6 @@ func (orgRepo *orgRepo) extractOwners(repoRoot string) (err error) { return nil } -// namespaceAliases collects a set of aliases including all upstream -// aliases. If multiple upstreams define the same alias with different -// member sets, namespaceAliases renames the colliding aliases in both -// the input 'orgRepos' and the output 'collected' to use -// unique-to-each-upstream alias names. -func namespaceAliases(orgRepos []*orgRepo) (collected *aliases, err error) { - consumerMap := map[string][]*orgRepo{} - for _, orgRepo := range orgRepos { - if orgRepo.Aliases == nil { - continue - } - - for alias := range orgRepo.Aliases.Aliases { - consumerMap[alias] = append(consumerMap[alias], orgRepo) - } - } - - if len(consumerMap) == 0 { - return nil, nil - } - - collected = &aliases{ - Aliases: map[string][]string{}, - } - - for alias, consumers := range consumerMap { - namespace := false - members := consumers[0].Aliases.Aliases[alias] - for _, consumer := range consumers[1:] { - otherMembers := consumer.Aliases.Aliases[alias] - if !reflect.DeepEqual(members, otherMembers) { - namespace = true - break - } - } - - for i, consumer := range consumers { - newAlias := alias - if namespace { - newAlias = fmt.Sprintf("%s-%s-%s", consumer.Organization, consumer.Repository, alias) - consumer.Aliases.Aliases[newAlias] = consumer.Aliases.Aliases[alias] - delete(consumer.Aliases.Aliases, alias) - } - fmt.Fprintf( - os.Stderr, - "injecting alias %q from https://github.com/%s/%s/blob/%s/OWNERS_ALIASES\n", - alias, - consumer.Organization, - consumer.Repository, - consumer.Commit, - ) - if i == 0 || namespace { - _, ok := collected.Aliases[newAlias] - if ok { - return nil, fmt.Errorf("namespaced alias collision: %q", newAlias) - } - collected.Aliases[newAlias] = consumer.Aliases.Aliases[newAlias] - } - } - } - - return collected, nil -} - func writeYAML(path string, data interface{}, prefix []string) (err error) { file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0666) if err != nil { @@ -363,6 +300,52 @@ func writeYAML(path string, data interface{}, prefix []string) (err error) { return encoder.Encode(data) } +// insertStringSlice inserts a string slice into a given index +// in another string slice. Returns a new slice with the insert +// slice replacing the elements between begin and end index. +func insertStringSlice(insert []string, intoSlice []string, + begin int, end int) []string { + if begin > end || begin < 0 || end > len(intoSlice) { + fmt.Printf("invalid begin: %v, or end: %v \n", begin, end) + return intoSlice + } + firstPart := intoSlice[:begin] + secondPart := append(insert, intoSlice[end:]...) + return append(firstPart, secondPart...) +} + +// resolveAliases resolves names in the list of owners that +// match one of the given aliases. Returns a list of owners +// with each alias replaced by the list of owners it represents. +func resolveAliases(aliases *aliases, owners []string) []string { + offset := 0 // Keeps track of how many new names we've inserted + for i, owner := range owners { + if aliasOwners, ok := aliases.Aliases[owner]; ok { + index := i + offset + owners = insertStringSlice(aliasOwners, owners, index, (index + 1)) + offset += len(aliasOwners) - 1 + } + } + return owners +} + +// resolveOwnerAliases checks whether the orgRepo includes any +// owner aliases, and attempts to resolve them to the appropriate +// set of owners. Returns an owners which replaces any +// matching aliases with the set of owner names belonging to that alias. +func (orgRepo *orgRepo) resolveOwnerAliases() *owners { + if orgRepo.Aliases == nil || len(orgRepo.Aliases.Aliases) == 0 { + return orgRepo.Owners + } + + return &owners{ + resolveAliases(orgRepo.Aliases, orgRepo.Owners.Approvers), + resolveAliases(orgRepo.Aliases, orgRepo.Owners.Reviewers), + orgRepo.Owners.RequiredReviewers, + orgRepo.Owners.Labels, + } +} + func (orgRepo *orgRepo) writeOwners() (err error) { for _, directory := range orgRepo.Directories { path := filepath.Join(directory, "OWNERS") @@ -374,7 +357,7 @@ func (orgRepo *orgRepo) writeOwners() (err error) { continue } - err = writeYAML(path, orgRepo.Owners, []string{ + err = writeYAML(path, orgRepo.resolveOwnerAliases(), []string{ doNotEdit, fmt.Sprintf( "# from https://github.com/%s/%s/blob/%s/OWNERS\n", @@ -393,24 +376,7 @@ func (orgRepo *orgRepo) writeOwners() (err error) { return nil } -func writeOwnerAliases(repoRoot string, aliases *aliases) (err error) { - path := filepath.Join(repoRoot, "OWNERS_ALIASES") - if aliases == nil || len(aliases.Aliases) == 0 { - err = os.Remove(path) - if err != nil && !os.IsNotExist(err) { - return err - } - return nil - } - - return writeYAML(path, aliases, []string{ - doNotEdit, - ownersAliasesComment, - "\n", - }) -} - -func pullOwners(directory string) (err error) { +func pullOwners(directory string, pattern string) (err error) { repoRoot, err := getRepoRoot(directory) if err != nil { return err @@ -425,6 +391,10 @@ func pullOwners(directory string) (err error) { config := filepath.Join(operatorRoot, "config") templates := filepath.Join(operatorRoot, "templates") for _, orgRepo := range orgRepos { + matched, _ := regexp.MatchString(pattern, orgRepo.Repository) + if !matched { + continue + } err = orgRepo.getDirectories(config, templates) if err != nil && !os.IsNotExist(err) { return err @@ -434,31 +404,37 @@ func pullOwners(directory string) (err error) { if err != nil && !os.IsNotExist(err) { return err } - fmt.Fprintf(os.Stderr, "got owners for %s\n", orgRepo.String()) - } - - aliases, err := namespaceAliases(orgRepos) - if err != nil { - return err - } - - err = writeOwnerAliases(repoRoot, aliases) - if err != nil { - return err - } - for _, orgRepo := range orgRepos { err = orgRepo.writeOwners() if err != nil { return err } + fmt.Fprintf(os.Stderr, "updated owners for %s\n", orgRepo.String()) } return nil } +const ( + usage = `Update the OWNERS files from remote repositories. + +Usage: + %s [repo-name-regex] + +Args: + [repo-name-regex] A go regex which which matches the repos to update + +` +) + func main() { - err := pullOwners(".") + flag.Usage = func() { + fmt.Fprintf(flag.CommandLine.Output(), usage, "populate-owners") + } + flag.Parse() + repoPattern := flag.Arg(0) + + err := pullOwners(".", repoPattern) if err != nil { fmt.Fprintln(os.Stderr, err.Error()) os.Exit(1) diff --git a/tools/populate-owners/main_test.go b/tools/populate-owners/main_test.go index f54bf701d013..902ea5d12062 100644 --- a/tools/populate-owners/main_test.go +++ b/tools/populate-owners/main_test.go @@ -2,6 +2,7 @@ package main import ( "io/ioutil" + "log" "os" "os/exec" "path/filepath" @@ -268,174 +269,20 @@ func TestExtractOwners(t *testing.T) { } } -func TestNamespaceAliases(t *testing.T) { - for _, test := range []struct { - name string - input []*orgRepo - namespaced []*orgRepo - collected *aliases - error *regexp.Regexp - }{ - { - name: "no alias name collisions, so no namespaced aliases created", - input: []*orgRepo{ - { - Organization: "a", - Repository: "b", - Aliases: &aliases{Aliases: map[string][]string{ - "ab": {"alice", "bob"}, - }}, - }, - { - Organization: "c", - Repository: "d", - Aliases: &aliases{Aliases: map[string][]string{ - "cd": {"bob", "charlie"}, - }}, - }, - }, - namespaced: []*orgRepo{ - { - Organization: "a", - Repository: "b", - Aliases: &aliases{Aliases: map[string][]string{ - "ab": {"alice", "bob"}, - }}, - }, - { - Organization: "c", - Repository: "d", - Aliases: &aliases{Aliases: map[string][]string{ - "cd": {"bob", "charlie"}, - }}, - }, - }, - collected: &aliases{Aliases: map[string][]string{ - "ab": {"alice", "bob"}, - "cd": {"bob", "charlie"}, - }}, - }, - { - name: "matching members, so no namespaced aliases created", - input: []*orgRepo{ - { - Organization: "a", - Repository: "b", - Aliases: &aliases{Aliases: map[string][]string{ - "ab": {"alice", "bob"}, - }}, - }, - { - Organization: "c", - Repository: "d", - Aliases: &aliases{Aliases: map[string][]string{ - "ab": {"alice", "bob"}, - }}, - }, - }, - namespaced: []*orgRepo{ - { - Organization: "a", - Repository: "b", - Aliases: &aliases{Aliases: map[string][]string{ - "ab": {"alice", "bob"}, - }}, - }, - { - Organization: "c", - Repository: "d", - Aliases: &aliases{Aliases: map[string][]string{ - "ab": {"alice", "bob"}, - }}, - }, - }, - collected: &aliases{Aliases: map[string][]string{ - "ab": {"alice", "bob"}, - }}, - }, - { - name: "different members, so namespaced aliases created", - input: []*orgRepo{ - { - Organization: "a", - Repository: "b", - Aliases: &aliases{Aliases: map[string][]string{ - "ab": {"alice", "bob"}, - }}, - }, - { - Organization: "c", - Repository: "d", - Aliases: &aliases{Aliases: map[string][]string{ - "ab": {"bob", "charlie"}, - }}, - }, - }, - namespaced: []*orgRepo{ - { - Organization: "a", - Repository: "b", - Aliases: &aliases{Aliases: map[string][]string{ - "a-b-ab": {"alice", "bob"}, - }}, - }, - { - Organization: "c", - Repository: "d", - Aliases: &aliases{Aliases: map[string][]string{ - "c-d-ab": {"bob", "charlie"}, - }}, - }, - }, - collected: &aliases{Aliases: map[string][]string{ - "a-b-ab": {"alice", "bob"}, - "c-d-ab": {"bob", "charlie"}, - }}, - }, - { - name: "collisions after namespacing", - input: []*orgRepo{ - { - Organization: "a", - Repository: "b", - Aliases: &aliases{Aliases: map[string][]string{ - "ab": {"alice", "bob"}, - }}, - }, - { - Organization: "c", - Repository: "d", - Aliases: &aliases{Aliases: map[string][]string{ - "ab": {"bob", "charlie"}, - }}, - }, - { - Organization: "e", - Repository: "f", - Aliases: &aliases{Aliases: map[string][]string{ - "a-b-ab": {"bob", "charlie"}, - }}, - }, - }, - error: regexp.MustCompile("^namespaced alias collision: \"a-b-ab\"$"), - }, - } { - t.Run(test.name, func(t *testing.T) { - collected, err := namespaceAliases(test.input) - if test.error == nil { - if err != nil { - t.Fatal(err) - } - } else if !test.error.MatchString(err.Error()) { - t.Fatalf("unexpected error: %v does not match %v", err, test.error) - } - - assertEqual(t, collected, test.collected) - if test.namespaced != nil { - assertEqual(t, test.input, test.namespaced) - } - }) +func TestResolveAliases(t *testing.T) { + given := &orgRepo{ + Owners: &owners{Approvers: []string{"alice", "sig-alias", "david"}, + Reviewers: []string{"adam", "sig-alias"}}, + Aliases: &aliases{Aliases: map[string][]string{"sig-alias": {"bob", "carol"}}}, + } + expected := &orgRepo{ + Owners: &owners{Approvers: []string{"alice", "bob", "carol", "david"}, + Reviewers: []string{"adam", "bob", "carol"}}, + Aliases: &aliases{Aliases: map[string][]string{"sig-alias": {"bob", "carol"}}}, } + log.Println("given:", given) + log.Println("expected:", expected) + assertEqual(t, given.resolveOwnerAliases(), expected.Owners) } func TestWriteYAML(t *testing.T) { From a9f4c3accf50e0aa49ff23dd530c6a44616a8ad9 Mon Sep 17 00:00:00 2001 From: Paul Gier Date: Wed, 16 Jan 2019 12:57:31 -0600 Subject: [PATCH 3/3] populate-owners: add test for insertStringSlice Also update readme, add comments, and remove some console logging --- tools/populate-owners/README.md | 19 ++++++++++--------- tools/populate-owners/main.go | 15 +++++++++------ tools/populate-owners/main_test.go | 24 +++++++++++++++++++++--- 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/tools/populate-owners/README.md b/tools/populate-owners/README.md index 068505a0d211..d56f2e78af0e 100644 --- a/tools/populate-owners/README.md +++ b/tools/populate-owners/README.md @@ -1,10 +1,15 @@ # Populating `OWNERS` and `OWNERS_ALIASES` -This utility pulls `OWNERS` and `OWNERS_ALIASES` from upstream OpenShift repositories. +This utility updates the OWNERS files from remote Openshift repositories. + Usage: + populate-owners [repo-name-regex] + +Args: + [repo-name-regex] A go regex which which matches the repos to update, by default all repos are selected ```console -$ go run main.go +$ go run main.go [repo-name-regex] ``` Or, equivalently, execute [`populate-owners.sh`](../../ci-operator/populate-owners.sh) from anywhere in this repository. @@ -15,13 +20,9 @@ For example, the presence of [`ci-operator/jobs/openshift/origin`](../../ci-oper The `HEAD` branch for each upstream repository is pulled to extract its `OWNERS` and `OWNERS_ALIASES`. If `OWNERS` is missing, the utility will ignore `OWNERS_ALIASES`, even if it is present upstream. -Once all the upstream content has been fetched, the utility namespaces any colliding upstream aliases. -Collisions only occur if multiple upstreams define the same alias with different member sets. -When that happens, the utility replaces the upstream alias with a `{organization}-{repository}-{upstream-alias}`. -For example, if [openshift/origin][] and [openshift/installer][] both defined an alias for `security` with different member sets, the utility would rename them to `openshift-origin-security` and `openshift-installer-security` respectively. - -After namespacing aliases, the utility writes `OWNERS_ALIASES` to the root of this repository. -If no upstreams define aliases, then the utility removes `OWNER_ALIASES` from the root of this repository. +Any aliases present in the upstream `OWNERS` file will be resolved to the set of usernames they represent in the associated +`OWNERS_ALIASES` file. The local `OWNERS` files will therefore not contain any alias names. This avoids any conflicts between +upstream alias names coming from different repos. The utility also iterates through the `ci-operator/{type}/{organization}/{repository}` for `{type}` in `config`, `jobs`, and `templates`, writing `OWNERS` to reflect the upstream configuration. If the upstream did not have an `OWNERS` file, the utility removes the associated `ci-operator/*/{organization}/{repository}/OWNERS`. diff --git a/tools/populate-owners/main.go b/tools/populate-owners/main.go index 20132369dbc8..9ec932c5b67b 100644 --- a/tools/populate-owners/main.go +++ b/tools/populate-owners/main.go @@ -300,13 +300,16 @@ func writeYAML(path string, data interface{}, prefix []string) (err error) { return encoder.Encode(data) } -// insertStringSlice inserts a string slice into a given index -// in another string slice. Returns a new slice with the insert -// slice replacing the elements between begin and end index. +// insertStringSlice inserts a string slice into another string slice +// replacing the elements starting with the begin index up to the end +// index. The element at end index in the original slice will remain +// in the resulting slice. Returns a new slice with the elements +// replaced. If the begin index is larger than the end, or either of the +// indexes are out of range of the slice, the original slice is returned +// unmodified. func insertStringSlice(insert []string, intoSlice []string, begin int, end int) []string { if begin > end || begin < 0 || end > len(intoSlice) { - fmt.Printf("invalid begin: %v, or end: %v \n", begin, end) return intoSlice } firstPart := intoSlice[:begin] @@ -419,10 +422,10 @@ const ( usage = `Update the OWNERS files from remote repositories. Usage: - %s [repo-name-regex] + %s [repo-name-regex] Args: - [repo-name-regex] A go regex which which matches the repos to update + [repo-name-regex] A go regex which which matches the repos to update, by default all repos are selected ` ) diff --git a/tools/populate-owners/main_test.go b/tools/populate-owners/main_test.go index 902ea5d12062..59e76fb2945d 100644 --- a/tools/populate-owners/main_test.go +++ b/tools/populate-owners/main_test.go @@ -2,7 +2,6 @@ package main import ( "io/ioutil" - "log" "os" "os/exec" "path/filepath" @@ -269,6 +268,27 @@ func TestExtractOwners(t *testing.T) { } } +func TestInsertSlice(t *testing.T) { + // test replacing two elements of a slice + given := []string{"alice", "bob", "carol", "david", "emily"} + expected := []string{"alice", "bob", "charlie", "debbie", "emily"} + actual := insertStringSlice([]string{"charlie", "debbie"}, given, 2, 4) + assertEqual(t, actual, expected) + + // test replacing all elements after the first + expected = []string{"alice", "eddie"} + actual = insertStringSlice([]string{"eddie"}, given, 1, len(given)) + assertEqual(t, actual, expected) + + // test invalid begin and end indexes, should return the slice unmodified + actual = insertStringSlice([]string{}, given, 5, 2) + assertEqual(t, given, given) + actual = insertStringSlice([]string{}, given, -1, 2) + assertEqual(t, given, given) + actual = insertStringSlice([]string{}, given, 1, len(given)+1) + assertEqual(t, given, given) +} + func TestResolveAliases(t *testing.T) { given := &orgRepo{ Owners: &owners{Approvers: []string{"alice", "sig-alias", "david"}, @@ -280,8 +300,6 @@ func TestResolveAliases(t *testing.T) { Reviewers: []string{"adam", "bob", "carol"}}, Aliases: &aliases{Aliases: map[string][]string{"sig-alias": {"bob", "carol"}}}, } - log.Println("given:", given) - log.Println("expected:", expected) assertEqual(t, given.resolveOwnerAliases(), expected.Owners) }