PR comment for empty aliases, handle other cases.

pkg/plugins/verify-owners/verify-owners.go

@@ -276,6 +276,22 @@ func handle(ghc githubClient, gc git.ClientFactory, roc repoownersClient, log *l
 		return err
 	}
 
+	// Check if OWNERS_ALIASES has empty aliases, then add a warning comment
+	// suggesting users to not have empty aliases.
+	if len(repoAliases) > 0 {
+		numberOfEmptyAliases := 0
+		for _, owners := range repoAliases {
+			if len(owners) == 0 {
+				numberOfEmptyAliases++
+			}
+		}
+		if numberOfEmptyAliases > 0 {
+			if err := ghc.CreateComment(org, repo, number, "There are empty aliases in OWNER_ALIASES, cleanup is advised."); err != nil {
+				return err
+			}
+		}
+	}
+
 	// Check if OWNERS files have the correct config and if they do,
 	// check if all newly added owners are trusted users.
 	oc, err := roc.LoadRepoOwners(org, repo, pr.Base.Ref)

pkg/plugins/verify-owners/verify-owners_test.go

@@ -832,6 +832,11 @@ var ownersAliases = map[string][]byte{
 	"collaborators": []byte(`aliases:
   foo-reviewers:
   - alice
+`),
+	"emptyAliasGroupOwnersAliasesWithEmptyBraces": []byte(`aliases:
+  foo-reviewers:
+  - alice
+  empty-aliases-group: {}
 `),
 }
 
@@ -1435,3 +1440,109 @@ func testOwnersRemoval(clients localgit.Clients, t *testing.T) {
 func TestOwnersRemovalV2(t *testing.T) {
 	testOwnersRemoval(localgit.NewV2, t)
 }
+
+func TestHandleParseAliasesConfigWarningLabels(t *testing.T) {
+	testHandleParseAliasesConfigWarningLabels(localgit.NewV2, t)
+}
+
+func testHandleParseAliasesConfigWarningLabels(clients localgit.Clients, t *testing.T) {
+	var tests = []struct {
+		name              string
+		filesChanged      []string
+		ownersAliasesFile string
+	}{
+		{
+			name:              "empty alias group with braces in OWNERS_ALIASES file",
+			filesChanged:      []string{"OWNERS_ALIASES"},
+			ownersAliasesFile: "emptyAliasGroupOwnersAliasesWithEmptyBraces",
+		},
+	}
+
+	lg, c, err := clients()
+	if err != nil {
+		t.Fatalf("Making localgit: %v", err)
+	}
+	defer func() {
+		if err := lg.Clean(); err != nil {
+			t.Fatalf("Cleaning up localgit: %v", err)
+		}
+		if err := c.Clean(); err != nil {
+			t.Fatalf("Cleaning up client: %v", err)
+		}
+	}()
+	if err := lg.MakeFakeRepo("org", "repo"); err != nil {
+		t.Fatalf("Making fake repo: %v", err)
+	}
+	for i, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			pr := i + 1
+			// make sure we're on master before branching
+			if err := lg.Checkout("org", "repo", defaultBranch); err != nil {
+				t.Fatalf("Switching to master branch: %v", err)
+			}
+			if err := lg.CheckoutNewBranch("org", "repo", fmt.Sprintf("pull/%d/head", pr)); err != nil {
+				t.Fatalf("Checking out pull branch: %v", err)
+			}
+			pullFiles := map[string][]byte{}
+
+			for _, file := range test.filesChanged {
+				if strings.Contains(file, "OWNERS_ALIASES") {
+					pullFiles[file] = ownersAliases[test.ownersAliasesFile]
+				}
+			}
+
+			if err := lg.AddCommit("org", "repo", pullFiles); err != nil {
+				t.Fatalf("Adding PR commit: %v", err)
+			}
+			sha, err := lg.RevParse("org", "repo", "HEAD")
+			if err != nil {
+				t.Fatalf("Getting commit SHA: %v", err)
+			}
+			pre := &github.PullRequestEvent{
+				PullRequest: github.PullRequest{
+					User: github.User{Login: "author"},
+					Base: github.PullRequestBranch{
+						Ref: defaultBranch,
+					},
+					Head: github.PullRequestBranch{
+						SHA: sha,
+					},
+				},
+			}
+			fghc := newFakeGitHubClient(emptyPatch(test.filesChanged), nil, pr)
+
+			fghc.PullRequests = map[int]*github.PullRequest{}
+			fghc.PullRequests[pr] = &github.PullRequest{
+				Base: github.PullRequestBranch{
+					Ref: fakegithub.TestRef,
+				},
+			}
+
+			froc := makeFakeRepoOwnersClient()
+
+			prInfo := info{
+				org:          "org",
+				repo:         "repo",
+				repoFullName: "org/repo",
+				number:       pr,
+			}
+
+			if err := handle(fghc, c, froc, logrus.WithField("plugin", PluginName), &pre.PullRequest, prInfo, []string{labels.Approved, labels.LGTM}, plugins.Trigger{}, true, &fakePruner{}, ownersconfig.FakeResolver); err != nil {
+				t.Fatalf("Found errors when no error was expected")
+			} else {
+				// Check if the PR created had empty alias comment comment.
+				expectedComment := "There are empty aliases in OWNER_ALIASES, cleanup is advised."
+				found := false
+				for _, comment := range fghc.IssueComments[pr] {
+					if comment.Body == expectedComment {
+						found = true
+						break
+					}
+				}
+				if !found {
+					t.Fatalf("Expected comment not found in PR %d", pr)
+				}
+			}
+		})
+	}
+}

pkg/repoowners/repoowners.go

@@ -663,14 +663,41 @@ func ParseAliasesConfig(b []byte) (RepoAliases, error) {
 	result := make(RepoAliases)
 
 	config := &struct {
-		Data map[string][]string `json:"aliases,omitempty"`
+		Data map[string]interface{} `json:"aliases,omitempty"`
 	}{}
 	if err := yaml.Unmarshal(b, config); err != nil {
 		return result, err
 	}
 
 	for alias, expanded := range config.Data {
-		result[github.NormLogin(alias)] = NormLogins(expanded)
+		switch v := expanded.(type) {
+		case []interface{}:
+			// Convert []interface{} to []string
+			var members []string
+			for _, member := range v {
+				memberAsString, ok := member.(string)
+				if !ok {
+					return result, fmt.Errorf("unexpected type for alias group member: %T", member)
+				}
+				members = append(members, memberAsString)
+			}
+			result[github.NormLogin(alias)] = NormLogins(members)
+		case string:
+			// Alias group must contain a list of members.
+			return result, fmt.Errorf("alias group '%s' must contain a list of members", alias)
+		case map[string]interface{}:
+			// Handle Flow Style Mapping (Inline Dictionary/Object Syntax). Example - aliases: { alias-group: { alias1, alias2 } }
+			var members []string
+			for key := range v {
+				members = append(members, key)
+			}
+			result[github.NormLogin(alias)] = NormLogins(members)
+		case nil:
+			// Handle empty alias group as an empty list. Examples - aliases: { alias-group: }
+			result[github.NormLogin(alias)] = sets.New[string]()
+		default:
+			return result, fmt.Errorf("unexpected type for alias group: %T", v)
+		}
 	}
 	return result, nil
 }

pkg/repoowners/repoowners_test.go

@@ -1427,3 +1427,105 @@ func TestRepoOwners_AllReviewers(t *testing.T) {
 		t.Errorf("Expected reviewers: %v\tFound reviewers: %v ", expectedReviewers, sets.List(foundReviewers))
 	}
 }
+
+func TestParseAliasesConfig(t *testing.T) {
+	tests := []struct {
+		name              string
+		input             string
+		expected          RepoAliases
+		wantErr           bool
+		errContainsRegexp string
+	}{
+		{
+			name: "valid aliases",
+			input: `
+aliases:
+  team1:
+    - alice
+    - bob
+  team2:
+    - nikhilnishad
+`,
+			expected: RepoAliases{
+				"team1": sets.New[string]("alice", "bob"),
+				"team2": sets.New[string]("nikhilnishad"),
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid aliases in flow style mapping syntax",
+			input: `
+aliases:
+  team1: {alice, bob}
+`,
+			expected: RepoAliases{
+				"team1": sets.New[string]("alice", "bob"),
+			},
+			wantErr: false,
+		},
+		{
+			name: "empty alias group with braces",
+			input: `
+aliases:
+  empty-alias-group-with-braces: {}
+`,
+			expected: RepoAliases{
+				"empty-alias-group-with-braces": sets.New[string](),
+			},
+			wantErr: false,
+		},
+		{
+			name: "empty alias group",
+			input: `
+aliases:
+  empty-alias-group:
+`,
+			expected: RepoAliases{
+				"empty-alias-group": sets.New[string](),
+			},
+			wantErr: false,
+		},
+		{
+			name: "incorrect indentation in alias groups",
+			input: `
+aliases:
+  alias-group-1:
+    - user1
+    alias-group-2:
+    - user2
+`,
+			expected:          RepoAliases{},
+			wantErr:           true,
+			errContainsRegexp: "error converting YAML",
+		},
+		{
+			name: "incorrect representation of alias group list",
+			input: `
+aliases:
+  alias-group-1: ""
+`,
+			expected:          RepoAliases{},
+			wantErr:           true,
+			errContainsRegexp: "must contain a list of members",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ParseAliasesConfig([]byte(tt.input))
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ParseAliasesConfig() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if err != nil {
+				matched, _ := regexp.MatchString(tt.errContainsRegexp, err.Error())
+				if !matched {
+					t.Errorf("ParseAliasesConfig() error = %v, expected to contain %v", err, tt.errContainsRegexp)
+				}
+			}
+			if !reflect.DeepEqual(got, tt.expected) {
+				t.Errorf("ParseAliasesConfig() = %v, want %v", got, tt.expected)
+			}
+		})
+	}
+}