Skip to content

Commit e4e90ff

Browse files
JoshuaTheMillerJoshuaTheMiller
authored
Improve error response for add/remove members (#68)
As title says. This doesn't solve issues with adding certain members to teams (which is typically outside the control of this bot), but it does help clarify problems faster by exposing the underlying issues with add/remove team member operations.
1 parent 9ee1cae commit e4e90ff

4 files changed

Lines changed: 96 additions & 28 deletions

File tree

‎src/services/gitHub.ts‎

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { Octokit } from "octokit";
22
import { createAppAuth } from "@octokit/auth-app";
33
import { Config } from "../config";
4-
import { GitHubClient, GitHubId, GitHubTeamId, InstalledClient, Org, OrgInvite, OrgRoles, Response } from "./gitHubTypes";
4+
import { AddMemberResponse, GitHubClient, GitHubId, GitHubTeamId, InstalledClient, Org, OrgInvite, OrgRoles, RemoveMemberResponse, Response } from "./gitHubTypes";
55
import { AppConfig } from "./appConfig";
66
import yaml from "js-yaml";
77
import { throttling } from "@octokit/plugin-throttling";
@@ -230,15 +230,15 @@ class InstalledGitHubClient implements InstalledClient {
230230
this.orgName = orgName;
231231
}
232232

233-
async AddTeamsToCopilotSubscription(teamNames: string[]): Response<string[]> {
233+
async AddTeamsToCopilotSubscription(teamNames: string[]): Response<string[]> {
234234
// Such logic should not generally go in a facade, though the convenience
235235
// and lack of actual problems makes this violation of pattern more "okay."
236-
if(teamNames.length < 1) {
236+
if (teamNames.length < 1) {
237237
return {
238238
// Should be "no op"
239239
successful: true,
240240
data: []
241-
}
241+
}
242242
}
243243

244244
try {
@@ -255,13 +255,13 @@ class InstalledGitHubClient implements InstalledClient {
255255
successful: false
256256
}
257257
}
258-
258+
259259
return {
260260
successful: true,
261261
data: teamNames
262262
}
263263
}
264-
catch(e) {
264+
catch (e) {
265265
console.log(e);
266266
// TODO: actually catch exception and investigate...
267267
return {
@@ -434,7 +434,7 @@ class InstalledGitHubClient implements InstalledClient {
434434
}
435435
}
436436

437-
public async AddTeamMember(team: GitHubTeamName, id: GitHubId): Response<unknown> {
437+
public async AddTeamMember(team: GitHubTeamName, id: GitHubId): AddMemberResponse {
438438
const safeTeam = MakeTeamNameSafeAndApiFriendly(team);
439439

440440
try {
@@ -446,13 +446,25 @@ class InstalledGitHubClient implements InstalledClient {
446446

447447
return {
448448
successful: true,
449-
// TODO: make this type better to avoid nulls...
450-
data: null
449+
team: team,
450+
user: id
451451
}
452452
}
453-
catch {
453+
catch (e) {
454+
if (e instanceof Error) {
455+
return {
456+
successful: false,
457+
team: team,
458+
user: id,
459+
message: e.message
460+
}
461+
}
462+
454463
return {
455-
successful: false
464+
successful: false,
465+
team: team,
466+
user: id,
467+
message: JSON.stringify(e)
456468
}
457469
}
458470
}
@@ -529,7 +541,7 @@ class InstalledGitHubClient implements InstalledClient {
529541
}
530542
}
531543

532-
public async RemoveTeamMemberAsync(team: GitHubTeamName, user: GitHubId): Response<unknown> {
544+
public async RemoveTeamMemberAsync(team: GitHubTeamName, user: GitHubId): RemoveMemberResponse {
533545
const safeTeam = MakeTeamNameSafeAndApiFriendly(team);
534546

535547
try {
@@ -541,13 +553,25 @@ class InstalledGitHubClient implements InstalledClient {
541553

542554
return {
543555
successful: true,
544-
// TODO: make this type better to avoid nulls...
545-
data: null
556+
team: team,
557+
user: user
546558
}
547559
}
548-
catch {
560+
catch (e) {
561+
if (e instanceof Error) {
562+
return {
563+
successful: false,
564+
team: team,
565+
user: user,
566+
message: e.message
567+
}
568+
}
569+
549570
return {
550-
successful: false
571+
successful: false,
572+
team: team,
573+
user: user,
574+
message: JSON.stringify(e)
551575
}
552576
}
553577
}

‎src/services/gitHubCache.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { CacheClient } from "../app";
22
import { ILogger } from "../logging";
3-
import { GitHubTeamId, InstalledClient, OrgInvite, OrgRoles, Response } from "./gitHubTypes";
3+
import { AddMemberResponse, GitHubId, GitHubTeamId, InstalledClient, OrgInvite, OrgRoles, RemoveMemberResponse, Response } from "./gitHubTypes";
44
import { OrgConfig } from "./orgConfig";
55

66
export class GitHubClientCache implements InstalledClient {
@@ -81,7 +81,7 @@ export class GitHubClientCache implements InstalledClient {
8181
return this.client.GetAllTeams();
8282
}
8383

84-
AddTeamMember(team: string, id: string): Response<unknown> {
84+
AddTeamMember(team: string, id: string): AddMemberResponse {
8585
return this.client.AddTeamMember(team, id);
8686
}
8787

@@ -134,7 +134,7 @@ export class GitHubClientCache implements InstalledClient {
134134
return this.client.ListCurrentMembersOfGitHubTeam(team);
135135
}
136136

137-
RemoveTeamMemberAsync(team: string, user: string): Response<unknown> {
137+
RemoveTeamMemberAsync(team: string, user: string): RemoveMemberResponse {
138138
return this.client.RemoveTeamMemberAsync(team, user);
139139
}
140140

‎src/services/gitHubTypes.ts‎

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,11 @@ export interface InstalledClient {
2525
AddOrgMember(id: GitHubId): Response
2626
IsUserMember(id: GitHubId): Response<boolean>
2727
GetAllTeams(): Response<GitHubTeamId[]>
28-
AddTeamMember(team: GitHubTeamName, id: GitHubId): Response
28+
AddTeamMember(team: GitHubTeamName, id: GitHubId): AddMemberResponse
2929
CreateTeam(teamName: GitHubTeamName, description:string): Response
3030
DoesUserExist(gitHubId: string): Response<GitHubId>
3131
ListCurrentMembersOfGitHubTeam(team: GitHubTeamName): Response<GitHubId[]>
32-
RemoveTeamMemberAsync(team: GitHubTeamName, user: GitHubId): Response
32+
RemoveTeamMemberAsync(team: GitHubTeamName, user: GitHubId): RemoveMemberResponse
3333
UpdateTeamDetails(team: GitHubTeamName, description: string): Response
3434
AddSecurityManagerTeam(team: GitHubTeamName): Promise<unknown>
3535
GetConfigurationForInstallation(): Response<OrgConfig>
@@ -41,14 +41,46 @@ export interface InstalledClient {
4141
AddTeamsToCopilotSubscription(teamNames: GitHubTeamName[]):Response<string[]>
4242
}
4343

44+
export type AddMemberResponse = Promise<AddMemberSucceeded | AddMemberFailed>
45+
46+
export type AddMemberSucceeded = {
47+
successful: true,
48+
user: GitHubId,
49+
team: GitHubTeamName
50+
}
51+
52+
export type AddMemberFailed = {
53+
successful: false,
54+
user: GitHubId,
55+
team: GitHubTeamName,
56+
message: string
57+
}
58+
59+
export type RemoveMemberResponse = Promise<RemoveMemberSucceeded | RemoveMemberFailed>
60+
61+
export type RemoveMemberSucceeded = {
62+
successful: true,
63+
user: GitHubId,
64+
team: GitHubTeamName
65+
}
66+
67+
export type RemoveMemberFailed = {
68+
successful: false,
69+
user: GitHubId,
70+
team: GitHubTeamName,
71+
message: string
72+
}
73+
74+
4475

4576
export type GenericSucceededResponse<T> = {
4677
successful: true,
4778
data: T
4879
}
4980

5081
export type FailedResponse = {
51-
successful: false
82+
successful: false,
83+
message?:string
5284
}
5385

5486
export type Response<T = unknown> = Promise<GenericSucceededResponse<T> | FailedResponse>;

‎src/services/githubSync.ts‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import { Log, LogError } from "../logging";
44
import { AppConfig } from "./appConfig";
5-
import { GitHubId, InstalledClient, OrgInvite } from "./gitHubTypes";
5+
import { FailedResponse, GitHubId, InstalledClient, OrgInvite } from "./gitHubTypes";
66
import { IGitHubInvitations } from "./githubInvitations";
77
import { SearchAllAsync } from "./ldapClient";
88
import { OrgConfig } from "./orgConfig";
@@ -131,6 +131,7 @@ async function SynchronizeGitHubTeam(installedGitHubClient: InstalledClient, tea
131131
const teamMembersToAdd = trueValidTeamMembersList.filter(m => !currentTeamMembers.find((rm) => rm == m));
132132

133133
const teamSyncNotes = {
134+
operation: "TeamSync:Prepared",
134135
teamSlug: `${orgName}/${teamName}`,
135136
toRemove: teamMembersToRemove,
136137
toAdd: teamMembersToAdd,
@@ -139,10 +140,21 @@ async function SynchronizeGitHubTeam(installedGitHubClient: InstalledClient, tea
139140

140141
Log(JSON.stringify(teamSyncNotes));
141142

142-
await Promise.all(teamMembersToRemove.map(mtr => installedGitHubClient.RemoveTeamMemberAsync(teamName, mtr)));
143-
await Promise.all(teamMembersToAdd.map(mta => installedGitHubClient.AddTeamMember(teamName, mta)));
143+
const deleteResponses = await Promise.all(teamMembersToRemove.map(mtr => installedGitHubClient.RemoveTeamMemberAsync(teamName, mtr)));
144+
const addResponses = await Promise.all(teamMembersToAdd.map(mta => installedGitHubClient.AddTeamMember(teamName, mta)));
144145

145-
return teamSyncNotes;
146+
const teamSyncCompleteNotes = {
147+
operation: "TeamSync:Completed",
148+
teamSlug: `${orgName}/${teamName}`,
149+
toRemove: teamMembersToRemove,
150+
toAdd: teamMembersToAdd,
151+
// TODO: improve clarity of issues
152+
issues: [...deleteResponses.filter(t => !t.successful), ...addResponses.filter(t => !t.successful)]
153+
}
154+
155+
Log(JSON.stringify(teamSyncCompleteNotes));
156+
157+
return teamSyncCompleteNotes;
146158
}
147159

148160
type ReturnTypeOfSyncOrg = {
@@ -306,14 +318,14 @@ async function syncOrg(installedGitHubClient: InstalledClient, appConfig: AppCon
306318

307319
async function syncOrgMembersByTeam(teamName: string, sourceTeamMap: Map<string, string>) {
308320
const sourceTeamName = sourceTeamMap.get(teamName) ?? teamName;
309-
Log(`Adding Org Members via ${sourceTeamName} membership in ${installedGitHubClient.GetCurrentOrgName()}`);
321+
Log(`Adding Org Members via ${sourceTeamName} membership in ${installedGitHubClient.GetCurrentOrgName()}`);
310322
await SynchronizeOrgMembers(installedGitHubClient, sourceTeamName, appConfig);
311323
}
312324

313325
if (orgConfig.AssumeMembershipViaTeams) {
314326
// TODO: this method is getting very busy, and most likely could benefit from a larger refactor.
315327
// Benefits most likely include performance gains.
316-
Log(`Syncing Members for ${installedGitHubClient.GetCurrentOrgName()} by individual teams.`);
328+
Log(`Syncing Members for ${installedGitHubClient.GetCurrentOrgName()} by individual teams.`);
317329
const orgMembershipPromises = gitHubTeams.map(t => syncOrgMembersByTeam(t, orgConfig.DisplayNameToSourceMap));
318330
await Promise.all(orgMembershipPromises);
319331
}

0 commit comments

Comments
 (0)