populate-owners: add test for insertStringSlice

Also update readme, add comments, and remove some console logging

Author: pgier

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`.

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

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