Skip to content

fix: support OWNERS files with both top-level config and filters #5128

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.

Sign up for GitHub

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

Open
christophermancini wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: support OWNERS files with both top-level config and filters #5128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b171f6
fix: support OWNERS files with both top-level config and filters
christophermancini Apr 23, 2026
62b1fa4
fix: handle all OWNERS file formats correctly
christophermancini Apr 23, 2026
ff36134
Fix edge case: merge top-level config with explicit ".*" filter
christophermancini Apr 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 62 additions & 21 deletions cmd/autoowners/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,25 +139,14 @@ type httpResult struct {

// resolveOwnerAliases computes the resolved (simple or full config) format of the OWNERS file
func (r httpResult) resolveOwnerAliases(cleaner ownersCleaner) interface{} {
if !r.simpleConfig.Empty() {
sc := SimpleConfig{
Config: repoowners.Config{
Approvers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.Approvers)))),
Reviewers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.Reviewers)))),
RequiredReviewers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.RequiredReviewers)))),
Labels: sets.List(sets.New[string](r.simpleConfig.Labels...)),
},
Options: r.simpleConfig.Options,
}
if len(sc.Reviewers) == 0 {
sc.Reviewers = sc.Approvers
}
return sc
} else {
// If we have filters, we must use FullConfig format (even if we also have top-level config)
if len(r.fullConfig.Filters) > 0 {
fc := FullConfig{
Filters: map[string]repoowners.Config{},
Options: r.fullConfig.Options,
}

// Process all specific filters from fullConfig first
for k, v := range r.fullConfig.Filters {
cfg := repoowners.Config{
Approvers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(v.Approvers)))),
Expand All @@ -170,8 +159,47 @@ func (r httpResult) resolveOwnerAliases(cleaner ownersCleaner) interface{} {
}
fc.Filters[k] = cfg
}

// If we also have top-level config, merge it into the ".*" catch-all filter
if !r.simpleConfig.Empty() {
topLevelCfg := repoowners.Config{
Approvers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.Approvers)))),
Reviewers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.Reviewers)))),
RequiredReviewers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.RequiredReviewers)))),
Labels: sets.List(sets.New[string](r.simpleConfig.Labels...)),
}
if len(topLevelCfg.Reviewers) == 0 {
topLevelCfg.Reviewers = topLevelCfg.Approvers
}

// Merge with existing ".*" filter if one exists
if existing, ok := fc.Filters[".*"]; ok {
topLevelCfg.Approvers = sets.List(sets.New[string](append(topLevelCfg.Approvers, existing.Approvers...)...))
topLevelCfg.Reviewers = sets.List(sets.New[string](append(topLevelCfg.Reviewers, existing.Reviewers...)...))
topLevelCfg.RequiredReviewers = sets.List(sets.New[string](append(topLevelCfg.RequiredReviewers, existing.RequiredReviewers...)...))
topLevelCfg.Labels = sets.List(sets.New[string](append(topLevelCfg.Labels, existing.Labels...)...))
}
fc.Filters[".*"] = topLevelCfg
}
return fc
} else if !r.simpleConfig.Empty() {
// No filters, just use SimpleConfig
sc := SimpleConfig{
Config: repoowners.Config{
Approvers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.Approvers)))),
Reviewers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.Reviewers)))),
RequiredReviewers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(r.simpleConfig.RequiredReviewers)))),
Labels: sets.List(sets.New[string](r.simpleConfig.Labels...)),
},
Options: r.simpleConfig.Options,
}
if len(sc.Reviewers) == 0 {
sc.Reviewers = sc.Approvers
}
return sc
}
// Empty config - should not happen but return empty SimpleConfig
return SimpleConfig{}
Comment on lines +185 to +202
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In sigs.k8s.io/prow/pkg/repoowners, does repoowners.SimpleConfig define its own Empty method, or is Empty promoted from the embedded Config? Does that emptiness check include Options such as no_parent_owners and auto_approve_unowned_subfolders?

💡 Result:

In sigs.k8s.io/prow/pkg/repoowners, repoowners.SimpleConfig defines its own Empty method, not promoted from the embedded Config. The method checks only the lists from the embedded Config (Approvers, Reviewers, RequiredReviewers, Labels) and ignores the Options field: func (s *SimpleConfig) Empty bool { return len(s.Approvers) == 0 && len(s.Reviewers) == 0 && len(s.RequiredReviewers) == 0 && len(s.Labels) == 0 } Thus, the emptiness check does NOT include Options such as no_parent_owners and auto_approve_unowned_subfolders. A SimpleConfig with only options set (e.g., no_parent_owners: true) and empty lists would return true from Empty.

Citations:

🏁 Script executed:

#!/bin/bash
# Check the actual code in cmd/autoowners/main.go around the flagged lines
cat -n cmd/autoowners/main.go | sed -n '135,260p'

Repository: openshift/ci-tools

Length of output: 1870

OWNERS files with only options: set are dropped during round-trip.

repoowners.SimpleConfig.Empty() checks only the approver/reviewer lists and ignores the Options field entirely. A file with just no_parent_owners: true or auto_approve_unowned_subfolders: true and no approvers/reviewers returns Empty() == true, causing valid configuration to be lost on read-write cycles.

This affects both the serialization gate at lines 178–195 and the fallback logic at lines 232–241.

Also applies to: 239-241

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@cmd/autoowners/main.go` around lines 178 - 195, The code treats a
SimpleConfig with only Options set as empty because
repoowners.SimpleConfig.Empty() ignores Options; update the logic so Options are
considered non-empty: either modify repoowners.SimpleConfig.Empty() to return
false when Options (e.g., no_parent_owners, auto_approve_unowned_subfolders) are
set, or change the branch in main.go (the block constructing sc and the fallback
that returns SimpleConfig{}) to check r.simpleConfig.Options (or a helper like
SimpleConfig.HasOptions()) in addition to approver/reviewer lists before
treating the config as empty; ensure references to SimpleConfig.Empty(),
r.simpleConfig, sc, and the fallback return are updated accordingly.

}

type FileGetter interface {
Expand Down Expand Up @@ -208,18 +236,31 @@ func getOwnersHTTP(fg FileGetter, orgRepo orgRepo, filenames ownersconfig.Filena
switch filename {
case filenames.Owners:
httpResult.ownersFileExists = true
// Try to load as SimpleConfig first (this works for all valid OWNERS files)
simple, err := repoowners.LoadSimpleConfig(data)
if err != nil {
logrus.WithError(err).Error("Unable to load simple config.")
return httpResult, err
}
httpResult.simpleConfig = simple
if httpResult.simpleConfig.Empty() {
full, err := repoowners.LoadFullConfig(data)
if err != nil {
logrus.WithError(err).Error("Unable to load full config.")
return httpResult, err

// If SimpleConfig is not empty, store it for top-level approvers/reviewers
if !simple.Empty() {
httpResult.simpleConfig = simple
}

// Also try to load as FullConfig to check for filters
full, err := repoowners.LoadFullConfig(data)
if err != nil {
// If FullConfig fails but we have SimpleConfig, that's OK
if !simple.Empty() {
Comment on lines +254 to +255
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When repoowners.LoadFullConfig fails and simple is non-empty, the error is currently swallowed without any log/visibility. This can hide broken/misindented filters: blocks and lead to silently ignoring intended filter rules. Consider logging a warning/debug message in this branch indicating that FullConfig parsing failed and filters (if any) will be ignored, while continuing with SimpleConfig.

Suggested change
// If FullConfig fails but we have SimpleConfig, that's OK
if !simple.Empty() {
// If FullConfig fails but we have SimpleConfig, continue with it but
// make the degraded behavior visible because any filters will be ignored.
if !simple.Empty() {
logrus.WithError(err).Warn("Unable to load full config; continuing with simple config and ignoring filters, if any.")

Copilot uses AI. Check for mistakes.
break
}
logrus.WithError(err).Error("Unable to load full config.")
return httpResult, err
Comment on lines +252 to +259
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On LoadFullConfig failure, the code silently falls back to SimpleConfig when simple is non-empty (break without even a warning). This can mask malformed filters: sections in mixed OWNERS files and reintroduce the “filters ignored” outcome in an error scenario. Consider returning the error (or at least logging a warning and only falling back when you’ve confirmed the file has no filters: key).

Copilot uses AI. Check for mistakes.
Comment on lines +252 to +259
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In sigs.k8s.io/prow/pkg/repoowners, how do LoadSimpleConfig and LoadFullConfig behave for an OWNERS file with valid top-level approvers/reviewers but an invalid filters section? Does LoadSimpleConfig ignore the filters key while LoadFullConfig returns an error?

💡 Result:

Yes, LoadSimpleConfig ignores the filters key while LoadFullConfig returns an error for an OWNERS file with valid top-level approvers/reviewers but an invalid filters section. LoadSimpleConfig unmarshals YAML bytes into a SimpleConfig struct, which has fields Options and inline Config (containing approvers, reviewers, etc.) but no filters field. Go's yaml.Unmarshal ignores unknown fields by default, so an invalid filters section (e.g., malformed YAML) would still parse the valid top-level fields successfully if the YAML is otherwise valid up to that point, though strictly invalid YAML causes unmarshal error for both. LoadFullConfig unmarshals into FullConfig, which has Options and Filters map[string]Config. An invalid filters section (malformed map or Config values) causes unmarshal to fail with a YAML syntax or type error. Both functions use plain yaml.Unmarshal without custom error handling or strict mode, returning the unmarshal error directly.

Citations:

Reject silently falling back to SimpleConfig when FullConfig parsing fails.

If an OWNERS file has valid top-level approvers/reviewers but an invalid filters: block, LoadSimpleConfig succeeds (ignoring the unknown key), while LoadFullConfig fails. The current code falls back to SimpleConfig, converting a malformed mixed config into a valid default-only file and dropping filter overrides instead of surfacing the parsing error.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@cmd/autoowners/main.go` around lines 245 - 252, Currently when
repoowners.LoadFullConfig(data) returns an error the code silently falls back to
the previously-parsed simple config if simple.Empty() is false; change this so
parsing errors from LoadFullConfig are surfaced instead of ignored: remove the
conditional fallback (the if !simple.Empty() break logic) and always treat a
non-nil err from repoowners.LoadFullConfig as a fatal parse error—log the error
with context (using logrus.WithError(err).Error with a clear message referencing
OWNERS parsing) and return the error (return httpResult, err) so malformed mixed
configs are rejected; adjust any related control flow around the full, err,
simple variables accordingly.

}

// If the file has filters, store FullConfig
if len(full.Filters) > 0 {
httpResult.fullConfig = full
}
case filenames.OwnersAliases:
Expand Down
184 changes: 184 additions & 0 deletions cmd/autoowners/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,8 @@ type fakeFileGetter struct {
aliases []byte
customOwnersAliases []byte
invalidOwners []byte
mixedOwners []byte
filtersOnly []byte
someError error
notFound error
}
Expand Down Expand Up @@ -290,6 +292,22 @@ func (fg fakeFileGetter) GetFile(org, repo, filepath, commit string) ([]byte, er
return nil, fg.notFound
}
}
if org == "org8" && repo == "repo8" {
if filepath == "OWNERS" {
return fg.mixedOwners, nil
}
if filepath == "OWNERS_ALIASES" {
return nil, fg.notFound
}
}
if org == "org9" && repo == "repo9" {
if filepath == "OWNERS" {
return fg.filtersOnly, nil
}
if filepath == "OWNERS_ALIASES" {
return nil, fg.notFound
}
}

if filepath == "CUSTOM_OWNERS" {
return fg.customOwners, nil
Expand Down Expand Up @@ -326,6 +344,30 @@ aliases:
approvers:
- @abc
- @team-a
`)
fakeMixedOwners := []byte(`---
reviewers:
- reviewer1
- reviewer2
approvers:
- approver1
- approver2

filters:
"^nightly-.*\\.yaml$":
approvers:
- nightly-approver1
- nightly-approver2
- approver1
`)
fakeFiltersOnly := []byte(`---
filters:
".*\\.go$":
approvers:
- go-approver1
- go-approver2
reviewers:
- go-reviewer1
`)
someError := fmt.Errorf("some error")
notFound := &github.FileNotFound{}
Expand All @@ -339,6 +381,8 @@ approvers:
aliases: fakeOwnersAliases,
customOwnersAliases: fakeCustomAliases,
invalidOwners: fakeInvalidOwners,
mixedOwners: fakeMixedOwners,
filtersOnly: fakeFiltersOnly,
someError: someError,
notFound: notFound,
}
Expand Down Expand Up @@ -453,6 +497,49 @@ approvers:
ownersFileExists: true,
},
},
{
description: "OWNERS with both top-level config and filters should preserve both",
given: orgRepo{
Organization: "org8",
Repository: "repo8",
},
expectedHTTPResult: httpResult{
simpleConfig: SimpleConfig{
Config: repoowners.Config{
Approvers: []string{"approver1", "approver2"},
Reviewers: []string{"reviewer1", "reviewer2"},
},
},
fullConfig: FullConfig{
Filters: map[string]repoowners.Config{
"^nightly-.*\\.yaml$": {
Approvers: []string{"nightly-approver1", "nightly-approver2", "approver1"},
},
},
},
ownersFileExists: true,
},
Comment on lines +500 to +521
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case for a mixed OWNERS config only asserts fullConfig.Filters, but it doesn’t assert that the top-level approvers/reviewers from fakeMixedOwners are preserved when using FullConfig. Given the PR’s goal (“support OWNERS files with both top-level config and filters”), the expected FullConfig should include those top-level defaults as well; otherwise the tool could still drop them when writing the synced OWNERS file.

Copilot uses AI. Check for mistakes.
expectedError: nil,
},
Comment thread
coderabbitai[bot] marked this conversation as resolved.
{
description: "OWNERS with only filters (no top-level config)",
given: orgRepo{
Organization: "org9",
Repository: "repo9",
},
expectedHTTPResult: httpResult{
fullConfig: FullConfig{
Filters: map[string]repoowners.Config{
".*\\.go$": {
Approvers: []string{"go-approver1", "go-approver2"},
Reviewers: []string{"go-reviewer1"},
},
},
},
ownersFileExists: true,
},
expectedError: nil,
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -498,6 +585,32 @@ func TestResolveOwnerAliasesCleans(t *testing.T) {
},
}},
},
{
name: "mixed config - top-level becomes .* filter",
in: httpResult{
simpleConfig: SimpleConfig{Config: repoowners.Config{
Approvers: []string{"alice", "bob"},
Reviewers: []string{"charlie"},
}},
fullConfig: FullConfig{Filters: map[string]repoowners.Config{
".*\\.go$": {Approvers: []string{"go-team"}},
}},
},
expectedResult: FullConfig{Filters: map[string]repoowners.Config{
".*": {
Approvers: []string{"hans"},
Reviewers: []string{"hans"},
RequiredReviewers: []string{"hans"},
Labels: []string{},
},
".*\\.go$": {
Approvers: []string{"hans"},
Reviewers: []string{"hans"},
RequiredReviewers: []string{"hans"},
Labels: []string{},
},
}},
},
}

for _, tc := range testCases {
Expand All @@ -507,6 +620,77 @@ func TestResolveOwnerAliasesCleans(t *testing.T) {
}
}

func TestResolveOwnerAliasesMixedConfig(t *testing.T) {
testCases := []struct {
name string
in httpResult
expectedResult interface{}
}{
{
name: "mixed config - top-level becomes .* filter",
in: httpResult{
simpleConfig: SimpleConfig{Config: repoowners.Config{
Approvers: []string{"alice", "bob"},
Reviewers: []string{"charlie"},
}},
fullConfig: FullConfig{Filters: map[string]repoowners.Config{
".*\\.go$": {Approvers: []string{"go-team"}},
}},
},
expectedResult: FullConfig{Filters: map[string]repoowners.Config{
".*": {
Approvers: []string{"alice", "bob"},
Reviewers: []string{"charlie"},
RequiredReviewers: []string{},
Labels: []string{},
},
".*\\.go$": {
Approvers: []string{"go-team"},
Reviewers: []string{"go-team"},
RequiredReviewers: []string{},
Labels: []string{},
},
}},
},
{
name: "mixed config - top-level merges with explicit .* filter",
in: httpResult{
simpleConfig: SimpleConfig{Config: repoowners.Config{
Approvers: []string{"alice", "bob"},
Reviewers: []string{"charlie"},
}},
fullConfig: FullConfig{Filters: map[string]repoowners.Config{
".*": {
Approvers: []string{"dan"},
Reviewers: []string{"eve"},
},
".*\\.go$": {Approvers: []string{"go-team"}},
}},
},
expectedResult: FullConfig{Filters: map[string]repoowners.Config{
".*": {
Approvers: []string{"alice", "bob", "dan"},
Reviewers: []string{"charlie", "eve"},
RequiredReviewers: []string{},
Labels: []string{},
},
".*\\.go$": {
Approvers: []string{"go-team"},
Reviewers: []string{"go-team"},
RequiredReviewers: []string{},
Labels: []string{},
},
}},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
assertEqual(t, tc.in.resolveOwnerAliases(noOpCleaner), tc.expectedResult)
})
}
}

func noOpCleaner(in []string) []string {
return in
}
Expand Down