From 0882bec330561e14a04fd30fa577e9ef81a0b8b4 Mon Sep 17 00:00:00 2001
From: bri becker <39387238+bbckr@users.noreply.github.com>
Date: Tue, 7 Nov 2023 03:21:04 -0600
Subject: [PATCH] feat(pr): expandCodeOwnersGroups (#25317)
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
Co-authored-by: Rhys Arkins <rhys@arkins.net>
---
docs/usage/configuration-options.md | 5 ++
lib/config/options/index.ts | 8 +++
lib/config/types.ts | 1 +
lib/modules/platform/gitlab/http.ts | 13 +++-
lib/modules/platform/gitlab/index.spec.ts | 64 +++++++++++++++++++
lib/modules/platform/gitlab/index.ts | 42 +++++++++++-
lib/modules/platform/types.ts | 1 +
lib/util/common.ts | 4 ++
lib/workers/repository/init/index.spec.ts | 4 ++
lib/workers/repository/init/index.ts | 8 +++
.../repository/update/pr/participants.spec.ts | 61 ++++++++++++++++++
.../repository/update/pr/participants.ts | 19 ++++--
12 files changed, 220 insertions(+), 10 deletions(-)
diff --git a/docs/usage/configuration-options.md b/docs/usage/configuration-options.md
index dd3cdf42b6..a3ed397c64 100644
--- a/docs/usage/configuration-options.md
+++ b/docs/usage/configuration-options.md
@@ -1224,6 +1224,11 @@ Example:
The above would mean Renovate would not include files matching the above glob pattern in the commit, even if it thinks they should be updated.
+## expandCodeOwnersGroups
+
+If configured, Renovate will expand any matching `CODEOWNERS` groups into a full list of group members and assign them individually instead of the group.
+This is particularly useful when combined with `assigneesSampleSize` and `assigneesFromCodeOwners`, so that only a subset of the Codeowners are assigned instead of the whole group.
+
## extends
See [shareable config presets](./config-presets.md) for details.
diff --git a/lib/config/options/index.ts b/lib/config/options/index.ts
index ccdef64f89..8c4869252f 100644
--- a/lib/config/options/index.ts
+++ b/lib/config/options/index.ts
@@ -2094,6 +2094,14 @@ const options: RenovateOptions[] = [
type: 'boolean',
default: false,
},
+ {
+ name: 'expandCodeOwnersGroups',
+ description:
+ 'Expand the configured code owner groups into a full list of group members.',
+ type: 'boolean',
+ default: false,
+ supportedPlatforms: ['gitlab'],
+ },
{
name: 'assigneesSampleSize',
description: 'Take a random sample of given size from `assignees`.',
diff --git a/lib/config/types.ts b/lib/config/types.ts
index 0c4ef22649..f9732cba2f 100644
--- a/lib/config/types.ts
+++ b/lib/config/types.ts
@@ -275,6 +275,7 @@ export interface AllConfig
export interface AssigneesAndReviewersConfig {
assigneesFromCodeOwners?: boolean;
+ expandCodeOwnersGroups?: boolean;
assignees?: string[];
assigneesSampleSize?: number;
ignoreReviewers?: string[];
diff --git a/lib/modules/platform/gitlab/http.ts b/lib/modules/platform/gitlab/http.ts
index 07cbd04ed9..54a5555be8 100644
--- a/lib/modules/platform/gitlab/http.ts
+++ b/lib/modules/platform/gitlab/http.ts
@@ -10,14 +10,23 @@ export async function getUserID(username: string): Promise<number> {
).body[0].id;
}
-export async function getMemberUserIDs(group: string): Promise<number[]> {
+async function getMembers(group: string): Promise<GitLabUser[]> {
const groupEncoded = encodeURIComponent(group);
- const members = (
+ return (
await gitlabApi.getJson<GitLabUser[]>(`groups/${groupEncoded}/members`)
).body;
+}
+
+export async function getMemberUserIDs(group: string): Promise<number[]> {
+ const members = await getMembers(group);
return members.map((u) => u.id);
}
+export async function getMemberUsernames(group: string): Promise<string[]> {
+ const members = await getMembers(group);
+ return members.map((u) => u.username);
+}
+
export async function isUserBusy(user: string): Promise<boolean> {
try {
const url = `/users/${user}/status`;
diff --git a/lib/modules/platform/gitlab/index.spec.ts b/lib/modules/platform/gitlab/index.spec.ts
index fadc5178b0..31d0cb4028 100644
--- a/lib/modules/platform/gitlab/index.spec.ts
+++ b/lib/modules/platform/gitlab/index.spec.ts
@@ -2779,4 +2779,68 @@ These updates have all been created already. Click a checkbox below to force a r
expect(filteredUsers).toEqual(['maria']);
});
});
+
+ describe('expandGroupMembers(reviewersOrAssignees)', () => {
+ it('expands group members for groups with members', async () => {
+ httpMock
+ .scope(gitlabApiHost)
+ .get('/api/v4/groups/group-a/members')
+ .reply(200, [{ username: 'maria' }, { username: 'jimmy' }])
+ .get('/api/v4/groups/group-b/members')
+ .reply(200, [{ username: 'john' }]);
+ const expandedGroupMembers = await gitlab.expandGroupMembers?.([
+ 'u@email.com',
+ '@group-a',
+ '@group-b',
+ ]);
+ expect(expandedGroupMembers).toEqual([
+ 'u@email.com',
+ 'maria',
+ 'jimmy',
+ 'john',
+ ]);
+ });
+
+ it('users are not expanded when 404', async () => {
+ httpMock
+ .scope(gitlabApiHost)
+ .get('/api/v4/groups/john/members')
+ .reply(404, { message: '404 Group Not Found' });
+ const expandedGroupMembers = await gitlab.expandGroupMembers?.(['john']);
+ expect(expandedGroupMembers).toEqual(['john']);
+ });
+
+ it('users are not expanded when non 404', async () => {
+ httpMock
+ .scope(gitlabApiHost)
+ .get('/api/v4/groups/group/members')
+ .reply(403, { message: '403 Authorization' });
+ const expandedGroupMembers = await gitlab.expandGroupMembers?.([
+ '@group',
+ ]);
+ expect(expandedGroupMembers).toEqual(['group']);
+ expect(logger.debug).toHaveBeenCalledWith(
+ expect.any(Object),
+ 'Unable to fetch group'
+ );
+ });
+
+ it('groups with no members expand into empty list', async () => {
+ httpMock
+ .scope(gitlabApiHost)
+ .get('/api/v4/groups/group-c/members')
+ .reply(200, []);
+ const expandedGroupMembers = await gitlab.expandGroupMembers?.([
+ '@group-c',
+ ]);
+ expect(expandedGroupMembers).toEqual([]);
+ });
+
+ it('includes email in final result', async () => {
+ const expandedGroupMembers = await gitlab.expandGroupMembers?.([
+ 'u@email.com',
+ ]);
+ expect(expandedGroupMembers).toEqual(['u@email.com']);
+ });
+ });
});
diff --git a/lib/modules/platform/gitlab/index.ts b/lib/modules/platform/gitlab/index.ts
index 58e8c6bf9f..ce4bd3ec53 100644
--- a/lib/modules/platform/gitlab/index.ts
+++ b/lib/modules/platform/gitlab/index.ts
@@ -18,6 +18,7 @@ import {
import { logger } from '../../../logger';
import type { BranchStatus } from '../../../types';
import { coerceArray } from '../../../util/array';
+import { noLeadingAtSymbol } from '../../../util/common';
import { parseJson } from '../../../util/common';
import * as git from '../../../util/git';
import * as hostRules from '../../../util/host-rules';
@@ -53,7 +54,13 @@ import type {
} from '../types';
import { repoFingerprint } from '../util';
import { smartTruncate } from '../utils/pr-body';
-import { getMemberUserIDs, getUserID, gitlabApi, isUserBusy } from './http';
+import {
+ getMemberUserIDs,
+ getMemberUsernames,
+ getUserID,
+ gitlabApi,
+ isUserBusy,
+} from './http';
import { getMR, updateMR } from './merge-request';
import { LastPipelineId } from './schema';
import type {
@@ -1339,3 +1346,36 @@ export async function filterUnavailableUsers(
}
return filteredUsers;
}
+
+export async function expandGroupMembers(
+ reviewersOrAssignees: string[]
+): Promise<string[]> {
+ const expandedReviewersOrAssignees: string[] = [];
+ const normalizedReviewersOrAssigneesWithoutEmails: string[] = [];
+
+ // Skip passing user emails to Gitlab API, but include them in the final result
+ for (const reviewerOrAssignee of reviewersOrAssignees) {
+ if (reviewerOrAssignee.indexOf('@') > 0) {
+ expandedReviewersOrAssignees.push(reviewerOrAssignee);
+ continue;
+ }
+
+ // Normalize the potential group names before passing to Gitlab API
+ normalizedReviewersOrAssigneesWithoutEmails.push(
+ noLeadingAtSymbol(reviewerOrAssignee)
+ );
+ }
+
+ for (const reviewerOrAssignee of normalizedReviewersOrAssigneesWithoutEmails) {
+ try {
+ const members = await getMemberUsernames(reviewerOrAssignee);
+ expandedReviewersOrAssignees.push(...members);
+ } catch (err) {
+ if (err.statusCode !== 404) {
+ logger.debug({ err, reviewerOrAssignee }, 'Unable to fetch group');
+ }
+ expandedReviewersOrAssignees.push(reviewerOrAssignee);
+ }
+ }
+ return expandedReviewersOrAssignees;
+}
diff --git a/lib/modules/platform/types.ts b/lib/modules/platform/types.ts
index ca98ae3337..28a1a699ef 100644
--- a/lib/modules/platform/types.ts
+++ b/lib/modules/platform/types.ts
@@ -229,6 +229,7 @@ export interface Platform {
initPlatform(config: PlatformParams): Promise<PlatformResult>;
filterUnavailableUsers?(users: string[]): Promise<string[]>;
commitFiles?(config: CommitFilesConfig): Promise<CommitSha | null>;
+ expandGroupMembers?(reviewersOrAssignees: string[]): Promise<string[]>;
}
export interface PlatformScm {
diff --git a/lib/util/common.ts b/lib/util/common.ts
index d7348a763c..cb1af76f3d 100644
--- a/lib/util/common.ts
+++ b/lib/util/common.ts
@@ -62,6 +62,10 @@ export function detectPlatform(
return null;
}
+export function noLeadingAtSymbol(input: string): string {
+ return input.startsWith('@') ? input.slice(1) : input;
+}
+
export function parseJson(content: string | null, filename: string): unknown {
if (!content) {
return null;
diff --git a/lib/workers/repository/init/index.spec.ts b/lib/workers/repository/init/index.spec.ts
index 4300e9231c..cd69dc65bc 100644
--- a/lib/workers/repository/init/index.spec.ts
+++ b/lib/workers/repository/init/index.spec.ts
@@ -52,6 +52,7 @@ describe('workers/repository/init/index', () => {
onboarding.checkOnboardingBranch.mockResolvedValueOnce({});
config.getRepoConfig.mockResolvedValueOnce({
filterUnavailableUsers: true,
+ expandCodeOwnersGroups: true,
});
merge.mergeRenovateConfig.mockResolvedValueOnce({});
secrets.applySecretsToConfig.mockReturnValueOnce(
@@ -61,6 +62,9 @@ describe('workers/repository/init/index', () => {
expect(logger.logger.warn).toHaveBeenCalledWith(
"Configuration option 'filterUnavailableUsers' is not supported on the current platform 'undefined'."
);
+ expect(logger.logger.warn).toHaveBeenCalledWith(
+ "Configuration option 'expandCodeOwnersGroups' is not supported on the current platform 'undefined'."
+ );
});
});
});
diff --git a/lib/workers/repository/init/index.ts b/lib/workers/repository/init/index.ts
index 77cef2b149..e67a255d9c 100644
--- a/lib/workers/repository/init/index.ts
+++ b/lib/workers/repository/init/index.ts
@@ -30,6 +30,14 @@ function warnOnUnsupportedOptions(config: RenovateConfig): void {
`Configuration option 'filterUnavailableUsers' is not supported on the current platform '${platform}'.`
);
}
+
+ if (config.expandCodeOwnersGroups && !platform.expandGroupMembers) {
+ // TODO: types (#22198)
+ const platform = GlobalConfig.get('platform')!;
+ logger.warn(
+ `Configuration option 'expandCodeOwnersGroups' is not supported on the current platform '${platform}'.`
+ );
+ }
}
export async function initRepo(
diff --git a/lib/workers/repository/update/pr/participants.spec.ts b/lib/workers/repository/update/pr/participants.spec.ts
index 04e3f148cc..8da83ebb2c 100644
--- a/lib/workers/repository/update/pr/participants.spec.ts
+++ b/lib/workers/repository/update/pr/participants.spec.ts
@@ -46,6 +46,67 @@ describe('workers/repository/update/pr/participants', () => {
expect(platform.addAssignees).toHaveBeenCalledWith(123, ['a', 'b']);
});
+ it('expands group code owners assignees', async () => {
+ codeOwners.codeOwnersForPr.mockResolvedValueOnce([
+ 'user',
+ '@group',
+ 'u@email.com',
+ ]);
+ platform.expandGroupMembers = jest
+ .fn()
+ .mockResolvedValueOnce(['u@email.com', 'user', 'group.user']);
+ await addParticipants(
+ {
+ ...config,
+ assigneesFromCodeOwners: true,
+ expandCodeOwnersGroups: true,
+ },
+ pr
+ );
+ expect(platform.expandGroupMembers).toHaveBeenCalledWith([
+ 'user',
+ '@group',
+ 'u@email.com',
+ ]);
+ expect(codeOwners.codeOwnersForPr).toHaveBeenCalledOnce();
+ expect(platform.addAssignees).toHaveBeenCalledWith(123, [
+ 'a',
+ 'b',
+ 'c',
+ 'u@email.com',
+ 'user',
+ 'group.user',
+ ]);
+ });
+
+ it('does not expand group code owners assignees when assigneesFromCodeOwners disabled', async () => {
+ codeOwners.codeOwnersForPr.mockResolvedValueOnce(['user', '@group']);
+ platform.expandGroupMembers = jest
+ .fn()
+ .mockResolvedValueOnce(['user', 'group.user']);
+ await addParticipants(config, pr);
+ expect(codeOwners.codeOwnersForPr).not.toHaveBeenCalled();
+ expect(platform.expandGroupMembers).not.toHaveBeenCalled();
+ expect(platform.addAssignees).toHaveBeenCalledWith(123, ['a', 'b', 'c']);
+ });
+
+ it('does not expand group code owners assignees when expandCodeOwnersGroups disabled', async () => {
+ codeOwners.codeOwnersForPr.mockResolvedValueOnce(['user', '@group']);
+ platform.expandGroupMembers = jest
+ .fn()
+ .mockResolvedValueOnce(['user', 'group.user']);
+ await addParticipants({ ...config, assigneesFromCodeOwners: true }, pr);
+ expect(codeOwners.codeOwnersForPr).toHaveBeenCalledOnce();
+ expect(platform.expandGroupMembers).not.toHaveBeenCalled();
+ expect(platform.addAssignees).toHaveBeenCalledWith(123, [
+ 'a',
+ 'b',
+ 'c',
+ 'user',
+ 'group',
+ ]);
+ });
+
it('supports assigneesSampleSize', async () => {
util.sampleSize.mockReturnValueOnce(['a', 'c']);
await addParticipants({ ...config, assigneesSampleSize: 2 }, pr);
diff --git a/lib/workers/repository/update/pr/participants.ts b/lib/workers/repository/update/pr/participants.ts
index 9e2c5e218a..a8ee37928f 100644
--- a/lib/workers/repository/update/pr/participants.ts
+++ b/lib/workers/repository/update/pr/participants.ts
@@ -3,14 +3,23 @@ import { GlobalConfig } from '../../../../config/global';
import type { RenovateConfig } from '../../../../config/types';
import { logger } from '../../../../logger';
import { Pr, platform } from '../../../../modules/platform';
+import { noLeadingAtSymbol } from '../../../../util/common';
import { sampleSize } from '../../../../util/sample';
import { codeOwnersForPr } from './code-owners';
async function addCodeOwners(
+ config: RenovateConfig,
assigneesOrReviewers: string[],
pr: Pr
): Promise<string[]> {
- return [...new Set(assigneesOrReviewers.concat(await codeOwnersForPr(pr)))];
+ const codeOwners = await codeOwnersForPr(pr);
+
+ const assignees =
+ config.expandCodeOwnersGroups && platform.expandGroupMembers
+ ? await platform.expandGroupMembers(codeOwners)
+ : codeOwners;
+
+ return [...new Set(assigneesOrReviewers.concat(assignees))];
}
function filterUnavailableUsers(
@@ -22,10 +31,6 @@ function filterUnavailableUsers(
: Promise.resolve(users);
}
-function noLeadingAtSymbol(input: string): string {
- return input.length && input.startsWith('@') ? input.slice(1) : input;
-}
-
function prepareParticipants(
config: RenovateConfig,
usernames: string[]
@@ -41,7 +46,7 @@ export async function addParticipants(
let assignees = config.assignees ?? [];
logger.debug(`addParticipants(pr=${pr?.number})`);
if (config.assigneesFromCodeOwners) {
- assignees = await addCodeOwners(assignees, pr);
+ assignees = await addCodeOwners(config, assignees, pr);
}
if (assignees.length > 0) {
try {
@@ -67,7 +72,7 @@ export async function addParticipants(
let reviewers = config.reviewers ?? [];
if (config.reviewersFromCodeOwners) {
- reviewers = await addCodeOwners(reviewers, pr);
+ reviewers = await addCodeOwners(config, reviewers, pr);
}
if (
is.array(config.additionalReviewers) &&
--
GitLab