From 6b171f6ad63fee5ca98efa3ea818b8b60080ea7a Mon Sep 17 00:00:00 2001 From: Christopher Mancini Date: Thu, 23 Apr 2026 11:05:38 -0400 Subject: [PATCH 1/3] fix: support OWNERS files with both top-level config and filters The autoowners tool previously only checked for filters if the top-level SimpleConfig was empty. This caused OWNERS files with both top-level approvers/reviewers AND filters to have their filters silently ignored. This change modifies the logic to: 1. Always attempt to load the file as FullConfig first 2. Use FullConfig if filters are present 3. Fall back to SimpleConfig only if no filters exist This allows repository OWNERS files to define both default approvers/reviewers at the top level and filter-specific overrides for different file patterns, which is a valid and useful configuration pattern. Added test coverage for the mixed config scenario to prevent regression. Fixes: DPTP-XXXX --- cmd/autoowners/main.go | 17 +++++++++------ cmd/autoowners/main_test.go | 43 +++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/cmd/autoowners/main.go b/cmd/autoowners/main.go index b01ee0b4c64..d97634479fe 100644 --- a/cmd/autoowners/main.go +++ b/cmd/autoowners/main.go @@ -208,19 +208,22 @@ func getOwnersHTTP(fg FileGetter, orgRepo orgRepo, filenames ownersconfig.Filena switch filename { case filenames.Owners: httpResult.ownersFileExists = true - simple, err := repoowners.LoadSimpleConfig(data) + // Try to load as FullConfig first to check for filters + full, err := repoowners.LoadFullConfig(data) if err != nil { - logrus.WithError(err).Error("Unable to load simple config.") + logrus.WithError(err).Error("Unable to load full config.") return httpResult, err } - httpResult.simpleConfig = simple - if httpResult.simpleConfig.Empty() { - full, err := repoowners.LoadFullConfig(data) + // If the file has filters, use FullConfig; otherwise use SimpleConfig + if len(full.Filters) > 0 { + httpResult.fullConfig = full + } else { + simple, err := repoowners.LoadSimpleConfig(data) if err != nil { - logrus.WithError(err).Error("Unable to load full config.") + logrus.WithError(err).Error("Unable to load simple config.") return httpResult, err } - httpResult.fullConfig = full + httpResult.simpleConfig = simple } case filenames.OwnersAliases: aliases, err := repoowners.ParseAliasesConfig(data) diff --git a/cmd/autoowners/main_test.go b/cmd/autoowners/main_test.go index b1411e997f2..c9469621c77 100644 --- a/cmd/autoowners/main_test.go +++ b/cmd/autoowners/main_test.go @@ -229,6 +229,7 @@ type fakeFileGetter struct { aliases []byte customOwnersAliases []byte invalidOwners []byte + mixedOwners []byte someError error notFound error } @@ -290,6 +291,14 @@ 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 filepath == "CUSTOM_OWNERS" { return fg.customOwners, nil @@ -326,6 +335,21 @@ aliases: approvers: - @abc - @team-a +`) + fakeMixedOwners := []byte(`--- +reviewers: + - reviewer1 + - reviewer2 +approvers: + - approver1 + - approver2 + +filters: + "^nightly-.*\\.yaml$": + approvers: + - nightly-approver1 + - nightly-approver2 + - approver1 `) someError := fmt.Errorf("some error") notFound := &github.FileNotFound{} @@ -339,6 +363,7 @@ approvers: aliases: fakeOwnersAliases, customOwnersAliases: fakeCustomAliases, invalidOwners: fakeInvalidOwners, + mixedOwners: fakeMixedOwners, someError: someError, notFound: notFound, } @@ -453,6 +478,24 @@ approvers: ownersFileExists: true, }, }, + { + description: "OWNERS with both top-level config and filters should use FullConfig", + given: orgRepo{ + Organization: "org8", + Repository: "repo8", + }, + expectedHTTPResult: httpResult{ + fullConfig: FullConfig{ + Filters: map[string]repoowners.Config{ + "^nightly-.*\\.yaml$": { + Approvers: []string{"nightly-approver1", "nightly-approver2", "approver1"}, + }, + }, + }, + ownersFileExists: true, + }, + expectedError: nil, + }, } for _, tc := range testCases { From 62b1fa4ac14619e0a3900cd82f6b335067e4f0e7 Mon Sep 17 00:00:00 2001 From: Christopher Mancini Date: Thu, 23 Apr 2026 11:58:22 -0400 Subject: [PATCH 2/3] fix: handle all OWNERS file formats correctly Address CodeRabbit feedback to fix edge cases and regressions: 1. **Fallback to SimpleConfig when FullConfig fails**: Previously, if LoadFullConfig failed on a pure SimpleConfig file, the entire parsing would fail. Now we try SimpleConfig first (which works for all valid OWNERS files), then also try FullConfig to extract filters. 2. **Preserve both top-level config AND filters**: When an OWNERS file has both top-level approvers/reviewers AND filters, we now preserve both in httpResult. Previously only one would be kept. 3. **Merge top-level into FullConfig output**: When writing OWNERS files with both top-level and filters, we convert the top-level config into a ".*" catch-all filter, matching Prow's FullConfig format. This ensures files not matched by specific filters fall back to top-level approvers/reviewers. 4. **Comprehensive test coverage**: Added tests for: - Pure SimpleConfig files (no filters) - Pure FullConfig files (only filters, no top-level) - Mixed files (both top-level and filters) - Verify resolveOwnerAliases correctly merges both into FullConfig These changes ensure autoowners handles all valid OWNERS file formats without data loss or parsing failures. --- cmd/autoowners/main.go | 77 ++++++++++++++++++++++++++----------- cmd/autoowners/main_test.go | 72 +++++++++++++++++++++++++++++++++- 2 files changed, 125 insertions(+), 24 deletions(-) diff --git a/cmd/autoowners/main.go b/cmd/autoowners/main.go index d97634479fe..8e3da301537 100644 --- a/cmd/autoowners/main.go +++ b/cmd/autoowners/main.go @@ -139,25 +139,29 @@ 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{ + // 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, + } + + // If we also have top-level config, add it as a ".*" catch-all filter + // (unless a ".*" filter already exists in fullConfig) + if !r.simpleConfig.Empty() && r.fullConfig.Filters[".*"].Approvers == nil { + 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...)), - }, - Options: r.simpleConfig.Options, - } - if len(sc.Reviewers) == 0 { - sc.Reviewers = sc.Approvers - } - return sc - } else { - fc := FullConfig{ - Filters: map[string]repoowners.Config{}, - Options: r.fullConfig.Options, + } + if len(topLevelCfg.Reviewers) == 0 { + topLevelCfg.Reviewers = topLevelCfg.Approvers + } + fc.Filters[".*"] = topLevelCfg } + + // Add all the specific filters from fullConfig for k, v := range r.fullConfig.Filters { cfg := repoowners.Config{ Approvers: cleaner(sets.List(r.repoAliases.ExpandAliases(repoowners.NormLogins(v.Approvers)))), @@ -171,7 +175,24 @@ func (r httpResult) resolveOwnerAliases(cleaner ownersCleaner) interface{} { fc.Filters[k] = cfg } 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{} } type FileGetter interface { @@ -208,22 +229,32 @@ func getOwnersHTTP(fg FileGetter, orgRepo orgRepo, filenames ownersconfig.Filena switch filename { case filenames.Owners: httpResult.ownersFileExists = true - // Try to load as FullConfig first to check for filters + // 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 + } + + // 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() { + break + } logrus.WithError(err).Error("Unable to load full config.") return httpResult, err } - // If the file has filters, use FullConfig; otherwise use SimpleConfig + + // If the file has filters, store FullConfig if len(full.Filters) > 0 { httpResult.fullConfig = full - } else { - simple, err := repoowners.LoadSimpleConfig(data) - if err != nil { - logrus.WithError(err).Error("Unable to load simple config.") - return httpResult, err - } - httpResult.simpleConfig = simple } case filenames.OwnersAliases: aliases, err := repoowners.ParseAliasesConfig(data) diff --git a/cmd/autoowners/main_test.go b/cmd/autoowners/main_test.go index c9469621c77..96229696ae1 100644 --- a/cmd/autoowners/main_test.go +++ b/cmd/autoowners/main_test.go @@ -230,6 +230,7 @@ type fakeFileGetter struct { customOwnersAliases []byte invalidOwners []byte mixedOwners []byte + filtersOnly []byte someError error notFound error } @@ -299,6 +300,14 @@ func (fg fakeFileGetter) GetFile(org, repo, filepath, commit string) ([]byte, er 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 @@ -350,6 +359,15 @@ filters: - 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{} @@ -364,6 +382,7 @@ filters: customOwnersAliases: fakeCustomAliases, invalidOwners: fakeInvalidOwners, mixedOwners: fakeMixedOwners, + filtersOnly: fakeFiltersOnly, someError: someError, notFound: notFound, } @@ -479,12 +498,18 @@ filters: }, }, { - description: "OWNERS with both top-level config and filters should use FullConfig", + 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$": { @@ -496,6 +521,25 @@ filters: }, expectedError: nil, }, + { + 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 { @@ -541,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 { From ff361349d1d1ee17769d6477cd3184950a6dd1e0 Mon Sep 17 00:00:00 2001 From: Christopher Mancini Date: Wed, 29 Apr 2026 14:55:44 -0400 Subject: [PATCH 3/3] Fix edge case: merge top-level config with explicit ".*" filter Previously, if an OWNERS file had both top-level approvers/reviewers AND an explicit ".*" filter, the top-level config was silently dropped because the check `r.fullConfig.Filters[".*"].Approvers == nil` would skip the merge. Changes: - Process all filters from fullConfig first - Then merge top-level config into ".*" filter (creating or merging) - Use set union to combine approvers/reviewers/labels when merging Added comprehensive test coverage: - Test case for top-level + non-".*" filters (becomes ".*") - Test case for top-level + explicit ".*" filter (merge case) - Tests use noOpCleaner to verify actual values (not hidden by cleaner) Co-Authored-By: Claude Sonnet 4.5 --- cmd/autoowners/main.go | 39 +++++++++++--------- cmd/autoowners/main_test.go | 71 +++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 16 deletions(-) diff --git a/cmd/autoowners/main.go b/cmd/autoowners/main.go index 8e3da301537..d9e5613eee0 100644 --- a/cmd/autoowners/main.go +++ b/cmd/autoowners/main.go @@ -146,22 +146,7 @@ func (r httpResult) resolveOwnerAliases(cleaner ownersCleaner) interface{} { Options: r.fullConfig.Options, } - // If we also have top-level config, add it as a ".*" catch-all filter - // (unless a ".*" filter already exists in fullConfig) - if !r.simpleConfig.Empty() && r.fullConfig.Filters[".*"].Approvers == nil { - 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 - } - fc.Filters[".*"] = topLevelCfg - } - - // Add all the specific filters from fullConfig + // 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)))), @@ -174,6 +159,28 @@ 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 diff --git a/cmd/autoowners/main_test.go b/cmd/autoowners/main_test.go index 96229696ae1..2eebcb884e5 100644 --- a/cmd/autoowners/main_test.go +++ b/cmd/autoowners/main_test.go @@ -620,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 }