ci: First-class team package maintainer review requests (193deb85) · Commits · nix / nixpkgs

ci/eval/compare/default.nix

+9 −5

Original line number	Diff line number	Diff line
		@@ -161,12 +161,13 @@ let
		);

		inherit
		(callPackage ./maintainers.nix { } {
		(callPackage ./maintainers.nix {
		changedattrs = lib.attrNames (lib.groupBy (a: a.name) changedPackagePlatformAttrs);
		changedpathsjson = touchedFilesJson;
		removedattrs = lib.attrNames (lib.groupBy (a: a.name) removedPackagePlatformAttrs);
		})
		maintainers
		users
		teams
		packages
		;
		in
		@@ -178,10 +179,12 @@ runCommand "compare"
		cmp-stats
		codeowners
		];
		maintainers = builtins.toJSON maintainers;
		users = builtins.toJSON users;
		teams = builtins.toJSON teams;
		packages = builtins.toJSON packages;
		passAsFile = [
		"maintainers"
		"users"
		"teams"
		"packages"
		];
		}
		@@ -262,6 +265,7 @@ runCommand "compare"

		done

		cp "$maintainersPath" "$out/maintainers.json"
		cp "$usersPath" "$out/maintainers.json"
		cp "$teamsPath" "$out/teams.json"
		cp "$packagesPath" "$out/packages.json"
		''

ci/eval/compare/maintainers.nix

+38 −15

Original line number	Diff line number	Diff line
		{
		lib,
		}:
		{
		changedattrs,
		changedpathsjson,
		removedattrs,
		@@ -51,15 +49,16 @@ let
		# updates to .json files.
		# TODO: Support by-name package sets.
		filenames = lib.optional (lib.length path == 1) "pkgs/by-name/${sharded (lib.head path)}/";
		# TODO: Refactor this so we can ping entire teams instead of the individual members.
		# Note that this will require keeping track of GH team IDs in "maintainers/teams.nix".
		maintainers = package.meta.maintainers or [ ];
		# meta.maintainers also contains all individual team members.
		# We only want to ping individuals if they're added individually as maintainers, not via teams.
		users = package.meta.nonTeamMaintainers or [ ];
		teams = package.meta.teams or [ ];
		}
		))
		# No need to match up packages without maintainers with their files.
		# This also filters out attributes where `packge = null`, which is the
		# case for libintl, for example.
		(lib.filter (pkg: pkg.maintainers != [ ]))
		(lib.filter (pkg: pkg.users != [ ] \|\| pkg.teams != [ ]))
		];

		relevantFilenames =
		@@ -94,20 +93,44 @@ let

		attrsWithModifiedFiles = lib.filter (pkg: anyMatchingFiles pkg.filenames) attrsWithFilenames;

		listToPing = lib.concatMap (
		userPings =
		pkg:
		map (maintainer: {
		id = maintainer.githubId;
		inherit (maintainer) github;
		type = "user";
		userId = maintainer.githubId;
		packageName = pkg.name;
		});

		teamPings =
		pkg: team:
		if team ? github then
		[
		{
		type = "team";
		teamId = team.githubId;
		packageName = pkg.name;
		dueToFiles = pkg.filenames;
		}) pkg.maintainers
		}
		]
		else
		userPings pkg team.members;

		maintainersToPing = lib.concatMap (
		pkg: userPings pkg pkg.users ++ lib.concatMap (teamPings pkg) pkg.teams
		) attrsWithModifiedFiles;

		byMaintainer = lib.groupBy (ping: toString ping.id) listToPing;
		byType = lib.groupBy (ping: ping.type) maintainersToPing;

		byUser = lib.pipe (byType.user or [ ]) [
		(lib.groupBy (ping: toString ping.userId))
		(lib.mapAttrs (_user: lib.map (pkg: pkg.packageName)))
		];
		byTeam = lib.pipe (byType.team or [ ]) [
		(lib.groupBy (ping: toString ping.teamId))
		(lib.mapAttrs (_team: lib.map (pkg: pkg.packageName)))
		];
		in
		{
		maintainers = lib.mapAttrs (_: lib.catAttrs "packageName") byMaintainer;

		packages = lib.catAttrs "packageName" listToPing;
		users = byUser;
		teams = byTeam;
		packages = lib.catAttrs "name" attrsWithModifiedFiles;
		}

ci/github-script/bot.js

+77 −11

Original line number	Diff line number	Diff line
		@@ -143,6 +143,29 @@ module.exports = async ({ github, context, core, dry }) => {
		return users[id]
		}

		// Same for teams
		const teams = {}
		function getTeam(id) {
		if (!teams[id]) {
		teams[id] = github
		.request({
		method: 'GET',
		url: '/organizations/{orgId}/team/{id}',
		// TODO: Make this work without pull_requests payloads
		orgId: context.payload.pull_request.base.user.id,
		id,
		})
		.then((resp) => resp.data)
		.catch((e) => {
		// Team may have been deleted
		if (e.status === 404) return null
		throw e
		})
		}

		return teams[id]
		}

		async function handlePullRequest({ item, stats, events }) {
		const log = (k, v) => core.info(`PR #${item.number} - ${k}: ${v}`)

		@@ -175,17 +198,49 @@ module.exports = async ({ github, context, core, dry }) => {
		})

		// Check for any human reviews other than the PR author, GitHub actions and other GitHub apps.
		// Accounts could be deleted as well, so don't count them.
		const reviews = (
		await github.paginate(github.rest.pulls.listReviews, {
		...context.repo,
		pull_number,
		})
		).filter(
		await github.graphql(
		`query($owner: String!, $repo: String!, $pr: Int!) {
		repository(owner: $owner, name: $repo) {
		pullRequest(number: $pr) {
		# Unlikely that there's ever more than 100 reviews, so let's not bother,
		# but once https://github.com/actions/github-script/issues/309 is resolved,
		# it would be easy to enable pagination.
		reviews(first: 100) {
		nodes {
		state
		user: author {
		# Only get users, no bots
		... on User {
		login
		# Set the id field in the resulting JSON to GraphQL's databaseId
		# databaseId in GraphQL-land is the same as id in REST-land
		id: databaseId
		}
		}
		onBehalfOf(first: 100) {
		nodes {
		slug
		}
		}
		}
		}
		}
		}
		}`,
		{
		owner: context.repo.owner,
		repo: context.repo.repo,
		pr: pull_number,
		},
		)
		).repository.pullRequest.reviews.nodes.filter(
		(r) =>
		r.user &&
		!r.user.login.endsWith('[bot]') &&
		r.user.type !== 'Bot' &&
		// The `... on User` makes it such that .login only exists for users,
		// but we still need to filter the others out.
		// Accounts could be deleted as well, so don't count them.
		r.user?.login &&
		// Also exclude author reviews, can't request their review in any case
		r.user.id !== pull_request.user?.id,
		)

		@@ -354,6 +409,16 @@ module.exports = async ({ github, context, core, dry }) => {
		if (e.code !== 'ENOENT') throw e
		}

		let team_maintainers = []
		try {
		team_maintainers = Object.keys(
		JSON.parse(await readFile(`${pull_number}/teams.json`, 'utf-8')),
		).map((id) => parseInt(id))
		} catch (e) {
		// Older artifacts don't have the teams.json, yet.
		if (e.code !== 'ENOENT') throw e
		}

		// We set this label earlier already, but the current PR state can be very different
		// after handleReviewers has requested reviews, so update it in this case to prevent
		// this label from flip-flopping.
		@@ -366,14 +431,15 @@ module.exports = async ({ github, context, core, dry }) => {
		pull_request,
		reviews,
		// TODO: Use maintainer map instead of the artifact.
		maintainers: Object.keys(
		user_maintainers: Object.keys(
		JSON.parse(
		await readFile(`${pull_number}/maintainers.json`, 'utf-8'),
		),
		).map((id) => parseInt(id)),
		team_maintainers,
		owners,
		getTeamMembers,
		getUser,
		getTeam,
		})
		}
		}

ci/github-script/merge.js

+1 −1

Original line number	Diff line number	Diff line
		@@ -171,7 +171,7 @@ async function handleMerge({
		async function merge() {
		if (dry) {
		core.info(`Merging #${pull_number}... (dry)`)
		return 'Merge completed (dry)'
		return ['Merge completed (dry)']
		}

		// Using GraphQL mutations instead of the REST /merge endpoint, because the latter

ci/github-script/reviewers.js

+113 −77

Original line number	Diff line number	Diff line
		@@ -6,28 +6,29 @@ async function handleReviewers({
		dry,
		pull_request,
		reviews,
		maintainers,
		user_maintainers,
		team_maintainers,
		owners,
		getTeamMembers,
		getUser,
		getTeam,
		}) {
		const pull_number = pull_request.number

		const requested_reviewers = new Set(
		pull_request.requested_reviewers.map(({ login }) => login.toLowerCase()),
		)
		log(
		'reviewers - requested_reviewers',
		Array.from(requested_reviewers).join(', '),
		)
		// Users that the PR has already reached, e.g. they've left a review or have been requested for one
		const users_reached = new Set([
		...pull_request.requested_reviewers.map(({ login }) => login.toLowerCase()),
		...reviews.map(({ user }) => user.login.toLowerCase()),
		])
		log('reviewers - users_reached', Array.from(users_reached).join(', '))

		const existing_reviewers = new Set(
		reviews.map(({ user }) => user?.login.toLowerCase()).filter(Boolean),
		)
		log(
		'reviewers - existing_reviewers',
		Array.from(existing_reviewers).join(', '),
		)
		// Same for teams
		const teams_reached = new Set([
		...pull_request.requested_teams.map(({ slug }) => slug.toLowerCase()),
		...reviews.flatMap(({ onBehalfOf }) =>
		onBehalfOf.nodes.map(({ slug }) => slug.toLowerCase()),
		),
		])
		log('reviewers - teams_reached', Array.from(teams_reached).join(', '))

		// Early sanity check, before we start making any API requests. The list of maintainers
		// does not have duplicates so the only user to filter out from this list would be the
		@@ -35,16 +36,17 @@ async function handleReviewers({
		// further down again.
		// This is to protect against huge treewides consuming all our API requests for no
		// reason.
		if (maintainers.length > 16) {
		if (user_maintainers.length + team_maintainers.length > 16) {
		core.warning('Too many potential reviewers, skipping review requests.')
		// Return a boolean on whether the "needs: reviewers" label should be set.
		return existing_reviewers.size === 0 && requested_reviewers.size === 0
		return users_reached.size === 0 && teams_reached.size === 0
		}

		const users = new Set([
		// Users that should be reached
		var users_to_reach = new Set([
		...(
		await Promise.all(
		maintainers.map(async (id) => {
		user_maintainers.map(async (id) => {
		const user = await getUser(id)
		// User may have deleted their account
		return user?.login?.toLowerCase()
		@@ -55,34 +57,15 @@ async function handleReviewers({
		.filter((handle) => handle && !handle.includes('/'))
		.map((handle) => handle.toLowerCase()),
		])
		log('reviewers - users', Array.from(users).join(', '))

		const teams = new Set(
		owners
		.map((handle) => handle.split('/'))
		.filter(([org, slug]) => org === context.repo.owner && slug)
		.map(([, slug]) => slug),
		)
		log('reviewers - teams', Array.from(teams).join(', '))

		const team_members = new Set(
		(await Promise.all(Array.from(teams, getTeamMembers)))
		.flat(1)
		.map(({ login }) => login.toLowerCase()),
		)
		log('reviewers - team_members', Array.from(team_members).join(', '))

		const new_reviewers = users
		.union(team_members)
		// We can't request a review from the author.
		.difference(new Set([pull_request.user?.login.toLowerCase()]))
		log('reviewers - new_reviewers', Array.from(new_reviewers).join(', '))

		// Filter users to repository collaborators. If they're not, they can't be requested
		// for review. In that case, they probably missed their invite to the maintainers team.
		const reviewers = (
		users_to_reach = new Set(
		(
		await Promise.all(
		Array.from(new_reviewers, async (username) => {
		Array.from(users_to_reach, async (username) => {
		// TODO: Restructure this file to only do the collaborator check for those users
		// who were not already part of a team. Being a member of a team makes them
		// collaborators by definition.
		@@ -100,31 +83,82 @@ async function handleReviewers({
		}
		}),
		)
		).filter(Boolean)
		log('reviewers - reviewers', reviewers.join(', '))
		).filter(Boolean),
		)
		log('reviewers - users_to_reach', Array.from(users_to_reach).join(', '))

		// Similar for teams
		var teams_to_reach = new Set([
		...(
		await Promise.all(
		team_maintainers.map(async (id) => {
		const team = await getTeam(id)
		// Team may have been deleted
		return team?.slug?.toLowerCase()
		}),
		)
		).filter(Boolean),
		...owners
		.map((handle) => handle.split('/'))
		.filter(
		([org, slug]) =>
		org.toLowerCase() === context.repo.owner.toLowerCase() && slug,
		)
		.map(([, slug]) => slug.toLowerCase()),
		])
		teams_to_reach = new Set(
		(
		await Promise.all(
		Array.from(teams_to_reach, async (slug) => {
		try {
		await github.rest.teams.checkPermissionsForRepoInOrg({
		org: context.repo.owner,
		team_slug: slug,
		owner: context.repo.owner,
		repo: context.repo.repo,
		})
		return slug
		} catch (e) {
		if (e.status !== 404) throw e
		core.warning(
		`PR #${pull_number}: Team ${slug} cannot be requested for review because it doesn't exist or has no repository permissions, ignoring. Probably wasn't added to the nixpkgs-maintainers team (see https://github.com/NixOS/nixpkgs/tree/master/maintainers#maintainer-teams)`,
		)
		}
		}),
		)
		).filter(Boolean),
		)
		log('reviewers - teams_to_reach', Array.from(teams_to_reach).join(', '))

		if (reviewers.length > 15) {
		if (users_to_reach.size + teams_to_reach.size > 15) {
		core.warning(
		`Too many reviewers (${reviewers.join(', ')}), skipping review requests.`,
		`Too many reviewers (users: ${Array.from(users_to_reach).join(', ')}, teams: ${Array.from(teams_to_reach).join(', ')}), skipping review requests.`,
		)
		// Return a boolean on whether the "needs: reviewers" label should be set.
		return existing_reviewers.size === 0 && requested_reviewers.size === 0
		return users_reached.size === 0 && teams_reached.size === 0
		}

		const non_requested_reviewers = new Set(reviewers)
		.difference(requested_reviewers)
		// We don't want to rerequest reviews from people who already reviewed.
		.difference(existing_reviewers)
		// We don't want to rerequest reviews from people who already reviewed or were requested
		const users_not_yet_reached = users_to_reach.difference(users_reached)
		log(
		'reviewers - users_not_yet_reached',
		Array.from(users_not_yet_reached).join(', '),
		)
		// We don't want to rerequest reviews from teams who already reviewed or were requested
		const teams_not_yet_reached = teams_to_reach.difference(teams_reached)
		log(
		'reviewers - non_requested_reviewers',
		Array.from(non_requested_reviewers).join(', '),
		'reviewers - teams_not_yet_reached',
		Array.from(teams_not_yet_reached).join(', '),
		)

		if (non_requested_reviewers.size === 0) {
		if (users_not_yet_reached.size === 0 && teams_not_yet_reached.size === 0) {
		log('Has reviewer changes', 'false (skipped)')
		} else if (dry) {
		core.info(
		`Requesting reviewers for #${pull_number}: ${Array.from(non_requested_reviewers).join(', ')} (dry)`,
		`Requesting user reviewers for #${pull_number}: ${Array.from(users_not_yet_reached).join(', ')} (dry)`,
		)
		core.info(
		`Requesting team reviewers for #${pull_number}: ${Array.from(teams_not_yet_reached).join(', ')} (dry)`,
		)
		} else {
		// We had tried the "request all reviewers at once" thing in the past, but it didn't work out:
		@@ -134,15 +168,17 @@ async function handleReviewers({
		await github.rest.pulls.requestReviewers({
		...context.repo,
		pull_number,
		reviewers,
		reviewers: users_not_yet_reached,
		team_reviewers: teams_not_yet_reached,
		})
		}

		// Return a boolean on whether the "needs: reviewers" label should be set.
		return (
		non_requested_reviewers.size === 0 &&
		existing_reviewers.size === 0 &&
		requested_reviewers.size === 0
		users_not_yet_reached.size === 0 &&
		teams_not_yet_reached.size === 0 &&
		users_reached.size === 0 &&
		teams_reached.size === 0
		)
		}