Add support for sourcing feature flags from bicepconfig.json #8559

jeskew · 2022-10-03T14:32:06Z

Resolves #7955

This PR updates Bicep's feature flag façade to support multiple sources and adds two more: configuration via bicepconfig.json files and CLI arguments. The existing feature flag environment variables are still supported, but they may be overridden with a CLI argument, and if a feature is not explicitly enabled or disabled via CLI args or environment variables, the appropriate bicepconfig.json file for the template/module being compiled will be queried.

This PR updates IFeatureProvider to read from bicepconfig.json instead of the currently supported environment variables.

As a consequence of reading bicepconfig.json for feature flags, we can no longer associated an IFeatureProvider with a Compilation instance but must instead bind feature providers to the Compilation's constituent SemanticModel instances, as each bicep file may have different features enabled or disabled depending on its location on disk. The same is true of IApiVersionProvider, which takes an IFeatureProvider as a constructor argument.

Microsoft Reviewers: Open in CodeFlow

global.json

src/Bicep.Core/Configuration/ExperimentalFeaturesEnabledConfiguration.cs

anthony-c-martin · 2022-10-04T00:54:47Z

src/Bicep.Cli/InvocationContext.cs

@@ -17,14 +16,14 @@ public class InvocationContext
            IAzResourceTypeLoader azResourceTypeLoader,
            TextWriter outputWriter,
            TextWriter errorWriter,
-            IFeatureProvider? features = null,
+            IFeatureProviderFactory? featureProviderFactory = null,


Does this need to be nullable?

Yes; this parameter is a backdoor for the integration tests to toggle various feature flags and set the assembly version to a static string. It is never set on contexts created in the Bicep.Cli package, which instead relies on the DI container to instantiate the factory.

Also emit a deprecation notice when a feature flag is sourced from an environment variable.

src/Bicep.Core/Analyzers/Linter/ApiVersions/ApiVersionProviderFactory.cs

anthony-c-martin · 2022-10-04T18:02:20Z

src/Bicep.Core/Features/FeatureProvider.cs

+        public bool RegistryEnabled => true;
+


Should we remove this? (feel free to create a follow-up issue)

Probably. A fair number of tests explicitly disable the registry, so there may be something to address there.

anthony-c-martin · 2022-10-04T19:14:01Z

src/Bicep.Core/Configuration/ExperimentalFeaturesEnabled.cs

+    bool? SourceMapping,
+    bool? ParamsFiles)
+{
+    public static ExperimentalFeaturesEnabled Bind(JsonElement element, string? configurationPath)


Sorry to be difficult:

What benefit do we get out of this being an object vs an array? e.g. "experimentalFeatures": ["foo", "bar"]

What's the experience if the user opts-in to a feature which isn't supported by the compiler? Should we error/warning if that happens?

Using an object lets us turn on an experimental feature by default while still giving users a way to disable it if they run into any issues. We currently support this form of transition today (e.g., if we enabled symbolic name codegen by default, a user could turn it off with export BICEP_SYMBOLIC_NAME_CODEGEN_EXPERIMENTAL=false).

The behavior when a user tries to enable a non-existent feature flag is a config load error, which blocks compilation. This could be converted from an error to a warning, but I think that creates a confusing situation: with "symbolcNameCodegen": true, we would emit a non-blocking warning and then not use symbolic names.

I think it makes sense to start strict and wait for feedback to relax it.

I can see a hypothetical situation where different users sharing the bicepconfig.json in a repo run different versions of bicep that have different sets of experimental features, which would force the enabled experimental features being a common denominator but I'm not terribly worried about that scenario... yet.

My preference is object as well for the same reasons. Disabling in an array requires removing the whole item from the array or comment out the item with block comments. With an object you can just change to false. (With completions that's as simple as selecting true, typing f and enter or tab.)

As far as disabling features goes - my expectation is that we won't be shipping any "experimental" features that are true by default - instead we'll just remove the property from IFeatureProvider.

The scenarios for the object do feel a little hypothetical - but I also don't feel strongly about it being an array vs object.

majastrz · 2022-10-04T20:16:12Z

src/Bicep.Core/Configuration/bicepconfig.json

@@ -61,5 +61,6 @@
        }
      }
    }
-  }
+  },
+  "experimentalFeaturesEnabled": {}


experimentalFeaturesEnabled

We have a JSON schema for the configuration. Should we add the new property and sub-properties to it? This way we'll get completions for the available experimental features.

I added this to schema file but didn't document the sub-properties. We don't document the environment variables today, and it seemed like a lot of content to generate 😅

anthony-c-martin

Looks good!

anthony-c-martin · 2022-10-04T20:56:58Z

src/Bicep.Cli/Commands/BuildCommand.cs


-            if (invocationContext.EmitterSettings.EnableSymbolicNames)
+            if (emitterSettings.EnableSymbolicNames)


As a follow-up item, I think we should converge on a single message that covers enablement of all experimental features.

jeskew requested a review from a team October 3, 2022 14:32

jeskew commented Oct 3, 2022

View reviewed changes

global.json Outdated Show resolved Hide resolved

jeskew force-pushed the jeskew/source-feature-flags-from-config branch from 5b447a5 to fe8fd4d Compare October 3, 2022 23:19

jeskew changed the title ~~Add support for sourcing feature flags from bicepconfig.json and CLI args~~ Add support for sourcing feature flags from bicepconfig.json Oct 3, 2022

anthony-c-martin reviewed Oct 4, 2022

View reviewed changes

src/Bicep.Core/Configuration/ExperimentalFeaturesEnabledConfiguration.cs Outdated Show resolved Hide resolved

anthony-c-martin reviewed Oct 4, 2022

View reviewed changes

jeskew added 2 commits October 4, 2022 02:29

Add support for sourcing feature flags from bicepconfig.json

caaa1be

Also emit a deprecation notice when a feature flag is sourced from an environment variable.

Leave only bicepconfig.json as a feature flag source

a7cd125

jeskew force-pushed the jeskew/source-feature-flags-from-config branch from fe8fd4d to a7cd125 Compare October 4, 2022 12:56

jeskew added 3 commits October 4, 2022 09:25

Merge branch 'main' into jeskew/source-feature-flags-from-config

6ca40cd

Use fixed property names in Config.ExperimentalFeaturesEnabled

7308527

Merge branch 'main' into jeskew/source-feature-flags-from-config

ab7903d

jeskew commented Oct 4, 2022

View reviewed changes

src/Bicep.Core/Analyzers/Linter/ApiVersions/ApiVersionProviderFactory.cs Outdated Show resolved Hide resolved

jeskew added 3 commits October 4, 2022 10:56

Merge branch 'main' into jeskew/source-feature-flags-from-config

f197b63

Cleanup diff

5670a87

Fix failing tests

91c89f2

anthony-c-martin reviewed Oct 4, 2022

View reviewed changes

majastrz reviewed Oct 4, 2022

View reviewed changes

Update bicepconfig.json schema

f8e1774

anthony-c-martin approved these changes Oct 4, 2022

View reviewed changes

majastrz approved these changes Oct 4, 2022

View reviewed changes

anthony-c-martin reviewed Oct 4, 2022

View reviewed changes

This was referenced Oct 4, 2022

Remove RegistryEnabled feature flag #8581

Closed

Consolidate CLI warnings about experimental features #8582

Closed

jeskew enabled auto-merge (squash) October 4, 2022 21:02

jeskew merged commit 6715860 into main Oct 4, 2022

jeskew deleted the jeskew/source-feature-flags-from-config branch October 4, 2022 21:55

jeskew mentioned this pull request Oct 26, 2022

Add experimental support for aggregate type declarations #8673

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sourcing feature flags from bicepconfig.json #8559

Add support for sourcing feature flags from bicepconfig.json #8559

jeskew commented Oct 3, 2022 •

edited

Loading

anthony-c-martin Oct 4, 2022

jeskew Oct 4, 2022

anthony-c-martin Oct 4, 2022

jeskew Oct 4, 2022

anthony-c-martin Oct 4, 2022

jeskew Oct 4, 2022 •

edited

Loading

majastrz Oct 4, 2022 •

edited

Loading

anthony-c-martin Oct 4, 2022

majastrz Oct 4, 2022 •

edited

Loading

jeskew Oct 4, 2022

anthony-c-martin left a comment

anthony-c-martin Oct 4, 2022


		if (invocationContext.EmitterSettings.EnableSymbolicNames)
		if (emitterSettings.EnableSymbolicNames)

Add support for sourcing feature flags from bicepconfig.json #8559

Add support for sourcing feature flags from bicepconfig.json #8559

Conversation

jeskew commented Oct 3, 2022 • edited Loading

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeskew Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

majastrz Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majastrz Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthony-c-martin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeskew commented Oct 3, 2022 •

edited

Loading

jeskew Oct 4, 2022 •

edited

Loading

majastrz Oct 4, 2022 •

edited

Loading

majastrz Oct 4, 2022 •

edited

Loading