Stop using feature-flag support for determining if a feature is active

Using the feature flag mechanism for checking if uploads are enabled was
too clunky. I'm moving the change to checking versions directly.

src/analyze-action-post.ts

@@ -7,18 +7,12 @@ import * as core from "@actions/core";
 
 import * as actionsUtil from "./actions-util";
 import { getGitHubVersion } from "./api-client";
+import { getCodeQL } from "./codeql";
 import { getConfig } from "./config-utils";
 import * as debugArtifacts from "./debug-artifacts";
 import { EnvVar } from "./environment";
-import { Features } from "./feature-flags";
-import { getActionsLogger, Logger, withGroup } from "./logging";
-import { parseRepositoryNwo } from "./repository";
-import {
-  checkGitHubVersionInRange,
-  getErrorMessage,
-  getRequiredEnvParam,
-  GitHubVersion,
-} from "./util";
+import { getActionsLogger, withGroup } from "./logging";
+import { checkGitHubVersionInRange, getErrorMessage } from "./util";
 
 async function runWrapper() {
   try {
@@ -27,8 +21,6 @@ async function runWrapper() {
     const gitHubVersion = await getGitHubVersion();
     checkGitHubVersionInRange(gitHubVersion, logger);
 
-    const features = createFeatures(gitHubVersion, logger);
-
     // Upload SARIF artifacts if we determine that this is a first-party analysis run.
     // For third-party runs, this artifact will be uploaded in the `upload-sarif-post` step.
     if (process.env[EnvVar.INIT_ACTION_HAS_RUN] === "true") {
@@ -37,11 +29,13 @@ async function runWrapper() {
         logger,
       );
       if (config !== undefined) {
+        const codeql = await getCodeQL(config.codeQLCmd);
+        const version = await codeql.getVersion();
         await withGroup("Uploading combined SARIF debug artifact", () =>
           debugArtifacts.uploadCombinedSarifArtifacts(
             logger,
             config.gitHubVersion.type,
-            features,
+            version.version,
           ),
         );
       }
@@ -53,18 +47,4 @@ async function runWrapper() {
   }
 }
 
-function createFeatures(gitHubVersion: GitHubVersion, logger: Logger) {
-  const repositoryNwo = parseRepositoryNwo(
-    getRequiredEnvParam("GITHUB_REPOSITORY"),
-  );
-
-  const features = new Features(
-    gitHubVersion,
-    repositoryNwo,
-    actionsUtil.getTemporaryDirectory(),
-    logger,
-  );
-  return features;
-}
-
 void runWrapper();

src/debug-artifacts.test.ts

@@ -1,9 +1,7 @@
 import test from "ava";
 
 import * as debugArtifacts from "./debug-artifacts";
-import { Feature } from "./feature-flags";
 import { getActionsLogger } from "./logging";
-import { createFeatures } from "./testing-utils";
 import { GitHubVariant } from "./util";
 
 test("sanitizeArtifactName", (t) => {
@@ -32,7 +30,7 @@ test("uploadDebugArtifacts when artifacts empty", async (t) => {
       "i-dont-exist",
       "artifactName",
       GitHubVariant.DOTCOM,
-      true,
+      undefined,
     );
     t.is(
       uploaded,
@@ -42,7 +40,7 @@ test("uploadDebugArtifacts when artifacts empty", async (t) => {
   });
 });
 
-test("uploadDebugArtifacts when true", async (t) => {
+test("uploadDebugArtifacts when no codeql version is used", async (t) => {
   // Test that the artifact is uploaded.
   const logger = getActionsLogger();
   await t.notThrowsAsync(async () => {
@@ -52,7 +50,7 @@ test("uploadDebugArtifacts when true", async (t) => {
       "i-dont-exist",
       "artifactName",
       GitHubVariant.DOTCOM,
-      true,
+      undefined,
     );
     t.is(
       uploaded,
@@ -62,27 +60,7 @@ test("uploadDebugArtifacts when true", async (t) => {
   });
 });
 
-test("uploadDebugArtifacts when false", async (t) => {
-  // Test that the artifact is not uploaded.
-  const logger = getActionsLogger();
-  await t.notThrowsAsync(async () => {
-    const uploaded = await debugArtifacts.uploadDebugArtifacts(
-      logger,
-      ["hucairz"],
-      "i-dont-exist",
-      "artifactName",
-      GitHubVariant.DOTCOM,
-      false,
-    );
-    t.is(
-      uploaded,
-      "upload-not-supported",
-      "Should not have uploaded any artifacts",
-    );
-  });
-});
-
-test("uploadDebugArtifacts when feature enabled", async (t) => {
+test("uploadDebugArtifacts when new codeql version is used", async (t) => {
   // Test that the artifact is uploaded.
   const logger = getActionsLogger();
   await t.notThrowsAsync(async () => {
@@ -92,7 +70,7 @@ test("uploadDebugArtifacts when feature enabled", async (t) => {
       "i-dont-exist",
       "artifactName",
       GitHubVariant.DOTCOM,
-      createFeatures([Feature.SafeArtifactUpload]),
+      "2.20.3",
     );
     t.is(
       uploaded,
@@ -102,7 +80,7 @@ test("uploadDebugArtifacts when feature enabled", async (t) => {
   });
 });
 
-test("uploadDebugArtifacts when feature disabled", async (t) => {
+test("uploadDebugArtifacts when old codeql is used", async (t) => {
   // Test that the artifact is not uploaded.
   const logger = getActionsLogger();
   await t.notThrowsAsync(async () => {
@@ -112,7 +90,7 @@ test("uploadDebugArtifacts when feature disabled", async (t) => {
       "i-dont-exist",
       "artifactName",
       GitHubVariant.DOTCOM,
-      createFeatures([]),
+      "2.20.2",
     );
     t.is(
       uploaded,

src/debug-artifacts.ts

@@ -12,14 +12,12 @@ import { dbIsFinalized } from "./analyze";
 import { getCodeQL } from "./codeql";
 import { Config } from "./config-utils";
 import { EnvVar } from "./environment";
-import {
-  Feature,
-  featureConfig,
-  FeatureEnablement,
-  Features,
-} from "./feature-flags";
 import { Language } from "./languages";
 import { Logger, withGroup } from "./logging";
+import {
+  isSafeArtifactUpload,
+  SafeArtifactUploadVersion,
+} from "./tools-features";
 import {
   bundleDb,
   doesDirectoryExist,
@@ -40,7 +38,7 @@ export function sanitizeArtifactName(name: string): string {
 export async function uploadCombinedSarifArtifacts(
   logger: Logger,
   gitHubVariant: GitHubVariant,
-  features: Features | boolean,
+  codeQlVersion: string | undefined,
 ) {
   const tempDir = getTemporaryDirectory();
 
@@ -75,7 +73,7 @@ export async function uploadCombinedSarifArtifacts(
         baseTempDir,
         "combined-sarif-artifacts",
         gitHubVariant,
-        features,
+        codeQlVersion,
       );
     } catch (e) {
       logger.warning(
@@ -168,7 +166,7 @@ async function tryBundleDatabase(
 export async function tryUploadAllAvailableDebugArtifacts(
   config: Config,
   logger: Logger,
-  features: FeatureEnablement,
+  codeQlVersion: string | undefined,
 ) {
   const filesToUpload: string[] = [];
   try {
@@ -232,7 +230,7 @@ export async function tryUploadAllAvailableDebugArtifacts(
         config.dbLocation,
         config.debugArtifactName,
         config.gitHubVersion.type,
-        features,
+        codeQlVersion,
       ),
     );
   } catch (e) {
@@ -248,7 +246,7 @@ export async function uploadDebugArtifacts(
   rootDir: string,
   artifactName: string,
   ghVariant: GitHubVariant,
-  features: FeatureEnablement | boolean,
+  codeQlVersion: string | undefined,
 ): Promise<
   | "no-artifacts-to-upload"
   | "upload-successful"
@@ -258,14 +256,11 @@ export async function uploadDebugArtifacts(
   if (toUpload.length === 0) {
     return "no-artifacts-to-upload";
   }
-  const uploadSupported =
-    typeof features === "boolean"
-      ? features
-      : await features.getValue(Feature.SafeArtifactUpload);
+  const uploadSupported = isSafeArtifactUpload(codeQlVersion);
 
   if (!uploadSupported) {
     core.info(
-      `Skipping debug artifact upload because the current CLI does not support safe upload. Please upgrade to CLI v${featureConfig.safe_artifact_upload.minimumVersion} or later.`,
+      `Skipping debug artifact upload because the current CLI does not support safe upload. Please upgrade to CLI v${SafeArtifactUploadVersion} or later.`,
     );
     return "upload-not-supported";
   }

src/feature-flags.ts

@@ -54,7 +54,6 @@ export enum Feature {
   PythonDefaultIsToNotExtractStdlib = "python_default_is_to_not_extract_stdlib",
   QaTelemetryEnabled = "qa_telemetry_enabled",
   ZstdBundleStreamingExtraction = "zstd_bundle_streaming_extraction",
-  SafeArtifactUpload = "safe_artifact_upload",
 }
 
 export const featureConfig: Record<
@@ -155,18 +154,6 @@ export const featureConfig: Record<
     legacyApi: true,
     minimumVersion: undefined,
   },
-
-  /**
-   * The first version of the CodeQL CLI where artifact upload is safe to use
-   * for failed runs. This is not really a feature flag, but it is easiest to
-   * model the behavior as a feature flag.
-   */
-  [Feature.SafeArtifactUpload]: {
-    defaultValue: true,
-    envVar: "CODEQL_ACTION_SAFE_ARTIFACT_UPLOAD",
-    legacyApi: true,
-    minimumVersion: "2.20.3",
-  },
 };
 
 /**

src/init-action-post-helper.ts

@@ -161,7 +161,7 @@ export async function run(
   uploadAllAvailableDebugArtifacts: (
     config: Config,
     logger: Logger,
-    features: FeatureEnablement,
+    codeQlVersion: string,
   ) => Promise<void>,
   printDebugLogs: (config: Config) => Promise<void>,
   config: Config,
@@ -211,7 +211,9 @@ export async function run(
     logger.info(
       "Debug mode is on. Uploading available database bundles and logs as Actions debugging artifacts...",
     );
-    await uploadAllAvailableDebugArtifacts(config, logger, features);
+    const codeql = await getCodeQL(config.codeQLCmd);
+    const version = await codeql.getVersion();
+    await uploadAllAvailableDebugArtifacts(config, logger, version.version);
     await printDebugLogs(config);
   }
 

src/tools-features.ts

@@ -1,3 +1,5 @@
+import * as semver from "semver";
+
 import type { VersionInfo } from "./codeql";
 
 export enum ToolsFeature {
@@ -26,3 +28,21 @@ export function isSupportedToolsFeature(
 ): boolean {
   return !!versionInfo.features && versionInfo.features[feature];
 }
+
+export const SafeArtifactUploadVersion = "2.20.3";
+
+/**
+ * The first version of the CodeQL CLI where artifact upload is safe to use
+ * for failed runs. This is not really a feature flag, but it is easiest to
+ * model the behavior as a feature flag.
+ *
+ * This was not captured in a tools feature, so we need to use semver.
+ *
+ * @param codeQlVersion The version of the CodeQL CLI to check. If not provided, it is assumed to be safe.
+ * @returns True if artifact upload is safe to use for failed runs or false otherwise.
+ */
+export function isSafeArtifactUpload(codeQlVersion?: string): boolean {
+  return !codeQlVersion
+    ? true
+    : semver.gte(codeQlVersion, SafeArtifactUploadVersion);
+}

src/upload-sarif-action-post.ts

@@ -33,7 +33,9 @@ async function runWrapper() {
         debugArtifacts.uploadCombinedSarifArtifacts(
           logger,
           gitHubVersion.type,
-          true,
+          // The codeqlVersion is not applicable for uploading non-codeql sarif.
+          // We can assume all versions are safe to upload.
+          undefined,
         ),
       );
     }