From c778270ff86a6a746b0b6971156985ec5af3de80 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Wed, 3 Feb 2021 16:57:34 +0100 Subject: [PATCH] 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. --- label_sync/BUILD.bazel | 4 ++ label_sync/labels_example.yaml | 12 ++++++ label_sync/main.go | 67 +++++++++++++++++++++++------ label_sync/main_test.go | 78 ++++++++++++++++++++++++++++++---- 4 files changed, 141 insertions(+), 20 deletions(-) diff --git a/label_sync/BUILD.bazel b/label_sync/BUILD.bazel index 5a4b1a680faa..be082c92c0e8 100644 --- 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 index 7e152489e99d..71d6836b9a86 100644 --- 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 index 831f5dd41016..5cc3177e980f 100644 --- 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,7 +269,7 @@ 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) } @@ -273,21 +277,34 @@ func (c Configuration) validate(orgs string) error { // 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 index a1ed8f126bde..5cd456cf79e0 100644 --- 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) } } }