Refactor handling of feature flags by aeisenberg · Pull Request #1284 · github/codeql-action

aeisenberg · 2022-10-05T22:56:49Z

This commit centralizes how feature flags are handled. All feature flags must now add an entry in the featureFlagConfig dictionary. This dictionary associates the flag with an environment variable name and optionally a minimum version for CodeQL.

The new logic is:

if the environment variable is set to false: disabled
if the minimum version requirement specified and not met: disabled
if a minimum version requirement is specified, but a codeql instance is not passed in: throw
if the environment variable is set to true: enable
Otherwise check feature flag enablement from the server

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.
Confirm the changelog has been updated if necessary.

src/util.test.ts

This commit centralizes how feature flags are handled. All feature flags must now add an entry in the `featureFlagConfig` dictionary. This dictionary associates the flag with an environment variable name and optionally a minimum version for CodeQL. The new logic is: - if the environment variable is set to false: disabled - if the minimum version requirement specified and met: disabled - if the environment variable is set to true: enable - Otherwise check feature flag enablement from the server

henrymercer

Thanks for doing this. As a general piece of feedback, I wonder whether it would be appropriate to rename FeatureFlags to Features. This would reflect a distinction between "feature flags", which are flippers on the monolith, and "features", which are functions in the CodeQL Action whose enablement depends on environment variables, CLI versions, and feature flags.

src/feature-flags.test.ts

henrymercer · 2022-10-06T14:16:32Z

src/feature-flags.test.ts

+        // feature flag should be disabled when an old CLI version is set
+        let codeql = mockCodeQLVersion("2.0.0");
+        t.assert(
+          !(await featureFlags.getValue(featureFlag as FeatureFlag, codeql))
+        );
+
+        // even setting the env var to true should not enable the feature flag if
+        // the minimum CLI version is not met
+        process.env[featureFlagConfig[featureFlag].envVar] = "true";
+        t.assert(
+          !(await featureFlags.getValue(featureFlag as FeatureFlag, codeql))
+        );
+
+        // feature flag should be enabled when a new CLI version is set
+        // and env var is not set
+        process.env[featureFlagConfig[featureFlag].envVar] = "";
+        codeql = mockCodeQLVersion(
+          featureFlagConfig[featureFlag].minimumVersion
+        );
+        t.assert(
+          await featureFlags.getValue(featureFlag as FeatureFlag, codeql)
+        );
+
+        // set env var to false and check that the feature flag is now disabled
+        process.env[featureFlagConfig[featureFlag].envVar] = "false";
+        t.assert(
+          !(await featureFlags.getValue(featureFlag as FeatureFlag, codeql))
+        );


Should each of these be separate tests?

src/feature-flags.ts

aeisenberg · 2022-10-06T16:42:34Z

I wonder whether it would be appropriate to rename FeatureFlags to Features.

Sure, that makes sense. Maybe FeatureEnablement is better?

henrymercer · 2022-10-06T18:39:19Z

Sure, that makes sense. Maybe FeatureEnablement is better?

Sure, that works too.

aeisenberg · 2022-10-06T19:07:51Z

Actually, I like Features better since the names are used for more than just enablement.

- Change env var name for `MlPoweredQueriesEnabled` - Throw error if minimumVersion is specified, but CodeQL argument is not supplied. - Fix failing tests. Note that I removed a config-utils test because it is no longer relevant since we handle codeql minimum versions in the `getValue` function.

aeisenberg · 2022-10-06T19:34:57Z

Addressed all comments. Please look at the two commits individually since the second commit adds a lot of noise through a renaming.

src/feature-flags.test.ts

src/feature-flags.ts

henrymercer · 2022-10-06T19:59:25Z

src/feature-flags.ts

 export interface FeatureFlags {
-  getValue(flag: FeatureFlag): Promise<boolean>;
+  getValue(flag: Feature, codeql?: CodeQL): Promise<boolean>;
 }


I think we probably ought to rename this too to Features.

This is a little bit of bike shedding, but I thought this should remain FeatureFlags since it really is an interface that checks whether a Feature is turned on or off. Also, the concrete instantiation of this is GitHubFeatureFlags.

My thinking was that FeatureFlags.getValue(x) tells us whether the feature x should be enabled, which as of this PR is no longer the same as whether the feature flag on the monolith is enabled for x.

Agree we're in bikeshedding territory here, but this interface is also public facing and is really about feature enablement, not feature flags. Couple of suggestions:

Rename this interface to Features, and the class that implements it to GithubFeatures

Rename this interface to FeatureEnablement

All right. A slightly more complex structure now, but I think it is better:

Interface is FeatureEnablement

The public facing class is Features

This delegates to another internal class GitHubFeatureFlags for the portion of the enablement checking that needs to get information from github.

Internal refactoring so that `GitHubFeatureFlags` is private only. The public facing class is `Features`.

henrymercer

This is looking generally in good shape to me. Some follow up suggestions, some of which are optional.

henrymercer · 2022-10-07T09:25:30Z

src/feature-flags.ts

 export interface FeatureFlags {
-  getValue(flag: FeatureFlag): Promise<boolean>;
+  getValue(flag: Feature, codeql?: CodeQL): Promise<boolean>;
 }


Agree we're in bikeshedding territory here, but this interface is also public facing and is really about feature enablement, not feature flags. Couple of suggestions:

Rename this interface to Features, and the class that implements it to GithubFeatures

Rename this interface to FeatureEnablement

src/feature-flags.ts

henrymercer · 2022-10-07T09:27:34Z

src/feature-flags.ts

+    // Bypassing the toolcache is disabled in test mode.
+    if (flag === Feature.BypassToolcacheEnabled && util.isInTestMode()) {
+      return false;
+    }


Optional: Make this a property of the featureConfig record.

henrymercer · 2022-10-07T09:28:37Z

src/feature-flags.ts

      return false;
    }
-    return flagValue;
+    return flagValue || false;


For my understanding: When would we need the || here? We already tackled the undefined case above.

We are getting this information from an API. I just wanted to ensure the value was coerced into a boolean. I should have put:

return !!flagValue;

and I will change.

henrymercer · 2022-10-07T09:30:20Z

src/feature-flags.ts

+    // Do nothing when not running against github.com
+    if (this.gitHubVersion.type !== util.GitHubVariant.DOTCOM) {
+      this.logger.debug(
+        "Not running against github.com. Disabling all feature flags."


Suggested change

"Not running against github.com. Disabling all feature flags."

"Not running against github.com. Feature flags will only be enabled if they are enabled explicitly, for example through an environment variable."

Going to use toggleable features instead of feature flags since that seems more appropriate. "Feature flags" is only when they are coming from github. "Features" is the more general concept.

However, here you are suggesting we mention environment variables, but we don't have any place where we actually document them, so it might be confusing. So, I think it would be better if we just do:

Not running against github.com. Disabling all toggleable features.

henrymercer · 2022-10-07T09:31:05Z

src/feature-flags.ts

 * This should be only used within tests.
 */
-export function createFeatureFlags(enabledFlags: FeatureFlag[]): FeatureFlags {
+export function createFeatureFlags(enabledFlags: Feature[]): FeatureFlags {


Rename this if you go with my suggestion to rename the FeatureFlags interface

henrymercer · 2022-10-07T09:31:33Z

src/init-action.ts

  );

-  const featureFlags = new GitHubFeatureFlags(
+  const featureFlags = new Features(


These variable assignments ought to be renamed too. It's a shame it's quite a lot of noise.

henrymercer · 2022-10-07T09:32:18Z

src/init-action.ts

+  if (trapCaching !== undefined) {
+    return trapCaching === "true";
+  }
+  return await featureFlags.getValue(Feature.TrapCachingEnabled);


Optional

Suggested change

if (trapCaching !== undefined) {

return trapCaching === "true";

}

return await featureFlags.getValue(Feature.TrapCachingEnabled);

return trapCaching === "true" || (await featureFlags.getValue(Feature.TrapCachingEnabled));

Avoid usage of "Feature Flag" unless we are talking specifically about the response from github features api. Otherwise, use terms like "Toggleable features". Note both "toggleable" and "togglable" appear to be valid spellings of the word. I chose the first for no good reason.

…toring

angelapwen · 2022-10-07T22:25:57Z

From PR body —

The new logic is:
...
if the minimum version requirement specified and met: disabled

Should the above be if the minimum version requirement specified is not met?

angelapwen

Just small comment typo nits 😸

Thanks for the code comments + clear variable naming!

angelapwen · 2022-10-07T23:01:15Z

src/feature-flags.ts

-export interface FeatureFlags {
-  getValue(flag: FeatureFlag): Promise<boolean>;
+export interface FeatureEnablement {
+  getValue(feaature: Feature, codeql?: CodeQL): Promise<boolean>;


Suggested change

getValue(feaature: Feature, codeql?: CodeQL): Promise<boolean>;

getValue(feature: Feature, codeql?: CodeQL): Promise<boolean>;

(+autogenerated code)

angelapwen · 2022-10-07T23:04:19Z

src/feature-flags.ts

+            `please ensure the Action has the 'security-events: write' permission. Details: ${e}`
+        );
+      } else {
+        // Some feature, such as `ml_powered_queries_enabled` affect the produced alerts.


Suggested change

// Some feature, such as `ml_powered_queries_enabled` affect the produced alerts.

// Some features, such as `ml_powered_queries_enabled` affect the produced alerts.

angelapwen · 2022-10-07T23:04:33Z

src/feature-flags.ts

+        );
+      } else {
+        // Some feature, such as `ml_powered_queries_enabled` affect the produced alerts.
+        // Considering these feature disabled in the event of a transient error could


Suggested change

// Considering these feature disabled in the event of a transient error could

// Considering these features disabled in the event of a transient error could

src/feature-flags.test.ts

angelapwen · 2022-10-07T23:12:32Z

src/feature-flags.test.ts

+// specify a minimum version, then we will have a bunch of code no longer being
+// tested. This is unlikely, and this test will fail if that happens.
+// If we do end up in that situation, then we should consider adding a synthetic
+// feature flag with a minium version that is only used for tests.


Suggested change

// feature flag with a minium version that is only used for tests.

// feature flag with a minimum version that is only used for tests.

aeisenberg · 2022-10-07T23:27:42Z

Done!

henrymercer

There are a bunch of uses of featureFlags across the codebase that probably ought to become features or featureEnablement. I would be open to merging this and cleaning those up as follow up though.

henrymercer · 2022-10-10T14:14:52Z

src/codeql.test.ts

 }: {
  apiDetails?: GitHubApiDetails;
-  featureFlags?: FeatureFlags;
+  featureFlags?: FeatureEnablement;


This should be features or featureEnablement

henrymercer · 2022-10-10T14:15:28Z

src/codeql.test.ts

+        "",
+        undefined,
+        undefined,
+        createFeatureFlags([Feature.CliConfigFileEnabled]),


This should be createFeatures or createFeatureEnablement

henrymercer · 2022-10-10T14:16:11Z

src/feature-flags.test.ts

+        );
+      }

+      // Alls flags should be false except the one we're testing


Suggested change

// Alls flags should be false except the one we're testing

// All flags should be false except the one we're testing

henrymercer · 2022-10-10T14:17:59Z

src/feature-flags.test.ts

-        testApiDetails,
-        testRepositoryNwo,
-        getRecordingLogger(loggedMessages)
+      const featureFlags = setUpTests(


I think this should be features / featureEnablement throughout.

henrymercer · 2022-10-10T14:18:31Z

src/feature-flags.ts

+
+  /**
+   *
+   * @param feature The feature flag to check.


Suggested change

* @param feature The feature flag to check.

* @param feature The feature to check.

henrymercer · 2022-10-10T14:20:12Z

src/feature-flags.ts

+  }
+}
+
+class GitHubFeatureFlags implements FeatureEnablement {


Perhaps add a doc comment here saying something like "Feature enablement based solely on the GitHub API. Most of the time you'll want to use Features instead."

henrymercer · 2022-10-10T14:20:29Z

src/feature-flags.ts

@@ -1,115 +1,200 @@
 import { getApiClient, GitHubApiDetails } from "./api-client";
+import { CodeQL } from "./codeql";


Consider renaming this file to features or feature-enablement.

aeisenberg · 2022-10-11T17:40:00Z

Let's just get it all done now.

henrymercer · 2022-10-11T18:02:04Z

Let's just get it all done now.

Sounds good to me! I've pushed a commit to another branch with some more renaming suggestions to avoid needing to make them all in the UI :) https://github.com/github/codeql-action/compare/aeisenberg/ff-refactoring...henrymercer/more-ff-renaming?expand=1.

henrymercer

Partial review up to the penultimate commit, since I authored the last one.

aeisenberg · 2022-10-12T14:04:48Z

I reviewed the last commit. 🤠

…toring

aeisenberg · 2022-10-12T15:58:08Z

@henrymercer, can you give another approval? Not sure why your previous one was dismissed.

aeisenberg requested a review from a team as a code owner October 5, 2022 22:56

github-advanced-security bot found potential problems Oct 5, 2022

View reviewed changes

src/util.test.ts Fixed Show fixed Hide fixed

aeisenberg force-pushed the aeisenberg/ff-refactoring branch from bf52457 to 4de6737 Compare October 5, 2022 23:10

aeisenberg force-pushed the aeisenberg/ff-refactoring branch from 4de6737 to e5c3375 Compare October 5, 2022 23:41

henrymercer reviewed Oct 6, 2022

View reviewed changes

src/feature-flags.ts Outdated Show resolved Hide resolved

henrymercer reviewed Oct 6, 2022

View reviewed changes

Rename FeatureFlag -> Feature

6de05e4

aeisenberg force-pushed the aeisenberg/ff-refactoring branch from 0a4f3d3 to 6de05e4 Compare October 6, 2022 20:06

Address comments from review

5915e70

aeisenberg force-pushed the aeisenberg/ff-refactoring branch from aa1634e to 730228d Compare October 6, 2022 23:17

Extract GitHubFeatureFlags to a separate class

b27aed7

Internal refactoring so that `GitHubFeatureFlags` is private only. The public facing class is `Features`.

aeisenberg force-pushed the aeisenberg/ff-refactoring branch from 730228d to b27aed7 Compare October 7, 2022 01:00

henrymercer reviewed Oct 7, 2022

View reviewed changes

aeisenberg added 2 commits October 7, 2022 11:33

Merge remote-tracking branch 'upstream/main' into aeisenberg/ff-refac…

919e4ca

…toring

aeisenberg force-pushed the aeisenberg/ff-refactoring branch from 9a002a0 to 919e4ca Compare October 7, 2022 21:14

angelapwen reviewed Oct 7, 2022

View reviewed changes

Fix typos

6c869f8

henrymercer reviewed Oct 10, 2022

View reviewed changes

More renaming

701cea3

More feature flag renaming

43c3ed9

henrymercer previously approved these changes Oct 12, 2022

View reviewed changes

aeisenberg enabled auto-merge October 12, 2022 14:06

aeisenberg added the Update dependencies Trigger PR workflow to update dependencies label Oct 12, 2022

github-actions bot removed the Update dependencies Trigger PR workflow to update dependencies label Oct 12, 2022

Merge remote-tracking branch 'upstream/main' into aeisenberg/ff-refac…

34d48f8

…toring

aeisenberg dismissed henrymercer’s stale review via 34d48f8 October 12, 2022 15:36

henrymercer approved these changes Oct 12, 2022

View reviewed changes

aeisenberg merged commit 160e3fe into main Oct 12, 2022

aeisenberg deleted the aeisenberg/ff-refactoring branch October 12, 2022 17:41

This was referenced Oct 14, 2022

Merge main into releases/v2 #1303

Closed

Merge main into releases/v2 #1311

Merged

Merge releases/v2 into releases/v1 #1314

Merged

	"Not running against github.com. Disabling all feature flags."
	"Not running against github.com. Feature flags will only be enabled if they are enabled explicitly, for example through an environment variable."

	getValue(feaature: Feature, codeql?: CodeQL): Promise<boolean>;
	getValue(feature: Feature, codeql?: CodeQL): Promise<boolean>;

	// Some feature, such as `ml_powered_queries_enabled` affect the produced alerts.
	// Some features, such as `ml_powered_queries_enabled` affect the produced alerts.

	// Considering these feature disabled in the event of a transient error could
	// Considering these features disabled in the event of a transient error could

	// feature flag with a minium version that is only used for tests.
	// feature flag with a minimum version that is only used for tests.

	// Alls flags should be false except the one we're testing
	// All flags should be false except the one we're testing

	* @param feature The feature flag to check.
	* @param feature The feature to check.

		@@ -1,115 +1,200 @@
		import { getApiClient, GitHubApiDetails } from "./api-client";
		import { CodeQL } from "./codeql";

Conversation

aeisenberg commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge / deployment checklist

Uh oh!

Uh oh!

henrymercer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aeisenberg commented Oct 6, 2022

Uh oh!

henrymercer commented Oct 6, 2022

Uh oh!

aeisenberg commented Oct 6, 2022

Uh oh!

aeisenberg commented Oct 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrymercer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angelapwen commented Oct 7, 2022

Uh oh!

angelapwen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented Oct 7, 2022

Uh oh!

aeisenberg commented Oct 5, 2022 •

edited

Loading

henrymercer left a comment •

edited

Loading