Skip to content

Commit

Permalink
💡 make org config issues clearer (#85)
Browse files Browse the repository at this point in the history
* This adds a new response state- bad_config- so that those impacted by
this application can more easily identify what problem their org sync is
running into.
  • Loading branch information
JoshuaTheMiller authored Aug 12, 2024
1 parent db9341e commit ac5ea5a
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 21 deletions.
35 changes: 24 additions & 11 deletions src/services/gitHub.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Octokit } from "octokit";
import { createAppAuth } from "@octokit/auth-app";
import { Config } from "../config";
import { AddMemberResponse, CopilotAddResponse, GitHubClient, GitHubId, GitHubTeamId, InstalledClient, Org, OrgInvite, OrgRoles, RemoveMemberResponse, Response } from "./gitHubTypes";
import { AddMemberResponse, CopilotAddResponse, GitHubClient, GitHubId, GitHubTeamId, InstalledClient, Org, OrgConfigResponse, OrgInvite, OrgRoles, RemoveMemberResponse, Response } from "./gitHubTypes";
import { AppConfig } from "./appConfig";
import yaml from "js-yaml";
import { throttling } from "@octokit/plugin-throttling";
Expand Down Expand Up @@ -615,7 +615,7 @@ class InstalledGitHubClient implements InstalledClient {

}

public async GetConfigurationForInstallation(): Response<OrgConfig> {
public async GetConfigurationForInstallation(): OrgConfigResponse {
// TODO: this function doesn't really belong on this class...
// i.e., it doesn't fit with a "GitHub Facade"
const getContentRequest = {
Expand All @@ -631,15 +631,17 @@ class InstalledGitHubClient implements InstalledClient {
}
catch {
return {
successful: false
successful: false,
state: "NoConfig"
}
}

const potentialFiles = filesResponse.data;

if (!Array.isArray(potentialFiles)) {
return {
successful: false
successful: false,
state: "NoConfig"
}
}

Expand All @@ -649,7 +651,9 @@ class InstalledGitHubClient implements InstalledClient {

if (onlyConfigFiles.length != 1) {
return {
successful: false
successful: false,
state: "BadConfig",
message: "Multiple configuration files are not supported at this point in time."
}
}

Expand All @@ -664,15 +668,24 @@ class InstalledGitHubClient implements InstalledClient {

if (Array.isArray(contentData) || contentData.type != "file") {
return {
successful: false
successful: false,
state: "BadConfig"
}
}

const configuration = yaml.load(Buffer.from(contentData.content, 'base64').toString()) as OrgConfigurationOptions;

return {
successful: true,
data: new OrgConfig(configuration)
try {
const configuration = yaml.load(Buffer.from(contentData.content, 'base64').toString()) as OrgConfigurationOptions;
return {
successful: true,
data: new OrgConfig(configuration)
}
}
catch {
return {
successful: false,
state: "BadConfig",
message: "Error parsing configuration- check configuration file for validity: https://github.com/cloudpups/github-teams-user-sync/blob/main/docs/OrganizationConfiguration.md"
}
}
}
}
4 changes: 2 additions & 2 deletions src/services/gitHubCache.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { CacheClient } from "../app";
import { ILogger } from "../logging";
import { AddMemberResponse, CopilotAddResponse, GitHubId, GitHubTeamId, InstalledClient, OrgInvite, OrgRoles, RemoveMemberResponse, Response } from "./gitHubTypes";
import { AddMemberResponse, CopilotAddResponse, GitHubId, GitHubTeamId, InstalledClient, OrgConfigResponse, OrgInvite, OrgRoles, RemoveMemberResponse, Response } from "./gitHubTypes";
import { OrgConfig } from "./orgConfig";

export class GitHubClientCache implements InstalledClient {
Expand Down Expand Up @@ -146,7 +146,7 @@ export class GitHubClientCache implements InstalledClient {
return this.client.AddSecurityManagerTeam(team);
}

GetConfigurationForInstallation(): Response<OrgConfig> {
GetConfigurationForInstallation(): OrgConfigResponse {
return this.client.GetConfigurationForInstallation();
}
}
15 changes: 14 additions & 1 deletion src/services/gitHubTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,19 @@ export type OrgInvite = {
GitHubUser: string
}

export type OrgConfigResponseSuccess = {
successful: true,
data: OrgConfig
}

export type OrgConfigResponseBad = {
successful: false,
state: "NoConfig" | "BadConfig",
message?: string
}

export type OrgConfigResponse = Promise<OrgConfigResponseSuccess | OrgConfigResponseBad>;

export interface InstalledClient {
GetCurrentOrgName(): string
GetCurrentRateLimit(): Promise<{ remaining: number }>
Expand All @@ -32,7 +45,7 @@ export interface InstalledClient {
RemoveTeamMemberAsync(team: GitHubTeamName, user: GitHubId): RemoveMemberResponse
UpdateTeamDetails(team: GitHubTeamName, description: string): Response
AddSecurityManagerTeam(team: GitHubTeamName): Promise<unknown>
GetConfigurationForInstallation(): Response<OrgConfig>
GetConfigurationForInstallation(): OrgConfigResponse
SetOrgRole(id: GitHubId, role: OrgRoles): Response
GetPendingOrgInvites():Response<OrgInvite[]>
CancelOrgInvite(invite:OrgInvite): Response
Expand Down
32 changes: 25 additions & 7 deletions src/services/githubSync.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// REMEMBER TO REPLACE '_' with '-' for GitHub Names! 🤦‍♂️

import e from "express";
import { Log, LogError } from "../logging";
import { AppConfig } from "./appConfig";
import { CopilotAddResponse, FailedResponse, GitHubId, InstalledClient, OrgInvite } from "./gitHubTypes";
Expand Down Expand Up @@ -167,7 +168,7 @@ async function SynchronizeGitHubTeam(installedGitHubClient: InstalledClient, tea

type ReturnTypeOfSyncOrg = {
message?: string;
status: "failed" | "completed" | "no_config";
status: "failed" | "completed" | "no_config" | "bad_config";
orgName: string;
syncedSecurityManagerTeams: string[];
orgOwnersGroup: string;
Expand Down Expand Up @@ -261,13 +262,14 @@ async function syncOrg(installedGitHubClient: InstalledClient, appConfig: AppCon
}
const setOfExistingTeams = new Set(existingTeamsResponse.data.map(t => t.Name.toUpperCase()));

const orgConfigResponse = await installedGitHubClient.GetConfigurationForInstallation();
const orgConfigResponse = await installedGitHubClient.GetConfigurationForInstallation();

const orgConfig = orgConfigResponse.successful ? orgConfigResponse.data : undefined;
const securityManagersFromOrgConfig = orgConfigResponse.successful ? orgConfigResponse.data.AdditionalSecurityManagerGroups : [];
const securityManagersDisplayNameSourceMap = orgConfigResponse.successful ? orgConfigResponse.data.DisplayNameToSourceMap : new Map<string,string>();

const securityManagerTeams = [
...appConfig.SecurityManagerTeams,
...orgConfig?.AdditionalSecurityManagerGroups ?? []
...securityManagersFromOrgConfig
];

if (securityManagerTeams.length > 0) {
Expand All @@ -278,7 +280,7 @@ async function syncOrg(installedGitHubClient: InstalledClient, appConfig: AppCon
securityManagerTeams,
setOfExistingTeams,
shortLink: appConfig.Description.ShortLink,
displayNameToSourceMap: orgConfig?.DisplayNameToSourceMap ?? new Map<string,string>()
displayNameToSourceMap: securityManagersDisplayNameSourceMap
});

if (!syncManagersResponse.Success) {
Expand All @@ -293,15 +295,31 @@ async function syncOrg(installedGitHubClient: InstalledClient, appConfig: AppCon
...response,
syncedSecurityManagerTeams: syncManagersResponse.SyncedSecurityManagerTeams
}
}
}

if (orgConfig == undefined) {
if (!orgConfigResponse.successful && orgConfigResponse.state == "NoConfig") {
return {
...response,
message: "Cannot access/fetch organization config",
status: "no_config"
}
}
else if(!orgConfigResponse.successful && orgConfigResponse.state == "BadConfig") {
return {
...response,
message: orgConfigResponse.message,
status: "bad_config"
}
}
else if (!orgConfigResponse.successful) {
return {
...response,
message: orgConfigResponse.message,
status: "bad_config"
}
}

const orgConfig = orgConfigResponse.data;

Log(JSON.stringify(orgConfig));

Expand Down

0 comments on commit ac5ea5a

Please sign in to comment.