Skip to content

Commit 193deb8

Browse files
infinisilbalsoft
infinisil
and
balsoft
committed
ci: First-class team package maintainer review requests
Co-Authored-By: Alexander Bantyev <alexander.bantyev@tweag.io>
1 parent f700cee commit 193deb8

6 files changed

Lines changed: 243 additions & 109 deletions

File tree

‎ci/eval/compare/default.nix‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -161,12 +161,13 @@ let
161161
);
162162

163163
inherit
164-
(callPackage ./maintainers.nix { } {
164+
(callPackage ./maintainers.nix {
165165
changedattrs = lib.attrNames (lib.groupBy (a: a.name) changedPackagePlatformAttrs);
166166
changedpathsjson = touchedFilesJson;
167167
removedattrs = lib.attrNames (lib.groupBy (a: a.name) removedPackagePlatformAttrs);
168168
})
169-
maintainers
169+
users
170+
teams
170171
packages
171172
;
172173
in
@@ -178,10 +179,12 @@ runCommand "compare"
178179
cmp-stats
179180
codeowners
180181
];
181-
maintainers = builtins.toJSON maintainers;
182+
users = builtins.toJSON users;
183+
teams = builtins.toJSON teams;
182184
packages = builtins.toJSON packages;
183185
passAsFile = [
184-
"maintainers"
186+
"users"
187+
"teams"
185188
"packages"
186189
];
187190
}
@@ -262,6 +265,7 @@ runCommand "compare"
262265
263266
done
264267
265-
cp "$maintainersPath" "$out/maintainers.json"
268+
cp "$usersPath" "$out/maintainers.json"
269+
cp "$teamsPath" "$out/teams.json"
266270
cp "$packagesPath" "$out/packages.json"
267271
''

‎ci/eval/compare/maintainers.nix‎

Lines changed: 38 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
{
22
lib,
3-
}:
4-
{
53
changedattrs,
64
changedpathsjson,
75
removedattrs,
@@ -51,15 +49,16 @@ let
5149
# updates to .json files.
5250
# TODO: Support by-name package sets.
5351
filenames = lib.optional (lib.length path == 1) "pkgs/by-name/${sharded (lib.head path)}/";
54-
# TODO: Refactor this so we can ping entire teams instead of the individual members.
55-
# Note that this will require keeping track of GH team IDs in "maintainers/teams.nix".
56-
maintainers = package.meta.maintainers or [ ];
52+
# meta.maintainers also contains all individual team members.
53+
# We only want to ping individuals if they're added individually as maintainers, not via teams.
54+
users = package.meta.nonTeamMaintainers or [ ];
55+
teams = package.meta.teams or [ ];
5756
}
5857
))
5958
# No need to match up packages without maintainers with their files.
6059
# This also filters out attributes where `packge = null`, which is the
6160
# case for libintl, for example.
62-
(lib.filter (pkg: pkg.maintainers != [ ]))
61+
(lib.filter (pkg: pkg.users != [ ] || pkg.teams != [ ]))
6362
];
6463

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

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

97-
listToPing = lib.concatMap (
96+
userPings =
9897
pkg:
9998
map (maintainer: {
100-
id = maintainer.githubId;
101-
inherit (maintainer) github;
99+
type = "user";
100+
userId = maintainer.githubId;
102101
packageName = pkg.name;
103-
dueToFiles = pkg.filenames;
104-
}) pkg.maintainers
102+
});
103+
104+
teamPings =
105+
pkg: team:
106+
if team ? github then
107+
[
108+
{
109+
type = "team";
110+
teamId = team.githubId;
111+
packageName = pkg.name;
112+
}
113+
]
114+
else
115+
userPings pkg team.members;
116+
117+
maintainersToPing = lib.concatMap (
118+
pkg: userPings pkg pkg.users ++ lib.concatMap (teamPings pkg) pkg.teams
105119
) attrsWithModifiedFiles;
106120

107-
byMaintainer = lib.groupBy (ping: toString ping.id) listToPing;
121+
byType = lib.groupBy (ping: ping.type) maintainersToPing;
122+
123+
byUser = lib.pipe (byType.user or [ ]) [
124+
(lib.groupBy (ping: toString ping.userId))
125+
(lib.mapAttrs (_user: lib.map (pkg: pkg.packageName)))
126+
];
127+
byTeam = lib.pipe (byType.team or [ ]) [
128+
(lib.groupBy (ping: toString ping.teamId))
129+
(lib.mapAttrs (_team: lib.map (pkg: pkg.packageName)))
130+
];
108131
in
109132
{
110-
maintainers = lib.mapAttrs (_: lib.catAttrs "packageName") byMaintainer;
111-
112-
packages = lib.catAttrs "packageName" listToPing;
133+
users = byUser;
134+
teams = byTeam;
135+
packages = lib.catAttrs "name" attrsWithModifiedFiles;
113136
}

‎ci/github-script/bot.js‎

Lines changed: 77 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,29 @@ module.exports = async ({ github, context, core, dry }) => {
143143
return users[id]
144144
}
145145

146+
// Same for teams
147+
const teams = {}
148+
function getTeam(id) {
149+
if (!teams[id]) {
150+
teams[id] = github
151+
.request({
152+
method: 'GET',
153+
url: '/organizations/{orgId}/team/{id}',
154+
// TODO: Make this work without pull_requests payloads
155+
orgId: context.payload.pull_request.base.user.id,
156+
id,
157+
})
158+
.then((resp) => resp.data)
159+
.catch((e) => {
160+
// Team may have been deleted
161+
if (e.status === 404) return null
162+
throw e
163+
})
164+
}
165+
166+
return teams[id]
167+
}
168+
146169
async function handlePullRequest({ item, stats, events }) {
147170
const log = (k, v) => core.info(`PR #${item.number} - ${k}: ${v}`)
148171

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

177200
// Check for any human reviews other than the PR author, GitHub actions and other GitHub apps.
178-
// Accounts could be deleted as well, so don't count them.
179201
const reviews = (
180-
await github.paginate(github.rest.pulls.listReviews, {
181-
...context.repo,
182-
pull_number,
183-
})
184-
).filter(
202+
await github.graphql(
203+
`query($owner: String!, $repo: String!, $pr: Int!) {
204+
repository(owner: $owner, name: $repo) {
205+
pullRequest(number: $pr) {
206+
# Unlikely that there's ever more than 100 reviews, so let's not bother,
207+
# but once https://github.com/actions/github-script/issues/309 is resolved,
208+
# it would be easy to enable pagination.
209+
reviews(first: 100) {
210+
nodes {
211+
state
212+
user: author {
213+
# Only get users, no bots
214+
... on User {
215+
login
216+
# Set the id field in the resulting JSON to GraphQL's databaseId
217+
# databaseId in GraphQL-land is the same as id in REST-land
218+
id: databaseId
219+
}
220+
}
221+
onBehalfOf(first: 100) {
222+
nodes {
223+
slug
224+
}
225+
}
226+
}
227+
}
228+
}
229+
}
230+
}`,
231+
{
232+
owner: context.repo.owner,
233+
repo: context.repo.repo,
234+
pr: pull_number,
235+
},
236+
)
237+
).repository.pullRequest.reviews.nodes.filter(
185238
(r) =>
186-
r.user &&
187-
!r.user.login.endsWith('[bot]') &&
188-
r.user.type !== 'Bot' &&
239+
// The `... on User` makes it such that .login only exists for users,
240+
// but we still need to filter the others out.
241+
// Accounts could be deleted as well, so don't count them.
242+
r.user?.login &&
243+
// Also exclude author reviews, can't request their review in any case
189244
r.user.id !== pull_request.user?.id,
190245
)
191246

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

412+
let team_maintainers = []
413+
try {
414+
team_maintainers = Object.keys(
415+
JSON.parse(await readFile(`${pull_number}/teams.json`, 'utf-8')),
416+
).map((id) => parseInt(id))
417+
} catch (e) {
418+
// Older artifacts don't have the teams.json, yet.
419+
if (e.code !== 'ENOENT') throw e
420+
}
421+
357422
// We set this label earlier already, but the current PR state can be very different
358423
// after handleReviewers has requested reviews, so update it in this case to prevent
359424
// this label from flip-flopping.
@@ -366,14 +431,15 @@ module.exports = async ({ github, context, core, dry }) => {
366431
pull_request,
367432
reviews,
368433
// TODO: Use maintainer map instead of the artifact.
369-
maintainers: Object.keys(
434+
user_maintainers: Object.keys(
370435
JSON.parse(
371436
await readFile(`${pull_number}/maintainers.json`, 'utf-8'),
372437
),
373438
).map((id) => parseInt(id)),
439+
team_maintainers,
374440
owners,
375-
getTeamMembers,
376441
getUser,
442+
getTeam,
377443
})
378444
}
379445
}

‎ci/github-script/merge.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ async function handleMerge({
171171
async function merge() {
172172
if (dry) {
173173
core.info(`Merging #${pull_number}... (dry)`)
174-
return 'Merge completed (dry)'
174+
return ['Merge completed (dry)']
175175
}
176176

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

0 commit comments

Comments
 (0)