From 94f76c4ceb3115bbdb4772ac7e7d5985284555f7 Mon Sep 17 00:00:00 2001 From: Alex Pavel Date: Fri, 4 Jun 2021 16:11:07 -0700 Subject: [PATCH 1/3] plugins: add sharding support for bugzilla This PR adds config sharding support for the bugzilla plugin. --- prow/plugins/config.go | 53 +++++- prow/plugins/config_test.go | 323 +++++++++++++++++++++++++++++++++++- 2 files changed, 374 insertions(+), 2 deletions(-) diff --git a/prow/plugins/config.go b/prow/plugins/config.go index 0ce9703dfe5d..5e8701bfe73b 100644 --- a/prow/plugins/config.go +++ b/prow/plugins/config.go @@ -1771,7 +1771,7 @@ type Override struct { func (c *Configuration) mergeFrom(other *Configuration) error { var errs []error - if diff := cmp.Diff(other, &Configuration{Plugins: other.Plugins}); diff != "" { + if diff := cmp.Diff(other, &Configuration{Plugins: other.Plugins, Bugzilla: other.Bugzilla}); diff != "" { errs = append(errs, fmt.Errorf("supplemental plugin configuration has config that doesn't support merging: %s", diff)) } @@ -1782,6 +1782,10 @@ func (c *Configuration) mergeFrom(other *Configuration) error { errs = append(errs, fmt.Errorf("failed to merge .plugins from supplemental config: %w", err)) } + if err := (&(c.Bugzilla)).mergeFrom(&other.Bugzilla); err != nil { + errs = append(errs, fmt.Errorf("failed to merge .bugzilla from supplemental config: %w", err)) + } + return utilerrors.NewAggregate(errs) } @@ -1806,6 +1810,53 @@ func (p *Plugins) mergeFrom(other *Plugins) error { return utilerrors.NewAggregate(errs) } +func (p *Bugzilla) mergeFrom(other *Bugzilla) error { + if other == nil { + return nil + } + + var errs []error + if other.Default != nil { + if (*p).Default != nil { + errs = append(errs, errors.New("configuration of global default defined in multiple places")) + } else { + (*p).Default = other.Default + } + } + if len(other.Orgs) != 0 && (*p).Orgs == nil { + newConfig := (*p) + newConfig.Orgs = make(map[string]BugzillaOrgOptions) + (*p) = newConfig + } + for org, orgConfig := range other.Orgs { + if _, ok := (*p).Orgs[org]; !ok { + (*p).Orgs[org] = BugzillaOrgOptions{} + } + if orgConfig.Default != nil { + if (*p).Orgs[org].Default != nil { + errs = append(errs, fmt.Errorf("found duplicate organization config for bugzilla.%s", org)) + continue + } + newConfig := (*p).Orgs[org] + newConfig.Default = orgConfig.Default + (*p).Orgs[org] = newConfig + } + if len(orgConfig.Repos) != 0 && (*p).Orgs[org].Repos == nil { + newConfig := (*p).Orgs[org] + newConfig.Repos = make(map[string]BugzillaRepoOptions) + (*p).Orgs[org] = newConfig + } + for repo, repoConfig := range orgConfig.Repos { + if _, ok := (*p).Orgs[org].Repos[repo]; ok { + errs = append(errs, fmt.Errorf("found duplicate repository config for bugzilla.%s/%s", org, repo)) + continue + } + (*p).Orgs[org].Repos[repo] = repoConfig + } + } + return utilerrors.NewAggregate(errs) +} + func (c *Configuration) HasConfigFor() (global bool, orgs sets.String, repos sets.String) { if !reflect.DeepEqual(c, &Configuration{Plugins: c.Plugins}) { global = true diff --git a/prow/plugins/config_test.go b/prow/plugins/config_test.go index daeadf1f1927..5d6193156633 100644 --- a/prow/plugins/config_test.go +++ b/prow/plugins/config_test.go @@ -1534,7 +1534,7 @@ func TestConfigMergingProperties(t *testing.T) { { name: "Plugins config", makeMergeable: func(c *Configuration) { - *c = Configuration{Plugins: c.Plugins} + *c = Configuration{Plugins: c.Plugins, Bugzilla: c.Bugzilla} }, }, } @@ -1663,6 +1663,327 @@ func TestPluginsMergeFrom(t *testing.T) { } } +func TestBugzillaMergeFrom(t *testing.T) { + t.Parallel() + + yes := true + targetRelease1 := "target-release-1" + targetRelease2 := "target-release-2" + + testCases := []struct { + name string + + from *Bugzilla + to *Bugzilla + + expected *Bugzilla + expectedErrMsg string + }{ + { + name: "Merging for two different repos", + + from: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Repos: map[string]BugzillaRepoOptions{ + "repo-1": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }, + }, + }, + }, + }}, + to: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Repos: map[string]BugzillaRepoOptions{ + "repo-2": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease2, + }, + }, + }, + }, + }, + }}, + + expected: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Repos: map[string]BugzillaRepoOptions{ + "repo-1": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }, + }, + "repo-2": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease2, + }, + }, + }, + }, + }, + }}, + }, + { + name: "Merging organization defaults and repo in org", + + from: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Repos: map[string]BugzillaRepoOptions{ + "repo-2": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease2, + }, + }, + }, + }, + }, + }}, + to: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Default: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }, + }, + }}, + + expected: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Default: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }, + Repos: map[string]BugzillaRepoOptions{ + "repo-2": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease2, + }, + }, + }, + }, + }, + }}, + }, + { + name: "Merging 2 organizations", + + from: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Repos: map[string]BugzillaRepoOptions{ + "repo-1": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }, + }, + }, + }, + }}, + to: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org-2": { + Repos: map[string]BugzillaRepoOptions{ + "repo-1": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease2, + }, + }, + }, + }, + }, + }}, + + expected: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Repos: map[string]BugzillaRepoOptions{ + "repo-1": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }, + }, + }}, + "org-2": { + Repos: map[string]BugzillaRepoOptions{ + "repo-1": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease2, + }, + }, + }, + }, + }, + }}, + }, + { + name: "Merging global defaults succeeds", + + from: &Bugzilla{Default: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }}, + to: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Repos: map[string]BugzillaRepoOptions{ + "repo-1": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }, + }, + }, + }, + }}, + expected: &Bugzilla{Default: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }, Orgs: map[string]BugzillaOrgOptions{ + "org": { + Repos: map[string]BugzillaRepoOptions{ + "repo-1": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }, + }, + }, + }, + }}, + }, + { + name: "Merging multiple global defaults fails", + + from: &Bugzilla{Default: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }}, + to: &Bugzilla{Default: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease2, + }, + }}, + expectedErrMsg: "configuration of global default defined in multiple places", + }, + { + name: "Merging same organization defaults fails", + + from: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Default: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }, + }, + }}, + to: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Default: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease2, + }, + }, + }, + }}, + + expectedErrMsg: "found duplicate organization config for bugzilla.org", + }, + { + name: "Merging same repository fails", + + from: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Repos: map[string]BugzillaRepoOptions{ + "repo-1": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease1, + }, + }, + }, + }, + }, + }}, + to: &Bugzilla{Orgs: map[string]BugzillaOrgOptions{ + "org": { + Repos: map[string]BugzillaRepoOptions{ + "repo-1": { + Branches: map[string]BugzillaBranchOptions{ + "master": { + IsOpen: &yes, + TargetRelease: &targetRelease2, + }, + }, + }, + }, + }, + }}, + + expectedErrMsg: "found duplicate repository config for bugzilla.org/repo-1", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var errMsg string + err := tc.to.mergeFrom(tc.from) + if err != nil { + errMsg = err.Error() + } + if tc.expectedErrMsg != errMsg { + t.Fatalf("expected error message %q, got %q", tc.expectedErrMsg, errMsg) + } + if err != nil { + return + } + + if diff := cmp.Diff(tc.expected, tc.to); diff != "" { + t.Errorf("expexcted config differs from actual: %s", diff) + } + }) + } +} + func TestHasConfigFor(t *testing.T) { t.Parallel() testCases := []struct { From 87785e672af6ccfcd26243a3ad29595135b0e276 Mon Sep 17 00:00:00 2001 From: Alex Pavel Date: Tue, 8 Jun 2021 15:34:40 -0700 Subject: [PATCH 2/3] plugins/config: clean up code and update HasConfigFor --- prow/plugins/config.go | 43 +++++++++++++++++++++---------------- prow/plugins/config_test.go | 22 ++++++++++++++++++- 2 files changed, 46 insertions(+), 19 deletions(-) diff --git a/prow/plugins/config.go b/prow/plugins/config.go index 5e8701bfe73b..c58cf87415c1 100644 --- a/prow/plugins/config.go +++ b/prow/plugins/config.go @@ -1782,7 +1782,7 @@ func (c *Configuration) mergeFrom(other *Configuration) error { errs = append(errs, fmt.Errorf("failed to merge .plugins from supplemental config: %w", err)) } - if err := (&(c.Bugzilla)).mergeFrom(&other.Bugzilla); err != nil { + if err := c.Bugzilla.mergeFrom(&other.Bugzilla); err != nil { errs = append(errs, fmt.Errorf("failed to merge .bugzilla from supplemental config: %w", err)) } @@ -1817,48 +1817,46 @@ func (p *Bugzilla) mergeFrom(other *Bugzilla) error { var errs []error if other.Default != nil { - if (*p).Default != nil { + if p.Default != nil { errs = append(errs, errors.New("configuration of global default defined in multiple places")) } else { - (*p).Default = other.Default + p.Default = other.Default } } - if len(other.Orgs) != 0 && (*p).Orgs == nil { - newConfig := (*p) - newConfig.Orgs = make(map[string]BugzillaOrgOptions) - (*p) = newConfig + if len(other.Orgs) != 0 && p.Orgs == nil { + p.Orgs = make(map[string]BugzillaOrgOptions) } for org, orgConfig := range other.Orgs { - if _, ok := (*p).Orgs[org]; !ok { - (*p).Orgs[org] = BugzillaOrgOptions{} + if _, ok := p.Orgs[org]; !ok { + p.Orgs[org] = BugzillaOrgOptions{} } if orgConfig.Default != nil { - if (*p).Orgs[org].Default != nil { + if p.Orgs[org].Default != nil { errs = append(errs, fmt.Errorf("found duplicate organization config for bugzilla.%s", org)) continue } - newConfig := (*p).Orgs[org] + newConfig := p.Orgs[org] newConfig.Default = orgConfig.Default - (*p).Orgs[org] = newConfig + p.Orgs[org] = newConfig } - if len(orgConfig.Repos) != 0 && (*p).Orgs[org].Repos == nil { - newConfig := (*p).Orgs[org] + if len(orgConfig.Repos) != 0 && p.Orgs[org].Repos == nil { + newConfig := p.Orgs[org] newConfig.Repos = make(map[string]BugzillaRepoOptions) - (*p).Orgs[org] = newConfig + p.Orgs[org] = newConfig } for repo, repoConfig := range orgConfig.Repos { - if _, ok := (*p).Orgs[org].Repos[repo]; ok { + if _, ok := p.Orgs[org].Repos[repo]; ok { errs = append(errs, fmt.Errorf("found duplicate repository config for bugzilla.%s/%s", org, repo)) continue } - (*p).Orgs[org].Repos[repo] = repoConfig + p.Orgs[org].Repos[repo] = repoConfig } } return utilerrors.NewAggregate(errs) } func (c *Configuration) HasConfigFor() (global bool, orgs sets.String, repos sets.String) { - if !reflect.DeepEqual(c, &Configuration{Plugins: c.Plugins}) { + if !reflect.DeepEqual(c, &Configuration{Plugins: c.Plugins, Bugzilla: c.Bugzilla}) || c.Bugzilla.Default != nil { global = true } orgs = sets.String{} @@ -1871,5 +1869,14 @@ func (c *Configuration) HasConfigFor() (global bool, orgs sets.String, repos set } } + for org, orgConfig := range c.Bugzilla.Orgs { + if orgConfig.Default != nil { + orgs.Insert(org) + } + for repo := range orgConfig.Repos { + repos.Insert(repo) + } + } + return global, orgs, repos } diff --git a/prow/plugins/config_test.go b/prow/plugins/config_test.go index 5d6193156633..f3a2c54810c3 100644 --- a/prow/plugins/config_test.go +++ b/prow/plugins/config_test.go @@ -1991,15 +1991,18 @@ func TestHasConfigFor(t *testing.T) { resultGenerator func(fuzzedConfig *Configuration) (toCheck *Configuration, expectGlobal bool, expectOrgs sets.String, expectRepos sets.String) }{ { - name: "Any non-empty config with empty Plugins is considered to be global", + name: "Any non-empty config with empty Plugins and Bugzilla is considered to be global", resultGenerator: func(fuzzedConfig *Configuration) (toCheck *Configuration, expectGlobal bool, expectOrgs sets.String, expectRepos sets.String) { fuzzedConfig.Plugins = nil + fuzzedConfig.Bugzilla = Bugzilla{} return fuzzedConfig, !reflect.DeepEqual(fuzzedConfig, &Configuration{}), nil, nil }, }, { name: "Any config with plugins is considered to be for the orgs and repos references there", resultGenerator: func(fuzzedConfig *Configuration) (toCheck *Configuration, expectGlobal bool, expectOrgs sets.String, expectRepos sets.String) { + // exclude non-plugins configs to test plugins specifically + fuzzedConfig.Bugzilla = Bugzilla{} expectOrgs, expectRepos = sets.String{}, sets.String{} for orgOrRepo := range fuzzedConfig.Plugins { if strings.Contains(orgOrRepo, "/") { @@ -2011,6 +2014,23 @@ func TestHasConfigFor(t *testing.T) { return fuzzedConfig, !reflect.DeepEqual(fuzzedConfig, &Configuration{Plugins: fuzzedConfig.Plugins}), expectOrgs, expectRepos }, }, + { + name: "Any config with bugzilla is considered to be for the orgs and repos references there", + resultGenerator: func(fuzzedConfig *Configuration) (toCheck *Configuration, expectGlobal bool, expectOrgs sets.String, expectRepos sets.String) { + // exclude non-plugins configs to test bugzilla specifically + fuzzedConfig.Plugins = nil + expectOrgs, expectRepos = sets.String{}, sets.String{} + for org, orgConfig := range fuzzedConfig.Bugzilla.Orgs { + if orgConfig.Default != nil { + expectOrgs.Insert(org) + } + for repo := range orgConfig.Repos { + expectRepos.Insert(repo) + } + } + return fuzzedConfig, !reflect.DeepEqual(fuzzedConfig, &Configuration{Bugzilla: fuzzedConfig.Bugzilla}), expectOrgs, expectRepos + }, + }, } seed := time.Now().UnixNano() From ec898fc2b5bdbc25963fd238e54765a2c7ed5642 Mon Sep 17 00:00:00 2001 From: Alex Pavel Date: Wed, 9 Jun 2021 10:52:39 -0700 Subject: [PATCH 3/3] plugins: fix HasConfigFor --- prow/plugins/config.go | 2 +- prow/plugins/config_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/prow/plugins/config.go b/prow/plugins/config.go index c58cf87415c1..6adf7f5953f0 100644 --- a/prow/plugins/config.go +++ b/prow/plugins/config.go @@ -1874,7 +1874,7 @@ func (c *Configuration) HasConfigFor() (global bool, orgs sets.String, repos set orgs.Insert(org) } for repo := range orgConfig.Repos { - repos.Insert(repo) + repos.Insert(org + "/" + repo) } } diff --git a/prow/plugins/config_test.go b/prow/plugins/config_test.go index f3a2c54810c3..52ac96901d4b 100644 --- a/prow/plugins/config_test.go +++ b/prow/plugins/config_test.go @@ -2025,7 +2025,7 @@ func TestHasConfigFor(t *testing.T) { expectOrgs.Insert(org) } for repo := range orgConfig.Repos { - expectRepos.Insert(repo) + expectRepos.Insert(org + "/" + repo) } } return fuzzedConfig, !reflect.DeepEqual(fuzzedConfig, &Configuration{Bugzilla: fuzzedConfig.Bugzilla}), expectOrgs, expectRepos