Add default values to feature flags

This changes adds default values to feature flags. These default values
will be used whenever the full set of feature flags are not available
(for example on GHES or if the flag request fails).

The goal is to enable cli config parsing on GHES without removing the
feature flag on dotcom.
This commit is contained in:
Andrew Eisenberg
2023-03-07 16:24:55 -08:00
parent e4b846c482
commit 0c27d0da4a
6 changed files with 41 additions and 32 deletions

15
lib/feature-flags.js generated
View File

@@ -43,18 +43,22 @@ exports.featureConfig = {
[Feature.DisableKotlinAnalysisEnabled]: { [Feature.DisableKotlinAnalysisEnabled]: {
envVar: "CODEQL_DISABLE_KOTLIN_ANALYSIS", envVar: "CODEQL_DISABLE_KOTLIN_ANALYSIS",
minimumVersion: undefined, minimumVersion: undefined,
defaultValue: false,
}, },
[Feature.CliConfigFileEnabled]: { [Feature.CliConfigFileEnabled]: {
envVar: "CODEQL_PASS_CONFIG_TO_CLI", envVar: "CODEQL_PASS_CONFIG_TO_CLI",
minimumVersion: "2.11.6", minimumVersion: "2.11.6",
defaultValue: true,
}, },
[Feature.MlPoweredQueriesEnabled]: { [Feature.MlPoweredQueriesEnabled]: {
envVar: "CODEQL_ML_POWERED_QUERIES", envVar: "CODEQL_ML_POWERED_QUERIES",
minimumVersion: "2.7.5", minimumVersion: "2.7.5",
defaultValue: false,
}, },
[Feature.UploadFailedSarifEnabled]: { [Feature.UploadFailedSarifEnabled]: {
envVar: "CODEQL_ACTION_UPLOAD_FAILED_SARIF", envVar: "CODEQL_ACTION_UPLOAD_FAILED_SARIF",
minimumVersion: "2.11.3", minimumVersion: "2.11.3",
defaultValue: false,
}, },
}; };
exports.FEATURE_FLAGS_FILE_NAME = "cached-feature-flags.json"; exports.FEATURE_FLAGS_FILE_NAME = "cached-feature-flags.json";
@@ -103,7 +107,8 @@ class Features {
return true; return true;
} }
// Ask the GitHub API if the feature is enabled. // Ask the GitHub API if the feature is enabled.
return await this.gitHubFeatureFlags.getValue(feature); return ((await this.gitHubFeatureFlags.getValue(feature)) ??
exports.featureConfig[feature].defaultValue);
} }
} }
exports.Features = Features; exports.Features = Features;
@@ -179,13 +184,13 @@ class GitHubFeatureFlags {
async getValue(feature) { async getValue(feature) {
const response = await this.getAllFeatures(); const response = await this.getAllFeatures();
if (response === undefined) { if (response === undefined) {
this.logger.debug(`No feature flags API response for ${feature}, considering it disabled.`); this.logger.debug(`No feature flags API response for ${feature}.`);
return false; return undefined;
} }
const featureEnablement = response[feature]; const featureEnablement = response[feature];
if (featureEnablement === undefined) { if (featureEnablement === undefined) {
this.logger.debug(`Feature '${feature}' undefined in API response, considering it disabled.`); this.logger.debug(`Feature '${feature}' undefined in API response.`);
return false; return undefined;
} }
return !!featureEnablement; return !!featureEnablement;
} }

File diff suppressed because one or more lines are too long

View File

@@ -53,7 +53,7 @@ for (const variant of ALL_FEATURES_DISABLED_VARIANTS) {
const loggedMessages = []; const loggedMessages = [];
const featureEnablement = setUpFeatureFlagTests(tmpDir, (0, testing_utils_1.getRecordingLogger)(loggedMessages), variant.gitHubVersion); const featureEnablement = setUpFeatureFlagTests(tmpDir, (0, testing_utils_1.getRecordingLogger)(loggedMessages), variant.gitHubVersion);
for (const feature of Object.values(feature_flags_1.Feature)) { for (const feature of Object.values(feature_flags_1.Feature)) {
t.false(await featureEnablement.getValue(feature, includeCodeQlIfRequired(feature))); t.deepEqual(await featureEnablement.getValue(feature, includeCodeQlIfRequired(feature)), feature_flags_1.featureConfig[feature].defaultValue);
} }
t.assert(loggedMessages.find((v) => v.type === "debug" && t.assert(loggedMessages.find((v) => v.type === "debug" &&
v.message === v.message ===
@@ -61,24 +61,24 @@ for (const variant of ALL_FEATURES_DISABLED_VARIANTS) {
}); });
}); });
} }
(0, ava_1.default)("API response missing", async (t) => { (0, ava_1.default)("API response missing and features use default value", async (t) => {
await (0, util_1.withTmpDir)(async (tmpDir) => { await (0, util_1.withTmpDir)(async (tmpDir) => {
const loggedMessages = []; const loggedMessages = [];
const featureEnablement = setUpFeatureFlagTests(tmpDir, (0, testing_utils_1.getRecordingLogger)(loggedMessages)); const featureEnablement = setUpFeatureFlagTests(tmpDir, (0, testing_utils_1.getRecordingLogger)(loggedMessages));
(0, testing_utils_1.mockFeatureFlagApiEndpoint)(403, {}); (0, testing_utils_1.mockFeatureFlagApiEndpoint)(403, {});
for (const feature of Object.values(feature_flags_1.Feature)) { for (const feature of Object.values(feature_flags_1.Feature)) {
t.assert((await featureEnablement.getValue(feature, includeCodeQlIfRequired(feature))) === false); t.assert((await featureEnablement.getValue(feature, includeCodeQlIfRequired(feature))) === feature_flags_1.featureConfig[feature].defaultValue);
} }
assertAllFeaturesUndefinedInApi(t, loggedMessages); assertAllFeaturesUndefinedInApi(t, loggedMessages);
}); });
}); });
(0, ava_1.default)("Features are disabled if they're not returned in API response", async (t) => { (0, ava_1.default)("Features use default value if they're not returned in API response", async (t) => {
await (0, util_1.withTmpDir)(async (tmpDir) => { await (0, util_1.withTmpDir)(async (tmpDir) => {
const loggedMessages = []; const loggedMessages = [];
const featureEnablement = setUpFeatureFlagTests(tmpDir, (0, testing_utils_1.getRecordingLogger)(loggedMessages)); const featureEnablement = setUpFeatureFlagTests(tmpDir, (0, testing_utils_1.getRecordingLogger)(loggedMessages));
(0, testing_utils_1.mockFeatureFlagApiEndpoint)(200, {}); (0, testing_utils_1.mockFeatureFlagApiEndpoint)(200, {});
for (const feature of Object.values(feature_flags_1.Feature)) { for (const feature of Object.values(feature_flags_1.Feature)) {
t.assert((await featureEnablement.getValue(feature, includeCodeQlIfRequired(feature))) === false); t.assert((await featureEnablement.getValue(feature, includeCodeQlIfRequired(feature))) === feature_flags_1.featureConfig[feature].defaultValue);
} }
assertAllFeaturesUndefinedInApi(t, loggedMessages); assertAllFeaturesUndefinedInApi(t, loggedMessages);
}); });
@@ -283,7 +283,7 @@ function assertAllFeaturesUndefinedInApi(t, loggedMessages) {
for (const feature of Object.keys(feature_flags_1.featureConfig)) { for (const feature of Object.keys(feature_flags_1.featureConfig)) {
t.assert(loggedMessages.find((v) => v.type === "debug" && t.assert(loggedMessages.find((v) => v.type === "debug" &&
v.message.includes(feature) && v.message.includes(feature) &&
v.message.includes("considering it disabled")) !== undefined); v.message.includes("undefined in API response")) !== undefined);
} }
} }
function initializeFeatures(initialValue) { function initializeFeatures(initialValue) {

File diff suppressed because one or more lines are too long

View File

@@ -54,11 +54,12 @@ for (const variant of ALL_FEATURES_DISABLED_VARIANTS) {
); );
for (const feature of Object.values(Feature)) { for (const feature of Object.values(Feature)) {
t.false( t.deepEqual(
await featureEnablement.getValue( await featureEnablement.getValue(
feature, feature,
includeCodeQlIfRequired(feature) includeCodeQlIfRequired(feature)
) ),
featureConfig[feature].defaultValue
); );
} }
@@ -74,7 +75,7 @@ for (const variant of ALL_FEATURES_DISABLED_VARIANTS) {
}); });
} }
test("API response missing", async (t) => { test("API response missing and features use default value", async (t) => {
await withTmpDir(async (tmpDir) => { await withTmpDir(async (tmpDir) => {
const loggedMessages: LoggedMessage[] = []; const loggedMessages: LoggedMessage[] = [];
const featureEnablement = setUpFeatureFlagTests( const featureEnablement = setUpFeatureFlagTests(
@@ -89,14 +90,14 @@ test("API response missing", async (t) => {
(await featureEnablement.getValue( (await featureEnablement.getValue(
feature, feature,
includeCodeQlIfRequired(feature) includeCodeQlIfRequired(feature)
)) === false )) === featureConfig[feature].defaultValue
); );
} }
assertAllFeaturesUndefinedInApi(t, loggedMessages); assertAllFeaturesUndefinedInApi(t, loggedMessages);
}); });
}); });
test("Features are disabled if they're not returned in API response", async (t) => { test("Features use default value if they're not returned in API response", async (t) => {
await withTmpDir(async (tmpDir) => { await withTmpDir(async (tmpDir) => {
const loggedMessages: LoggedMessage[] = []; const loggedMessages: LoggedMessage[] = [];
const featureEnablement = setUpFeatureFlagTests( const featureEnablement = setUpFeatureFlagTests(
@@ -111,7 +112,7 @@ test("Features are disabled if they're not returned in API response", async (t)
(await featureEnablement.getValue( (await featureEnablement.getValue(
feature, feature,
includeCodeQlIfRequired(feature) includeCodeQlIfRequired(feature)
)) === false )) === featureConfig[feature].defaultValue
); );
} }
@@ -471,7 +472,7 @@ function assertAllFeaturesUndefinedInApi(
(v) => (v) =>
v.type === "debug" && v.type === "debug" &&
(v.message as string).includes(feature) && (v.message as string).includes(feature) &&
(v.message as string).includes("considering it disabled") (v.message as string).includes("undefined in API response")
) !== undefined ) !== undefined
); );
} }

View File

@@ -42,23 +42,27 @@ export enum Feature {
export const featureConfig: Record< export const featureConfig: Record<
Feature, Feature,
{ envVar: string; minimumVersion: string | undefined } { envVar: string; minimumVersion: string | undefined; defaultValue: boolean }
> = { > = {
[Feature.DisableKotlinAnalysisEnabled]: { [Feature.DisableKotlinAnalysisEnabled]: {
envVar: "CODEQL_DISABLE_KOTLIN_ANALYSIS", envVar: "CODEQL_DISABLE_KOTLIN_ANALYSIS",
minimumVersion: undefined, minimumVersion: undefined,
defaultValue: false,
}, },
[Feature.CliConfigFileEnabled]: { [Feature.CliConfigFileEnabled]: {
envVar: "CODEQL_PASS_CONFIG_TO_CLI", envVar: "CODEQL_PASS_CONFIG_TO_CLI",
minimumVersion: "2.11.6", minimumVersion: "2.11.6",
defaultValue: true,
}, },
[Feature.MlPoweredQueriesEnabled]: { [Feature.MlPoweredQueriesEnabled]: {
envVar: "CODEQL_ML_POWERED_QUERIES", envVar: "CODEQL_ML_POWERED_QUERIES",
minimumVersion: "2.7.5", minimumVersion: "2.7.5",
defaultValue: false,
}, },
[Feature.UploadFailedSarifEnabled]: { [Feature.UploadFailedSarifEnabled]: {
envVar: "CODEQL_ACTION_UPLOAD_FAILED_SARIF", envVar: "CODEQL_ACTION_UPLOAD_FAILED_SARIF",
minimumVersion: "2.11.3", minimumVersion: "2.11.3",
defaultValue: false,
}, },
}; };
@@ -141,11 +145,14 @@ export class Features implements FeatureEnablement {
return true; return true;
} }
// Ask the GitHub API if the feature is enabled. // Ask the GitHub API if the feature is enabled.
return await this.gitHubFeatureFlags.getValue(feature); return (
(await this.gitHubFeatureFlags.getValue(feature)) ??
featureConfig[feature].defaultValue
);
} }
} }
class GitHubFeatureFlags implements FeatureEnablement { class GitHubFeatureFlags {
private cachedApiResponse: GitHubFeatureFlagsApiResponse | undefined; private cachedApiResponse: GitHubFeatureFlagsApiResponse | undefined;
// We cache whether the feature flags were accessed or not in order to accurately report whether flags were // We cache whether the feature flags were accessed or not in order to accurately report whether flags were
@@ -251,20 +258,16 @@ class GitHubFeatureFlags implements FeatureEnablement {
return { version: maxCliVersion, toolsFeatureFlagsValid: true }; return { version: maxCliVersion, toolsFeatureFlagsValid: true };
} }
async getValue(feature: Feature): Promise<boolean> { async getValue(feature: Feature): Promise<boolean | undefined> {
const response = await this.getAllFeatures(); const response = await this.getAllFeatures();
if (response === undefined) { if (response === undefined) {
this.logger.debug( this.logger.debug(`No feature flags API response for ${feature}.`);
`No feature flags API response for ${feature}, considering it disabled.` return undefined;
);
return false;
} }
const featureEnablement = response[feature]; const featureEnablement = response[feature];
if (featureEnablement === undefined) { if (featureEnablement === undefined) {
this.logger.debug( this.logger.debug(`Feature '${feature}' undefined in API response.`);
`Feature '${feature}' undefined in API response, considering it disabled.` return undefined;
);
return false;
} }
return !!featureEnablement; return !!featureEnablement;
} }