diff --git a/cmd/autoowners/main.go b/cmd/autoowners/main.go index b01ee0b4c64..d9e5613eee0 100644 --- a/cmd/autoowners/main.go +++ b/cmd/autoowners/main.go @@ -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)))), @@ -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{} } type FileGetter interface { @@ -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() { + break } + logrus.WithError(err).Error("Unable to load full config.") + return httpResult, err + } + + // If the file has filters, store FullConfig + if len(full.Filters) > 0 { httpResult.fullConfig = full } case filenames.OwnersAliases: diff --git a/cmd/autoowners/main_test.go b/cmd/autoowners/main_test.go index b1411e997f2..2eebcb884e5 100644 --- a/cmd/autoowners/main_test.go +++ b/cmd/autoowners/main_test.go @@ -229,6 +229,8 @@ type fakeFileGetter struct { aliases []byte customOwnersAliases []byte invalidOwners []byte + mixedOwners []byte + filtersOnly []byte someError error notFound error } @@ -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 @@ -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{} @@ -339,6 +381,8 @@ approvers: aliases: fakeOwnersAliases, customOwnersAliases: fakeCustomAliases, invalidOwners: fakeInvalidOwners, + mixedOwners: fakeMixedOwners, + filtersOnly: fakeFiltersOnly, someError: someError, notFound: notFound, } @@ -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, + }, + 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 { @@ -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 { @@ -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 }