diff --git a/src/api/load.js b/src/api/load.js index f212c90..91881a8 100644 --- a/src/api/load.js +++ b/src/api/load.js @@ -4,6 +4,7 @@ import fs from "fs-extra"; import path from "path"; import {TaskReporter} from "../util/taskReporter"; +import {Graph} from "../core/graph"; import {loadGraph} from "../plugins/github/loadGraph"; import { type TimelineCredParameters, @@ -14,12 +15,15 @@ import {DEFAULT_CRED_CONFIG} from "../plugins/defaultCredConfig"; import {type Project} from "../core/project"; import {setupProjectDirectory} from "../core/project_io"; +import {loadDiscourse} from "../plugins/discourse/loadDiscourse"; +import * as NullUtil from "../util/null"; export type LoadOptions = {| +project: Project, +params: TimelineCredParameters, +sourcecredDirectory: string, - +githubToken: string, + +githubToken: string | null, + +discourseKey: string | null, |}; /** @@ -46,13 +50,48 @@ export async function load( const cacheDirectory = path.join(sourcecredDirectory, "cache"); await fs.mkdirp(cacheDirectory); - // future: support loading more plugins, and merging their graphs - const githubOptions = { - repoIds: project.repoIds, - token: githubToken, - cacheDirectory, - }; - const graph = await loadGraph(githubOptions, taskReporter); + function discourseGraph(): ?Promise { + const discourseServer = project.discourseServer; + if (discourseServer != null) { + const {serverUrl, apiUsername} = discourseServer; + if (options.discourseKey == null) { + throw new Error("Tried to load Discourse, but no Discourse key set"); + } + const discourseOptions = { + fetchOptions: {apiKey: options.discourseKey, serverUrl, apiUsername}, + cacheDirectory, + }; + return loadDiscourse(discourseOptions, taskReporter); + } + } + + function githubGraph(): ?Promise { + if (project.repoIds.length) { + if (githubToken == null) { + throw new Error("Tried to load GitHub, but no GitHub token set."); + } + const githubOptions = { + repoIds: project.repoIds, + token: githubToken, + cacheDirectory, + }; + + return loadGraph(githubOptions, taskReporter); + } + } + + // For each plugin that wants to provide a Graph, get a Promise for the + // graph. That way we can request them in parallel, via Promise.all, rather + // than blocking on the plugins sequentially. + // Since plugins often perform rate-limited IO, this may be a big performance + // improvement. + const pluginGraphPromises: Promise[] = NullUtil.filter([ + discourseGraph(), + githubGraph(), + ]); + + const pluginGraphs = await Promise.all(pluginGraphPromises); + const graph = Graph.merge(pluginGraphs); const projectDirectory = await setupProjectDirectory( project, diff --git a/src/api/load.test.js b/src/api/load.test.js index b5ba502..2e0340e 100644 --- a/src/api/load.test.js +++ b/src/api/load.test.js @@ -6,6 +6,7 @@ import path from "path"; import fs from "fs-extra"; import type {Options as LoadGraphOptions} from "../plugins/github/loadGraph"; +import type {Options as LoadDiscourseOptions} from "../plugins/discourse/loadDiscourse"; import type {Project} from "../core/project"; import { directoryForProjectId, @@ -14,7 +15,8 @@ import { } from "../core/project_io"; import {makeRepoId} from "../core/repoId"; import {defaultWeights} from "../analysis/weights"; -import {NodeAddress} from "../core/graph"; +import {NodeAddress, Graph} from "../core/graph"; +import {node} from "../core/graphTestUtil"; import {TestTaskReporter} from "../util/taskReporter"; import {load, type LoadOptions} from "./load"; import {DEFAULT_CRED_CONFIG} from "../plugins/defaultCredConfig"; @@ -25,6 +27,11 @@ jest.mock("../plugins/github/loadGraph", () => ({ })); const loadGraph: JestMockFn = (require("../plugins/github/loadGraph") .loadGraph: any); +jest.mock("../plugins/discourse/loadDiscourse", () => ({ + loadDiscourse: jest.fn(), +})); +const loadDiscourse: JestMockFn = (require("../plugins/discourse/loadDiscourse") + .loadDiscourse: any); jest.mock("../analysis/timeline/timelineCred", () => ({ TimelineCred: {compute: jest.fn()}, @@ -36,17 +43,30 @@ describe("api/load", () => { const fakeTimelineCred = deepFreeze({ toJSON: () => ({is: "fake-timeline-cred"}), }); - const fakeGraph = deepFreeze({toJSON: () => ({is: "fake-graph"})}); + const githubSentinel = node("github-sentinel"); + const githubGraph = () => new Graph().addNode(githubSentinel); + const discourseSentinel = node("discourse-sentinel"); + const discourseGraph = () => new Graph().addNode(discourseSentinel); + const combinedGraph = () => Graph.merge([githubGraph(), discourseGraph()]); beforeEach(() => { jest.clearAllMocks(); - loadGraph.mockResolvedValue(fakeGraph); + loadGraph.mockResolvedValue(githubGraph()); + loadDiscourse.mockResolvedValue(discourseGraph()); timelineCredCompute.mockResolvedValue(fakeTimelineCred); }); - const project: Project = deepFreeze({ + const discourseServerUrl = "https://example.com"; + const discourseApiUsername = "credbot"; + const project: Project = { id: "foo", repoIds: [makeRepoId("foo", "bar")], - }); + discourseServer: { + serverUrl: discourseServerUrl, + apiUsername: discourseApiUsername, + }, + }; + deepFreeze(project); const githubToken = "EXAMPLE_TOKEN"; + const discourseKey = "EXAMPLE_KEY"; const weights = defaultWeights(); // Tweaks the weights so that we can ensure we aren't overriding with default weights weights.nodeManualWeights.set(NodeAddress.empty, 33); @@ -60,6 +80,7 @@ describe("api/load", () => { githubToken, params, project, + discourseKey, }; return {options, taskReporter, sourcecredDirectory}; }; @@ -86,7 +107,22 @@ describe("api/load", () => { ); }); - it("saves the resultant graph to disk", async () => { + it("calls loadDiscourse with the right options", async () => { + const {options, taskReporter, sourcecredDirectory} = example(); + await load(options, taskReporter); + const cacheDirectory = path.join(sourcecredDirectory, "cache"); + const expectedOptions: LoadDiscourseOptions = { + fetchOptions: { + apiUsername: discourseApiUsername, + apiKey: discourseKey, + serverUrl: discourseServerUrl, + }, + cacheDirectory, + }; + expect(loadDiscourse).toHaveBeenCalledWith(expectedOptions, taskReporter); + }); + + it("saves a merged graph to disk", async () => { const {options, taskReporter, sourcecredDirectory} = example(); await load(options, taskReporter); const projectDirectory = directoryForProjectId( @@ -95,17 +131,21 @@ describe("api/load", () => { ); const graphFile = path.join(projectDirectory, "graph.json"); const graphJSON = JSON.parse(await fs.readFile(graphFile)); - expect(graphJSON).toEqual(fakeGraph.toJSON()); + const expectedJSON = combinedGraph().toJSON(); + expect(graphJSON).toEqual(expectedJSON); }); it("calls TimelineCred.compute with the right graph and options", async () => { const {options, taskReporter} = example(); await load(options, taskReporter); expect(timelineCredCompute).toHaveBeenCalledWith( - fakeGraph, + expect.anything(), params, DEFAULT_CRED_CONFIG ); + expect(timelineCredCompute.mock.calls[0][0].equals(combinedGraph())).toBe( + true + ); }); it("saves the resultant cred.json to disk", async () => { @@ -131,4 +171,54 @@ describe("api/load", () => { {type: "FINISH", taskId: "load-foo"}, ]); }); + + it("errors if a discourse server is provided without a discourse key", () => { + const {options, taskReporter} = example(); + const optionsWithoutKey = {...options, discourseKey: null}; + expect.assertions(1); + return load(optionsWithoutKey, taskReporter).catch((e) => + expect(e.message).toMatch("no Discourse key") + ); + }); + + it("errors if GitHub repoIds are provided without a GitHub token", () => { + const {options, taskReporter} = example(); + const optionsWithoutToken = {...options, githubToken: null}; + expect.assertions(1); + return load(optionsWithoutToken, taskReporter).catch((e) => + expect(e.message).toMatch("no GitHub token") + ); + }); + + it("only loads GitHub if no Discourse server set", async () => { + const {options, taskReporter, sourcecredDirectory} = example(); + const newProject = {...options.project, discourseServer: null}; + const newOptions = {...options, project: newProject, discourseKey: null}; + await load(newOptions, taskReporter); + expect(loadDiscourse).not.toHaveBeenCalled(); + const projectDirectory = directoryForProjectId( + project.id, + sourcecredDirectory + ); + const graphFile = path.join(projectDirectory, "graph.json"); + const graphJSON = JSON.parse(await fs.readFile(graphFile)); + const expectedJSON = githubGraph().toJSON(); + expect(graphJSON).toEqual(expectedJSON); + }); + + it("only loads Discourse if no GitHub repoIds set ", async () => { + const {options, taskReporter, sourcecredDirectory} = example(); + const newProject = {...options.project, repoIds: []}; + const newOptions = {...options, project: newProject, githubToken: null}; + await load(newOptions, taskReporter); + expect(loadGraph).not.toHaveBeenCalled(); + const projectDirectory = directoryForProjectId( + project.id, + sourcecredDirectory + ); + const graphFile = path.join(projectDirectory, "graph.json"); + const graphJSON = JSON.parse(await fs.readFile(graphFile)); + const expectedJSON = discourseGraph().toJSON(); + expect(graphJSON).toEqual(expectedJSON); + }); }); diff --git a/src/cli/load.js b/src/cli/load.js index 51f4297..f8e9196 100644 --- a/src/cli/load.js +++ b/src/cli/load.js @@ -110,6 +110,7 @@ const loadCommand: Command = async (args, std) => { params, sourcecredDirectory: Common.sourcecredDirectory(), githubToken, + discourseKey: null, })); // Deliberately load in serial because GitHub requests that their API not // be called concurrently diff --git a/src/cli/load.test.js b/src/cli/load.test.js index 3339cb3..1973551 100644 --- a/src/cli/load.test.js +++ b/src/cli/load.test.js @@ -7,6 +7,7 @@ import {LoggingTaskReporter} from "../util/taskReporter"; import {NodeAddress} from "../core/graph"; import {run} from "./testUtil"; import loadCommand, {help} from "./load"; +import type {LoadOptions} from "../api/load"; import {defaultWeights, toJSON as weightsToJSON} from "../analysis/weights"; import * as Common from "./common"; @@ -67,11 +68,16 @@ describe("cli/load", () => { it("calls load with a single repo", async () => { const invocation = run(loadCommand, ["foo/bar"]); - const expectedOptions = { - project: {id: "foo/bar", repoIds: [makeRepoId("foo", "bar")]}, + const expectedOptions: LoadOptions = { + project: { + id: "foo/bar", + repoIds: [makeRepoId("foo", "bar")], + discourseServer: null, + }, params: {alpha: 0.05, intervalDecay: 0.5, weights: defaultWeights()}, sourcecredDirectory: Common.sourcecredDirectory(), githubToken: fakeGithubToken, + discourseKey: null, }; expect(await invocation).toEqual({ exitCode: 0, @@ -86,11 +92,16 @@ describe("cli/load", () => { it("calls load with multiple repos", async () => { const invocation = run(loadCommand, ["foo/bar", "zoink/zod"]); - const expectedOptions = (projectId: string) => ({ - project: {id: projectId, repoIds: [stringToRepoId(projectId)]}, + const expectedOptions: (string) => LoadOptions = (projectId: string) => ({ + project: { + id: projectId, + repoIds: [stringToRepoId(projectId)], + discourseServer: null, + }, params: {alpha: 0.05, intervalDecay: 0.5, weights: defaultWeights()}, sourcecredDirectory: Common.sourcecredDirectory(), githubToken: fakeGithubToken, + discourseKey: null, }); expect(await invocation).toEqual({ exitCode: 0, @@ -118,11 +129,16 @@ describe("cli/load", () => { "--weights", weightsFile, ]); - const expectedOptions = { - project: {id: "foo/bar", repoIds: [makeRepoId("foo", "bar")]}, + const expectedOptions: LoadOptions = { + project: { + id: "foo/bar", + repoIds: [makeRepoId("foo", "bar")], + discourseServer: null, + }, params: {alpha: 0.05, intervalDecay: 0.5, weights}, sourcecredDirectory: Common.sourcecredDirectory(), githubToken: fakeGithubToken, + discourseKey: null, }; expect(await invocation).toEqual({ exitCode: 0, diff --git a/src/core/project.js b/src/core/project.js index 887a73c..272614d 100644 --- a/src/core/project.js +++ b/src/core/project.js @@ -12,8 +12,9 @@ export type ProjectId = string; * Right now it has an `id` (which should be unique across a user's projects) * and an array of GitHub RepoIds. * - * In the future, we will add support for more plugins (and remove the - * hardcoded GitHub support). + * In the future, instead of hardcoding support for plugins like GitHub and Discourse, + * we will have a generic system for storing plugin-specific config, keyed by plugin + * identifier. * * We may add more fields (e.g. a description) to this object in the futre. * @@ -24,6 +25,10 @@ export type ProjectId = string; export type Project = {| +id: ProjectId, +repoIds: $ReadOnlyArray, + +discourseServer: {| + +serverUrl: string, + +apiUsername: string, + |} | null, |}; const COMPAT_INFO = {type: "sourcecred/project", version: "0.1.0"}; diff --git a/src/core/project.test.js b/src/core/project.test.js index 4baf6d4..7605019 100644 --- a/src/core/project.test.js +++ b/src/core/project.test.js @@ -17,10 +17,12 @@ describe("core/project", () => { const p1: Project = deepFreeze({ id: "foo/bar", repoIds: [foobar], + discourseServer: null, }); const p2: Project = deepFreeze({ id: "@foo", repoIds: [foobar, foozod], + discourseServer: {serverUrl: "https://example.com", apiUsername: "credbot"}, }); describe("to/fro JSON", () => { it("round trip is identity", () => { diff --git a/src/core/project_io.test.js b/src/core/project_io.test.js index 1be7655..8681014 100644 --- a/src/core/project_io.test.js +++ b/src/core/project_io.test.js @@ -22,10 +22,12 @@ describe("core/project_io", () => { const p1: Project = deepFreeze({ id: "foo/bar", repoIds: [foobar], + discourseServer: null, }); const p2: Project = deepFreeze({ id: "@foo", repoIds: [foobar, foozod], + discourseServer: {serverUrl: "https://example.com", apiUsername: "credbot"}, }); it("setupProjectDirectory results in a loadable project", async () => { diff --git a/src/plugins/github/specToProject.js b/src/plugins/github/specToProject.js index 401488f..5c5f2e2 100644 --- a/src/plugins/github/specToProject.js +++ b/src/plugins/github/specToProject.js @@ -36,12 +36,17 @@ export async function specToProject( const project: Project = { id: spec, repoIds: [stringToRepoId(spec)], + discourseServer: null, }; return project; } else if (spec.match(ownerSpecMatcher)) { const owner = spec.slice(1); const org = await fetchGithubOrg(owner, token); - const project: Project = {id: spec, repoIds: org.repos}; + const project: Project = { + id: spec, + repoIds: org.repos, + discourseServer: null, + }; return project; } throw new Error(`invalid spec: ${spec}`); diff --git a/src/plugins/github/specToProject.test.js b/src/plugins/github/specToProject.test.js index c3085c6..4a17231 100644 --- a/src/plugins/github/specToProject.test.js +++ b/src/plugins/github/specToProject.test.js @@ -2,6 +2,7 @@ import {specToProject} from "./specToProject"; import {stringToRepoId} from "../../core/repoId"; +import {type Project} from "../../core/project"; jest.mock("./fetchGithubOrg", () => ({fetchGithubOrg: jest.fn()})); type JestMockFn = $Call; const fetchGithubOrg: JestMockFn = (require("./fetchGithubOrg") @@ -13,9 +14,10 @@ describe("plugins/github/specToProject", () => { }); it("works for a repoId", async () => { const spec = "foo/bar"; - const expected = { + const expected: Project = { id: spec, repoIds: [stringToRepoId(spec)], + discourseServer: null, }; const actual = await specToProject(spec, "FAKE_TOKEN"); expect(expected).toEqual(actual); @@ -29,7 +31,7 @@ describe("plugins/github/specToProject", () => { fetchGithubOrg.mockResolvedValueOnce(fakeOrg); const actual = await specToProject(spec, token); expect(fetchGithubOrg).toHaveBeenCalledWith(fakeOrg.name, token); - const expected = {id: spec, repoIds: repos}; + const expected: Project = {id: spec, repoIds: repos, discourseServer: null}; expect(actual).toEqual(expected); }); describe("fails for malformed spec strings", () => {