Merge pull request #4049 from github/mbg/start-proxy/job-uuid

Generate job UUID in `start-proxy` action

src/action-common.test.ts

@@ -0,0 +1,123 @@
+import * as core from "@actions/core";
+import test from "ava";
+import sinon from "sinon";
+
+import * as common from "./action-common";
+import * as actionsUtil from "./actions-util";
+import * as environment from "./environment";
+import * as logging from "./logging";
+import { ActionName } from "./status-report";
+import * as statusReport from "./status-report";
+import {
+  getTestActionsEnv,
+  getTestEnv,
+  makeMacro,
+  RecordingLogger,
+  setupTests,
+} from "./testing-utils";
+import { getErrorMessage } from "./util";
+
+setupTests(test);
+
+interface RunInActionsTestOpts {
+  runFn?: () => Promise<any>;
+  expectedErrorMessage?: string;
+  expectedTelemetryError?: string;
+}
+
+const runInActionsMacro = makeMacro({
+  exec: async (t, opts: RunInActionsTestOpts) => {
+    const expectFailure = opts?.expectedErrorMessage !== undefined;
+
+    const logger = new RecordingLogger();
+    const getActionsLogger = sinon
+      .stub(logging, "getActionsLogger")
+      .returns(logger);
+
+    const env = getTestEnv();
+    const getEnv = sinon.stub(environment, "getEnv").returns(env);
+
+    const actionsEnv = getTestActionsEnv(env);
+    const getActionsEnv = sinon
+      .stub(actionsUtil, "getActionsEnv")
+      .returns(actionsEnv);
+
+    const getJobUUID = sinon
+      .stub(statusReport, "getJobUUID")
+      .returns("test-job-uuid");
+
+    const setFailed = sinon.stub(core, "setFailed");
+    const sendUnhandledErrorStatusReport = sinon.stub(
+      statusReport,
+      "sendUnhandledErrorStatusReport",
+    );
+
+    const name = ActionName.Init;
+    const run = sinon.stub();
+
+    if (opts?.runFn) {
+      run.callsFake(opts.runFn);
+    }
+
+    const transformTelemetryError = sinon
+      .stub()
+      .callsFake((err) => opts?.expectedTelemetryError ?? getErrorMessage(err));
+    const testAction: common.Action = {
+      name,
+      run,
+      transformTelemetryError,
+    };
+
+    await common.runInActions(testAction);
+
+    // These always should have been called once.
+    t.true(getActionsLogger.calledOnce);
+    t.true(getEnv.calledOnce);
+    t.true(getActionsEnv.calledOnce);
+
+    const expectedActionState = {
+      actions: actionsEnv,
+      env,
+      logger,
+      name: ActionName.Init,
+    };
+
+    t.true(getJobUUID.calledOnceWithExactly(sinon.match(expectedActionState)));
+    t.true(run.calledOnceWithExactly(sinon.match(expectedActionState)));
+
+    t.is(setFailed.calledOnce, expectFailure ?? false);
+    t.is(sendUnhandledErrorStatusReport.calledOnce, expectFailure ?? false);
+
+    if (expectFailure) {
+      t.true(
+        setFailed.calledOnceWithExactly(
+          `${statusReport.getDisplayActionName(name)} action failed: ${opts?.expectedErrorMessage}`,
+        ),
+      );
+      t.true(
+        sendUnhandledErrorStatusReport.calledOnceWithExactly(
+          name,
+          sinon.match.any,
+          opts?.expectedTelemetryError ?? opts?.expectedErrorMessage,
+          logger,
+        ),
+      );
+    }
+  },
+  title: (providedTitle) => `runInActions - ${providedTitle}`,
+});
+
+runInActionsMacro.serial("calls run", {});
+runInActionsMacro.serial("handles run exceptions", {
+  runFn: () => {
+    throw new Error("Test failure");
+  },
+  expectedErrorMessage: "Test failure",
+});
+runInActionsMacro.serial("transforms run exceptions", {
+  runFn: () => {
+    throw new Error("Test failure");
+  },
+  expectedErrorMessage: "Test failure",
+  expectedTelemetryError: "Transformed failure message",
+});

src/action-common.ts

@@ -8,9 +8,10 @@ import { getActionsLogger, Logger } from "./logging";
 import {
   ActionName,
   getDisplayActionName,
+  getJobUUID,
   sendUnhandledErrorStatusReport,
 } from "./status-report";
-import { getEnv, getErrorMessage } from "./util";
+import { getEnv, getErrorMessage, wrapError } from "./util";
 
 /** Base state that is available to an Action on startup. */
 export interface BaseState {
@@ -78,6 +79,12 @@ export interface Action {
   name: ActionName;
   /** The entry point for the Action. */
   run: ActionMain;
+  /**
+   * An optional function that transforms a caught error into a message suitable for
+   * inclusion in a status report. This is primarily intended for the `start-proxy`
+   * action to replace the thrown `Error`'s message with a safe one.
+   */
+  transformTelemetryError?: (error: Error) => string;
 }
 
 /** A generic entry point that sets up the basic environment for the `action` and runs it. */
@@ -88,17 +95,32 @@ export async function runInActions(action: Action) {
   const actionsEnv = getActionsEnv();
 
   try {
-    await action.run({
+    const actionState = {
       name: action.name,
       startedAt,
       logger,
       env,
       actions: actionsEnv,
-    });
+    };
+
+    // Create a unique identifier for this run.
+    getJobUUID(actionState);
+
+    await action.run(actionState);
   } catch (error) {
     core.setFailed(
       `${getDisplayActionName(action.name)} action failed: ${getErrorMessage(error)}`,
     );
-    await sendUnhandledErrorStatusReport(action.name, startedAt, error, logger);
+
+    const statusReportError =
+      action.transformTelemetryError !== undefined
+        ? action.transformTelemetryError(wrapError(error))
+        : error;
+    await sendUnhandledErrorStatusReport(
+      action.name,
+      startedAt,
+      statusReportError,
+      logger,
+    );
   }
 }

src/actions-util.ts

@@ -27,14 +27,20 @@ declare const __CODEQL_ACTION_VERSION__: string;
  * global functions in tests.
  */
 export interface ActionsEnv {
+  getRequiredInput: (name: string) => string;
   getOptionalInput: (name: string) => string | undefined;
+  exportVariable: (name: string, value: string) => void;
 }
 
 /**
  * Gets the real `ActionsEnv` used by production code.
  */
 export function getActionsEnv(): ActionsEnv {
-  return { getOptionalInput };
+  return {
+    getRequiredInput,
+    getOptionalInput,
+    exportVariable: core.exportVariable,
+  };
 }
 
 /**

src/config/inputs.test.ts

@@ -1,7 +1,7 @@
 import test from "ava";
 import sinon from "sinon";
 
-import { getActionsEnv } from "../actions-util";
+import { ActionsEnv } from "../actions-util";
 import { Feature } from "../feature-flags";
 import { RepositoryPropertyName } from "../feature-flags/properties";
 import { callee } from "../testing-utils";
@@ -22,32 +22,26 @@ const expectedRepositoryPropertyResult: ComputedInput = {
   value: "repo-property-input-value",
 };
 
-function stubGetToolsInput() {
-  const actions = getActionsEnv();
+function stubGetToolsInput(actions: ActionsEnv) {
   sinon
     .stub(actions, "getOptionalInput")
     .withArgs(InputName.Tools)
     .returns(expectedWorkflowResult.value);
-  return actions;
 }
 
 const workflowLogMessage = `Using ${InputName.Tools} input from workflow:`;
 
 test("getToolsInput - returns workflow input if available", async (t) => {
-  const actions = stubGetToolsInput();
-
   await callee(getToolsInput)
-    .withActions(actions)
+    .withActions(stubGetToolsInput)
     .withArgs({})
     .logs(t, workflowLogMessage)
     .passes(t.deepEqual, expectedWorkflowResult);
 });
 
 test("getToolsInput - returns repository property value if enforced", async (t) => {
-  const actions = stubGetToolsInput();
-
   const target = callee(getToolsInput)
-    .withActions(actions)
+    .withActions(stubGetToolsInput)
     .withArgs({
       [RepositoryPropertyName.TOOLS]: `!${expectedRepositoryPropertyResult.value}`,
     });
@@ -65,10 +59,8 @@ test("getToolsInput - returns repository property value if enforced", async (t)
 });
 
 test("getToolsInput - prefers workflow input", async (t) => {
-  const actions = stubGetToolsInput();
-
   const target = callee(getToolsInput)
-    .withActions(actions)
+    .withActions(stubGetToolsInput)
     .withArgs({
       [RepositoryPropertyName.TOOLS]: expectedRepositoryPropertyResult.value,
     });

src/environment.ts

@@ -88,7 +88,7 @@ export enum EnvVar {
   LOG_VERSION_DEPRECATION = "CODEQL_ACTION_DID_LOG_VERSION_DEPRECATION",
 
   /** UUID representing the current job run. */
-  JOB_RUN_UUID = "JOB_RUN_UUID",
+  JOB_RUN_UUID = "CODEQL_ACTION_JOB_RUN_UUID",
 
   /** Status for the entire job, submitted to the status report in `init-post` */
   JOB_STATUS = "CODEQL_ACTION_JOB_STATUS",
@@ -270,6 +270,11 @@ export class ReadOnlyEnv<T extends string | undefined = string | undefined> {
     return Object.create(this, { vars: { value: { ...this.vars } } }) as this;
   }
 
+  /** Gets a copy of the underlying environment. */
+  public get(): Record<string, T> {
+    return { ...this.vars };
+  }
+
   /** Tries to get the value for `name` and throws if there isn't one. */
   public getRequired(name: string): string {
     return getRequiredEnvVar(this.vars, name);

src/init-action.ts

@@ -4,7 +4,6 @@ import * as path from "path";
 import * as core from "@actions/core";
 import * as io from "@actions/io";
 import * as semver from "semver";
-import { v4 as uuidV4 } from "uuid";
 
 import { Action, ActionState, runInActions } from "./action-common";
 import {
@@ -255,11 +254,6 @@ async function run(
     );
     const repositoryProperties = repositoryPropertiesResult.orElse({});
 
-    // Create a unique identifier for this run.
-    const jobRunUuid = uuidV4();
-    logger.info(`Job run UUID is ${jobRunUuid}.`);
-    core.exportVariable(EnvVar.JOB_RUN_UUID, jobRunUuid);
-
     core.exportVariable(EnvVar.INIT_ACTION_HAS_RUN, "true");
 
     // path.resolve() respects the intended semantics of source-root. If

src/setup-codeql-action.ts

@@ -1,5 +1,4 @@
 import * as core from "@actions/core";
-import { v4 as uuidV4 } from "uuid";
 
 import { Action, ActionState, runInActions } from "./action-common";
 import {
@@ -95,7 +94,7 @@ async function sendCompletedStatusReport(
 
 /** The main behaviour of this action. */
 async function run(
-  actionState: ActionState<["Base", "Logger", "Actions"]>,
+  actionState: ActionState<["Base", "Logger", "Env", "Actions"]>,
 ): Promise<void> {
   // To capture errors appropriately, keep as much code within the try-catch as
   // possible, and only use safe functions outside.
@@ -140,10 +139,6 @@ async function run(
 
     const actionStateWithFeatures = { ...actionState, features };
 
-    const jobRunUuid = uuidV4();
-    logger.info(`Job run UUID is ${jobRunUuid}.`);
-    core.exportVariable(EnvVar.JOB_RUN_UUID, jobRunUuid);
-
     const statusReportBase = await createStatusReportBase(
       ActionName.SetupCodeQL,
       "starting",

src/start-proxy-action.ts

@@ -3,11 +3,12 @@ import * as path from "path";
 
 import * as core from "@actions/core";
 
+import { Action, ActionState, runInActions } from "./action-common";
 import * as actionsUtil from "./actions-util";
 import { getGitHubVersion } from "./api-client";
 import { FeatureEnablement, initFeatures } from "./feature-flags";
 import { BuiltInLanguage, parseBuiltInLanguage } from "./languages";
-import { getActionsLogger, Logger } from "./logging";
+import { Logger } from "./logging";
 import { getRepositoryNwo } from "./repository";
 import {
   credentialToStr,
@@ -23,14 +24,14 @@ import {
 import { generateCertificateAuthority } from "./start-proxy/ca";
 import { checkProxyEnvironment } from "./start-proxy/environment";
 import { checkConnections } from "./start-proxy/reachability";
-import { ActionName, sendUnhandledErrorStatusReport } from "./status-report";
+import { ActionName } from "./status-report";
 import * as util from "./util";
 
-async function run(startedAt: Date) {
+async function run(action: ActionState<["Base", "Logger", "Env", "Actions"]>) {
   // To capture errors appropriately, keep as much code within the try-catch as
   // possible, and only use safe functions outside.
-
-  const logger = getActionsLogger();
+  const startedAt = action.startedAt;
+  const logger = action.logger;
   let features: FeatureEnablement | undefined;
   let language: BuiltInLanguage | undefined;
 
@@ -122,21 +123,15 @@ async function run(startedAt: Date) {
   }
 }
 
-export async function runWrapper() {
-  const startedAt = new Date();
-  const logger = getActionsLogger();
+/** Defines the `start-proxy` Action. */
+const startProxyAction: Action = {
+  name: ActionName.StartProxy,
+  run,
+  transformTelemetryError: getSafeErrorMessage,
+};
 
-  try {
-    await run(startedAt);
-  } catch (error) {
-    core.setFailed(`start-proxy action failed: ${util.getErrorMessage(error)}`);
-    await sendUnhandledErrorStatusReport(
-      ActionName.StartProxy,
-      startedAt,
-      getSafeErrorMessage(util.wrapError(error)),
-      logger,
-    );
-  }
+export async function runWrapper() {
+  await runInActions(startProxyAction);
 }
 
 async function startProxy(

src/status-report.test.ts

@@ -1,5 +1,6 @@
 import test from "ava";
 import * as sinon from "sinon";
+import * as uuid from "uuid";
 
 import * as actionsUtil from "./actions-util";
 import { Config } from "./config-utils";
@@ -12,6 +13,7 @@ import {
   createInitWithConfigStatusReport,
   createStatusReportBase,
   getActionsStatus,
+  getJobUUID,
   InitStatusReport,
   InitWithConfigStatusReport,
 } from "./status-report";
@@ -20,11 +22,49 @@ import {
   setupActionsVars,
   createTestConfig,
   makeMacro,
+  callee,
 } from "./testing-utils";
 import { BuildMode, ConfigurationError, withTmpDir, wrapError } from "./util";
 
 setupTests(test);
 
+test("getJobUUID - generates valid UUIDs", async (t) => {
+  await callee(getJobUUID)
+    .withArgs()
+    .logs(t, "Job run UUID is ")
+    .hasEnv(t, (val) => {
+      return {
+        [EnvVar.JOB_RUN_UUID]: val,
+      };
+    })
+    .passes((val) => {
+      t.true(uuid.validate(val));
+    });
+});
+
+test("getJobUUID - retrieves existing job UUIDs", async (t) => {
+  const existingJobUuid = uuid.v4();
+  await callee(getJobUUID)
+    .withArgs()
+    .withEnv((env) => {
+      env.set(EnvVar.JOB_RUN_UUID, existingJobUuid);
+    })
+    .logs(t, `Existing job run UUID is ${existingJobUuid}.`)
+    .passes(t.deepEqual, existingJobUuid);
+});
+
+test("getJobUUID - doesn't retrieve invalid UUIDs", async (t) => {
+  const existingJobUuid = "not-a-uuid";
+  await callee(getJobUUID)
+    .withArgs()
+    .withEnv((env) => {
+      env.set(EnvVar.JOB_RUN_UUID, existingJobUuid);
+    })
+    .logs(t, `Job run UUID is `)
+    .notLogs(t, `Existing job run UUID is ${existingJobUuid}.`)
+    .passes(t.notDeepEqual, existingJobUuid);
+});
+
 function setupEnvironmentAndStub(tmpDir: string) {
   setupActionsVars(tmpDir, tmpDir, {
     GITHUB_EVENT_NAME: "dynamic",

src/status-report.ts

@@ -1,7 +1,9 @@
 import * as os from "os";
 
 import * as core from "@actions/core";
+import * as uuid from "uuid";
 
+import type { ActionState } from "./action-common";
 import {
   getWorkflowEventName,
   getOptionalInput,
@@ -59,6 +61,30 @@ export function getDisplayActionName(actionName: ActionName): string {
   return actionName;
 }
 
+/**
+ * Either creates a UUIDv4 for the analysis or retrieves an existing one from the
+ * environment and returns it.
+ * If a new UUID is generated, it is also exported as an environment variable.
+ */
+export function getJobUUID(
+  action: ActionState<["Logger", "ReadOnlyEnv", "Actions"]>,
+) {
+  // Check if we already have a UUID for the analysis and return it if so.
+  const existingJobRunUuid = action.env.getOptional(EnvVar.JOB_RUN_UUID);
+
+  if (existingJobRunUuid !== undefined && uuid.validate(existingJobRunUuid)) {
+    action.logger.info(`Existing job run UUID is ${existingJobRunUuid}.`);
+    return existingJobRunUuid;
+  }
+
+  // Otherwise generate a new UUID.
+  const jobRunUuid = uuid.v4();
+  action.logger.info(`Job run UUID is ${jobRunUuid}.`);
+
+  action.actions.exportVariable(EnvVar.JOB_RUN_UUID, jobRunUuid);
+  return jobRunUuid;
+}
+
 /**
  * @returns a boolean indicating whether the analysis is considered to be first party.
  *

src/testing-utils.ts

@@ -34,11 +34,14 @@ import { ActionName } from "./status-report";
 import {
   DEFAULT_DEBUG_ARTIFACT_NAME,
   DEFAULT_DEBUG_DATABASE_NAME,
+  Failure,
   getEnv,
   GitHubVariant,
   GitHubVersion,
   HTTPError,
   resetCachedCodeQlVersion,
+  Result,
+  Success,
 } from "./util";
 
 export const SAMPLE_DOTCOM_API_DETAILS = {
@@ -182,13 +185,32 @@ export function getTestEnv(testEnv: NodeJS.ProcessEnv = {}): Env {
   return getEnv(testEnv);
 }
 
+/** An implementation of `ActionsEnv` for use in tests. */
+class TestActionsEnv implements ActionsEnv {
+  constructor(private readonly env: Env) {}
+
+  public clone(env: Env): this {
+    return Object.create(this, { env: { value: env } }) as this;
+  }
+
+  public getRequiredInput(name: string): string {
+    throw new Error(`Input required and not supplied: ${name}`);
+  }
+
+  public getOptionalInput(_name: string): string | undefined {
+    return undefined;
+  }
+
+  public exportVariable(name: string, value: string): void {
+    this.env.set(name, value);
+  }
+}
+
 /**
  * Gets an `ActionsEnv` instance for use in tests.
  */
-export function getTestActionsEnv(): ActionsEnv {
-  return {
-    getOptionalInput: () => undefined,
-  };
+export function getTestActionsEnv(env: Env): TestActionsEnv {
+  return new TestActionsEnv(env);
 }
 
 /** For testing purposes, we make all available state features accessible in `TestEnv`. */
@@ -206,12 +228,13 @@ type AllState = [
 export function initAllState(
   overrides?: Partial<ActionState<AllState>>,
 ): ActionState<AllState> {
+  const env = getTestEnv();
   return {
     name: ActionName.Init,
     startedAt: new Date(),
     logger: new RecordingLogger(),
-    env: getTestEnv(),
-    actions: getTestActionsEnv(),
+    env,
+    actions: getTestActionsEnv(env),
     apiClient: github.getOctokit("123"),
     features: createFeatures([]),
     ...overrides,
@@ -222,9 +245,13 @@ type DelayedCheck<
   Args extends readonly any[],
   R,
   Fs extends ReadonlyArray<AllState[number]>,
-> = (env: Readonly<BaseEnvBuilder<Args, R, Fs>>) => Promise<any>;
+> = (
+  env: Readonly<BaseEnvBuilder<Args, R, Fs>>,
+  result: Result<Awaited<R>, ThrownError<ErrorConstructor | Error>>,
+) => Promise<any>;
 
-export type ValueOrMutation<T> = T | ((val: T) => void);
+export type Mutation<T> = (val: T) => void;
+export type ValueOrMutation<T> = T | Mutation<T>;
 
 /**
  * Wraps a function that accepts an `ActionState` for testing in different environments.
@@ -236,6 +263,7 @@ abstract class BaseEnvBuilder<
 > {
   protected readonly fn: (state: ActionState<Fs>, ...args: Args) => R;
   private logger: RecordingLogger;
+  private actions: TestActionsEnv;
   protected state: ActionState<AllState>;
   protected checks: Array<DelayedCheck<Args, R, Fs>>;
 
@@ -245,15 +273,26 @@ abstract class BaseEnvBuilder<
   ) {
     this.fn = fn;
     this.logger = new RecordingLogger();
-    this.state =
-      cloneFrom !== undefined
-        ? ({
-            ...cloneFrom.state,
-            env: cloneFrom.state.env.clone(),
-            actions: Object.create(cloneFrom.state.actions),
-            logger: this.logger,
-          } satisfies ActionState<AllState>)
-        : initAllState({ logger: this.logger });
+
+    if (cloneFrom !== undefined) {
+      const env = cloneFrom.state.env.clone();
+      this.actions = cloneFrom.actions.clone(env);
+      this.state = {
+        ...cloneFrom.state,
+        env,
+        actions: this.actions,
+        logger: this.logger,
+      } satisfies ActionState<AllState>;
+    } else {
+      const env = getTestEnv();
+      this.actions = getTestActionsEnv(env);
+      this.state = initAllState({
+        logger: this.logger,
+        env,
+        actions: this.actions,
+      });
+    }
+
     this.checks = [...(cloneFrom?.checks ?? [])];
   }
 
@@ -320,13 +359,10 @@ abstract class BaseEnvBuilder<
     return result;
   }
 
-  public withActions(arg: ValueOrMutation<ActionsEnv>): this {
+  /** Applies `fn` to the `ActionsEnv`. */
+  public withActions(fn: Mutation<ActionsEnv>): this {
     const result = this.clone();
-    if (typeof arg === "function") {
-      arg(result.state.actions);
-    } else {
-      result.state.actions = arg;
-    }
+    fn(result.state.actions);
     return result;
   }
 
@@ -342,6 +378,28 @@ abstract class BaseEnvBuilder<
     return result;
   }
 
+  /**
+   * Adds a delayed check that the environment variables returned by `fn`
+   * are present in the environment after the main assertion passes.
+   */
+  public hasEnv(
+    t: ExecutionContext<unknown>,
+    fn: (
+      value: Awaited<R> | undefined,
+      error: ThrownError<ErrorConstructor | Error> | undefined,
+    ) => Record<string, string | undefined>,
+  ): this {
+    const result = this.clone();
+    result.checks.push(async (env, r) => {
+      const value = r.orElse(undefined);
+      const error = r.isFailure() ? r.value : undefined;
+      const expected = fn(value, error);
+
+      t.like(env.getState().env.get(), expected);
+    });
+    return result;
+  }
+
   /**
    * Adds a delayed check that `messages` are not logged. The check will be
    * performed after the main assertion passes.
@@ -439,7 +497,7 @@ class CallableEnvBuilder<
 
     // Run other delayed checks.
     for (const delayedCheck of this.checks) {
-      await delayedCheck(this);
+      await delayedCheck(this, new Success(result));
     }
 
     // Return the results of the function call and the main assertion.
@@ -465,7 +523,7 @@ class CallableEnvBuilder<
 
     // Run other delayed checks.
     for (const delayedCheck of this.checks) {
-      await delayedCheck(this);
+      await delayedCheck(this, new Failure(error));
     }
 
     // Return the error.