diff --git a/prow/plugins.yaml b/prow/plugins.yaml index b99efbde49d4f..cb0a50c908d18 100644 --- a/prow/plugins.yaml +++ b/prow/plugins.yaml @@ -42,57 +42,71 @@ use_deprecated_2018_implicit_self_approve_default_migrate_before_july_2019: true use_deprecated_2018_review_acts_as_approve_default_migrate_before_july_2019: true approve: -- repos: - - kubernetes/cloud-provider-aws - - kubernetes/cloud-provider-azure - - kubernetes/cluster-registry - - kubernetes/contrib - - kubernetes/dashboard - - kubernetes/dns - - kubernetes/enhancements - - kubernetes/examples - - kubernetes/federation - - kubernetes/gengo - - kubernetes/ingress-gce - - kubernetes/ingress-nginx - - kubernetes/klog - - kubernetes/kube-deploy - - kubernetes/kubeadm - - kubernetes/kubectl - - kubernetes/kubernetes-template-project - - kubernetes/kube-state-metrics - - kubernetes/minikube - - kubernetes/node-problem-detector - - kubernetes/perf-tests - - kubernetes/publishing-bot - - kubernetes/release - - kubernetes/repo-infra - - kubernetes/sig-release - - kubernetes/steering - - kubernetes/utils - - kubernetes-incubator/ip-masq-agent - require_self_approval: false - lgtm_acts_as_approve: true -- repos: - - kubernetes/kops - - kubernetes/kubernetes - - kubernetes-client - - kubernetes-csi - - kubernetes-sigs - - client-go/unofficial-docs - require_self_approval: false -- repos: - - bazelbuild - - kubernetes/community - - kubernetes/org - - kubernetes/test-infra - - kubernetes-sigs/kind - require_self_approval: false - ignore_review_state: false -- repos: - - helm/charts - require_self_approval: false - lgtm_acts_as_approve: true + orgs: + helm: + repos: + charts: + lgtm_acts_as_approve: true + kubernetes: + repos: + cloud-provider-aws: + lgtm_acts_as_approve: true + cloud-provider-azure: + lgtm_acts_as_approve: true + cluster-registry: + lgtm_acts_as_approve: true + contrib: + lgtm_acts_as_approve: true + dashboard: + lgtm_acts_as_approve: true + dns: + lgtm_acts_as_approve: true + enhancements: + lgtm_acts_as_approve: true + examples: + lgtm_acts_as_approve: true + federation: + lgtm_acts_as_approve: true + gengo: + lgtm_acts_as_approve: true + ingress-gce: + lgtm_acts_as_approve: true + ingress-nginx: + lgtm_acts_as_approve: true + klog: + lgtm_acts_as_approve: true + kube-deploy: + lgtm_acts_as_approve: true + kubeadm: + lgtm_acts_as_approve: true + kubectl: + lgtm_acts_as_approve: true + kubernetes-template-project: + lgtm_acts_as_approve: true + kube-state-metrics: + lgtm_acts_as_approve: true + minikube: + lgtm_acts_as_approve: true + node-problem-detector: + lgtm_acts_as_approve: true + perf-tests: + lgtm_acts_as_approve: true + publishing-bot: + lgtm_acts_as_approve: true + release: + lgtm_acts_as_approve: true + repo-infra: + lgtm_acts_as_approve: true + sig-release: + lgtm_acts_as_approve: true + steering: + lgtm_acts_as_approve: true + utils: + lgtm_acts_as_approve: true + kubernetes-incubator: + repos: + ip-masq-agent: + lgtm_acts_as_approve: true # Lower bounds in number of lines changed; XS is assumed to be zero. size: diff --git a/prow/plugins/BUILD.bazel b/prow/plugins/BUILD.bazel index 1694bd6ef1c40..ddba20b3fb141 100644 --- a/prow/plugins/BUILD.bazel +++ b/prow/plugins/BUILD.bazel @@ -10,6 +10,7 @@ go_test( name = "go_default_test", srcs = [ "config_test.go", + "helpers_test.go", "plugins_test.go", "respond_test.go", ], @@ -25,6 +26,7 @@ go_library( name = "go_default_library", srcs = [ "config.go", + "helpers.go", "plugins.go", "respond.go", ], diff --git a/prow/plugins/approve/approve.go b/prow/plugins/approve/approve.go index f910b417145be..dc1dc44385f8c 100644 --- a/prow/plugins/approve/approve.go +++ b/prow/plugins/approve/approve.go @@ -113,12 +113,12 @@ func helpProvider(config *plugins.Configuration, enabledRepos []string) (*plugin approveConfig := map[string]string{} for _, repo := range enabledRepos { parts := strings.Split(repo, "/") - var opts *plugins.Approve + var opts plugins.Approve switch len(parts) { case 1: - opts = optionsForRepo(config, repo, "") + opts = config.Approve.GetLeafConfig(repo, "", "").(plugins.Approve) case 2: - opts = optionsForRepo(config, parts[0], parts[1]) + opts = config.Approve.GetLeafConfig(parts[0], parts[1], "").(plugins.Approve) default: return nil, fmt.Errorf("invalid repo in enabledRepos: %q", repo) } @@ -142,17 +142,18 @@ func helpProvider(config *plugins.Configuration, enabledRepos []string) (*plugin } func handleGenericCommentEvent(pc plugins.Agent, ce github.GenericCommentEvent) error { + opts := pc.PluginConfig.Approve.GetLeafConfig(ce.Repo.Owner.Login, ce.Repo.Name, "").(plugins.Approve) return handleGenericComment( pc.Logger, pc.GitHubClient, pc.OwnersClient, pc.Config.GitHubOptions, - pc.PluginConfig, + &opts, &ce, ) } -func handleGenericComment(log *logrus.Entry, ghc githubClient, oc ownersClient, githubConfig config.GitHubOptions, config *plugins.Configuration, ce *github.GenericCommentEvent) error { +func handleGenericComment(log *logrus.Entry, ghc githubClient, oc ownersClient, githubConfig config.GitHubOptions, opts *plugins.Approve, ce *github.GenericCommentEvent) error { if ce.Action != github.GenericCommentActionCreated || !ce.IsPR || ce.IssueState == "closed" { return nil } @@ -162,7 +163,6 @@ func handleGenericComment(log *logrus.Entry, ghc githubClient, oc ownersClient, return err } - opts := optionsForRepo(config, ce.Repo.Owner.Login, ce.Repo.Name) if !isApprovalCommand(botName, opts.LgtmActsAsApprove, &comment{Body: ce.Body, Author: ce.User.Login}) { return nil } @@ -199,17 +199,18 @@ func handleGenericComment(log *logrus.Entry, ghc githubClient, oc ownersClient, // handleReviewEvent should only handle reviews that have no approval command. // Reviews with approval commands will be handled by handleGenericCommentEvent. func handleReviewEvent(pc plugins.Agent, re github.ReviewEvent) error { + opts := pc.PluginConfig.Approve.GetLeafConfig(re.Repo.Owner.Login, re.Repo.Name, "").(plugins.Approve) return handleReview( pc.Logger, pc.GitHubClient, pc.OwnersClient, pc.Config.GitHubOptions, - pc.PluginConfig, + &opts, &re, ) } -func handleReview(log *logrus.Entry, ghc githubClient, oc ownersClient, githubConfig config.GitHubOptions, config *plugins.Configuration, re *github.ReviewEvent) error { +func handleReview(log *logrus.Entry, ghc githubClient, oc ownersClient, githubConfig config.GitHubOptions, opts *plugins.Approve, re *github.ReviewEvent) error { if re.Action != github.ReviewActionSubmitted && re.Action != github.ReviewActionDismissed { return nil } @@ -219,8 +220,6 @@ func handleReview(log *logrus.Entry, ghc githubClient, oc ownersClient, githubCo return err } - opts := optionsForRepo(config, re.Repo.Owner.Login, re.Repo.Name) - // Check for an approval command is in the body. If one exists, let the // genericCommentEventHandler handle this event. Approval commands override // review state. @@ -244,7 +243,7 @@ func handleReview(log *logrus.Entry, ghc githubClient, oc ownersClient, githubCo ghc, repo, githubConfig, - optionsForRepo(config, re.Repo.Owner.Login, re.Repo.Name), + opts, &state{ org: re.Repo.Owner.Login, repo: re.Repo.Name, @@ -260,17 +259,18 @@ func handleReview(log *logrus.Entry, ghc githubClient, oc ownersClient, githubCo } func handlePullRequestEvent(pc plugins.Agent, pre github.PullRequestEvent) error { + opts := pc.PluginConfig.Approve.GetLeafConfig(pre.Repo.Owner.Login, pre.Repo.Name, "").(plugins.Approve) return handlePullRequest( pc.Logger, pc.GitHubClient, pc.OwnersClient, pc.Config.GitHubOptions, - pc.PluginConfig, + &opts, &pre, ) } -func handlePullRequest(log *logrus.Entry, ghc githubClient, oc ownersClient, githubConfig config.GitHubOptions, config *plugins.Configuration, pre *github.PullRequestEvent) error { +func handlePullRequest(log *logrus.Entry, ghc githubClient, oc ownersClient, githubConfig config.GitHubOptions, opts *plugins.Approve, pre *github.PullRequestEvent) error { if pre.Action != github.PullRequestActionOpened && pre.Action != github.PullRequestActionReopened && pre.Action != github.PullRequestActionSynchronize && @@ -296,7 +296,7 @@ func handlePullRequest(log *logrus.Entry, ghc githubClient, oc ownersClient, git ghc, repo, githubConfig, - optionsForRepo(config, pre.Repo.Owner.Login, pre.Repo.Name), + opts, &state{ org: pre.Repo.Owner.Login, repo: pre.Repo.Name, @@ -603,50 +603,6 @@ func addApprovers(approversHandler *approvers.Approvers, approveComments []*comm } } -// optionsForRepo gets the plugins.Approve struct that is applicable to the indicated repo. -func optionsForRepo(config *plugins.Configuration, org, repo string) *plugins.Approve { - fullName := fmt.Sprintf("%s/%s", org, repo) - - a := func() *plugins.Approve { - // First search for repo config - for _, c := range config.Approve { - if !strInSlice(fullName, c.Repos) { - continue - } - return &c - } - - // If you don't find anything, loop again looking for an org config - for _, c := range config.Approve { - if !strInSlice(org, c.Repos) { - continue - } - return &c - } - - // Return an empty config, and use plugin defaults - return &plugins.Approve{} - }() - if a.DeprecatedImplicitSelfApprove == nil && a.RequireSelfApproval == nil && config.UseDeprecatedSelfApprove { - no := false - a.DeprecatedImplicitSelfApprove = &no - } - if a.DeprecatedReviewActsAsApprove == nil && a.IgnoreReviewState == nil && config.UseDeprecatedReviewApprove { - no := false - a.DeprecatedReviewActsAsApprove = &no - } - return a -} - -func strInSlice(str string, slice []string) bool { - for _, elem := range slice { - if elem == str { - return true - } - } - return false -} - type comment struct { Body string Author string diff --git a/prow/plugins/approve/approve_test.go b/prow/plugins/approve/approve_test.go index 62c4ff9420c0b..d8e8110630718 100644 --- a/prow/plugins/approve/approve_test.go +++ b/prow/plugins/approve/approve_test.go @@ -55,23 +55,7 @@ func TestPluginConfig(t *testing.T) { } pa.Set(np) - orgs := map[string]bool{} - repos := map[string]bool{} - for _, config := range pa.Config().Approve { - for _, entry := range config.Repos { - if strings.Contains(entry, "/") { - if repos[entry] { - t.Errorf("The repo %q is duplicated in the 'approve' plugin configuration.", entry) - } - repos[entry] = true - } else { - if orgs[entry] { - t.Errorf("The org %q is duplicated in the 'approve' plugin configuration.", entry) - } - orgs[entry] = true - } - } - } + // no need to check for duplicates as the ConfigTree uses maps } func newTestComment(user, body string) github.IssueComment { @@ -1079,7 +1063,6 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen LinkURL: test.githubLinkURL, }, &plugins.Approve{ - Repos: []string{"org/repo"}, RequireSelfApproval: &rsa, IssueRequired: test.needsIssue, LgtmActsAsApprove: test.lgtmActsAsApprove, @@ -1357,17 +1340,14 @@ func TestHandleGenericComment(t *testing.T) { Host: "github.com", }, } - config := &plugins.Configuration{} - config.Approve = append(config.Approve, plugins.Approve{ - Repos: []string{test.commentEvent.Repo.Owner.Login}, - LgtmActsAsApprove: test.lgtmActsAsApprove, - }) err := handleGenericComment( logrus.WithField("plugin", "approve"), fghc, fakeOwnersClient{}, githubConfig, - config, + &plugins.Approve{ + LgtmActsAsApprove: test.lgtmActsAsApprove, + }, &test.commentEvent, ) @@ -1577,19 +1557,16 @@ func TestHandleReview(t *testing.T) { Host: "github.com", }, } - config := &plugins.Configuration{} irs := !test.reviewActsAsApprove - config.Approve = append(config.Approve, plugins.Approve{ - Repos: []string{test.reviewEvent.Repo.Owner.Login}, - LgtmActsAsApprove: test.lgtmActsAsApprove, - IgnoreReviewState: &irs, - }) err := handleReview( logrus.WithField("plugin", "approve"), fghc, fakeOwnersClient{}, githubConfig, - config, + &plugins.Approve{ + LgtmActsAsApprove: test.lgtmActsAsApprove, + IgnoreReviewState: &irs, + }, &test.reviewEvent, ) @@ -1733,7 +1710,7 @@ func TestHandlePullRequest(t *testing.T) { Host: "github.com", }, }, - &plugins.Configuration{}, + &plugins.Approve{}, &test.prEvent, ) @@ -1782,13 +1759,9 @@ func TestHelpProvider(t *testing.T) { { name: "All configs enabled", config: &plugins.Configuration{ - Approve: []plugins.Approve{ - { - Repos: []string{"org2"}, - IssueRequired: true, - RequireSelfApproval: &[]bool{true}[0], - LgtmActsAsApprove: true, - IgnoreReviewState: &[]bool{true}[0], + Approve: plugins.ConfigTree{ + Orgs: map[string]plugins.Org{ + "org2": {}, }, }, }, diff --git a/prow/plugins/config.go b/prow/plugins/config.go index 14a21b996662a..a049e82c56610 100644 --- a/prow/plugins/config.go +++ b/prow/plugins/config.go @@ -52,7 +52,7 @@ type Configuration struct { Owners Owners `json:"owners,omitempty"` // Built-in plugins specific configuration. - Approve []Approve `json:"approve,omitempty"` + Approve ConfigTree `json:"approve,omitempty"` UseDeprecatedSelfApprove bool `json:"use_deprecated_2018_implicit_self_approve_default_migrate_before_july_2019,omitempty"` UseDeprecatedReviewApprove bool `json:"use_deprecated_2018_review_acts_as_approve_default_migrate_before_july_2019,omitempty"` Blockades []Blockade `json:"blockades,omitempty"` @@ -226,8 +226,6 @@ type Blockade struct { // // The configuration for the approve plugin is defined as a list of these structures. type Approve struct { - // Repos is either of the form org/repos or just org. - Repos []string `json:"repos,omitempty"` // IssueRequired indicates if an associated issue is required for approval in // the specified repos. IssueRequired bool `json:"issue_required,omitempty"` @@ -251,6 +249,24 @@ type Approve struct { IgnoreReviewState *bool `json:"ignore_review_state,omitempty"` } +// MarshalJSON returns the JSON encoding of Approve +func (a Approve) MarshalJSON() ([]byte, error) { + // TODO + return []byte{}, nil +} + +// UnmarshalJSON parses the JSON-encoded data and stores the result in a +func (a Approve) UnmarshalJSON(b []byte) error { + // TODO + return nil +} + +// Apply returns a policy that merges the child into the parent +func (a Approve) Apply(parent LeafConfig) LeafConfig { + // TODO + return a +} + var ( warnImplicitSelfApprove time.Time warnReviewActsAsApprove time.Time diff --git a/prow/plugins/helpers.go b/prow/plugins/helpers.go new file mode 100644 index 0000000000000..d406f06c7cd4b --- /dev/null +++ b/prow/plugins/helpers.go @@ -0,0 +1,87 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package plugins + +// LeafConfig holds the config for a leaf +type LeafConfig interface { + MarshalJSON() ([]byte, error) + UnmarshalJSON([]byte) error + Apply(child LeafConfig) LeafConfig +} + +// ConfigTree specifies the global generic config for a plugin. +type ConfigTree struct { + LeafConfig + Orgs map[string]Org `json:"orgs,omitempty"` +} + +// GetOrg returns the org config after merging in any global config. +func (t ConfigTree) GetOrg(name string) *Org { + o, ok := t.Orgs[name] + if ok { + o.LeafConfig = t.LeafConfig.Apply(o.LeafConfig) + } else { + o.LeafConfig = t.LeafConfig + } + return &o +} + +// Org holds the default config for an entire org, as well as any repo overrides. +type Org struct { + LeafConfig + Repos map[string]Repo `json:"repos,omitempty"` +} + +// GetRepo returns the repo config after merging in any org policies. +func (o Org) GetRepo(name string) *Repo { + r, ok := o.Repos[name] + if ok { + r.LeafConfig = o.LeafConfig.Apply(r.LeafConfig) + } else { + r.LeafConfig = o.LeafConfig + } + return &r +} + +// Repo holds the default config for all branches in a repo, as well as specific branch overrides. +type Repo struct { + LeafConfig + Branches map[string]Branch `json:"branches,omitempty"` +} + +// GetBranch returns the branch config after merging in any repo policies. +func (r Repo) GetBranch(name string) *Branch { + b, ok := r.Branches[name] + if ok { + b.LeafConfig = r.LeafConfig.Apply(b.LeafConfig) + } else { + b.LeafConfig = r.LeafConfig + } + return &b +} + +// Branch holds config for a particular branch. +type Branch struct { + LeafConfig +} + +// GetLeafConfig returns the LeafConfig for a given org/repo/branch. +// +// Handles merging any policies defined at repo/org/global levels into the branch policy. +func (t *ConfigTree) GetLeafConfig(org, repo, branch string) LeafConfig { + return t.GetOrg(org).GetRepo(repo).GetBranch(branch) +} diff --git a/prow/plugins/helpers_test.go b/prow/plugins/helpers_test.go new file mode 100644 index 0000000000000..7f45cf86dc427 --- /dev/null +++ b/prow/plugins/helpers_test.go @@ -0,0 +1,98 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package plugins + +import ( + "testing" + + "sigs.k8s.io/yaml" +) + +func TestUnmarshal(t *testing.T) { + var cases = []struct { + name string + config []byte + expectError bool + }{ + { + name: "approve no orgs", + config: []byte(` +config: + issue_required: false + require_self_approval: true + lgtm_acts_as_approve: false + ignore_review_state: true +`), + expectError: true, + }, + { + name: "approve no default", + config: []byte(` +orgs: + bazelbuild: + config: + ignore_review_state: false + kubernetes: + config: + lgtm_acts_as_approve: true + repos: + kops: + config: + lgtm_acts_as_approve: false + kubernetes: + config: + issue_required: true +`), + }, + { + name: "approve full", + config: []byte(` +config: + issue_required: false + require_self_approval: true + lgtm_acts_as_approve: false + ignore_review_state: true +orgs: + bazelbuild: + config: + ignore_review_state: false + kubernetes: + config: + lgtm_acts_as_approve: true + repos: + kops: + config: + lgtm_acts_as_approve: false + kubernetes: + config: + issue_required: true + branches: + master: + config: + issue_required: false +`), + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + var tree ConfigTree + if err := yaml.Unmarshal(c.config, &tree); err != nil { + t.Errorf("error unmarshaling config: %v", err) + } + }) + } +}