Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prow: allow configuring the filenames used for OWNERS(_ALIASES) #20482

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions prow/cmd/hook/BUILD.bazel
Expand Up @@ -57,6 +57,7 @@ go_library(
"//prow/plugins:go_default_library",
"//prow/plugins/bugzilla:go_default_library",
"//prow/plugins/jira:go_default_library",
"//prow/plugins/ownersconfig:go_default_library",
"//prow/repoowners:go_default_library",
"//prow/slack:go_default_library",
"@com_github_sirupsen_logrus//:go_default_library",
Expand Down
6 changes: 5 additions & 1 deletion prow/cmd/hook/main.go
Expand Up @@ -42,6 +42,7 @@ import (
"k8s.io/test-infra/prow/plugins"
bzplugin "k8s.io/test-infra/prow/plugins/bugzilla"
"k8s.io/test-infra/prow/plugins/jira"
"k8s.io/test-infra/prow/plugins/ownersconfig"
"k8s.io/test-infra/prow/repoowners"
"k8s.io/test-infra/prow/slack"
)
Expand Down Expand Up @@ -204,7 +205,10 @@ func main() {
ownersDirBlacklist := func() config.OwnersDirBlacklist {
return configAgent.Config().OwnersDirBlacklist
}
ownersClient := repoowners.NewClient(git.ClientFactoryFrom(gitClient), githubClient, mdYAMLEnabled, skipCollaborators, ownersDirBlacklist)
resolver := func(org, repo string) ownersconfig.Filenames {
return pluginAgent.Config().OwnersFilenames(org, repo)
}
ownersClient := repoowners.NewClient(git.ClientFactoryFrom(gitClient), githubClient, mdYAMLEnabled, skipCollaborators, ownersDirBlacklist, resolver)

clientAgent := &plugins.ClientAgent{
GitHubClient: githubClient,
Expand Down
1 change: 1 addition & 0 deletions prow/hook/BUILD.bazel
Expand Up @@ -20,6 +20,7 @@ go_test(
"//prow/githubeventserver:go_default_library",
"//prow/phony:go_default_library",
"//prow/plugins:go_default_library",
"//prow/plugins/ownersconfig:go_default_library",
"//prow/repoowners:go_default_library",
],
)
Expand Down
3 changes: 2 additions & 1 deletion prow/hook/hook_test.go
Expand Up @@ -28,6 +28,7 @@ import (
"k8s.io/test-infra/prow/githubeventserver"
"k8s.io/test-infra/prow/phony"
"k8s.io/test-infra/prow/plugins"
"k8s.io/test-infra/prow/plugins/ownersconfig"
"k8s.io/test-infra/prow/repoowners"
)

Expand Down Expand Up @@ -107,7 +108,7 @@ func TestHook(t *testing.T) {
ca := &config.Agent{}
clientAgent := &plugins.ClientAgent{
GitHubClient: github.NewFakeClient(),
OwnersClient: repoowners.NewClient(nil, nil, func(org, repo string) bool { return false }, func(org, repo string) bool { return false }, func() config.OwnersDirBlacklist { return config.OwnersDirBlacklist{} }),
OwnersClient: repoowners.NewClient(nil, nil, func(org, repo string) bool { return false }, func(org, repo string) bool { return false }, func() config.OwnersDirBlacklist { return config.OwnersDirBlacklist{} }, ownersconfig.FakeResolver),
BugzillaClient: &bugzilla.Fake{},
}
metrics := githubeventserver.NewMetrics()
Expand Down
2 changes: 2 additions & 0 deletions prow/plugins/BUILD.bazel
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
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
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
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
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
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
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
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
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
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. Keys are in "org/repo" format.
Filenames map[string]ownersconfig.Filenames `json:"filenames,omitempty"`
stevekuznetsov marked this conversation as resolved.
Show resolved Hide resolved
}

// 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