labels_sync: allow setting org-level labels

In OpenShift we cargo-culted us into thinking the tool actually supports org-level labels but it doesn't. The *proper* structure for doing this is probably more hierarchic (like some of the Prow plugins, e.g. branch protector), but I wanted to avoid larger changes.
twilight0620 · Feb 3, 2021 · c778270 · c778270
1 parent 6c4950a
commit c778270
Show file tree

Hide file tree

Showing 4 changed files with 141 additions and 20 deletions.
diff --git a/label_sync/BUILD.bazel b/label_sync/BUILD.bazel
@@ -57,6 +57,10 @@ go_test(
         "//label_sync:test_examples",
     ],
     embed = [":go_default_library"],
+    deps = [
+        "@com_github_google_go_cmp//cmp:go_default_library",
+        "@com_github_google_go_cmp//cmp/cmpopts:go_default_library",
+    ],
 )
 
 filegroup(

diff --git a/label_sync/labels_example.yaml b/label_sync/labels_example.yaml
@@ -15,4 +15,16 @@ default:
     - name: dead-label
       description: Delete Me :)
       deleteAfter: 2017-01-01T13:00:00Z
+orgs:
+  org:
+    labels:
+      - color: green
+        name: sgtm
+        description: Sounds Good To Me
+repos:
+  org/repo:
+    labels:
+      - color: blue
+        name: tgtm
+        description: Tastes Good To Me
 ...
diff --git a/label_sync/main.go b/label_sync/main.go
@@ -84,6 +84,7 @@ type Label struct {
 // There is also a Default list of labels applied to every Repo
 type Configuration struct {
 	Repos   map[string]RepoConfig `json:"repos,omitempty"`
+	Orgs    map[string]RepoConfig `json:"orgs,omitempty"`
 	Default RepoConfig            `json:"default"`
 }
 
@@ -239,6 +240,9 @@ func stringInSortedSlice(a string, list []string) bool {
 func (c Configuration) Labels() []Label {
 	var labelarrays [][]Label
 	labelarrays = append(labelarrays, c.Default.Labels)
+	for _, org := range c.Orgs {
+		labelarrays = append(labelarrays, org.Labels)
+	}
 	for _, repo := range c.Repos {
 		labelarrays = append(labelarrays, repo.Labels)
 	}
@@ -265,29 +269,42 @@ func (c Configuration) Labels() []Label {
 // Ensures the config does not duplicate label names between default and repo
 func (c Configuration) validate(orgs string) error {
 	// Check default labels
-	seen, err := validate(c.Default.Labels, "default", make(map[string]string))
+	defaultSeen, err := validate(c.Default.Labels, "default", make(map[string]string))
 	if err != nil {
 		return fmt.Errorf("invalid config: %v", err)
 	}
 
 	// Generate list of orgs
 	sortedOrgs := strings.Split(orgs, ",")
 	sort.Strings(sortedOrgs)
-	// Check other repos labels
+
+	// Check org-level labels for duplicities with default labels
+	orgSeen := map[string]map[string]string{}
+	for org, orgConfig := range c.Orgs {
+		if orgSeen[org], err = validate(orgConfig.Labels, org, defaultSeen); err != nil {
+			return fmt.Errorf("invalid config: %v", err)
+		}
+	}
+
 	for repo, repoconfig := range c.Repos {
-		// Will complain if a label is both in default and repo
-		if _, err := validate(repoconfig.Labels, repo, seen); err != nil {
+		data := strings.Split(repo, "/")
+		if len(data) != 2 {
+			return fmt.Errorf("invalid repo name '%s', expected org/repo form", repo)
+		}
+		org := data[0]
+		if _, ok := orgSeen[org]; !ok {
+			orgSeen[org] = defaultSeen
+		}
+
+		// Check repo labels for duplicities with default and org-level labels
+		if _, err := validate(repoconfig.Labels, repo, orgSeen[org]); err != nil {
 			return fmt.Errorf("invalid config: %v", err)
 		}
 		// If orgs have been specified, warn if repo isn't under orgs
-		if len(orgs) != 0 {
-			data := strings.Split(repo, "/")
-			if len(data) == 2 {
-				if !stringInSortedSlice(data[0], sortedOrgs) {
-					logrus.WithField("orgs", orgs).WithField("org", data[0]).WithField("repo", repo).Warn("Repo isn't inside orgs")
-				}
-			}
+		if len(orgs) > 0 && !stringInSortedSlice(org, sortedOrgs) {
+			logrus.WithField("orgs", orgs).WithField("org", org).WithField("repo", repo).Warn("Repo isn't inside orgs")
 		}
+
 	}
 	return nil
 }
@@ -472,6 +489,9 @@ func copyLabelMap(originalMap map[string]Label) map[string]Label {
 func syncLabels(config Configuration, org string, repos RepoLabels) (RepoUpdates, error) {
 	// Find required, dead and archaic labels
 	defaultRequired, defaultArchaic, defaultDead := classifyLabels(config.Default.Labels, make(map[string]Label), make(map[string]Label), make(map[string]Label), time.Now(), nil)
+	if orgLabels, ok := config.Orgs[org]; ok {
+		defaultRequired, defaultArchaic, defaultDead = classifyLabels(orgLabels.Labels, defaultRequired, defaultArchaic, defaultDead, time.Now(), nil)
+	}
 
 	var validationErrors []error
 	var actions []Update
@@ -811,8 +831,31 @@ func writeDocs(template string, output string, config Configuration) error {
 	data = append(data, labelData{desc, linkify(desc), LabelsForTarget(config.Default.Labels, issueTarget)})
 	desc = "all repos, only for PRs"
 	data = append(data, labelData{desc, linkify(desc), LabelsForTarget(config.Default.Labels, prTarget)})
+	// Let's sort orgs
+	var orgs []string
+	for org := range config.Orgs {
+		orgs = append(orgs, org)
+	}
+	sort.Strings(orgs)
+	// And append their labels
+	for _, org := range orgs {
+		lead := fmt.Sprintf("all repos in %s", org)
+		if l := LabelsForTarget(config.Orgs[org].Labels, bothTarget); len(l) > 0 {
+			desc = lead + ", for both issues and PRs"
+			data = append(data, labelData{desc, linkify(desc), l})
+		}
+		if l := LabelsForTarget(config.Orgs[org].Labels, issueTarget); len(l) > 0 {
+			desc = lead + ", only for issues"
+			data = append(data, labelData{desc, linkify(desc), l})
+		}
+		if l := LabelsForTarget(config.Orgs[org].Labels, prTarget); len(l) > 0 {
+			desc = lead + ", only for PRs"
+			data = append(data, labelData{desc, linkify(desc), l})
+		}
+	}
+
 	// Let's sort repos
-	repos := make([]string, 0)
+	var repos []string
 	for repo := range config.Repos {
 		repos = append(repos, repo)
 	}

diff --git a/label_sync/main_test.go b/label_sync/main_test.go
@@ -18,10 +18,12 @@ package main
 
 import (
 	"encoding/json"
-	"reflect"
 	"strings"
 	"testing"
 	"time"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 )
 
 // Tests for getting data from GitHub are not needed:
@@ -82,6 +84,36 @@ func TestValidate(t *testing.T) {
 			},
 			expectedError: false,
 		},
+		{
+			name: "Required label defined in default and org",
+			config: Configuration{
+				Default: RepoConfig{Labels: []Label{
+					{Name: "lab1", Description: "Test Label 1", Color: "deadbe"},
+				}},
+				Orgs: map[string]RepoConfig{
+					"org": {Labels: []Label{
+						{Name: "lab1", Description: "Test Label 1", Color: "deadbe"},
+					}},
+				},
+			},
+			expectedError: true,
+		},
+		{
+			name: "Required label defined in org and repo",
+			config: Configuration{
+				Orgs: map[string]RepoConfig{
+					"org": {Labels: []Label{
+						{Name: "lab1", Description: "Test Label 1", Color: "deadbe"},
+					}},
+				},
+				Repos: map[string]RepoConfig{
+					"org/repo1": {Labels: []Label{
+						{Name: "lab1", Description: "Test Label 1", Color: "deadbe"},
+					}},
+				},
+			},
+			expectedError: true,
+		},
 	}
 	// Do tests
 	for _, tc := range testcases {
@@ -162,6 +194,32 @@ func TestSyncLabels(t *testing.T) {
 					{repo: "repo1", Why: "missing", Wanted: &Label{Name: "lab1", Description: "Test Label 1", Color: "deadbe"}}},
 			},
 		},
+		{
+			name: "Required label defined on org-level - update required on one repo",
+			config: Configuration{
+				Default: RepoConfig{Labels: []Label{
+					{Name: "lab1", Description: "Test Label 1", Color: "deadbe"},
+				}},
+				Orgs: map[string]RepoConfig{
+					"org": {Labels: []Label{
+						{Name: "lab2", Description: "Test Label 2", Color: "deadbe"},
+					}},
+				},
+			},
+			current: RepoLabels{
+				"repo1": {
+					{Name: "lab1", Description: "Test Label 1", Color: "deadbe"},
+					{Name: "lab2", Description: "Test Label 2", Color: "deadbe"},
+				},
+				"repo2": {
+					{Name: "lab1", Description: "Test Label 1", Color: "deadbe"},
+				},
+			},
+			expectedUpdates: RepoUpdates{
+				"repo2": {
+					{repo: "repo2", Why: "missing", Wanted: &Label{Name: "lab2", Description: "Test Label 2", Color: "deadbe"}}},
+			},
+		},
 		{
 			name: "Duplicate label on repo1",
 			current: RepoLabels{
@@ -423,11 +481,15 @@ func TestLoadYAML(t *testing.T) {
 	}{
 		{
 			path: "labels_example.yaml",
-			expected: Configuration{Default: RepoConfig{Labels: []Label{
-				{Name: "lgtm", Description: "LGTM", Color: "green"},
-				{Name: "priority/P0", Description: "P0 Priority", Color: "red", Previously: []Label{{Name: "P0", Description: "P0 Priority", Color: "blue"}}},
-				{Name: "dead-label", Description: "Delete Me :)", DeleteAfter: &d},
-			}}},
+			expected: Configuration{
+				Default: RepoConfig{Labels: []Label{
+					{Name: "lgtm", Description: "LGTM", Color: "green"},
+					{Name: "priority/P0", Description: "P0 Priority", Color: "red", Previously: []Label{{Name: "P0", Description: "P0 Priority", Color: "blue"}}},
+					{Name: "dead-label", Description: "Delete Me :)", DeleteAfter: &d},
+				}},
+				Orgs:  map[string]RepoConfig{"org": {Labels: []Label{{Name: "sgtm", Description: "Sounds Good To Me", Color: "green"}}}},
+				Repos: map[string]RepoConfig{"org/repo": {Labels: []Label{{Name: "tgtm", Description: "Tastes Good To Me", Color: "blue"}}}},
+			},
 			ok: true,
 		},
 		{
@@ -452,8 +514,8 @@ func TestLoadYAML(t *testing.T) {
 		if !errNil && !strings.Contains(err.Error(), tc.errMsg) {
 			t.Errorf("TestLoadYAML: test case number %d, expected error '%v' to contain '%v'", i+1, err.Error(), tc.errMsg)
 		}
-		if errNil && !reflect.DeepEqual(*actual, tc.expected) {
-			t.Errorf("TestLoadYAML: test case number %d, expected labels %v, got %v", i+1, tc.expected, actual)
+		if diff := cmp.Diff(actual, &tc.expected, cmpopts.IgnoreUnexported(Label{})); errNil && diff != "" {
+			t.Errorf("TestLoadYAML: test case number %d, labels differ:%s", i+1, diff)
 		}
 	}
 }