Merge pull request #4014 from github/mbg/explicit-remote-prefix

Add support for an explicit remote path prefix

Author: mbg

lib/entry-points.js

@@ -6,12 +6,14 @@ import test, { ExecutionContext } from "ava";
 import * as yaml from "js-yaml";
 import * as sinon from "sinon";
 
+import { ActionState } from "./action-common";
 import * as actionsUtil from "./actions-util";
 import { AnalysisKind, supportedAnalysisKinds } from "./analyses";
 import * as api from "./api-client";
 import { CachingKind } from "./caching-utils";
 import { createStubCodeQL } from "./codeql";
 import { UserConfig } from "./config/db-config";
+import * as file from "./config/file";
 import * as configUtils from "./config-utils";
 import * as errorMessages from "./error-messages";
 import { Feature } from "./feature-flags";
@@ -38,6 +40,8 @@ import {
   makeMacro,
   initAllState,
   callee,
+  SAMPLE_DOTCOM_API_DETAILS,
+  AssertableTarget,
 } from "./testing-utils";
 import {
   GitHubVariant,
@@ -2529,3 +2533,177 @@ test("determineUserConfig - ignores config file input outside Default Setup if F
       });
   });
 });
+
+test("loadUserConfig - loads local configuration files", async (t) => {
+  await withTmpDir(async (workspaceDir) => {
+    await withTmpDir(async (tmpDir) => {
+      // Construct the test target.
+      const loadUserConfig = (
+        actionState: ActionState<["Logger", "Env", "FeatureFlags"]>,
+        filePath: string,
+      ) =>
+        configUtils.loadUserConfig(
+          actionState,
+          filePath,
+          workspaceDir,
+          SAMPLE_DOTCOM_API_DETAILS,
+          tmpDir,
+        );
+      const target = callee(loadUserConfig);
+
+      // `loadUserConfig` should load local configuration files if they are inside the workspace:
+      const insideOfWorkspace = path.join(workspaceDir, "some-file.yml");
+      fs.writeFileSync(insideOfWorkspace, "test-key: present", "utf8");
+
+      await target
+        .withArgs(insideOfWorkspace)
+        .passes(t.deepEqual, { "test-key": "present" });
+
+      // `loadUserConfig` should normally throw if the path is outside of the workspace:
+      const outsideOfWorkspace = path.join(
+        tmpDir,
+        "not-the-generated-file.yml",
+      );
+      fs.writeFileSync(outsideOfWorkspace, "test-key: present", "utf8");
+
+      await target
+        .withArgs(outsideOfWorkspace)
+        .throws(t, { instanceOf: ConfigurationError });
+
+      // `loadUserConfig` does not throw if the path is the result of `userConfigFromActionPath`:
+      const generatedPath = configUtils.userConfigFromActionPath(tmpDir);
+      fs.writeFileSync(generatedPath, "test-key: present", "utf8");
+
+      await target
+        .withArgs(generatedPath)
+        .passes(t.deepEqual, { "test-key": "present" });
+    });
+  });
+});
+
+test.serial("loadUserConfig - loads remote configuration files", async (t) => {
+  await withTmpDir(async (tmpDir) => {
+    const getRemoteConfig = sinon.stub(file, "getRemoteConfig").resolves({});
+
+    const remoteAddress = "owner/repo/file@ref";
+    await callee(configUtils.loadUserConfig)
+      .withArgs(remoteAddress, tmpDir, SAMPLE_DOTCOM_API_DETAILS, tmpDir)
+      .passes(t.deepEqual, {});
+
+    t.true(
+      getRemoteConfig.calledOnceWithExactly(
+        sinon.match.any,
+        remoteAddress,
+        SAMPLE_DOTCOM_API_DETAILS,
+      ),
+    );
+  });
+});
+
+test.serial(
+  "loadUserConfig - loads remote configuration files (new format, partial)",
+  async (t) => {
+    await withTmpDir(async (tmpDir) => {
+      const getRemoteConfig = sinon.stub(file, "getRemoteConfig").resolves({});
+
+      // Construct the basic test target.
+      const target = callee(configUtils.loadUserConfig)
+        .withDefaultActionsEnv()
+        .withFeatures([Feature.NewRemoteFileAddresses]);
+
+      // Utility function to assert that `targetWithArgs` has identified
+      // the input as a remote file address.
+      const checkIsRemote =
+        (address: string) =>
+        async <R>(targetWithArgs: AssertableTarget<R>) => {
+          // We have stubbed `getRemoteConfig` to resolve to `{}`, so we
+          // expect that result.
+          await targetWithArgs.passes(t.deepEqual, {});
+
+          // And `getRemoteConfig` should have been called exactly once.
+          t.is(getRemoteConfig.callCount, 1);
+
+          // Get the arguments for the call and check that there were three.
+          // We don't care about the first, but check that the other two
+          // match our expectations. We break it down like this to get
+          // more useful test output.
+          const args = getRemoteConfig.getCalls()[0].args;
+          t.is(args.length, 3);
+          t.deepEqual(args[1], address);
+          t.deepEqual(args[2], SAMPLE_DOTCOM_API_DETAILS);
+        };
+
+      // Utility function to assert that `targetWithArgs` has not identified
+      // the input as a remote file address.
+      const checkIsNotRemote = async <R>(
+        targetWithArgs: AssertableTarget<R>,
+      ) => {
+        // We expect `loadUserConfig` to have thrown if it thinks the path is local,
+        // since the inputs we provide aren't for files that exist.
+        await targetWithArgs.throws(t);
+
+        // Additionally, we expect that `getRemoteConfig` wasn't called.
+        t.is(getRemoteConfig.callCount, 0);
+      };
+
+      // Utility function to add the explicit `REMOTE_PATH_PREFIX` to the input.
+      const withExplicitPrefix = (str: string) =>
+        `${file.REMOTE_PATH_PREFIX}${str}`;
+
+      // Utility to set up a call to `loadUserConfig` with the provided `address`
+      // and pass it to `assertion`.
+      const testTargetWith = async (
+        address: string,
+        assertion: (
+          targetWithArgs: AssertableTarget<Promise<UserConfig>>,
+        ) => Promise<any>,
+      ) => {
+        // Reset the stub's history since we re-use it.
+        getRemoteConfig.resetHistory();
+
+        // Log the input we are testing so that, in the event of a failure,
+        // it is easier to see which input was responsible.
+        t.log(`testTargetWith("${address}")`);
+
+        // Prepare the test call to `loadUserConfig`.
+        const targetWithArgs = target.withArgs(
+          address,
+          tmpDir,
+          SAMPLE_DOTCOM_API_DETAILS,
+          tmpDir,
+        );
+
+        // Pass it to the provided assertion function.
+        await assertion(targetWithArgs);
+      };
+
+      // Since this input contains an '@' character, it is treated as a remote path
+      // by the old logic even without the explicit prefix.
+      const remoteWithoutPrefix = "repo@main";
+      await testTargetWith(
+        remoteWithoutPrefix,
+        checkIsRemote(remoteWithoutPrefix),
+      );
+      await testTargetWith(
+        withExplicitPrefix(remoteWithoutPrefix),
+        checkIsRemote(remoteWithoutPrefix),
+      );
+      // It is only treated as a local path with the corresponding prefix.
+      await testTargetWith(`./${remoteWithoutPrefix}`, checkIsNotRemote);
+
+      // The following test inputs are examples of ambiguous paths. They could refer to
+      // valid local or remote paths. For each, we check that they are treated as remote
+      // paths if the explicit remote file prefix is used and as local paths otherwise.
+      const testInputs = ["repo:file", "input", "../input"];
+
+      for (const testInput of testInputs) {
+        for (const addPrefix of [true, false]) {
+          await testTargetWith(
+            addPrefix ? withExplicitPrefix(testInput) : testInput,
+            addPrefix ? checkIsRemote(testInput) : checkIsNotRemote,
+          );
+        }
+      }
+    });
+  },
+);