Skip to content

Commit

Permalink
prow: allow configuring the filenames used for OWNERS(_ALIASES)
Browse files Browse the repository at this point in the history 
When a repository is a fork of an upstream repository with OWNERS, or
vendors a repository that also uses OWNERS, it is generally not intended
for users who are OWNERS upstream to have rights in the downstream.
Furthermore, changing the OWNERS files leads to merge conflicts as
changes from upstream are pulled in, which leads to headaches. Allowing
users to configure the files used by Prow to denote owners and aliases
side-steps this issue.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
  • Loading branch information
@stevekuznetsov
stevekuznetsov committed Jan 14, 2021
1 parent dfe4a7d commit abf0dcb
Show file tree
Hide file tree
Showing 25 changed files with 292 additions and 101 deletions.
2 changes: 2 additions & 0 deletions prow/plugins/BUILD.bazel
View file
Expand Up @@ -50,6 +50,7 @@ go_library(
"//prow/labels:go_default_library",
"//prow/logrusutil:go_default_library",
"//prow/pluginhelp:go_default_library",
"//prow/plugins/ownersconfig:go_default_library",
"//prow/repoowners:go_default_library",
"//prow/slack:go_default_library",
"//prow/version:go_default_library",
Expand Down Expand Up @@ -109,6 +110,7 @@ filegroup(
"//prow/plugins/milestonestatus:all-srcs",
"//prow/plugins/override:all-srcs",
"//prow/plugins/owners-label:all-srcs",
"//prow/plugins/ownersconfig:all-srcs",
"//prow/plugins/pony:all-srcs",
"//prow/plugins/project:all-srcs",
"//prow/plugins/projectmanager:all-srcs",
Expand Down
1 change: 1 addition & 0 deletions prow/plugins/approve/BUILD.bazel
View file
Expand Up @@ -32,6 +32,7 @@ go_test(
"//prow/pkg/layeredsets:go_default_library",
"//prow/plugins:go_default_library",
"//prow/plugins/approve/approvers:go_default_library",
"//prow/plugins/ownersconfig:go_default_library",
"//prow/repoowners:go_default_library",
"@com_github_sirupsen_logrus//:go_default_library",
"@io_k8s_apimachinery//pkg/util/sets:go_default_library",
Expand Down
5 changes: 5 additions & 0 deletions prow/plugins/approve/approve_test.go
View file
Expand Up @@ -39,6 +39,7 @@ import (
"k8s.io/test-infra/prow/pkg/layeredsets"
"k8s.io/test-infra/prow/plugins"
"k8s.io/test-infra/prow/plugins/approve/approvers"
"k8s.io/test-infra/prow/plugins/ownersconfig"
"k8s.io/test-infra/prow/repoowners"
)

Expand Down Expand Up @@ -134,6 +135,10 @@ type fakeRepo struct {
dirBlacklist []*regexp.Regexp
}

func (fr fakeRepo) Filenames() ownersconfig.Filenames {
return ownersconfig.FakeFilenames
}

func (fr fakeRepo) Approvers(path string) layeredsets.String {
return fr.approvers[path]
}
Expand Down
2 changes: 2 additions & 0 deletions prow/plugins/approve/approvers/BUILD.bazel
View file
Expand Up @@ -7,6 +7,7 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//prow/pkg/layeredsets:go_default_library",
"//prow/plugins/ownersconfig:go_default_library",
"@com_github_sirupsen_logrus//:go_default_library",
"@io_k8s_apimachinery//pkg/util/sets:go_default_library",
],
Expand All @@ -21,6 +22,7 @@ go_test(
embed = [":go_default_library"],
deps = [
"//prow/pkg/layeredsets:go_default_library",
"//prow/plugins/ownersconfig:go_default_library",
"@com_github_sirupsen_logrus//:go_default_library",
"@io_k8s_apimachinery//pkg/util/sets:go_default_library",
],
Expand Down
32 changes: 17 additions & 15 deletions prow/plugins/approve/approvers/approvers_test.go
View file
Expand Up @@ -25,6 +25,8 @@ import (
"reflect"

"k8s.io/apimachinery/pkg/util/sets"

"k8s.io/test-infra/prow/plugins/ownersconfig"
)

func TestUnapprovedFiles(t *testing.T) {
Expand Down Expand Up @@ -149,70 +151,70 @@ func TestGetFiles(t *testing.T) {
testName: "Single Root File PR Approved",
filenames: []string{"kubernetes.go"},
currentlyApproved: sets.NewString(rootApprovers.List()[0]),
expectedFiles: []File{ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "", sets.NewString(rootApprovers.List()[0]), "master"}},
expectedFiles: []File{ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "", ownersconfig.DefaultOwnersFile, sets.NewString(rootApprovers.List()[0]), "master"}},
},
{
testName: "Single File PR in B No One Approved",
filenames: []string{"b/test.go"},
currentlyApproved: sets.NewString(),
expectedFiles: []File{UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "b", "master"}},
expectedFiles: []File{UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "b", ownersconfig.DefaultOwnersFile, "master"}},
},
{
testName: "Single File PR in B Fully Approved",
filenames: []string{"b/test.go"},
currentlyApproved: bApprovers,
expectedFiles: []File{ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "b", bApprovers, "master"}},
expectedFiles: []File{ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "b", ownersconfig.DefaultOwnersFile, bApprovers, "master"}},
},
{
testName: "Single Root File PR No One Approved",
filenames: []string{"kubernetes.go"},
currentlyApproved: sets.NewString(),
expectedFiles: []File{UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "", "master"}},
expectedFiles: []File{UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "", ownersconfig.DefaultOwnersFile, "master"}},
},
{
testName: "Combo and Other; Neither Approved",
filenames: []string{"a/combo/test.go", "a/d/test.go"},
currentlyApproved: sets.NewString(),
expectedFiles: []File{
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", "master"},
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", "master"},
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", ownersconfig.DefaultOwnersFile, "master"},
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, "master"},
},
},
{
testName: "Combo and Other; Combo Approved",
filenames: []string{"a/combo/test.go", "a/d/test.go"},
currentlyApproved: eApprovers,
expectedFiles: []File{
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", eApprovers, "master"},
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", "master"},
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", ownersconfig.DefaultOwnersFile, eApprovers, "master"},
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, "master"},
},
},
{
testName: "Combo and Other; Both Approved",
filenames: []string{"a/combo/test.go", "a/d/test.go"},
currentlyApproved: edcApprovers.Intersection(dApprovers),
expectedFiles: []File{
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", edcApprovers.Intersection(dApprovers), "master"},
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", edcApprovers.Intersection(dApprovers), "master"},
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", ownersconfig.DefaultOwnersFile, edcApprovers.Intersection(dApprovers), "master"},
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, edcApprovers.Intersection(dApprovers), "master"},
},
},
{
testName: "Combo, C, D; Combo and C Approved",
filenames: []string{"a/combo/test.go", "a/d/test.go", "c/test"},
currentlyApproved: cApprovers,
expectedFiles: []File{
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", cApprovers, "master"},
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", "master"},
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "c", cApprovers, "master"},
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", ownersconfig.DefaultOwnersFile, cApprovers, "master"},
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, "master"},
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "c", ownersconfig.DefaultOwnersFile, cApprovers, "master"},
},
},
{
testName: "Files Approved Multiple times",
filenames: []string{"a/test.go", "a/d/test.go", "b/test"},
currentlyApproved: rootApprovers.Union(aApprovers).Union(bApprovers),
expectedFiles: []File{
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a", rootApprovers.Union(aApprovers), "master"},
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "b", rootApprovers.Union(bApprovers), "master"},
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a", ownersconfig.DefaultOwnersFile, rootApprovers.Union(aApprovers), "master"},
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "b", ownersconfig.DefaultOwnersFile, rootApprovers.Union(bApprovers), "master"},
},
},
}
Expand Down
36 changes: 21 additions & 15 deletions prow/plugins/approve/approvers/owners.go
View file
Expand Up @@ -30,11 +30,12 @@ import (
"github.com/sirupsen/logrus"

"k8s.io/apimachinery/pkg/util/sets"

"k8s.io/test-infra/prow/pkg/layeredsets"
"k8s.io/test-infra/prow/plugins/ownersconfig"
)

const (
ownersFileName = "OWNERS"
// ApprovalNotificationName defines the name used in the title for the approval notifications.
ApprovalNotificationName = "ApprovalNotifier"
)
Expand All @@ -45,6 +46,7 @@ type Repo interface {
LeafApprovers(path string) sets.String
FindApproverOwnersForFile(file string) string
IsNoParentOwners(path string) bool
Filenames() ownersconfig.Filenames
}

// Owners provides functionality related to owners of a specific code change.
Expand Down Expand Up @@ -436,16 +438,18 @@ func (ap Approvers) GetFiles(baseURL *url.URL, branch string) []File {
for _, file := range ap.owners.GetOwnersSet().List() {
if len(filesApprovers[file]) == 0 {
allOwnersFiles = append(allOwnersFiles, UnapprovedFile{
baseURL: baseURL,
filepath: file,
branch: branch,
baseURL: baseURL,
filepath: file,
ownersFilename: ap.owners.repo.Filenames().Owners,
branch: branch,
})
} else {
allOwnersFiles = append(allOwnersFiles, ApprovedFile{
baseURL: baseURL,
filepath: file,
approvers: filesApprovers[file],
branch: branch,
baseURL: baseURL,
filepath: file,
ownersFilename: ap.owners.repo.Filenames().Owners,
approvers: filesApprovers[file],
branch: branch,
})
}
}
Expand Down Expand Up @@ -541,22 +545,24 @@ type File interface {

// ApprovedFile contains the information of a an approved file.
type ApprovedFile struct {
baseURL *url.URL
filepath string
baseURL *url.URL
filepath string
ownersFilename string
// approvers is the set of users that approved this file change.
approvers sets.String
branch string
}

// UnapprovedFile contains the information of a an unapproved file.
type UnapprovedFile struct {
baseURL *url.URL
filepath string
branch string
baseURL *url.URL
filepath string
ownersFilename string
branch string
}

func (a ApprovedFile) String() string {
fullOwnersPath := filepath.Join(a.filepath, ownersFileName)
fullOwnersPath := filepath.Join(a.filepath, a.ownersFilename)
if strings.HasSuffix(a.filepath, ".md") {
fullOwnersPath = a.filepath
}
Expand All @@ -569,7 +575,7 @@ func (a ApprovedFile) String() string {
}

func (ua UnapprovedFile) String() string {
fullOwnersPath := filepath.Join(ua.filepath, ownersFileName)
fullOwnersPath := filepath.Join(ua.filepath, ua.ownersFilename)
if strings.HasSuffix(ua.filepath, ".md") {
fullOwnersPath = ua.filepath
}
Expand Down
13 changes: 9 additions & 4 deletions prow/plugins/approve/approvers/owners_test.go
View file
Expand Up @@ -17,16 +17,17 @@ limitations under the License.
package approvers

import (
"path/filepath"
"reflect"
"strings"
"testing"

"github.com/sirupsen/logrus"

"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/test-infra/prow/pkg/layeredsets"

"path/filepath"
"reflect"
"strings"
"k8s.io/test-infra/prow/pkg/layeredsets"
"k8s.io/test-infra/prow/plugins/ownersconfig"
)

const (
Expand All @@ -39,6 +40,10 @@ type FakeRepo struct {
noParentOwnersMap map[string]bool
}

func (f FakeRepo) Filenames() ownersconfig.Filenames {
return ownersconfig.FakeFilenames
}

func (f FakeRepo) Approvers(path string) layeredsets.String {
return f.approversMap[path]
}
Expand Down
1 change: 1 addition & 0 deletions prow/plugins/blunderbuss/BUILD.bazel
View file
Expand Up @@ -42,6 +42,7 @@ go_test(
"//prow/github:go_default_library",
"//prow/pkg/layeredsets:go_default_library",
"//prow/plugins:go_default_library",
"//prow/plugins/ownersconfig:go_default_library",
"//prow/repoowners:go_default_library",
"@com_github_shurcool_githubv4//:go_default_library",
"@com_github_sirupsen_logrus//:go_default_library",
Expand Down
5 changes: 5 additions & 0 deletions prow/plugins/blunderbuss/blunderbuss_test.go
View file
Expand Up @@ -36,6 +36,7 @@ import (
"k8s.io/test-infra/prow/github"
"k8s.io/test-infra/prow/pkg/layeredsets"
"k8s.io/test-infra/prow/plugins"
"k8s.io/test-infra/prow/plugins/ownersconfig"
"k8s.io/test-infra/prow/repoowners"
)

Expand Down Expand Up @@ -114,6 +115,10 @@ type fakeOwnersClient struct {
dirBlacklist []*regexp.Regexp
}

func (foc *fakeOwnersClient) Filenames() ownersconfig.Filenames {
return ownersconfig.FakeFilenames
}

func (foc *fakeOwnersClient) Approvers(path string) layeredsets.String {
return foc.approvers[path]
}
Expand Down
17 changes: 17 additions & 0 deletions prow/plugins/config.go
View file
Expand Up @@ -36,6 +36,7 @@ import (
"k8s.io/test-infra/prow/kube"
"k8s.io/test-infra/prow/labels"
"k8s.io/test-infra/prow/logrusutil"
"k8s.io/test-infra/prow/plugins/ownersconfig"
)

const (
Expand Down Expand Up @@ -167,6 +168,22 @@ type Owners struct {
// OWNERS file, preventing their automatic addition by the owners-label plugin.
// This check is performed by the verify-owners plugin.
LabelsBlackList []string `json:"labels_blacklist,omitempty"`

// Filenames allows configuring repos to use a separate set of filenames for
// any plugin that interacts with these files.
Filenames map[string]ownersconfig.Filenames `json:"filenames,omitempty"`
}

// OwnersFilenames determines which filenames to use for OWNERS and OWNERS_ALIASES for a repo.
func (c *Configuration) OwnersFilenames(org, repo string) ownersconfig.Filenames {
full := fmt.Sprintf("%s/%s", org, repo)
if config, configured := c.Owners.Filenames[full]; configured {
return config
}
return ownersconfig.Filenames{
Owners: ownersconfig.DefaultOwnersFile,
OwnersAliases: ownersconfig.DefaultOwnersAliasesFile,
}
}

// MDYAMLEnabled returns a boolean denoting if the passed repo supports YAML OWNERS config headers
Expand Down
2 changes: 2 additions & 0 deletions prow/plugins/heart/BUILD.bazel
View file
Expand Up @@ -15,6 +15,7 @@ go_library(
"//prow/github:go_default_library",
"//prow/pluginhelp:go_default_library",
"//prow/plugins:go_default_library",
"//prow/plugins/ownersconfig:go_default_library",
"@com_github_sirupsen_logrus//:go_default_library",
],
)
Expand All @@ -39,6 +40,7 @@ go_test(
deps = [
"//prow/github:go_default_library",
"//prow/github/fakegithub:go_default_library",
"//prow/plugins/ownersconfig:go_default_library",
"@com_github_sirupsen_logrus//:go_default_library",
],
)
12 changes: 6 additions & 6 deletions prow/plugins/heart/heart.go
View file
Expand Up @@ -29,12 +29,11 @@ import (
"k8s.io/test-infra/prow/github"
"k8s.io/test-infra/prow/pluginhelp"
"k8s.io/test-infra/prow/plugins"
"k8s.io/test-infra/prow/plugins/ownersconfig"
)

const (
pluginName = "heart"
ownersFilename = "OWNERS"
ownersAliasesFilename = "OWNERS_ALIASES"
pluginName = "heart"
)

var reactions = []string{
Expand Down Expand Up @@ -99,7 +98,7 @@ func handleIssueComment(pc plugins.Agent, ic github.IssueCommentEvent) error {
}

func handlePullRequest(pc plugins.Agent, pre github.PullRequestEvent) error {
return handlePR(getClient(pc), pre)
return handlePR(getClient(pc), pre, pc.PluginConfig.OwnersFilenames)
}

func handleIC(c client, adorees []string, commentRe *regexp.Regexp, ic github.IssueCommentEvent) error {
Expand Down Expand Up @@ -130,7 +129,7 @@ func handleIC(c client, adorees []string, commentRe *regexp.Regexp, ic github.Is
reactions[rand.Intn(len(reactions))])
}

func handlePR(c client, pre github.PullRequestEvent) error {
func handlePR(c client, pre github.PullRequestEvent, resolver ownersconfig.Resolver) error {
// Only consider newly opened PRs
if pre.Action != github.PullRequestActionOpened {
return nil
Expand All @@ -147,7 +146,8 @@ func handlePR(c client, pre github.PullRequestEvent) error {
// Smile at any change that adds to OWNERS files
for _, change := range changes {
_, filename := filepath.Split(change.Filename)
if (filename == ownersFilename || filename == ownersAliasesFilename) && change.Additions > 0 {
filenames := resolver(org, repo)
if (filename == filenames.Owners || filename == filenames.OwnersAliases) && change.Additions > 0 {
c.Logger.Info("Adding new OWNERS makes me happy!")
return c.GitHubClient.CreateIssueReaction(
pre.PullRequest.Base.Repo.Owner.Login,
Expand Down

0 comments on commit abf0dcb

Please sign in to comment.