Merge pull request #4023 from github/mbg/ff/remove-new-remote-file-addresses-ff

Promote `NewRemoteFileAddresses` FF

Author: mbg

.github/workflows/__start-proxy.yml

@@ -48,7 +48,6 @@ steps:
       CODEQL_PROXY_HOST: ${{ steps.proxy.outputs.proxy_host }}
       CODEQL_PROXY_PORT: ${{ steps.proxy.outputs.proxy_port }}
       CODEQL_PROXY_CA_CERTIFICATE: ${{ steps.proxy.outputs.proxy_ca_certificate }}
-      CODEQL_ACTION_NEW_REMOTE_FILE_ADDRESSES: "true"
     with:
       languages: java
       tools: ${{ steps.prepare-test.outputs.tools-url }}

lib/entry-points.js

@@ -2607,9 +2607,7 @@ test.serial(
       const getRemoteConfig = sinon.stub(file, "getRemoteConfig").resolves({});
 
       // Construct the basic test target.
-      const target = callee(configUtils.loadUserConfig)
-        .withDefaultActionsEnv()
-        .withFeatures([Feature.NewRemoteFileAddresses]);
+      const target = callee(configUtils.loadUserConfig).withDefaultActionsEnv();
 
       // Utility function to assert that `targetWithArgs` has identified
       // the input as a remote file address.

pr-checks/checks/start-proxy.yml

@@ -489,11 +489,7 @@ export async function loadUserConfig(
   apiDetails: api.GitHubApiCombinedDetails,
   tempDir: string,
 ): Promise<UserConfig> {
-  const allowNewFormat = await actionState.features.getValue(
-    Feature.NewRemoteFileAddresses,
-  );
-
-  if (isLocal(configFile, allowNewFormat)) {
+  if (isLocal(configFile)) {
     if (configFile !== userConfigFromActionPath(tempDir)) {
       // If the config file is not generated by the Action, it should be relative to the workspace.
       configFile = path.resolve(workspacePath, configFile);
@@ -512,7 +508,7 @@ export async function loadUserConfig(
     // Drop the explicit prefix if it is present. Since `REMOTE_PATH_PREFIX` is chosen
     // to not conflict with permissible characters in "owner" or "repo" components,
     // this does not risk removing valid parts of either component by accident.
-    if (allowNewFormat && isExplicitRemotePath(configFile)) {
+    if (isExplicitRemotePath(configFile)) {
       configFile = configFile.substring(REMOTE_PATH_PREFIX.length);
     }
     return await getRemoteConfig(actionState, configFile, apiDetails);
@@ -1326,7 +1322,7 @@ function containsAtRef(configPath: string): boolean {
  * @param configPath The path to test.
  * @returns True if it is local, or false otherwise.
  */
-function isLocal(configPath: string, allowNewFormat: boolean): boolean {
+function isLocal(configPath: string): boolean {
   // If the path starts with `LOCAL_PATH_PREFIX`, it is explicitly local.
   // This allows local paths that would otherwise contain '@'
   // to be used with a `LOCAL_PATH_PREFIX` prefix.
@@ -1335,7 +1331,7 @@ function isLocal(configPath: string, allowNewFormat: boolean): boolean {
   }
   // If the path starts with `REMOTE_PATH_PREFIX`, it is explicitly remote.
   // This allows users to resolve ambiguity by specifying `REMOTE_PATH_PREFIX`.
-  if (allowNewFormat && isExplicitRemotePath(configPath)) {
+  if (isExplicitRemotePath(configPath)) {
     return false;
   }
 

src/config-utils.test.ts

@@ -108,7 +108,7 @@ test.serial("getRemoteConfig uses proxy when it is supposed to", async (t) => {
 
   // Should use it when the FF is enabled and the environment variables are set.
   await target
-    .withFeatures([Feature.ProxyApiRequests, Feature.NewRemoteFileAddresses])
+    .withFeatures([Feature.ProxyApiRequests])
     .withEnv((env) => {
       env.set(RegistryProxyVars.PROXY_HOST, "localhost");
       env.set(RegistryProxyVars.PROXY_PORT, "1234");
@@ -118,7 +118,6 @@ test.serial("getRemoteConfig uses proxy when it is supposed to", async (t) => {
 
   // But not when the FF is not enabled.
   await target
-    .withFeatures([Feature.NewRemoteFileAddresses])
     .withEnv((env) => {
       env.set(RegistryProxyVars.PROXY_HOST, "localhost");
       env.set(RegistryProxyVars.PROXY_PORT, "1234");
@@ -128,7 +127,7 @@ test.serial("getRemoteConfig uses proxy when it is supposed to", async (t) => {
 
   // And not when the environment variables aren't set.
   await target
-    .withFeatures([Feature.ProxyApiRequests, Feature.NewRemoteFileAddresses])
+    .withFeatures([Feature.ProxyApiRequests])
     .notLogs(t, "Using private registry proxy at 'http://localhost:1234'")
     .throws(t, { message: errorMessage });
 });

src/config-utils.ts

@@ -2,8 +2,6 @@ import test from "ava";
 import sinon from "sinon";
 
 import { ActionsEnvVars } from "../environment";
-import * as errors from "../error-messages";
-import { Feature } from "../feature-flags";
 import { callee } from "../testing-utils";
 import { ConfigurationError } from "../util";
 
@@ -75,15 +73,7 @@ test("parseRemoteFileAddress accepts full remote addresses", async (t) => {
   for (const newFormatInput of newFormatInputs) {
     const targetWithArgs = target.withArgs(newFormatInput.input);
 
-    // Should fail when the FF is not enabled.
-    await targetWithArgs
-      .withFeatures([])
-      .throws(t, { instanceOf: ConfigurationError });
-
-    // And pass when the FF is enabled.
-    await targetWithArgs
-      .withFeatures([Feature.NewRemoteFileAddresses])
-      .passes(t.deepEqual, newFormatInput.expected);
+    await targetWithArgs.passes(t.deepEqual, newFormatInput.expected);
   }
 });
 
@@ -138,15 +128,7 @@ test("parseRemoteFileAddress accepts remote address without an owner", async (t)
   for (const testCase of testCases) {
     const targetWithArgs = target.withArgs(testCase.input);
 
-    // Should fail when the FF is not enabled.
-    await targetWithArgs
-      .withFeatures([])
-      .throws(t, { instanceOf: ConfigurationError });
-
-    // And pass when the FF is enabled.
-    await targetWithArgs
-      .withFeatures([Feature.NewRemoteFileAddresses])
-      .passes(t.deepEqual, testCase.expected);
+    await targetWithArgs.passes(t.deepEqual, testCase.expected);
   }
 });
 
@@ -160,9 +142,7 @@ test("parseRemoteFileAddress throws for invalid `GITHUB_REPOSITORY`", async (t)
       sinon.define(env, "getRequired", getRequired);
     });
 
-  await target
-    .withFeatures([Feature.NewRemoteFileAddresses])
-    .throws(t, { instanceOf: Error });
+  await target.throws(t, { instanceOf: Error });
 
   t.assert(getRequired.calledOnceWith(ActionsEnvVars.GITHUB_REPOSITORY));
 });
@@ -194,31 +174,19 @@ test("parseRemoteFileAddress accepts remote address without a path", async (t) =
   for (const testCase of testCases) {
     const targetWithArgs = target.withArgs(testCase.input);
 
-    // Should fail when the FF is not enabled.
-    await targetWithArgs
-      .withFeatures([])
-      .throws(t, { instanceOf: ConfigurationError });
-
-    // And pass when the FF is enabled.
-    await targetWithArgs
-      .withFeatures([Feature.NewRemoteFileAddresses])
-      .passes(t.deepEqual, testCase.expected);
+    await targetWithArgs.passes(t.deepEqual, testCase.expected);
   }
 });
 
 test("parseRemoteFileAddress accepts remote address without a ref", async (t) => {
   const target = callee(parseRemoteFileAddress).withArgs("owner/repo:path");
 
-  // Should only accept the input if the FF is enabled.
-  await target.withFeatures([]).throws(t);
-  await target
-    .withFeatures([Feature.NewRemoteFileAddresses])
-    .passes(t.deepEqual, {
-      owner: "owner",
-      repo: "repo",
-      path: "path",
-      ref: DEFAULT_CONFIG_FILE_REF,
-    } satisfies RemoteFileAddress);
+  await target.passes(t.deepEqual, {
+    owner: "owner",
+    repo: "repo",
+    path: "path",
+    ref: DEFAULT_CONFIG_FILE_REF,
+  } satisfies RemoteFileAddress);
 });
 
 test("parseRemoteFileAddress rejects invalid values", async (t) => {
@@ -251,18 +219,11 @@ test("parseRemoteFileAddress rejects invalid values", async (t) => {
   for (const testInput of testInputs) {
     const targetWithArgs = target.withArgs(testInput);
 
-    // Should throw both when the new format is and isn't accepted.
-    await targetWithArgs.withFeatures([]).throws(t, {
+    await targetWithArgs.throws(t, {
+      // When the new format is accepted, there are some more specific
+      // errors in some cases. It is sufficient for us to check that
+      // an exception is thrown.
       instanceOf: ConfigurationError,
-      message: errors.getConfigFileRepoOldFormatInvalidMessage(testInput),
     });
-    await targetWithArgs
-      .withFeatures([Feature.NewRemoteFileAddresses])
-      .throws(t, {
-        // When the new format is accepted, there are some more specific
-        // errors in some cases. It is sufficient for us to check that
-        // an exception is thrown.
-        instanceOf: ConfigurationError,
-      });
   }
 });

src/config/file.test.ts

@@ -1,7 +1,6 @@
 import { ActionState } from "../action-common";
 import { ActionsEnvVars, ReadOnlyEnv } from "../environment";
 import * as errorMessages from "../error-messages";
-import { Feature } from "../feature-flags";
 import { ConfigurationError, Failure, Result, Success } from "../util";
 
 /** Represents remote file addresses. */
@@ -126,16 +125,6 @@ export async function parseRemoteFileAddress(
     return oldFormatAddressResult.value;
   }
 
-  // If the FF for the new format is not enabled, throw the old format error.
-  const allowNewFormat = await actionState.features.getValue(
-    Feature.NewRemoteFileAddresses,
-  );
-  if (!allowNewFormat) {
-    throw new ConfigurationError(
-      errorMessages.getConfigFileRepoOldFormatInvalidMessage(configFile),
-    );
-  }
-
   // retrieve the various parts of the config location, and ensure they're present
   const newFormatAddressResult = parseNewRemoteFileAddress(
     actionState.env,

src/config/remote-file.test.ts

@@ -94,8 +94,6 @@ export enum Feature {
   ForceNightly = "force_nightly",
   IgnoreGeneratedFiles = "ignore_generated_files",
   JavaNetworkDebugging = "java_network_debugging",
-  /** Allow the new remote file address format. */
-  NewRemoteFileAddresses = "new_remote_file_addresses",
   OverlayAnalysis = "overlay_analysis",
   OverlayAnalysisCodeScanningCpp = "overlay_analysis_code_scanning_cpp",
   OverlayAnalysisCodeScanningCsharp = "overlay_analysis_code_scanning_csharp",
@@ -266,11 +264,6 @@ export const featureConfig = {
     envVar: "CODEQL_ACTION_JAVA_NETWORK_DEBUGGING",
     minimumVersion: undefined,
   },
-  [Feature.NewRemoteFileAddresses]: {
-    defaultValue: false,
-    envVar: "CODEQL_ACTION_NEW_REMOTE_FILE_ADDRESSES",
-    minimumVersion: undefined,
-  },
   [Feature.OverlayAnalysis]: {
     defaultValue: false,
     envVar: "CODEQL_ACTION_OVERLAY_ANALYSIS",